Merge pull request #931 from edx/efischer/tnl-4696

Override submitter requirements on staff override assessment

openassessment/assessment/api/peer.py

@@ -436,6 +436,8 @@ def get_assessment_median_scores(submission_uuid):
         assessments = [item.assessment for item in items]
         scores = Assessment.scores_by_criterion(assessments)
         return Assessment.get_median_score_dict(scores)
+    except PeerWorkflow.DoesNotExist:
+        return {}
     except DatabaseError:
         error_message = (
             u"Error getting assessment median scores for submission {uuid}"

openassessment/workflow/api.py

@@ -177,7 +177,7 @@ def get_workflow_for_submission(submission_uuid, assessment_requirements):
     return update_from_assessments(submission_uuid, assessment_requirements)
 
 
-def update_from_assessments(submission_uuid, assessment_requirements):
+def update_from_assessments(submission_uuid, assessment_requirements, override_submitter_requirements=False):
     """
     Update our workflow status based on the status of the underlying assessments.
 
@@ -204,6 +204,9 @@ def update_from_assessments(submission_uuid, assessment_requirements):
             `must_be_graded_by` to ensure that everyone will get scored.
             The intention is to eventually pass in more assessment sequence
             specific requirements in this dict.
+        override_submitter_requirements (bool): If True, the presence of a new
+            staff score will cause all of the submitter's requirements to be
+            fulfilled, moving the workflow to DONE and exposing their grade.
 
     Returns:
         dict: Assessment workflow information with the following
@@ -259,7 +262,7 @@ def update_from_assessments(submission_uuid, assessment_requirements):
     workflow = _get_workflow_model(submission_uuid)
 
     try:
-        workflow.update_from_assessments(assessment_requirements)
+        workflow.update_from_assessments(assessment_requirements, override_submitter_requirements)
         logger.info((
             u"Updated workflow for submission UUID {uuid} "
             u"with requirements {reqs}"

openassessment/workflow/models.py

@@ -171,7 +171,7 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
 
                 # If we auto-added a staff step, it is optional and should be marked complete immediately
                 if step.name == "staff" and staff_auto_added:
-                    step.assessment_completed_at=now()
+                    step.assessment_completed_at = now()
                     step.save()
 
                 # For the first valid step, update the workflow status
@@ -209,6 +209,12 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
         return score
 
     def status_details(self):
+        """
+        Returns workflow status in the form of a dictionary. Each step in the
+        workflow is a key, and each key maps to a dictionary defining whether
+        the step is complete (submitter requirements fulfilled) and graded (the
+        submission has been assessed).
+        """
         status_dict = {}
         steps = self._get_steps()
         for step in steps:
@@ -259,7 +265,7 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
 
         return score
 
-    def update_from_assessments(self, assessment_requirements):
+    def update_from_assessments(self, assessment_requirements, override_submitter_requirements=False):
         """Query assessment APIs and change our status if appropriate.
 
         If the status is done, we do nothing. Once something is done, we never
@@ -291,6 +297,9 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
                 can refer to this to decide whether the requirements have been
                 met.  Note that the requirements could change if the author
                 updates the problem definition.
+            override_submitter_requirements (bool): If True, the presence of a new
+                staff score will cause all of the submitter's requirements to be
+                fulfilled, moving the workflow to DONE and exposing their grade.
 
         """
         if self.status == self.STATUS.cancelled:
@@ -320,7 +329,10 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
                 # 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()
+                    common_now = now()
+                    step.assessment_completed_at = common_now
+                    if override_submitter_requirements:
+                        step.submitter_completed_at = common_now
                     step.save()
 
         if self.status == self.STATUS.done:
@@ -346,8 +358,10 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
 
         # If the submitter has done all they need to do, let's check to see if
         # all steps have been fully assessed (i.e. we can score it).
-        if (new_status == self.STATUS.waiting and
-            all(step.assessment_completed_at for step in steps)):
+        if (
+                new_status == self.STATUS.waiting and
+                all(step.assessment_completed_at for step in steps)
+        ):
 
             score = self.get_score(assessment_requirements, step_for_name)
             # If we found a score, then we're done
@@ -398,7 +412,7 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
 
         return steps
 
-    def set_staff_score(self, score, is_override=False, reason=None):
+    def set_staff_score(self, score, reason=None):
         """
         Set a staff score for the workflow.
 
@@ -425,9 +439,9 @@ class AssessmentWorkflow(TimeStampedModel, StatusModel):
             self.submission_uuid,
             score["points_earned"],
             score["points_possible"],
-            annotation_creator = score["staff_id"],
-            annotation_type = annotation_type,
-            annotation_reason = reason
+            annotation_creator=score["staff_id"],
+            annotation_type=annotation_type,
+            annotation_reason=reason
         )
 
     def set_score(self, score):
