refactor CAPA to use scorable xblock mixin

for TNL-6594

common/lib/xmodule/xmodule/capa_base.py

@@ -24,6 +24,7 @@ from capa.inputtypes import Status
 from capa.responsetypes import StudentInputError, ResponseError, LoncapaProblemError
 from capa.util import convert_files_to_filenames, get_inner_html_from_xpath
 from xblock.fields import Boolean, Dict, Float, Integer, Scope, String, XMLString
+from xblock.scorable import ScorableXBlockMixin, Score
 from xmodule.capa_base_constants import RANDOMIZATION, SHOWANSWER, SHOW_CORRECTNESS
 from xmodule.exceptions import NotFoundError
 from .fields import Date, Timedelta
@@ -219,7 +220,7 @@ class CapaFields(object):
     )
 
 
-class CapaMixin(CapaFields):
+class CapaMixin(ScorableXBlockMixin, CapaFields):
     """
         Core logic for Capa Problem, which can be used by XModules or XBlocks.
     """
@@ -290,6 +291,8 @@ class CapaMixin(CapaFields):
 
             self.set_state_from_lcp()
 
+        self.set_score(self.score_from_lcp())
+
         assert self.seed is not None
 
     def choose_new_seed(self):
@@ -372,32 +375,28 @@ class CapaMixin(CapaFields):
         """
         self.last_submission_time = datetime.datetime.now(utc)
 
-    def get_score(self):
-        """
-        Access the problem's score
-        """
-        return self.lcp.get_score()
-
     def get_progress(self):
         """
-        For now, just return score / max_score
+        For now, just return weighted earned / weighted possible
         """
-        score_dict = self.get_score()
-        score = score_dict['score']
-        total = score_dict['total']
+        score = self.get_score()
+        raw_earned = score.raw_earned
+        raw_possible = score.raw_possible
 
-        if total > 0:
+        if raw_possible > 0:
             if self.weight is not None:
                 # Progress objects expect total > 0
                 if self.weight == 0:
                     return None
 
                 # scale score and total by weight/total:
-                score = score * self.weight / total
-                total = self.weight
-
+                weighted_earned = raw_earned * self.weight / raw_possible
+                weighted_possible = self.weight
+            else:
+                weighted_earned = raw_earned
+                weighted_possible = raw_possible
             try:
-                return Progress(score, total)
+                return Progress(weighted_earned, weighted_possible)
             except (TypeError, ValueError):
                 log.exception("Got bad progress")
                 return None
@@ -572,7 +571,7 @@ class CapaMixin(CapaFields):
             # Next, generate a fresh LoncapaProblem
             self.lcp = self.new_lcp(None)
             self.set_state_from_lcp()
-
+            self.set_score(self.score_from_lcp())
             # Prepend a scary warning to the student
             _ = self.runtime.service(self, "i18n").ugettext
             warning_msg = _("Warning: The problem has been reset to its initial state!")
@@ -879,8 +878,7 @@ class CapaMixin(CapaFields):
         """
         True iff full points
         """
-        score_dict = self.get_score()
-        return score_dict['score'] == score_dict['total']
+        return self.score.raw_earned == self.score.raw_possible
 
     def answer_available(self):
         """
@@ -949,6 +947,7 @@ class CapaMixin(CapaFields):
         score_msg = data['xqueue_body']
         self.lcp.update_score(score_msg, queuekey)
         self.set_state_from_lcp()
+        self.set_score(self.score_from_lcp())
         self.publish_grade()
 
         return dict()  # No AJAX return is needed
@@ -1132,18 +1131,17 @@ class CapaMixin(CapaFields):
         """
         Publishes the student's current grade to the system as an event
         """
-        score = self.lcp.get_score()
         self.runtime.publish(
             self,
             'grade',
             {
-                'value': score['score'],
-                'max_value': score['total'],
+                'value': self.score.raw_earned,
+                'max_value': self.score.raw_possible,
                 'only_if_higher': only_if_higher,
             }
         )
 
-        return {'grade': score['score'], 'max_grade': score['total']}
+        return {'grade': self.score.raw_earned, 'max_grade': self.score.raw_possible}
 
     # pylint: disable=too-many-statements
     def submit_problem(self, data, override_time=False):
@@ -1216,6 +1214,7 @@ class CapaMixin(CapaFields):
             self.attempts = self.attempts + 1
             self.lcp.done = True
             self.set_state_from_lcp()
+            self.set_score(self.score_from_lcp())
             self.set_last_submission_time()
 
         except (StudentInputError, ResponseError, LoncapaProblemError) as inst:
