WIP: Save student state via StudentModule. Inheritance doesn't work

common/djangoapps/xmodule_modifiers.py

@@ -108,36 +108,33 @@ def add_histogram(get_html, module, user):
 
         # TODO (ichuang): Remove after fall 2012 LMS migration done
         if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'):
-            [filepath, filename] = module.definition.get('filename', ['', None])
+            [filepath, filename] = module.lms.filename
             osfs = module.system.filestore
             if filename is not None and osfs.exists(filename):
                 # if original, unmangled filename exists then use it (github
                 # doesn't like symlinks)
                 filepath = filename
             data_dir = osfs.root_path.rsplit('/')[-1]
-            giturl = module.metadata.get('giturl','https://github.com/MITx')
-            edit_link = "%s/%s/tree/master/%s" % (giturl,data_dir,filepath)
+            edit_link = "%s/%s/tree/master/%s" % (module.lms.giturl, data_dir, filepath)
         else:
             edit_link = False
             # Need to define all the variables that are about to be used
-            giturl = ""
             data_dir = ""
-        source_file = module.metadata.get('source_file','')	# source used to generate the problem XML, eg latex or word
+        source_file = module.lms.source_file  # source used to generate the problem XML, eg latex or word
 
         # useful to indicate to staff if problem has been released or not
         # TODO (ichuang): use _has_access_descriptor.can_load in lms.courseware.access, instead of now>mstart comparison here
         now = time.gmtime()
         is_released = "unknown"
-        mstart = getattr(module.descriptor,'start')
+        mstart = getattr(module.descriptor.lms,'start')
         if mstart is not None:
             is_released = "<font color='red'>Yes!</font>" if (now > mstart) else "<font color='green'>Not yet</font>"
 
