Merge pull request #1085 from MITx/feature/victor/fix-self-assessment

Feature/victor/fix self assessment

common/lib/xmodule/xmodule/js/src/selfassessment/display.coffee

@@ -120,7 +120,7 @@ class @SelfAssessment
     if @state == 'done'
       $.postWithPrefix "#{@ajax_url}/reset", {}, (response) =>
         if response.success
-          @answer_area.html('')
+          @answer_area.val('')
           @rubric_wrapper.html('')
           @hint_wrapper.html('')
           @message_wrapper.html('')

common/lib/xmodule/xmodule/self_assessment_module.py

@@ -7,20 +7,21 @@ Parses xml definition file--see below for exact format.
 
 import copy
 from fs.errors import ResourceNotFoundError
+import itertools
+import json
 import logging
-import os
-import sys
 from lxml import etree
 from lxml.html import rewrite_links
 from path import path
-import json
-from progress import Progress
+import os
+import sys
 
 from pkg_resources import resource_string
 
 from .capa_module import only_one, ComplexEncoder
 from .editing_module import EditingDescriptor
 from .html_checker import check_html
+from progress import Progress
 from .stringify import stringify_children
 from .x_module import XModule
 from .xml_module import XmlDescriptor
@@ -52,6 +53,8 @@ class SelfAssessmentModule(XModule):
          submissions too.)
     """
 
+    STATE_VERSION = 1
+
     # states
     INITIAL = 'initial'
     ASSESSING = 'assessing'
@@ -102,35 +105,130 @@ class SelfAssessmentModule(XModule):
         else:
             instance_state = {}
 
-        # Note: score responses are on scale from 0 to max_score
-        self.student_answers = instance_state.get('student_answers', [])
-        self.scores = instance_state.get('scores', [])
-        self.hints = instance_state.get('hints', [])
+        instance_state = self.convert_state_to_current_format(instance_state)
+
+        # History is a list of tuples of (answer, score, hint), where hint may be
+        # None for any element, and score and hint can be None for the last (current)
+        # element.
+        # Scores are on scale from 0 to max_score
+        self.history = instance_state.get('history', [])
 
         self.state = instance_state.get('state', 'initial')
 
+        self.attempts = instance_state.get('attempts', 0)
+        self.max_attempts = int(self.metadata.get('attempts', MAX_ATTEMPTS))
+
         # Used for progress / grading.  Currently get credit just for
         # completion (doesn't matter if you self-assessed correct/incorrect).
-
         self._max_score = int(self.metadata.get('max_score', MAX_SCORE))
 
-        self.attempts = instance_state.get('attempts', 0)
-
-        self.max_attempts = int(self.metadata.get('attempts', MAX_ATTEMPTS))
-
         self.rubric = definition['rubric']
         self.prompt = definition['prompt']
         self.submit_message = definition['submitmessage']
         self.hint_prompt = definition['hintprompt']
 
+
+    def latest_answer(self):
+        """None if not available"""
+        if not self.history:
+            return None
+        return self.history[-1].get('answer')
+
+    def latest_score(self):
+        """None if not available"""
+        if not self.history:
+            return None
+        return self.history[-1].get('score')
+
+    def latest_hint(self):
+        """None if not available"""
+        if not self.history:
+            return None
+        return self.history[-1].get('hint')
+
+    def new_history_entry(self, answer):
+        self.history.append({'answer': answer})
+
+    def record_latest_score(self, score):
+        """Assumes that state is right, so we're adding a score to the latest
+        history element"""
+        self.history[-1]['score'] = score
+
+    def record_latest_hint(self, hint):
+        """Assumes that state is right, so we're adding a score to the latest
+        history element"""
+        self.history[-1]['hint'] = hint
+
+
+    def change_state(self, new_state):
+        """
+        A centralized place for state changes--allows for hooks.  If the
+        current state matches the old state, don't run any hooks.
+        """
+        if self.state == new_state:
+            return
+
+        self.state = new_state
+
+        if self.state == self.DONE:
+            self.attempts += 1
+
+    @staticmethod
+    def convert_state_to_current_format(old_state):
+        """
+        This module used to use a problematic state representation. This method
+        converts that into the new format.
+
+        Args:
+            old_state: dict of state, as passed in.  May be old.
+
+        Returns:
+            new_state: dict of new state
+        """
+        if old_state.get('version', 0) == SelfAssessmentModule.STATE_VERSION:
+            # already current
+            return old_state
+
+        # for now, there's only one older format.
+
+        new_state = {'version': SelfAssessmentModule.STATE_VERSION}
+
+        def copy_if_present(key):
+            if key in old_state:
+                new_state[key] = old_state[key]
+
+        for to_copy in ['attempts', 'state']:
+            copy_if_present(to_copy)
+
+        # The answers, scores, and hints need to be kept together to avoid them
+        # getting out of sync.
+
+        # NOTE: Since there's only one problem with a few hundred submissions
+        # in production so far, not trying to be smart about matching up hints
+        # and submissions in cases where they got out of sync.
+
+        student_answers = old_state.get('student_answers', [])
+        scores = old_state.get('scores', [])
+        hints = old_state.get('hints', [])
+
+        new_state['history'] = [
+            {'answer': answer,
+             'score': score,
+             'hint': hint}
+             for answer, score, hint in itertools.izip_longest(
+                     student_answers, scores, hints)]
+        return new_state
+
+
     def _allow_reset(self):
         """Can the module be reset?"""
         return self.state == self.DONE and self.attempts < self.max_attempts
 
     def get_html(self):
         #set context variables and render template
-        if self.state != self.INITIAL and self.student_answers:
-            previous_answer = self.student_answers[-1]
+        if self.state != self.INITIAL:
+            latest = self.latest_answer()
+            previous_answer = latest if latest is not None else ''
         else:
             previous_answer = ''
 
@@ -149,26 +247,19 @@ class SelfAssessmentModule(XModule):
         # cdodge: perform link substitutions for any references to course static content (e.g. images)
         return rewrite_links(html, self.rewrite_content_links)
 
-    def get_score(self):
-        """
-        Returns dict with 'score' key
-        """
-        return {'score': self.get_last_score()}
-
     def max_score(self):
         """
         Return max_score
         """
         return self._max_score
 
-    def get_last_score(self):
+    def get_score(self):
         """
         Returns the last score in the list
         """
-        last_score=0
-        if(len(self.scores)>0):
-            last_score=self.scores[len(self.scores)-1]
-        return last_score
+        score = self.latest_score()
+        return {'score': score if score is not None else 0,
+                'total': self._max_score}
 
     def get_progress(self):
         '''
@@ -176,7 +267,7 @@ class SelfAssessmentModule(XModule):
         '''
         if self._max_score > 0:
             try:
-                return Progress(self.get_last_score(), self._max_score)
+                return Progress(self.get_score()['score'], self._max_score)
             except Exception as err:
                 log.exception("Got bad progress")
                 return None
@@ -250,9 +341,10 @@ class SelfAssessmentModule(XModule):
         if self.state in (self.INITIAL, self.ASSESSING):
             return ''
 
-        if self.state == self.DONE and len(self.hints) > 0:
+        if self.state == self.DONE:
             # display the previous hint
-            hint = self.hints[-1]
+            latest = self.latest_hint()
+            hint = latest if latest is not None else ''
         else:
             hint = ''
 
@@ -295,8 +387,9 @@ class SelfAssessmentModule(XModule):
         if self.state != self.INITIAL:
             return self.out_of_sync_error(get)
 
-        self.student_answers.append(get['student_answer'])
-        self.state = self.ASSESSING
+        # add new history element with answer and empty score and hint.
+        self.new_history_entry(get['student_answer'])
+        self.change_state(self.ASSESSING)
 
         return {
             'success': True,
@@ -318,27 +411,24 @@ class SelfAssessmentModule(XModule):
         'message_html' only if success is true
         """
 