@@ -1227,6 +1226,7 @@ class CapaMixin(CapaFields):
 
             # Save the user's state before failing
             self.set_state_from_lcp()
+            self.set_score(self.score_from_lcp())
 
             # If the user is a staff member, include
             # the full exception, including traceback,
@@ -1245,6 +1245,7 @@ class CapaMixin(CapaFields):
         except Exception as err:
             # Save the user's state before failing
             self.set_state_from_lcp()
+            self.set_score(self.score_from_lcp())
 
             if self.runtime.DEBUG:
                 msg = u"Error checking problem: {}".format(err.message)
@@ -1457,93 +1458,6 @@ class CapaMixin(CapaFields):
 
         return input_metadata
 
-    def rescore_problem(self, only_if_higher):
-        """
-        Checks whether the existing answers to a problem are correct.
-
-        This is called when the correct answer to a problem has been changed,
-        and the grade should be re-evaluated.
-
-        If only_if_higher is True, the answer and grade are updated
-        only if the resulting score is higher than before.
-
-        Returns a dict with one key:
-            {'success' : 'correct' | 'incorrect' | AJAX alert msg string }
-
-        Raises NotFoundError if called on a problem that has not yet been
-        answered, or NotImplementedError if it's a problem that cannot be rescored.
-
-        Returns the error messages for exceptions occurring while performing
-        the rescoring, rather than throwing them.
-        """
-        event_info = {'state': self.lcp.get_state(), 'problem_id': self.location.to_deprecated_string()}
-
-        _ = self.runtime.service(self, "i18n").ugettext
-
-        if not self.lcp.supports_rescoring():
-            event_info['failure'] = 'unsupported'
-            self.track_function_unmask('problem_rescore_fail', event_info)
-            # pylint: disable=line-too-long
-            # Translators: 'rescoring' refers to the act of re-submitting a student's solution so it can get a new score.
-            raise NotImplementedError(_("Problem's definition does not support rescoring."))
-            # pylint: enable=line-too-long
-
-        if not self.done:
-            event_info['failure'] = 'unanswered'
-            self.track_function_unmask('problem_rescore_fail', event_info)
-            raise NotFoundError(_("Problem must be answered before it can be graded again."))
-
-        # get old score, for comparison:
-        orig_score = self.lcp.get_score()
-        event_info['orig_score'] = orig_score['score']
-        event_info['orig_total'] = orig_score['total']
-
-        try:
-            correct_map = self.lcp.rescore_existing_answers()
-
-        except (StudentInputError, ResponseError, LoncapaProblemError) as inst:
-            log.warning("Input error in capa_module:problem_rescore", exc_info=True)
-            event_info['failure'] = 'input_error'
-            self.track_function_unmask('problem_rescore_fail', event_info)
-            return {'success': u"Error: {0}".format(inst.message)}
-
-        except Exception as err:
-            event_info['failure'] = 'unexpected'
-            self.track_function_unmask('problem_rescore_fail', event_info)
-            if self.runtime.DEBUG:
-                msg = u"Error checking problem: {0}".format(err.message)
-                msg += u'\nTraceback:\n' + traceback.format_exc()
-                return {'success': msg}
-            raise
-
-        # rescoring should have no effect on attempts, so don't
-        # need to increment here, or mark done.  Just save.
-        self.set_state_from_lcp()
-        self.publish_grade(only_if_higher)
-
-        new_score = self.lcp.get_score()
-        event_info['new_score'] = new_score['score']
-        event_info['new_total'] = new_score['total']
-
-        # success = correct if ALL questions in this problem are correct
-        success = 'correct'
-        for answer_id in correct_map:
-            if not correct_map.is_correct(answer_id):
-                success = 'incorrect'
-
-        # NOTE: We are logging both full grading and queued-grading submissions. In the latter,
-        #       'success' will always be incorrect
-        event_info['correct_map'] = correct_map.get_dict()
-        event_info['success'] = success
-        event_info['attempts'] = self.attempts
-        self.track_function_unmask('problem_rescore', event_info)
-
-        return {
-            'success': success,
-            'new_raw_earned': new_score['score'],
-            'new_raw_possible': new_score['total'],
-        }
-
     def save_problem(self, data):
         """
         Save the passed in answers.
@@ -1584,6 +1498,7 @@ class CapaMixin(CapaFields):
         self.lcp.has_saved_answers = True
 
         self.set_state_from_lcp()
+        self.set_score(self.score_from_lcp())
 
         self.track_function_unmask('save_problem_success', event_info)
         msg = _("Your answers have been saved.")
@@ -1642,6 +1557,7 @@ class CapaMixin(CapaFields):
 
         # Pull in the new problem seed
         self.set_state_from_lcp()
+        self.set_score(self.score_from_lcp())
 
         # Grade may have changed, so publish new value
         self.publish_grade()
@@ -1653,3 +1569,116 @@ class CapaMixin(CapaFields):
             'success': True,
             'html': self.get_problem_html(encapsulate=False),
         }
+
+    # ScorableXBlockMixin methods
+
+    def rescore(self, only_if_higher=False):
+        """
+        Checks whether the existing answers to a problem are correct.
+
+        This is called when the correct answer to a problem has been changed,
+        and the grade should be re-evaluated.
+
+        If only_if_higher is True, the answer and grade are updated
+        only if the resulting score is higher than before.
+
+        Returns a dict with one key:
+            {'success' : 'correct' | 'incorrect' | AJAX alert msg string }
+
+        Raises NotFoundError if called on a problem that has not yet been
+        answered, or NotImplementedError if it's a problem that cannot be rescored.
+
+        Returns the error messages for exceptions occurring while performing
+        the rescoring, rather than throwing them.
+        """
+        event_info = {'state': self.lcp.get_state(), 'problem_id': self.location.to_deprecated_string()}
+
+        _ = self.runtime.service(self, "i18n").ugettext
+
+        if not self.lcp.supports_rescoring():
+            event_info['failure'] = 'unsupported'
+            self.track_function_unmask('problem_rescore_fail', event_info)
+            # pylint: disable=line-too-long
+            # Translators: 'rescoring' refers to the act of re-submitting a student's solution so it can get a new score.
+            raise NotImplementedError(_("Problem's definition does not support rescoring."))
+            # pylint: enable=line-too-long
+
+        if not self.done:
+            event_info['failure'] = 'unanswered'
+            self.track_function_unmask('problem_rescore_fail', event_info)
+            raise NotFoundError(_("Problem must be answered before it can be graded again."))
+
+        # get old score, for comparison:
+        orig_score = self.get_score()
+        event_info['orig_score'] = orig_score.raw_earned
+        event_info['orig_total'] = orig_score.raw_possible
+
+        try:
+            calculated_score = self.calculate_score()
+
+        except (StudentInputError, ResponseError, LoncapaProblemError) as inst:
+            log.warning("Input error in capa_module:problem_rescore", exc_info=True)
+            event_info['failure'] = 'input_error'
+            self.track_function_unmask('problem_rescore_fail', event_info)
+            raise
+
+        except Exception:
+            event_info['failure'] = 'unexpected'
+            self.track_function_unmask('problem_rescore_fail', event_info)
+            raise
+
+        # rescoring should have no effect on attempts, so don't
+        # need to increment here, or mark done.  Just save.
+        self.set_state_from_lcp()
+        self.set_score(calculated_score)
+        self.publish_grade(only_if_higher)
+
+        event_info['new_score'] = calculated_score.raw_earned
+        event_info['new_total'] = calculated_score.raw_possible
+
+        # success = correct if ALL questions in this problem are correct
+        success = 'correct'
+        for answer_id in self.lcp.correct_map:
+            if not self.lcp.correct_map.is_correct(answer_id):
+                success = 'incorrect'
+
+        # NOTE: We are logging both full grading and queued-grading submissions. In the latter,
+        #       'success' will always be incorrect
+        event_info['correct_map'] = self.lcp.correct_map.get_dict()
+        event_info['success'] = success
+        event_info['attempts'] = self.attempts
+        self.track_function_unmask('problem_rescore', event_info)
+
+    def has_submitted_answer(self):
+        return self.done
+
+    def set_score(self, score):
+        """
+        Sets the internal score for the problem. This is not derived directly
+        from the internal LCP in keeping with the ScorableXBlock spec.
+        """
+        self.score = score
+
+    def get_score(self):
+        """
+        Returns the score currently set on the block.
+        """
+        return self.score
+
+    def calculate_score(self):
+        """
+        Returns the score calculated from the current problem state.
+        Operates by creating a new correctness map based on the current
+        state of the LCP, and having the LCP generate a score from that.
+        """
+        new_correctness = self.lcp.get_grade_from_current_answers(None)
+        new_score = self.lcp.calculate_score(new_correctness)
+        return Score(raw_earned=new_score['score'], raw_possible=new_score['total'])
+
+    def score_from_lcp(self):
+        """
+        Returns the score associated with the correctness map
+        currently stored by the LCP.
+        """
+        lcp_score = self.lcp.calculate_score()
+        return Score(raw_earned=lcp_score['score'], raw_possible=lcp_score['total'])

common/lib/xmodule/xmodule/capa_module.py

@@ -324,6 +324,7 @@ class CapaDescriptor(CapaFields, RawDescriptor):
     hint_button = module_attr('hint_button')
     handle_problem_html_error = module_attr('handle_problem_html_error')
     handle_ungraded_response = module_attr('handle_ungraded_response')
+    has_submitted_answer = module_attr('has_submitted_answer')
     is_attempted = module_attr('is_attempted')
     is_correct = module_attr('is_correct')
     is_past_due = module_attr('is_past_due')
@@ -332,7 +333,7 @@ class CapaDescriptor(CapaFields, RawDescriptor):
     make_dict_of_responses = module_attr('make_dict_of_responses')
     new_lcp = module_attr('new_lcp')
     publish_grade = module_attr('publish_grade')
-    rescore_problem = module_attr('rescore_problem')
+    rescore = module_attr('rescore')
     reset_problem = module_attr('reset_problem')
     save_problem = module_attr('save_problem')
     set_state_from_lcp = module_attr('set_state_from_lcp')

common/lib/xmodule/xmodule/tests/test_capa_module.py

@@ -29,6 +29,7 @@ from xmodule.capa_module import CapaModule, CapaDescriptor, ComplexEncoder
 from opaque_keys.edx.locations import Location
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
+from xblock.scorable import Score
 
 from . import get_test_system
 from pytz import UTC
@@ -130,9 +131,9 @@ class CapaFactory(object):
         if override_get_score:
             if correct:
                 # TODO: probably better to actually set the internal state properly, but...
-                module.get_score = lambda: {'score': 1, 'total': 1}
+                module.score = Score(raw_earned=1, raw_possible=1)
             else:
-                module.get_score = lambda: {'score': 0, 'total': 1}
+                module.score = Score(raw_earned=0, raw_possible=1)
 
         module.graded = 'False'
         return module
@@ -196,10 +197,10 @@ class CapaModuleTest(unittest.TestCase):
 
     def test_import(self):
         module = CapaFactory.create()
-        self.assertEqual(module.get_score()['score'], 0)
+        self.assertEqual(module.get_score().raw_earned, 0)
 
         other_module = CapaFactory.create()
-        self.assertEqual(module.get_score()['score'], 0)
+        self.assertEqual(module.get_score().raw_earned, 0)
         self.assertNotEqual(module.url_name, other_module.url_name,
                             "Factory should be creating unique names for each problem")
 
@@ -208,31 +209,33 @@ class CapaModuleTest(unittest.TestCase):
         Check that the factory creates correct and incorrect problems properly.
         """
         module = CapaFactory.create()