-        staff_context = {'definition': module.definition.get('data'),
-                         'metadata': json.dumps(module.metadata, indent=4),
+        staff_context = {'fields': [(field.name, getattr(module, field.name)) for field in module.fields],
                          'location': module.location,
-                         'xqa_key': module.metadata.get('xqa_key',''),
+                         'xqa_key': module.lms.xqa_key,
                          'source_file' : source_file,
-                         'source_url': '%s/%s/tree/master/%s' % (giturl,data_dir,source_file),
+                         'source_url': '%s/%s/tree/master/%s' % (module.lms.giturl, data_dir, source_file),
                          'category': str(module.__class__.__name__),
                          # Template uses element_id in js function names, so can't allow dashes
                          'element_id': module.location.html_id().replace('-','_'),

common/lib/capa/capa/capa_problem.py

@@ -83,7 +83,7 @@ class LoncapaProblem(object):
     Main class for capa Problems.
     '''
 
-    def __init__(self, problem_text, id, correct_map=None, done=None, seed=None, system=None):
+    def __init__(self, problem_text, id, state=None, seed=None, system=None):
         '''
         Initializes capa Problem.
 
@@ -91,8 +91,7 @@ class LoncapaProblem(object):
 
          - problem_text (string): xml defining the problem
          - id           (string): identifier for this problem; often a filename (no spaces)
-         - correct_map  (dict): data specifying whether the student has completed the problem
-         - done         (bool): Whether the student has answered the problem
+         - state        (dict): student state
          - seed         (int): random number generator seed (int)
          - system       (ModuleSystem): ModuleSystem instance which provides OS,
                                         rendering, and user context
@@ -103,12 +102,19 @@ class LoncapaProblem(object):
         self.do_reset()
         self.problem_id = id
         self.system = system
+        if self.system is None:
+            raise Exception()
         self.seed = seed
-        self.done = done
-        self.correct_map = CorrectMap()
 
-        if correct_map is not None:
-            self.correct_map.set_dict(correct_map)
+        if state:
+            if 'seed' in state:
+                self.seed = state['seed']
+            if 'student_answers' in state:
+                self.student_answers = state['student_answers']
+            if 'correct_map' in state:
+                self.correct_map.set_dict(state['correct_map'])
+            if 'done' in state:
+                self.done = state['done']
 
         # TODO: Does this deplete the Linux entropy pool? Is this fast enough?
         if not self.seed:

common/lib/xmodule/xmodule/capa_module.py

@@ -92,12 +92,14 @@ class CapaModule(XModule):
     due = String(help="Date that this problem is due by", scope=Scope.settings)
     graceperiod = Timedelta(help="Amount of time after the due date that submissions will be accepted", scope=Scope.settings)
     show_answer = String(help="When to show the problem answer to the student", scope=Scope.settings, default="closed")
-    force_save_button = Boolean(help="Whether to force the save button to appear on the page", scope=Scope.settings)
+    force_save_button = Boolean(help="Whether to force the save button to appear on the page", scope=Scope.settings, default=False)
     rerandomize = String(help="When to rerandomize the problem", default="always")
     data = String(help="XML data for the problem", scope=Scope.content)
-    correct_map = Object(help="Dictionary with the correctness of current student answers", scope=Scope.student_state)
+    correct_map = Object(help="Dictionary with the correctness of current student answers", scope=Scope.student_state, default={})
+    student_answers = Object(help="Dictionary with the current student responses", scope=Scope.student_state)
     done = Boolean(help="Whether the student has answered the problem", scope=Scope.student_state)
     display_name = String(help="Display name for this module", scope=Scope.settings)
+    seed = Int(help="Random seed for this student", scope=Scope.student_state)
 
     js = {'coffee': [resource_string(__name__, 'js/src/capa/display.coffee'),
                      resource_string(__name__, 'js/src/collapsible.coffee'),
@@ -124,23 +126,23 @@ class CapaModule(XModule):
         else:
             self.close_date = self.due
 
-        if self.rerandomize == 'never':
-            self.seed = 1
-        elif self.rerandomize == "per_student" and hasattr(self.system, 'id'):
-            # TODO: This line is badly broken:
-            # (1) We're passing student ID to xmodule.
-            # (2) There aren't bins of students.  -- we only want 10 or 20 randomizations, and want to assign students
-            # to these bins, and may not want cohorts.  So e.g. hash(your-id, problem_id) % num_bins.
-            #     - analytics really needs small number of bins.
-            self.seed = system.id
-        else:
-            self.seed = None
+        if self.seed is None:
+            if self.rerandomize == 'never':
+                self.seed = 1
+            elif self.rerandomize == "per_student" and hasattr(self.system, 'id'):
+                # TODO: This line is badly broken:
+                # (1) We're passing student ID to xmodule.
+                # (2) There aren't bins of students.  -- we only want 10 or 20 randomizations, and want to assign students
+                # to these bins, and may not want cohorts.  So e.g. hash(your-id, problem_id) % num_bins.
+                #     - analytics really needs small number of bins.
+                self.seed = system.id
+            else:
+                self.seed = None
 
         try:
             # TODO (vshnayder): move as much as possible of this work and error
             # checking to descriptor load time
-            self.lcp = LoncapaProblem(self.data, self.location.html_id(),
-                                      self.correct_map, self.done, self.seed, self.system)
+            self.lcp = self.new_lcp(self.get_state_for_lcp())
         except Exception as err:
             msg = 'cannot create LoncapaProblem {loc}: {err}'.format(
                 loc=self.location.url(), err=err)
@@ -157,9 +159,7 @@ class CapaModule(XModule):
                 problem_text = ('<problem><text><span class="inline-error">'
                                 'Problem %s has an error:</span>%s</text></problem>' %
                                 (self.location.url(), msg))
-                self.lcp = LoncapaProblem(
-                    problem_text, self.location.html_id(),
-                    self.correct_map, self.done, self.seed, self.system)
+                self.lcp = self.new_lcp(self.get_state_for_lcp(), text=problem_text)
             else:
                 # add extra info and raise
                 raise Exception(msg), None, sys.exc_info()[2]
@@ -169,10 +169,30 @@ class CapaModule(XModule):
         elif self.rerandomize == "false":
             self.rerandomize = "per_student"
 
-    def sync_lcp_state(self):
+    def new_lcp(self, state, text=None):
+        if text is None:
+            text = self.data
+
+        return LoncapaProblem(
+            problem_text=text,
+            id=self.location.html_id(),
+            state=state,
+            system=self.system,
+        )
+
+    def get_state_for_lcp(self):
+        return {
+            'done': self.done,
+            'correct_map': self.correct_map,
+            'student_answers': self.student_answers,
+            'seed': self.seed,
+        }
+
+    def set_state_from_lcp(self):
         lcp_state = self.lcp.get_state()
         self.done = lcp_state['done']
         self.correct_map = lcp_state['correct_map']
+        self.student_answers = lcp_state['student_answers']
         self.seed = lcp_state['seed']
 
     def get_score(self):
@@ -239,9 +259,8 @@ class CapaModule(XModule):
                             student_answers.pop(answer_id)
 
                 #   Next, generate a fresh LoncapaProblem
-                self.lcp = LoncapaProblem(self.definition['data'], self.location.html_id(),
-                               seed=self.seed, system=self.system)
-                self.sync_lcp_state()
+                self.lcp = self.new_lcp(None)
+                self.set_state_from_lcp()
 
                 # Prepend a scary warning to the student
                 warning  = '<div class="capa_reset">'\
@@ -305,7 +324,7 @@ class CapaModule(XModule):
         # We may not need a "save" button if infinite number of attempts and
         # non-randomized. The problem author can force it. It's a bit weird for
         # randomization to control this; should perhaps be cleaned up.
-        if (self.force_save_button == "false") and (self.max_attempts is None and self.rerandomize != "always"):
+        if (not self.force_save_button) and (self.max_attempts is None and self.rerandomize != "always"):
             save_button = False
 
         context = {'problem': content,
@@ -326,7 +345,7 @@ class CapaModule(XModule):
                 id=self.location.html_id(), ajax_url=self.system.ajax_url) + html + "</div>"
 
         # now do the substitutions which are filesystem based, e.g. '/static/' prefixes
-        return self.system.replace_urls(html, self.metadata['data_dir'], course_namespace=self.location)
+        return self.system.replace_urls(html, self.descriptor.data_dir, course_namespace=self.location)
 
     def handle_ajax(self, dispatch, get):
         '''
@@ -408,7 +427,7 @@ class CapaModule(XModule):
         queuekey = get['queuekey']
         score_msg = get['xqueue_body']
         self.lcp.update_score(score_msg, queuekey)
-        self.sync_lcp_state()
+        self.set_state_from_lcp()
 
         return dict()  # No AJAX return is needed
 
@@ -425,14 +444,18 @@ class CapaModule(XModule):
             raise NotFoundError('Answer is not available')
         else:
             answers = self.lcp.get_question_answers()
-            self.sync_lcp_state()
+            self.set_state_from_lcp()
 
         # answers (eg <solution>) may have embedded images
         #   but be careful, some problems are using non-string answer dicts
         new_answers = dict()
         for answer_id in answers:
             try:
+<<<<<<< HEAD
                 new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.metadata['data_dir'], course_namespace=self.location)}
+=======
+                new_answer = {answer_id: self.system.replace_urls(answers[answer_id], self.descriptor.data_dir)}
+>>>>>>> WIP: Save student state via StudentModule. Inheritance doesn't work
             except TypeError:
                 log.debug('Unable to perform URL substitution on answers[%s]: %s' % (answer_id, answers[answer_id]))
                 new_answer = {answer_id: answers[answer_id]}
@@ -509,7 +532,7 @@ class CapaModule(XModule):
 
         try:
             correct_map = self.lcp.grade_answers(answers)
-            self.sync_lcp_state()
+            self.set_state_from_lcp()
         except StudentInputError as inst:
             log.exception("StudentInputError in capa_module:problem_check")
             return {'success': inst.message}
@@ -609,13 +632,8 @@ class CapaModule(XModule):
             # in next line)
             self.lcp.seed = None
 
-        self.lcp = LoncapaProblem(self.data,
-                                  self.location.html_id(),
-                                  self.lcp.correct_map,
-                                  self.lcp.done,
-                                  self.lcp.seed,
-                                  self.system)
-        self.sync_lcp_state()
+        self.set_state_from_lcp()
+        self.lcp = self.new_lcp()
 
         event_info['new_state'] = self.lcp.get_state()
         self.system.track_function('reset_problem', event_info)

common/lib/xmodule/xmodule/model.py

@@ -10,8 +10,8 @@ class Scope(namedtuple('ScopeBase', 'student module')):
     pass
 
 Scope.content = Scope(student=False, module=ModuleScope.DEFINITION)
+Scope.settings = Scope(student=False, module=ModuleScope.USAGE)
 Scope.student_state = Scope(student=True, module=ModuleScope.USAGE)
-Scope.settings = Scope(student=True, module=ModuleScope.USAGE)
 Scope.student_preferences = Scope(student=True, module=ModuleScope.TYPE)
 Scope.student_info = Scope(student=True, module=ModuleScope.ALL)
 
@@ -54,7 +54,7 @@ class ModelType(object):
         del instance._model_data[self.name]
 
     def __repr__(self):
-        return "<{0.__class__.__name} {0.__name__}>".format(self)
+        return "<{0.__class__.__name__} {0._name}>".format(self)
 
     def __lt__(self, other):
         return self._seq < other._seq
@@ -100,9 +100,12 @@ class NamespacesMetaclass(type):
     the instance
     """
     def __new__(cls, name, bases, attrs):
+        namespaces = []
         for ns_name, namespace in Namespace.load_classes():
             if issubclass(namespace, Namespace):
                 attrs[ns_name] = NamespaceDescriptor(namespace)
+                namespaces.append(ns_name)
+        attrs['namespaces'] = namespaces
 
         return super(NamespacesMetaclass, cls).__new__(cls, name, bases, attrs)
 
@@ -114,7 +117,7 @@ class ParentModelMetaclass(type):
     """
     def __new__(cls, name, bases, attrs):
         if attrs.get('has_children', False):
-            attrs['children'] = List(help='The children of this XModule', default=[], scope=None)
+            attrs['children'] = List(help='The children of this XModule', default=[], scope=Scope.settings)
         else:
             attrs['has_children'] = False
 

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -175,7 +175,7 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
                 # Normally, we don't want lots of exception traces in our logs from common
                 # content problems.  But if you're debugging the xml loading code itself,
                 # uncomment the next line.
-                # log.exception(msg)
+                log.exception(msg)
 
                 self.error_tracker(msg)
                 err_msg = msg + "\n" + exc_info_to_str(sys.exc_info())

lms/djangoapps/courseware/models.py

@@ -64,6 +64,9 @@ class StudentModule(models.Model):
         return '/'.join([self.course_id, self.module_type,
                          self.student.username, self.module_state_key, str(self.state)[:20]])
 
+    def __repr__(self):
+        return 'StudentModule%r' % ((self.course_id, self.module_type, self.student, self.module_state_key, str(self.state)[:20]),)
+
 
 # TODO (cpennington): Remove these once the LMS switches to using XModuleDescriptors
 

lms/djangoapps/courseware/module_render.py

@@ -11,6 +11,8 @@ from django.http import Http404
 from django.http import HttpResponse
 from django.views.decorators.csrf import csrf_exempt
 
+from collections import namedtuple
+
 from requests.auth import HTTPBasicAuth
 
 from capa.xqueue_interface import XQueueInterface
@@ -26,6 +28,8 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.x_module import ModuleSystem
 from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor
+from xmodule.runtime import DbModel, KeyValueStore
+from xmodule.model import Scope
 from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule
 
 from xmodule.modulestore.exceptions import ItemNotFoundError
@@ -145,6 +149,70 @@ def get_module(user, request, location, student_module_cache, course_id, positio
         return None
 
 
+class LmsKeyValueStore(KeyValueStore):
+    def __init__(self, course_id, user, descriptor_model_data, student_module_cache):
+        self._course_id = course_id
+        self._user = user
+        self._descriptor_model_data = descriptor_model_data
+        self._student_module_cache = student_module_cache
+
+    def _student_module(self, key):
+        student_module = self._student_module_cache.lookup(
+            self._course_id, key.module_scope_id.category, key.module_scope_id.url()
+        )
+        return student_module
+
+    def get(self, key):
+        if not key.scope.student:
+            return self._descriptor_model_data[key.field_name]
+
+        if key.scope == Scope.student_state:
+            student_module = self._student_module(key)
+
+            if student_module is None:
+                raise KeyError(key.field_name)
+
+            return json.loads(student_module.state)[key.field_name]
+
+    def set(self, key, value):
+        if not key.scope.student:
+            self._descriptor_model_data[key.field_name] = value
+
+        if key.scope == Scope.student_state:
+            student_module = self._student_module(key)
+            if student_module is None:
+                student_module = StudentModule(
+                    course_id=self._course_id,
+                    student=self._user,
+                    module_type=key.module_scope_id.category,
+                    module_state_key=key.module_scope_id,
+                    state=json.dumps({})
+                )
+                self._student_module_cache.append(student_module)
+            state = json.loads(student_module.state)
+            state[key.field_name] = value
+            student_module.state = json.dumps(state)
+            student_module.save()
+
+    def delete(self, key):
+        if not key.scope.student:
+            del self._descriptor_model_data[key.field_name]
+
+        if key.scope == Scope.student_state:
+            student_module = self._student_module(key)
+
+            if student_module is None:
+                raise KeyError(key.field_name)
+
+            state = json.loads(student_module.state)
+            del state[key.field_name]
+            student_module.state = json.dumps(state)
+            student_module.save()
+
+
+LmsUsage = namedtuple('LmsUsage', 'id, def_id')
+
+
 def _get_module(user, request, location, student_module_cache, course_id, position=None, wrap_xmodule_display=True):
     """
     Actually implement get_module.  See docstring there for details.
@@ -162,23 +230,6 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
     h.update(str(user.id))
     anonymous_student_id = h.hexdigest()
 
-    # Only check the cache if this module can possibly have state
-    instance_module = None
-    shared_module = None
-    if user.is_authenticated():
-        if descriptor.stores_state:
-            instance_module = student_module_cache.lookup(
-                course_id, descriptor.category, descriptor.location.url())
-
-        shared_state_key = getattr(descriptor, 'shared_state_key', None)
-        if shared_state_key is not None:
-            shared_module = student_module_cache.lookup(course_id,
-                                                        descriptor.category,
-                                                        shared_state_key)
-
-    instance_state = instance_module.state if instance_module is not None else None
-    shared_state = shared_module.state if shared_module is not None else None
-
     # Setup system context for module instance
     ajax_url = reverse('modx_dispatch',
                        kwargs=dict(course_id=course_id,
@@ -218,6 +269,14 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
         return get_module(user, request, location,
                                        student_module_cache, course_id, position)
 
+    def xmodule_model_data(descriptor_model_data):
+        return DbModel(
+            LmsKeyValueStore(course_id, user, descriptor_model_data, student_module_cache),
+            descriptor.module_class,
+            user.id,
+            LmsUsage(location, location)
+        )
+
     # TODO (cpennington): When modules are shared between courses, the static
     # prefix is going to have to be specific to the module, not the directory
     # that the xml was loaded from
@@ -235,6 +294,7 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
                           replace_urls=replace_urls,
                           node_path=settings.NODE_PATH,
                           anonymous_student_id=anonymous_student_id,
+                          xmodule_model_data=xmodule_model_data
                           )
     # pass position specified in URL to module through ModuleSystem
     system.set('position', position)
@@ -453,19 +513,6 @@ def modx_dispatch(request, dispatch, location, course_id):
         log.debug("No module {0} for user {1}--access denied?".format(location, user))
         raise Http404
 
-    instance_module = get_instance_module(course_id, request.user, instance, student_module_cache)
-    shared_module = get_shared_instance_module(course_id, request.user, instance, student_module_cache)
-
-    # Don't track state for anonymous users (who don't have student modules)
-    if instance_module is not None:
-        oldgrade = instance_module.grade
-        # The max grade shouldn't change under normal circumstances, but
-        # sometimes the problem changes with the same name but a new max grade.
-        # This updates the module if that happens.
-        old_instance_max_grade = instance_module.max_grade
-        old_instance_state = instance_module.state
-        old_shared_state = shared_module.state if shared_module is not None else None
-
     # Let the module handle the AJAX
     try:
         ajax_return = instance.handle_ajax(dispatch, p)
@@ -476,34 +523,6 @@ def modx_dispatch(request, dispatch, location, course_id):
         log.exception("error processing ajax call")
         raise
 
-    # Save the state back to the database
-    # Don't track state for anonymous users (who don't have student modules)
-    if instance_module is not None:
-        instance_module.state = instance.get_instance_state()
-        instance_module.max_grade=instance.max_score()
-        if instance.get_score():
-            instance_module.grade = instance.get_score()['score']
-        if (instance_module.grade != oldgrade or
-            instance_module.state != old_instance_state or
-            instance_module.max_grade != old_instance_max_grade):
-            instance_module.save()
-
-            #Bin score into range and increment stats
-            score_bucket=get_score_bucket(instance_module.grade, instance_module.max_grade)
-            org, course_num, run=course_id.split("/")
-            statsd.increment("lms.courseware.question_answered",
-                            tags=["org:{0}".format(org),
-                                  "course:{0}".format(course_num),
-                                  "run:{0}".format(run),
-                                  "score_bucket:{0}".format(score_bucket),
-                                  "type:ajax"])
-
-
-    if shared_module is not None:
-        shared_module.state = instance.get_shared_state()
-        if shared_module.state != old_shared_state:
-            shared_module.save()
-
     # Return whatever the module wanted to return to the client/caller
     return HttpResponse(ajax_return)
 

lms/templates/staff_problem_info.html

@@ -46,8 +46,9 @@ github = <a href="${edit_link}">${edit_link | h}</a>
 %if source_file:
 source_url = <a href="${source_url}">${source_file | h}</a>
 %endif
-definition = <pre>${definition | h}</pre>
-metadata = ${metadata | h}
+%for name, field in fields:
+${name} = <pre style="display:inline-block">${field | h}</pre>
+%endfor
 category = ${category | h}
     </div>
     %if render_histogram:

lms/xmodule_namespace.py

-from xmodule.model import Namespace, Boolean, Scope, String
+from xmodule.model import Namespace, Boolean, Scope, String, List
 from xmodule.x_module import Date
 
 class LmsNamespace(Namespace):
@@ -21,3 +21,7 @@ class LmsNamespace(Namespace):
     display_name = String(help="Display name for this module", scope=Scope.settings)
     start = Date(help="Start time when this module is visible", scope=Scope.settings)
     due = String(help="Date that this problem is due by", scope=Scope.settings, default='')
+    filename = List(help="DO NOT USE", scope=Scope.content, default=['', None])
+    source_file = String(help="DO NOT USE", scope=Scope.settings)
+    giturl = String(help="DO NOT USE", scope=Scope.settings, default='https://github.com/MITx')
+    xqa_key = String(help="DO NOT USE", scope=Scope.settings)