-        n_answers = len(self.student_answers)
-        n_scores = len(self.scores)
-        if (self.state != self.ASSESSING or n_answers !=  n_scores + 1):
-            msg = "%d answers, %d scores" % (n_answers, n_scores)
-            return self.out_of_sync_error(get, msg)
+        if self.state != self.ASSESSING:
+            return self.out_of_sync_error(get)
 
         try:
             score = int(get['assessment'])
-        except:
+        except ValueError:
             return {'success': False, 'error': "Non-integer score value"}
 
-        self.scores.append(score)
+        self.record_latest_score(score)
 
         d = {'success': True,}
 
         if score == self.max_score():
-            self.state = self.DONE
+            self.change_state(self.DONE)
             d['message_html'] = self.get_message_html()
             d['allow_reset'] = self._allow_reset()
         else:
-            self.state = self.REQUEST_HINT
+            self.change_state(self.REQUEST_HINT)
             d['hint_html'] = self.get_hint_html()
 
         d['state'] = self.state
@@ -360,19 +450,15 @@ class SelfAssessmentModule(XModule):
             # the same number of hints and answers.
             return self.out_of_sync_error(get)
 
-        self.hints.append(get['hint'].lower())
-        self.state = self.DONE
-
-        # increment attempts
-        self.attempts = self.attempts + 1
+        self.record_latest_hint(get['hint'])
+        self.change_state(self.DONE)
 
         # To the tracking logs!
         event_info = {
             'selfassessment_id': self.location.url(),
             'state': {
-                'student_answers': self.student_answers,
-                'score': self.scores,
-                'hints': self.hints,
+                'version': self.STATE_VERSION,
+                'history': self.history,
                 }
             }
         self.system.track_function('save_hint', event_info)