-        self.assertEqual(module.get_score()['score'], 0)
+        self.assertEqual(module.get_score().raw_earned, 0)
 
         other_module = CapaFactory.create(correct=True)
-        self.assertEqual(other_module.get_score()['score'], 1)
+        self.assertEqual(other_module.get_score().raw_earned, 1)
 
     def test_get_score(self):
         """
-        Do 1 test where the internals of get_score are properly set
-
-        @jbau Note: this obviously depends on a particular implementation of get_score, but I think this is actually
-        useful as unit-code coverage for this current implementation.  I don't see a layer where LoncapaProblem
-        is tested directly
+        Tests the internals of get_score. In keeping with the ScorableXBlock spec,
+        Capa modules store their score independently of the LCP internals, so it must
+        be explicitly updated.
         """
         student_answers = {'1_2_1': 'abcd'}
         correct_map = CorrectMap(answer_id='1_2_1', correctness="correct", npoints=0.9)
         module = CapaFactory.create(correct=True, override_get_score=False)
         module.lcp.correct_map = correct_map
         module.lcp.student_answers = student_answers
-        self.assertEqual(module.get_score()['score'], 0.9)
+        self.assertEqual(module.get_score().raw_earned, 0.0)
+        module.set_score(module.score_from_lcp())
+        self.assertEqual(module.get_score().raw_earned, 0.9)
 
         other_correct_map = CorrectMap(answer_id='1_2_1', correctness="incorrect", npoints=0.1)
         other_module = CapaFactory.create(correct=False, override_get_score=False)
         other_module.lcp.correct_map = other_correct_map
         other_module.lcp.student_answers = student_answers
