Merge pull request #824 from edx/fix/vik/oe-progress

Properly display student progress

common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py

@@ -5,11 +5,11 @@ from xmodule.timeinfo import TimeInfo
 from xmodule.capa_module import ComplexEncoder
 from xmodule.progress import Progress
 from xmodule.stringify import stringify_children
-import self_assessment_module
-import open_ended_module
+from  xmodule.open_ended_grading_classes import self_assessment_module
+from  xmodule.open_ended_grading_classes import open_ended_module
 from functools import partial
 from .combined_open_ended_rubric import CombinedOpenEndedRubric, GRADER_TYPE_IMAGE_DICT, HUMAN_GRADER_TYPE, LEGEND_LIST
-from peer_grading_service import PeerGradingService, MockPeerGradingService, GradingServiceError
+from xmodule.open_ended_grading_classes.peer_grading_service import PeerGradingService, MockPeerGradingService, GradingServiceError
 
 log = logging.getLogger("mitx.courseware")
 
@@ -547,6 +547,11 @@ class CombinedOpenEndedV1Module():
         return last_response_dict
 
     def extract_human_name_from_task(self, task_xml):
+        """
+        Given the xml for a task, pull out the human name for it.
+        Input: xml string
+        Output: a human readable task name (ie Self Assessment)
+        """
         tree = etree.fromstring(task_xml)
         payload = tree.xpath("/openended/openendedparam/grader_payload")
         if len(payload) == 0:
@@ -644,7 +649,13 @@ class CombinedOpenEndedV1Module():
         all_responses = []
         success, can_see_rubric, error = self.check_if_student_has_done_needed_grading()
         if not can_see_rubric:
-            return {'html' : self.system.render_template('{0}/combined_open_ended_hidden_results.html'.format(self.TEMPLATE_DIR), {'error' : error}), 'success' : True, 'hide_reset' : True}
+            return {
+                'html': self.system.render_template(
+                    '{0}/combined_open_ended_hidden_results.html'.format(self.TEMPLATE_DIR),
+                    {'error': error}),
+                'success': True,
+                'hide_reset': True
+            }
 
         contexts = []
         rubric_number = self.current_task_number
@@ -717,6 +728,9 @@ class CombinedOpenEndedV1Module():
         return json.dumps(d, cls=ComplexEncoder)
 
     def get_current_state(self, data):
+        """
+        Gets the current state of the module.
+        """
         return self.get_context()
 
     def get_last_response_ajax(self, data):
@@ -823,6 +837,16 @@ class CombinedOpenEndedV1Module():
         """
         return (self.state == self.DONE or self.ready_to_reset) and self.is_scored
 
+    def get_weight(self):
+        """
+        Return the weight of the problem.  The old default weight was None, so set to 1 in that case.
+        Output - int weight
+        """
+        weight = self.weight
+        if weight is None:
+            weight = 1
+        return weight
+
     def get_score(self):
         """
         Score the student received on the problem, or None if there is no
@@ -837,9 +861,7 @@ class CombinedOpenEndedV1Module():
         score = None
 
         #The old default was None, so set to 1 if it is the old default weight
-        weight = self.weight
-        if weight is None:
-            weight = 1
+        weight = self.get_weight()
         if self.is_scored:
             # Finds the maximum score of all student attempts and keeps it.
             score_mat = []
@@ -858,7 +880,6 @@ class CombinedOpenEndedV1Module():
             if len(score_mat) > 0:
                 # Currently, assume that the final step is the correct one, and that those are the final scores.
                 # This will change in the future, which is why the machinery above exists to extract all scores on all steps
-                # TODO: better final score handling.
                 scores = score_mat[-1]
                 score = max(scores)
             else:
@@ -879,27 +900,35 @@ class CombinedOpenEndedV1Module():
         return score_dict
 
     def max_score(self):
-        ''' Maximum score. Two notes:
-
-            * This is generic; in abstract, a problem could be 3/5 points on one
-              randomization, and 5/7 on another
-        '''
+        """
+        Maximum score possible in this module.  Returns the max score if finished, None if not.
+        """
         max_score = None
         if self.check_if_done_and_scored():
             max_score = self._max_score
         return max_score
 
     def get_progress(self):
-        ''' Return a progress.Progress object that represents how far the
+        """
+        Generate a progress object. Progress objects represent how far the
         student has gone in this module.  Must be implemented to get correct
-        progress tracking behavior in nesting modules like sequence and
-        vertical.
+        progress tracking behavior in nested modules like sequence and
+        vertical.  This behavior is consistent with capa.
+
+        If the module is unscored, return None (consistent with capa).
+        """
 
-        If this module has no notion of progress, return None.
-        '''
-        progress_object = Progress(self.current_task_number, len(self.task_xml))
+        d = self.get_score()
+
+        if d['total'] > 0 and self.is_scored:
+
+            try:
+                return Progress(d['score'], d['total'])
+            except (TypeError, ValueError):
+                log.exception("Got bad progress")
+                return None
 
-        return progress_object
+        return None
 
     def out_of_sync_error(self, data, msg=''):
         """

common/lib/xmodule/xmodule/tests/test_combined_open_ended.py

@@ -22,6 +22,7 @@ from xmodule.open_ended_grading_classes.grading_service_module import GradingSer
 from xmodule.combined_open_ended_module import CombinedOpenEndedModule
 from xmodule.modulestore import Location
 from xmodule.tests import get_test_system, test_util_open_ended
+from xmodule.progress import Progress
 from xmodule.tests.test_util_open_ended import (MockQueryDict, DummyModulestore, TEST_STATE_SA_IN,
     MOCK_INSTANCE_STATE, TEST_STATE_SA, TEST_STATE_AI, TEST_STATE_AI2, TEST_STATE_AI2_INVALID,
     TEST_STATE_SINGLE, TEST_STATE_PE_SINGLE)
@@ -480,6 +481,29 @@ class CombinedOpenEndedModuleTest(unittest.TestCase):
         max_score = self.combinedoe_container.max_score()
         self.assertEqual(max_score, None)
 
+    def test_container_get_progress(self):
+        """
+        See if we can get the progress from the actual xmodule
+        """
+        progress = self.combinedoe_container.max_score()
+        self.assertEqual(progress, None)
+
+    def test_get_progress(self):
+        """
+        Test if we can get the correct progress from the combined open ended class
+        """
+        self.combinedoe.update_task_states()
+        self.combinedoe.state = "done"
+        self.combinedoe.is_scored = True
+        progress = self.combinedoe.get_progress()
+        self.assertIsInstance(progress, Progress)
+
+        # progress._a is the score of the xmodule, which is 0 right now.
+        self.assertEqual(progress._a, 0)
+
+        # progress._b is the max_score (which is 1), divided by the weight (which is 1).
+        self.assertEqual(progress._b, 1)
+
     def test_container_weight(self):
         """
         Check the problem weight in the container