@@ -397,7 +483,7 @@ class SelfAssessmentModule(XModule):
                 'success': False,
                 'error': 'Too many attempts.'
             }
-        self.state = self.INITIAL
+        self.change_state(self.INITIAL)
         return {'success': True}
 
 
@@ -407,12 +493,11 @@ class SelfAssessmentModule(XModule):
         """
 
         state = {
-                 'student_answers': self.student_answers,
-                 'hints': self.hints,
+                 'version': self.STATE_VERSION,
+                 'history': self.history,
                  'state': self.state,
-                 'scores': self.scores,
                  'max_score': self._max_score,
-                 'attempts': self.attempts
+                 'attempts': self.attempts,
         }
         return json.dumps(state)
 

common/lib/xmodule/xmodule/tests/__init__.py

@@ -4,7 +4,7 @@ unittests for xmodule
 Run like this:
 
     rake test_common/lib/xmodule
-    
+
 """
 
 import unittest
@@ -19,11 +19,12 @@ import xmodule
 from xmodule.x_module import ModuleSystem
 from mock import Mock
 
-i4xs = ModuleSystem(
+test_system = ModuleSystem(
     ajax_url='courses/course_id/modx/a_location',
     track_function=Mock(),
     get_module=Mock(),
-    render_template=Mock(),
+    # "render" to just the context...
+    render_template=lambda template, context: str(context),
     replace_urls=Mock(),
     user=Mock(),
     filestore=Mock(),

common/lib/xmodule/xmodule/tests/test_progress.py

@@ -5,7 +5,7 @@ import unittest
 from xmodule.progress import Progress
 from xmodule import x_module
 
-from . import i4xs
+from . import test_system
 
 class ProgressTest(unittest.TestCase):
     ''' Test that basic Progress objects work.  A Progress represents a
@@ -133,6 +133,6 @@ class ModuleProgressTest(unittest.TestCase):
     '''
     def test_xmodule_default(self):
         '''Make sure default get_progress exists, returns None'''
-        xm = x_module.XModule(i4xs, 'a://b/c/d/e', None, {})
+        xm = x_module.XModule(test_system, 'a://b/c/d/e', None, {})
         p = xm.get_progress()
         self.assertEqual(p, None)

common/lib/xmodule/xmodule/tests/test_self_assessment.py

+import json
+from mock import Mock
+import unittest
+
+from xmodule.self_assessment_module import SelfAssessmentModule
+from xmodule.modulestore import Location
+
+from . import test_system
+
+class SelfAssessmentTest(unittest.TestCase):
+
+    definition = {'rubric': 'A rubric',
+                  'prompt': 'Who?',
+                  'submitmessage': 'Shall we submit now?',
+                  'hintprompt': 'Consider this...',
+                  }
+
+    location = Location(["i4x", "edX", "sa_test", "selfassessment",
+                         "SampleQuestion"])
+
+    metadata = {'attempts': '10'}
+
+    descriptor = Mock()
+
+    def test_import(self):
+        state = json.dumps({'student_answers': ["Answer 1", "answer 2", "answer 3"],
+                            'scores': [0, 1],
+                            'hints': ['o hai'],
+                            'state': SelfAssessmentModule.ASSESSING,
+                            'attempts': 2})
+
+        module = SelfAssessmentModule(test_system, self.location,
+                                      self.definition, self.descriptor,
+                                      state, {}, metadata=self.metadata)
+
+        self.assertEqual(module.get_score()['score'], 0)
+
+        self.assertTrue('answer 3' in module.get_html())
+        self.assertFalse('answer 2' in module.get_html())
+
+        module.save_assessment({'assessment': '0'})
+        self.assertEqual(module.state, module.REQUEST_HINT)
+
+        module.save_hint({'hint': 'hint for ans 3'})
+        self.assertEqual(module.state, module.DONE)
+
+        d = module.reset({})
+        self.assertTrue(d['success'])
+        self.assertEqual(module.state, module.INITIAL)
+
+        # if we now assess as right, skip the REQUEST_HINT state
+        module.save_answer({'student_answer': 'answer 4'})
+        module.save_assessment({'assessment': '1'})
+        self.assertEqual(module.state, module.DONE)

common/lib/xmodule/xmodule/x_module.py

@@ -233,17 +233,17 @@ class XModule(HTMLSnippet):
             self._loaded_children = [c for c in children if c is not None]
 
         return self._loaded_children
-    
+
     def get_children_locations(self):
         '''
         Returns the locations of each of child modules.
-        
+
         Overriding this changes the behavior of get_children and
         anything that uses get_children, such as get_display_items.
-        
+
         This method will not instantiate the modules of the children
         unless absolutely necessary, so it is cheaper to call than get_children
-        
+
         These children will be the same children returned by the
         descriptor unless descriptor.has_dynamic_children() is true.
         '''
@@ -288,8 +288,20 @@ class XModule(HTMLSnippet):
         return '{}'
 
     def get_score(self):
-        ''' Score the student received on the problem.
-        '''
+        """
+        Score the student received on the problem, or None if there is no
+        score.
+
+        Returns:
+          dictionary
+             {'score': integer, from 0 to get_max_score(),
+              'total': get_max_score()}
+
+          NOTE (vshnayder): not sure if this was the intended return value, but
+          that's what it's doing now.  I suspect that we really want it to just
+          return a number.  Would need to change (at least) capa and
+          modx_dispatch to match if we did that.
+        """
         return None
 
     def max_score(self):
@@ -319,7 +331,7 @@ class XModule(HTMLSnippet):
             get is a dictionary-like object '''
         return ""
 
-    # cdodge: added to support dynamic substitutions of 
+    # cdodge: added to support dynamic substitutions of
     # links for courseware assets (e.g. images). <link> is passed through from lxml.html parser
     def rewrite_content_links(self, link):
         # see if we start with our format, e.g. 'xasset:<filename>'
@@ -408,7 +420,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates):
     # cdodge: this is a list of metadata names which are 'system' metadata
     # and should not be edited by an end-user
     system_metadata_fields = [ 'data_dir' ]
-    
+
     # A list of descriptor attributes that must be equal for the descriptors to
     # be equal
     equality_attributes = ('definition', 'metadata', 'location',
@@ -562,18 +574,18 @@ class XModuleDescriptor(Plugin, HTMLSnippet, ResourceTemplates):
             self,
             metadata=self.metadata
         )
-    
-    
+
+
     def has_dynamic_children(self):
         """
         Returns True if this descriptor has dynamic children for a given
         student when the module is created.
-        
+
         Returns False if the children of this descriptor are the same
-        children that the module will return for any student. 
+        children that the module will return for any student.
         """
         return False
-        
+
 
     # ================================= JSON PARSING ===========================
     @staticmethod