-        self.assertEqual(other_module.get_score()['score'], 0.1)
+        self.assertEqual(other_module.get_score().raw_earned, 0.0)
+        other_module.set_score(other_module.score_from_lcp())
+        self.assertEqual(other_module.get_score().raw_earned, 0.1)
 
     def test_showanswer_default(self):
         """
@@ -1007,14 +1010,15 @@ class CapaModuleTest(unittest.TestCase):
         # Simulate that all answers are marked correct, no matter
         # what the input is, by patching LoncapaResponse.evaluate_answers()
         with patch('capa.responsetypes.LoncapaResponse.evaluate_answers') as mock_evaluate_answers:
-            mock_evaluate_answers.return_value = CorrectMap(CapaFactory.answer_key(), 'correct')
-            result = module.rescore_problem(only_if_higher=False)
+            mock_evaluate_answers.return_value = CorrectMap(
+                answer_id=CapaFactory.answer_key(),
+                correctness='correct',
+                npoints=1,
+            )
+            module.rescore(only_if_higher=False)
 
         # Expect that the problem is marked correct
-        self.assertEqual(result['success'], 'correct')
-
-        # Expect that we get no HTML
-        self.assertNotIn('contents', result)
+        self.assertEqual(module.is_correct(), True)
 
         # Expect that the number of attempts is not incremented
         self.assertEqual(module.attempts, 1)
@@ -1028,10 +1032,10 @@ class CapaModuleTest(unittest.TestCase):
         # what the input is, by patching LoncapaResponse.evaluate_answers()
         with patch('capa.responsetypes.LoncapaResponse.evaluate_answers') as mock_evaluate_answers:
             mock_evaluate_answers.return_value = CorrectMap(CapaFactory.answer_key(), 'incorrect')
-            result = module.rescore_problem(only_if_higher=False)
+            module.rescore(only_if_higher=False)
 
         # Expect that the problem is marked incorrect
-        self.assertEqual(result['success'], 'incorrect')
+        self.assertEqual(module.is_correct(), False)
 
         # Expect that the number of attempts is not incremented
         self.assertEqual(module.attempts, 0)
@@ -1042,7 +1046,7 @@ class CapaModuleTest(unittest.TestCase):
 
         # Try to rescore the problem, and get exception
         with self.assertRaises(xmodule.exceptions.NotFoundError):
-            module.rescore_problem(only_if_higher=False)
+            module.rescore(only_if_higher=False)
 
     def test_rescore_problem_not_supported(self):
         module = CapaFactory.create(done=True)
@@ -1051,7 +1055,7 @@ class CapaModuleTest(unittest.TestCase):
         with patch('capa.capa_problem.LoncapaProblem.supports_rescoring') as mock_supports_rescoring:
             mock_supports_rescoring.return_value = False
             with self.assertRaises(NotImplementedError):
-                module.rescore_problem(only_if_higher=False)
+                module.rescore(only_if_higher=False)
 
     def _rescore_problem_error_helper(self, exception_class):
         """Helper to allow testing all errors that rescoring might return."""
@@ -1059,13 +1063,10 @@ class CapaModuleTest(unittest.TestCase):
         module = CapaFactory.create(attempts=1, done=True)
 
         # Simulate answering a problem that raises the exception
-        with patch('capa.capa_problem.LoncapaProblem.rescore_existing_answers') as mock_rescore:
+        with patch('capa.capa_problem.LoncapaProblem.get_grade_from_current_answers') as mock_rescore:
             mock_rescore.side_effect = exception_class(u'test error \u03a9')
-            result = module.rescore_problem(only_if_higher=False)
-
-        # Expect an AJAX alert message in 'success'
-        expected_msg = u'Error: test error \u03a9'
-        self.assertEqual(result['success'], expected_msg)
+            with self.assertRaises(exception_class):
+                module.rescore(only_if_higher=False)
 
         # Expect that the number of attempts is NOT incremented
         self.assertEqual(module.attempts, 1)

common/lib/xmodule/xmodule/tests/test_delay_between_attempts.py

@@ -18,6 +18,7 @@ from xmodule.capa_module import CapaModule
 from opaque_keys.edx.locations import Location
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
+from xblock.scorable import Score
 
 from . import get_test_system
 from pytz import UTC
@@ -111,9 +112,9 @@ class CapaFactoryWithDelay(object):
 
         if correct:
             # Could set the internal state formally, but here we just jam in the score.
-            module.get_score = lambda: {'score': 1, 'total': 1}
+            module.score = Score(raw_earned=1, raw_possible=1)
         else:
-            module.get_score = lambda: {'score': 0, 'total': 1}
+            module.score = Score(raw_earned=0, raw_possible=1)
 
         return module