@@ -602,9 +616,16 @@ class AssessmentWorkflowStep(models.Model):
         ordering = ["workflow", "order_num"]
 
     def is_submitter_complete(self):
+        """
+        Used to determine whether the submitter of the response has completed
+        their required actions.
+        """
         return self.submitter_completed_at is not None
 
     def is_assessment_complete(self):
+        """
+        Used to determine whether the response has been assessed at this step.
+        """
         return self.assessment_completed_at is not None
 
     def api(self):
@@ -660,12 +681,12 @@ class AssessmentWorkflowStep(models.Model):
         assessment_finished = getattr(self.api(), 'assessment_is_finished', default_finished)
 
         # Has the user completed their obligations for this step?
-        if (not self.is_submitter_complete() and submitter_finished(submission_uuid, step_reqs)):
+        if not self.is_submitter_complete() and submitter_finished(submission_uuid, step_reqs):
             self.submitter_completed_at = now()
             step_changed = True
 
         # Has the step received a score?
-        if (not self.is_assessment_complete() and assessment_finished(submission_uuid, step_reqs)):
+        if not self.is_assessment_complete() and assessment_finished(submission_uuid, step_reqs):
             self.assessment_completed_at = now()
             step_changed = True
 

openassessment/workflow/test/test_api.py

@@ -3,7 +3,6 @@ from django.test.utils import override_settings
 import ddt
 from mock import patch
 from nose.tools import raises
-from openassessment.assessment.models import PeerWorkflow
 
 from openassessment.test_utils import CacheResetTest
 
@@ -11,10 +10,11 @@ from submissions.models import Submission
 import openassessment.workflow.api as workflow_api
 from openassessment.assessment.api import ai as ai_api
 from openassessment.assessment.errors import AIError
-from openassessment.assessment.models import StudentTrainingWorkflow
+from openassessment.assessment.models import PeerWorkflow, StudentTrainingWorkflow
 import submissions.api as sub_api
 from openassessment.assessment.api import peer as peer_api
 from openassessment.assessment.api import self as self_api
+from openassessment.assessment.api import staff as staff_api
 from openassessment.workflow.models import AssessmentWorkflow, AssessmentApiLoadError
 from openassessment.workflow.errors import AssessmentWorkflowInternalError
 
@@ -254,6 +254,39 @@ class TestAssessmentWorkflowApi(CacheResetTest):
         submission = sub_api.create_submission(ITEM_1, ANSWER_2)
         workflow_api.create_workflow(submission["uuid"], ["peer", "self"], ON_INIT_PARAMS)
 
+    @patch.object(staff_api, 'get_score')
+    @patch.object(PeerWorkflow.objects, 'get')
+    def test_no_peer_assessment_error_handled(self, mock_get_workflow, mock_get_staff_score):
+        """
+        Tests to verify that, given a problem that requires the peer step and a submission associated with a workflow
+        that has no assessments, an overriding staff score will push the workflow into the done state and not crash
+        when there are no assessments in the "completed" peer step.
+        """
+        mock_get_workflow.raises = PeerWorkflow.DoesNotExist
+        mock_get_staff_score.return_value = {
+            "points_earned": 10,
+            "points_possible": 10,
+            "contributing_assessments": 123,
+            "staff_id": "staff 1",
+        }
+        _, submission = self._create_workflow_with_status(
+            "user 1",
+            "test/1/1",
+            "peer-problem",
+            "peer",
+            steps=["peer"]
+        )
+        workflow_api.update_from_assessments(
+            submission["uuid"],
+            {
+                "peer": {
+                    "must_grade": 5,
+                    "must_be_graded_by": 3
+                }
+            },
+            override_submitter_requirements=True
+        )
+
     @patch.object(AssessmentWorkflow.objects, 'get')
     @ddt.file_data('data/assessments.json')
     @raises(workflow_api.AssessmentWorkflowInternalError)

