From aa374ca12ab9ed424c91d02e7d477e1a6953f8b8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 27 Jul 2015 14:36:02 -0400 Subject: [PATCH] Make DjangoXBlockUserStateClient pass semantic tests This required the following changes to the DjangoXBlockUserStateClient semantics: 1) Changes get/get_many to return XBlockUserState tuples, rather than state dictionaries or (block_key, state) tuples. 2) Raises DoesNotExist if get_history is called on an XBlock that has had no data saved to it. 3) Returns XBlockUserState tuples as the results of get_history. --- .../management/commands/remove_input_state.py | 21 +-- lms/djangoapps/courseware/model_data.py | 17 ++- .../courseware/user_state_client.py | 131 +++++++++--------- lms/djangoapps/courseware/views.py | 28 +++- .../courseware/submission_history.html | 8 +- 5 files changed, 119 insertions(+), 86 deletions(-) diff --git a/lms/djangoapps/courseware/management/commands/remove_input_state.py b/lms/djangoapps/courseware/management/commands/remove_input_state.py index 988dcc3507..d987a3173a 100644 --- a/lms/djangoapps/courseware/management/commands/remove_input_state.py +++ b/lms/djangoapps/courseware/management/commands/remove_input_state.py @@ -76,16 +76,19 @@ class Command(BaseCommand): user_state_client = DjangoXBlockUserStateClient() hist_modules = user_state_client.get_history(module.student.username, module.module_state_key) - for hist_module in hist_modules: - self.remove_studentmodulehistory_input_state(hist_module, save_changes) + try: + for hist_module in hist_modules: + self.remove_studentmodulehistory_input_state(hist_module, save_changes) - if self.num_visited % 1000 == 0: - LOG.info(" Progress: updated %s of %s student modules", self.num_changed, self.num_visited) - LOG.info( - " Progress: updated %s of %s student history modules", - self.num_hist_changed, - self.num_hist_visited - ) + if self.num_visited % 1000 == 0: + LOG.info(" Progress: updated %s of %s student modules", self.num_changed, self.num_visited) + LOG.info( + " Progress: updated %s of %s student history modules", + self.num_hist_changed, + self.num_hist_visited + ) + except DjangoXBlockUserStateClient.DoesNotExist: + LOG.info("No history entries found for %s", module.module_state_key) @transaction.autocommit def remove_studentmodule_input_state(self, module, save_changes): diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 0fd45a183e..c7275831c7 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -368,8 +368,8 @@ class UserStateCache(object): self.user.username, _all_usage_keys(xblocks, aside_types), ) - for usage_key, field_state in block_field_state: - self._cache[usage_key] = field_state + for user_state in block_field_state: + self._cache[user_state.block_key] = user_state.state @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): @@ -392,11 +392,14 @@ class UserStateCache(object): Returns: datetime if there was a modified date, or None otherwise """ - return self._client.get( - self.user.username, - kvs_key.block_scope_id, - fields=[kvs_key.field_name], - ).updated + try: + return self._client.get( + self.user.username, + kvs_key.block_scope_id, + fields=[kvs_key.field_name], + ).updated + except self._client.DoesNotExist: + return None @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") def set_many(self, kv_dict): diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index f268c7617f..008dd914dc 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -13,7 +13,7 @@ except ImportError: from django.contrib.auth.models import User from xblock.fields import Scope, ScopeBase -from edx_user_state_client.interface import XBlockUserStateClient +from edx_user_state_client.interface import XBlockUserStateClient, XBlockUserState from courseware.models import StudentModule, StudentModuleHistory from contracts import contract, new_contract from opaque_keys.edx.keys import UsageKey @@ -24,6 +24,21 @@ new_contract('UsageKey', UsageKey) class DjangoXBlockUserStateClient(XBlockUserStateClient): """ An interface that uses the Django ORM StudentModule as a backend. + + A note on the format of state storage: + The state for an xblock is stored as a serialized JSON dictionary. The model + field that it is stored in can also take on a value of ``None``. To preserve + existing analytic uses, we will preserve the following semantics: + + A state of ``None`` means that the user hasn't ever looked at the xblock. + A state of ``"{}"`` means that the XBlock has at some point stored state for + the current user, but that that state has been deleted. + Otherwise, the dictionary contains all data stored for the user. + + None of these conditions should violate the semantics imposed by + XBlockUserStateClient (for instance, once all fields have been deleted from + an XBlock for a user, the state will be listed as ``None`` by :meth:`get_history`, + even though the actual stored state in the database will be ``"{}"``). """ class ServiceUnavailable(XBlockUserStateClient.ServiceUnavailable): @@ -53,26 +68,6 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): """ self.user = user - def get_mod_date(self, username, block_key, scope=Scope.user_state, fields=None): - """ - Get the last modification date for fields from the specified blocks. - - Arguments: - username: The name of the user whose state should be deleted - block_key (UsageKey): The UsageKey identifying which xblock modification dates to retrieve. - scope (Scope): The scope to retrieve from. - fields: A list of fields to query. If None, delete all stored fields. - Specific implementations are free to return the same modification date - for all fields, if they don't store changes individually per field. - Implementations may omit fields for which data has not been stored. - - Returns: list a dict of {field_name: modified_date} for each selected field. - """ - results = self.get_mod_date_many(username, [block_key], scope, fields=fields) - return { - field: date for (_, field, date) in results - } - def _get_student_modules(self, username, block_keys): """ Retrieve the :class:`~StudentModule`s for the supplied ``username`` and ``block_keys``. @@ -101,7 +96,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): def get_many(self, username, block_keys, scope=Scope.user_state, fields=None): """ - Retrieve the stored XBlock state for a single xblock usage. + Retrieve the stored XBlock state for the specified XBlock usages. Arguments: username: The name of the user whose state should be retrieved @@ -119,10 +114,22 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): modules = self._get_student_modules(username, block_keys) for module, usage_key in modules: if module.state is None: - state = {} - else: - state = json.loads(module.state) - yield (usage_key, state) + continue + + state = json.loads(module.state) + + # If the state is the empty dict, then it has been deleted, and so + # conformant UserStateClients should treat it as if it doesn't exist. + if state == {}: + continue + + if fields is not None: + state = { + field: state[field] + for field in fields + if field in state + } + yield XBlockUserState(username, usage_key, state, module.modified, scope) def set_many(self, username, block_keys_to_state, scope=Scope.user_state): """ @@ -143,7 +150,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): # that were queried in get_many) so that if the score has # been changed by some other piece of the code, we don't overwrite # that score. - if self.user.username == username: + if self.user is not None and self.user.username == username: user = self.user else: user = User.objects.get(username=username) @@ -185,7 +192,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): student_modules = self._get_student_modules(username, block_keys) for student_module, _ in student_modules: if fields is None: - student_module.state = "{}" + student_module.state = None else: current_state = json.loads(student_module.state) for field in fields: @@ -193,44 +200,25 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): del current_state[field] student_module.state = json.dumps(current_state) + # We just read this object, so we know that we can do an update student_module.save(force_update=True) - def get_mod_date_many(self, username, block_keys, scope=Scope.user_state, fields=None): - """ - Get the last modification date for fields from the specified blocks. - - Arguments: - username: The name of the user whose state should be deleted - block_key (UsageKey): The UsageKey identifying which xblock modification dates to retrieve. - scope (Scope): The scope to retrieve from. - fields: A list of fields to query. If None, delete all stored fields. - Specific implementations are free to return the same modification date - for all fields, if they don't store changes individually per field. - Implementations may omit fields for which data has not been stored. - - Yields: tuples of (block, field_name, modified_date) for each selected field. - """ - if scope != Scope.user_state: - raise ValueError("Only Scope.user_state is supported") - - student_modules = self._get_student_modules(username, block_keys) - for student_module, usage_key in student_modules: - if student_module.state is None: - continue - - for field in json.loads(student_module.state): - yield (usage_key, field, student_module.modified) - def get_history(self, username, block_key, scope=Scope.user_state): """ Retrieve history of state changes for a given block for a given student. We don't guarantee that history for many blocks will be fast. + If the specified block doesn't exist, raise :class:`~DoesNotExist`. + Arguments: - username: The name of the user whose history should be retrieved - block_key (UsageKey): The UsageKey identifying which xblock state to update. - scope (Scope): The scope to load data from + username: The name of the user whose history should be retrieved. + block_key: The key identifying which xblock history to retrieve. + scope (Scope): The scope to load data from. + + Yields: + XBlockUserState entries for each modification to the specified XBlock, from latest + to earliest. """ if scope != Scope.user_state: @@ -243,19 +231,32 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if len(student_modules) == 0: raise self.DoesNotExist() - history_entries = StudentModuleHistory.objects.filter( + history_entries = StudentModuleHistory.objects.prefetch_related('student_module').filter( student_module__in=student_modules ).order_by('-id') - # If no history records exist, let's force a save to get history started. + # If no history records exist, raise an error if not history_entries: - for student_module in student_modules: - student_module.save() - history_entries = StudentModuleHistory.objects.filter( - student_module__in=student_modules - ).order_by('-id') + raise self.DoesNotExist() - return history_entries + for history_entry in history_entries: + state = history_entry.state + + # If the state is serialized json, then load it + if state is not None: + state = json.loads(state) + + # If the state is empty, then for the purposes of `get_history`, it has been + # deleted, and so we list that entry as `None`. + if state == {}: + state = None + + block_key = history_entry.student_module.module_state_key + block_key = block_key.map_into_course( + history_entry.student_module.course_id + ) + + yield XBlockUserState(username, block_key, state, history_entry.created, scope) def iter_all_for_block(self, block_key, scope=Scope.user_state, batch_size=None): """ diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index ba2673c3a9..f95bcc5d3f 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -44,6 +44,7 @@ from openedx.core.djangoapps.credit.api import ( is_user_eligible_for_credit, is_credit_course ) +from courseware.models import StudentModuleHistory from courseware.model_data import FieldDataCache, ScoresClient from .module_render import toc_for_course, get_module_for_descriptor, get_module, get_module_by_usage_id from .entrance_exams import ( @@ -1201,15 +1202,40 @@ def submission_history(request, course_id, student_username, location): user_state_client = DjangoXBlockUserStateClient() try: - history_entries = user_state_client.get_history(student_username, usage_key) + history_entries = list(user_state_client.get_history(student_username, usage_key)) except DjangoXBlockUserStateClient.DoesNotExist: return HttpResponse(escape(_(u'User {username} has never accessed problem {location}').format( username=student_username, location=location ))) + # This is ugly, but until we have a proper submissions API that we can use to provide + # the scores instead, it will have to do. + scores = list(StudentModuleHistory.objects.filter( + student_module__module_state_key=usage_key + ).order_by('-id')) + + if len(scores) != len(history_entries): + log.warning( + "Mismatch when fetching scores for student " + "history for course %s, user %s, xblock %s. " + "Matching scores by date for display.", + course_id, + student_username, + location + ) + scores_by_date = { + score.modified: score + for score in scores + } + scores = [ + scores_by_date[history.updated] + for history in history_entries + ] + context = { 'history_entries': history_entries, + 'scores': scores, 'username': student_username, 'location': location, 'course_id': course_key.to_deprecated_string() diff --git a/lms/templates/courseware/submission_history.html b/lms/templates/courseware/submission_history.html index b4ce5ed2a6..b89fb420f9 100644 --- a/lms/templates/courseware/submission_history.html +++ b/lms/templates/courseware/submission_history.html @@ -1,13 +1,13 @@ <% import json %>

${username | h} > ${course_id | h} > ${location | h}

-% for i, entry in enumerate(history_entries): +% for i, (entry, score) in enumerate(zip(history_entries, scores)):
-#${len(history_entries) - i}: ${entry.created} (${TIME_ZONE} time)
-Score: ${entry.grade} / ${entry.max_grade} +#${len(history_entries) - i}: ${entry.updated} (${TIME_ZONE} time)
+Score: ${score.grade} / ${score.max_grade} 
-${json.dumps(json.loads(entry.state), indent=2, sort_keys=True) | h}
+${json.dumps(entry.state, indent=2, sort_keys=True) | h}
% endfor