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.

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)
-
-            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
-                )
+            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
+                        )
+            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):

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):

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()
+
+        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
+            )
 
-        return history_entries
+            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):
         """

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()

lms/templates/courseware/submission_history.html

 <% import json  %>
 <h3>${username | h} > ${course_id | h} > ${location | h}</h3>
 
-% for i, entry in enumerate(history_entries):
+% for i, (entry, score) in enumerate(zip(history_entries, scores)):
 <hr/>
 <div>
-<b>#${len(history_entries) - i}</b>: ${entry.created} (${TIME_ZONE} time)</br>
-Score: ${entry.grade} / ${entry.max_grade}
+<b>#${len(history_entries) - i}</b>: ${entry.updated} (${TIME_ZONE} time)</br>
+Score: ${score.grade} / ${score.max_grade}
 <pre>
-${json.dumps(json.loads(entry.state), indent=2, sort_keys=True) | h}
+${json.dumps(entry.state, indent=2, sort_keys=True) | h}
 </pre>
 </div>
 % endfor