openassessment/xblock/grade_mixin.py

@@ -567,15 +567,16 @@ class GradeMixin(object):
         # If criteria/options in the problem definition do NOT have a "label" field
         # (because they were created before this change),
         # we create a new label that has the same value as "name".
-        for part in assessment['parts']:
-            criterion_label_key = part['criterion']['name']
-            part['criterion']['label'] = criterion_labels.get(criterion_label_key, part['criterion']['name'])
-
-            # We need to be a little bit careful here: some assessment parts
-            # have only written feedback, so they're not associated with any options.
-            # If that's the case, we don't need to add the label field.
-            if part.get('option') is not None:
-                option_label_key = (part['criterion']['name'], part['option']['name'])
-                part['option']['label'] = option_labels.get(option_label_key, part['option']['name'])
+        if assessment is not None:
+            for part in assessment['parts']:
+                criterion_label_key = part['criterion']['name']
+                part['criterion']['label'] = criterion_labels.get(criterion_label_key, part['criterion']['name'])
+
+                # We need to be a little bit careful here: some assessment parts
+                # have only written feedback, so they're not associated with any options.
+                # If that's the case, we don't need to add the label field.
+                if part.get('option') is not None:
+                    option_label_key = (part['criterion']['name'], part['option']['name'])
+                    part['option']['label'] = option_labels.get(option_label_key, part['option']['name'])
 
         return assessment

openassessment/xblock/staff_assessment_mixin.py

@@ -53,7 +53,11 @@ class StaffAssessmentMixin(object):
             )
             assess_type = data.get('assess_type', 'regrade')
             self.publish_assessment_event("openassessmentblock.staff_assess", assessment, type=assess_type)
-            workflow_api.update_from_assessments(assessment["submission_uuid"], None)
+            workflow_api.update_from_assessments(
+                assessment["submission_uuid"],
+                None,
+                override_submitter_requirements=(assess_type == 'regrade')
+            )
 
         except StaffAssessmentRequestError:
             logger.warning(

scripts/run-pep8.sh

 #!/usr/bin/env bash
 
-MAX_PEP8_VIOLATIONS=110
+MAX_PEP8_VIOLATIONS=111
 
 mkdir -p test/logs
 PEP8_VIOLATIONS=test/logs/pep8.txt
 touch $PEP8_VIOLATIONS
 
-pep8 --config=.pep8 openassessment > $PEP8_VIOLATIONS
+pep8 --config=.pep8 openassessment test > $PEP8_VIOLATIONS
 NUM_PEP8_VIOLATIONS=$(cat $PEP8_VIOLATIONS | wc -l)
 
 echo "Found" $NUM_PEP8_VIOLATIONS "pep8 violations, threshold is" $MAX_PEP8_VIOLATIONS

scripts/run-pylint.sh

 #!/usr/bin/env bash
 
-MAX_PYLINT_VIOLATIONS=519
+MAX_PYLINT_VIOLATIONS=504
 
 mkdir -p test/logs
 PYLINT_VIOLATIONS=test/logs/pylint.txt
 touch $PYLINT_VIOLATIONS
 
-pylint --rcfile=pylintrc openassessment --msg-template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"'> $PYLINT_VIOLATIONS
+pylint --rcfile=pylintrc openassessment test --msg-template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"'> $PYLINT_VIOLATIONS
 ./scripts/run-pylint.py $PYLINT_VIOLATIONS $MAX_PYLINT_VIOLATIONS