Staff assessments and workflow status

Staff assessments can now mark workflow as being done, with proper handling
for blocking workflows (such as peer), to hide a score if the submitter
has not yet fulfilled their obligations. Includes tests.

Also contains a tiny bugfix for a previously unexecuted codepath.

openassessment/assessment/models/base.py

@@ -573,7 +573,7 @@ class Assessment(models.Model):
         """
         assessments = list(assessments)  # Force us to read it all
         if not assessments:
-            return []
+            return {}
 
         # Generate a cache key that represents all the assessments we're being
         # asked to grab scores from (comma separated list of assessment IDs)

openassessment/assessment/test/test_staff.py

@@ -46,7 +46,7 @@ class TestStaffAssessment(CacheResetTest):
 
     @staticmethod
     def _peer_assess(sub, scorer_id, scores):
-        bob_sub, bob = TestStaffAssessment._create_student_and_submission("Bob", "Bob's answer", override_steps=['peer'])
+        bob_sub, bob = TestStaffAssessment._create_student_and_submission("Bob", "Bob's answer", problem_steps=['peer'])
         peer_api.get_submission_to_assess(bob_sub["uuid"], 1)
         return peer_assess(bob_sub["uuid"], bob["student_id"], scores, dict(), "", RUBRIC, 1)
 
@@ -57,6 +57,19 @@ class TestStaffAssessment(CacheResetTest):
         ('ai', lambda sub, scorer_id, scores: TestStaffAssessment._ai_assess(sub))
     ]
 
+    def _verify_done_state(self, uuid, requirements, expect_done=True):
+        """
+        Asserts that a submision and workflow are (or aren't) set to status "done".
+        A False value for expect_done will confirm an assessment/workflow are NOT done.
+        """
+        workflow = workflow_api.get_workflow_for_submission(uuid, requirements)
+        if expect_done:
+            self.assertTrue(staff_api.assessment_is_finished(uuid, requirements))
+            self.assertEqual(workflow["status"], "done")
+        else:
+            self.assertFalse(staff_api.assessment_is_finished(uuid, requirements))
+            self.assertNotEqual(workflow["status"], "done")
+
     @data(*ASSESSMENT_SCORES_DDT)
     def test_create_assessment_not_required(self, key):
         """
@@ -78,8 +91,8 @@ class TestStaffAssessment(CacheResetTest):
         self.assertEqual(assessment["points_earned"], OPTIONS_SELECTED_DICT[key]["expected_points"])
         self.assertEqual(assessment["points_possible"], RUBRIC_POSSIBLE_POINTS)
 
-        # ensure submission is marked as finished
-        self.assertTrue(staff_api.assessment_is_finished(tim_sub["uuid"], self.STEP_REQUIREMENTS))
+        # Ensure submission and workflow are marked as finished
+        self._verify_done_state(tim_sub["uuid"], self.STEP_REQUIREMENTS)
 
     @data(*ASSESSMENT_SCORES_DDT)
     def test_create_assessment_required(self, key):
@@ -88,10 +101,10 @@ class TestStaffAssessment(CacheResetTest):
         when staff scores are required.
         """
         # Create assessment
-        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", override_steps=['staff'])
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", problem_steps=['staff'])
 
         # Verify that we're still waiting on a staff assessment
-        self.assertFalse(staff_api.assessment_is_finished(tim_sub["uuid"], self.STEP_REQUIREMENTS_WITH_STAFF))
+        self._verify_done_state(tim_sub["uuid"], self.STEP_REQUIREMENTS_WITH_STAFF, expect_done=False)
 
         # Staff assess
         staff_assessment = staff_api.create_assessment(
@@ -103,7 +116,7 @@ class TestStaffAssessment(CacheResetTest):
 
         # Verify assesment made, score updated, and no longer waiting
         self.assertEqual(staff_assessment["points_earned"], OPTIONS_SELECTED_DICT[key]["expected_points"])
-        self.assertTrue(staff_api.assessment_is_finished(tim_sub["uuid"], self.STEP_REQUIREMENTS_WITH_STAFF))
+        self._verify_done_state(tim_sub["uuid"], self.STEP_REQUIREMENTS_WITH_STAFF)
 
     @data(*ASSESSMENT_SCORES_DDT)
     def test_create_assessment_score_overrides(self, key):
@@ -118,7 +131,7 @@ class TestStaffAssessment(CacheResetTest):
             initial_assessment = OPTIONS_SELECTED_DICT["none"]
 
         # Create assessment
-        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer")
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", problem_steps=['self'])
 
         # Self assess it
         self_assessment = self_assess(
@@ -157,7 +170,7 @@ class TestStaffAssessment(CacheResetTest):
         initial_assessment = OPTIONS_SELECTED_DICT["most"]
 
         # Create assessment
-        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", override_steps=[initial_type])
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", problem_steps=[initial_type])
 
         # Initially assess it
         assessment = initial_assess(tim_sub["uuid"], tim["student_id"], initial_assessment["options"])
@@ -182,7 +195,7 @@ class TestStaffAssessment(CacheResetTest):
 
         # Verify both assessment and workflow report correct score
         self.assertEqual(staff_assessment["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
-        workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], self.STEP_REQUIREMENTS)
+        workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], requirements)
         self.assertEqual(workflow["score"]["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
 
     @data(*ASSESSMENT_TYPES_DDT)
@@ -196,8 +209,12 @@ class TestStaffAssessment(CacheResetTest):
         if after_type == 'staff':
             return
 
+        requirements = self.STEP_REQUIREMENTS
+        if after_type == 'peer':
+            requirements = {"peer": {"must_grade": 0, "must_be_graded_by": 1}}
+
         # Create assessment
-        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", override_steps=[after_type])
+        tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer", problem_steps=[after_type])
 
         staff_score = "few"
         # Staff assess it
@@ -210,23 +227,72 @@ class TestStaffAssessment(CacheResetTest):
 
         # Verify both assessment and workflow report correct score
         self.assertEqual(staff_assessment["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
-        workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], self.STEP_REQUIREMENTS)
-        self.assertEqual(workflow["score"]["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
+        workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], requirements)
+        # It's impossible to fake self requirements being complete, so we can't get the score for the self after_type
+        if after_type != 'self':
+            self.assertEqual(workflow["score"]["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
 
         # Now, non-force asses with a 'most' value
         # This was selected to match the value that the ai test will set
         unscored_assessment = OPTIONS_SELECTED_DICT["most"]
         assessment = after_assess(tim_sub["uuid"], tim["student_id"], unscored_assessment["options"])
-        # and update workflow with new scores
-        requirements = self.STEP_REQUIREMENTS
-        if after_type == 'peer':
-            requirements = {"peer": {"must_grade": 0, "must_be_graded_by": 1}}
 
         # Verify both assessment and workflow report correct score (workflow should report previous value)
         self.assertEqual(assessment["points_earned"], unscored_assessment["expected_points"])
         workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], requirements)
         self.assertEqual(workflow["score"]["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
 
+    def test_provisionally_done(self):
+        """
+        Test to ensure that blocking steps, such as peer, are not considered done and do not display a score
+        if the submitter's requirements have not yet been met, even if a staff score has been recorded.
+
+        This test also ensures that a user may submit peer assessments after having been staff assessed, which was
+        a bug that had been previously present.
+        """
+        # Tim(student) makes a submission, for a problem that requires peer assessment
+        tim_sub, tim = TestStaffAssessment._create_student_and_submission("Tim", "Tim's answer", problem_steps=['peer'])
+        # Bob(student) also makes a submission for that problem
+        bob_sub, bob = TestStaffAssessment._create_student_and_submission("Bob", "Bob's answer", problem_steps=['peer'])
+
+        # Define peer requirements. Note that neither submission will fulfill must_be_graded_by
+        requirements = {"peer": {"must_grade": 1, "must_be_graded_by": 2}}
+
+        staff_score = "none"
+        # Dumbledore(staff) uses override ability to provide a score for both submissions
+        tim_assessment = staff_api.create_assessment(
+            tim_sub["uuid"],
+            "Dumbledore",
+            OPTIONS_SELECTED_DICT[staff_score]["options"], dict(), "",
+            RUBRIC,
+        )
+        bob_assessment = staff_api.create_assessment(
+            bob_sub["uuid"],
+            "Dumbledore",
+            OPTIONS_SELECTED_DICT[staff_score]["options"], dict(), "",
+            RUBRIC,
+        )
+
+        # Bob completes his peer assessment duties, Tim does not
+        peer_api.get_submission_to_assess(bob_sub["uuid"], 1)
+        peer_assess(
+            bob_sub["uuid"],
+            bob["student_id"],
+            OPTIONS_SELECTED_DICT["most"]["options"], dict(), "",
+            RUBRIC,
+            requirements["peer"]["must_be_graded_by"]
+        )
+
+        # Verify that Bob's submission is marked done and returns the proper score
+        bob_workflow = workflow_api.get_workflow_for_submission(bob_sub["uuid"], requirements)
+        self.assertEqual(bob_workflow["score"]["points_earned"], OPTIONS_SELECTED_DICT[staff_score]["expected_points"])
+        self.assertEqual(bob_workflow["status"], "done")
+
+        # Verify that Tim's submission is not marked done, and he cannot get his score
+        tim_workflow = workflow_api.get_workflow_for_submission(tim_sub["uuid"], requirements)
+        self.assertEqual(tim_workflow["score"], None)
+        self.assertNotEqual(tim_workflow["status"], "done")
+
     def test_invalid_rubric_exception(self):
         # Create a submission
         tim_sub, tim = self._create_student_and_submission("Tim", "Tim's answer")
@@ -317,17 +383,17 @@ class TestStaffAssessment(CacheResetTest):
         )
 
     @staticmethod
-    def _create_student_and_submission(student, answer, date=None, override_steps=None):
+    def _create_student_and_submission(student, answer, date=None, problem_steps=None):
         """
         Helper method to create a student and submission for use in tests.
         """
         new_student_item = STUDENT_ITEM.copy()
         new_student_item["student_id"] = student
         submission = sub_api.create_submission(new_student_item, answer, date)
-        steps = ['self']
+        steps = []
         init_params = {}
-        if override_steps:
-            steps = override_steps
+        if problem_steps:
+            steps = problem_steps
         if 'peer' in steps:
             peer_api.on_start(submission["uuid"])
         if 'ai' in steps:

openassessment/workflow/models.py

@@ -193,10 +193,13 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
     def score(self):
         """Latest score for the submission we're tracking.
 
-        Note that while it is usually the case that we're setting the score,
-        that may not always be the case. We may have some course staff override.
+        Returns:
+            score (dict): The latest score for this workflow, or None if the workflow is incomplete.
         """
-        return sub_api.get_latest_score_for_submission(self.submission_uuid)
+        score = None
+        if self.status == self.STATUS.done:
+            score = sub_api.get_latest_score_for_submission(self.submission_uuid)
+        return score
 
     def status_details(self, assessment_requirements):
         status_dict = {}
@@ -309,17 +312,25 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
 
         new_staff_score = self.get_score(assessment_requirements, {'staff': step_for_name.get('staff', None)})
         if new_staff_score:
-            old_score = self.score
-            if not old_score or old_score['points_earned'] != new_staff_score['points_earned']:
+            # new_staff_score is just the most recent staff score, it may already be recorded in sub_api
+            old_score = sub_api.get_latest_score_for_submission(self.submission_uuid)
+            if (
+                    not old_score or # There is no recorded score
+                    not old_score.get('staff_id') or # The recorded score is not a staff score
+                    old_score['points_earned'] != new_staff_score['points_earned'] # Previous staff score doesn't match
+            ):
+                # Set the staff score using submissions api, and log that fact
                 self.set_staff_score(new_staff_score)
                 self.save()
                 logger.info((
                     u"Workflow for submission UUID {uuid} has updated score using staff assessment."
                 ).format(uuid=self.submission_uuid))
-                staff_step = step_for_name.get('staff')
-                staff_step.assessment_completed_at=now()
-                staff_step.save()
-                self.status = self.STATUS.done
+
+                # Update the assessment_completed_at field for all steps
+                # All steps are considered "assessment complete", as the staff score will override all
+                for step in steps:
+                    step.assessment_completed_at=now()
+                    step.save()
 
         if self.status == self.STATUS.done:
             return
@@ -350,7 +361,9 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
             score = self.get_score(assessment_requirements, step_for_name)
             # If we found a score, then we're done
             if score is not None:
-                self.set_score(score)
+                # Only set the score if it's not a staff score, in which case it will have already been set above
+                if score.get("staff_id") is None:
+                    self.set_score(score)
                 new_status = self.STATUS.done
 
         # Finally save our changes if the status has changed