Merge pull request #383 from edx/sanchez/TIM-610-Fix-IntegrityErrors

Split up create and get for Student Training Workflows

apps/openassessment/assessment/api/student_training.py

@@ -309,7 +309,11 @@ def get_training_example(submission_uuid, rubric, examples):
             raise StudentTrainingRequestError(msg)
 
         # Get or create the workflow
-        workflow = StudentTrainingWorkflow.get_or_create_workflow(submission_uuid=submission_uuid)
+        workflow = StudentTrainingWorkflow.get_workflow(submission_uuid=submission_uuid)
+        if not workflow:
+            raise StudentTrainingRequestError(
+                u"No student training workflow found for submission {}".format(submission_uuid)
+            )
 
         # Get or create the training examples
         examples = deserialize_training_examples(examples, rubric)
@@ -336,6 +340,34 @@ def get_training_example(submission_uuid, rubric, examples):
         raise StudentTrainingInternalError(msg)
 
 
+def create_student_training_workflow(submission_uuid):
+    """
+    Creates a new student training workflow.
+
+    This function should be called to indicate that a submission has entered the
+    student training workflow part of the assessment process.
+
+    Args:
+        submission_uuid (str): The submission UUID for the student that is
+            initiating training.
+
+    Returns:
+        None
+
+    Raises:
+        StudentTrainingInternalError: Raised when an error occurs persisting the
+            Student Training Workflow
+    """
+    try:
+        StudentTrainingWorkflow.create_workflow(submission_uuid)
+    except Exception:
+        msg = (
+            u"An internal error has occurred while creating the student "
+            u"training workflow for submission UUID {}".format(submission_uuid)
+        )
+        logger.exception(msg)
+        raise StudentTrainingInternalError(msg)
+
 def assess_training_example(submission_uuid, options_selected, update_workflow=True):
     """
     Assess a training example and update the workflow.

apps/openassessment/assessment/models/student_training.py

@@ -27,12 +27,13 @@ class StudentTrainingWorkflow(models.Model):
         app_label = "assessment"
 
     @classmethod
-    def get_or_create_workflow(cls, submission_uuid):
+    def create_workflow(cls, submission_uuid):
         """
         Create a student training workflow.
 
         Args:
-            submission_uuid (str): The UUID of the submission from the student being trained.
+            submission_uuid (str): The UUID of the submission from the student
+                being trained.
 
         Returns:
             StudentTrainingWorkflow
@@ -41,30 +42,42 @@ class StudentTrainingWorkflow(models.Model):
             SubmissionError: There was an error retrieving the submission.
 
         """
-        # Try to retrieve an existing workflow
-        # If we find one, return it immediately
-        try:
-            return cls.objects.get(submission_uuid=submission_uuid)   # pylint:disable=E1101
-        except cls.DoesNotExist:
-            pass
-
         # Retrieve the student item info
         submission = sub_api.get_submission_and_student(submission_uuid)
         student_item = submission['student_item']
 
         # Create the workflow
         try:
-            return cls.objects.create(
+            workflow, __ = cls.objects.get_or_create(
                 submission_uuid=submission_uuid,
                 student_id=student_item['student_id'],
                 item_id=student_item['item_id'],
                 course_id=student_item['course_id']
             )
+            return workflow
         # If we get an integrity error, it means we've violated a uniqueness constraint
         # (someone has created this object after we checked if it existed)
-        # We can therefore assume that the object exists and we can retrieve it.
+        # We can therefore assume that the object exists and do nothing.
         except IntegrityError:
-            return cls.objects.get(submission_uuid=submission_uuid)
+            pass
+
+    @classmethod
+    def get_workflow(cls, submission_uuid):
+        """
+        Get a student training workflow.
+
+        Args:
+            submission_uuid (str): The UUID of the submission from the student
+                being trained.
+
+        Returns:
+            StudentTrainingWorkflow. None if no workflow is found.
+
+        """
+        try:
+            return cls.objects.get(submission_uuid=submission_uuid)   # pylint:disable=E1101
+        except cls.DoesNotExist:
+            return None
 
     @property
     def num_completed(self):
@@ -132,15 +145,11 @@ class StudentTrainingWorkflow(models.Model):
             # If we get an integrity error, it means we've violated a uniqueness constraint
             # (someone has created this object after we checked if it existed)
             # Since the object already exists, we don't need to do anything
-            # However, the example might not be the one we intended to use, so
-            # we need to retrieve the actual training example.
+            # Use the example passed into the function, because attempting to
+            # retrieve the stored example would result in an race condition.
             except IntegrityError:
-                workflow = StudentTrainingWorkflowItem.objects.get(
-                    workflow=self, order_num=order_num
-                )
-                return workflow.training_example
-            else:
-                return next_example
+                pass
+            return next_example
 
     @property
     def current_item(self):

apps/openassessment/assessment/test/test_student_training_api.py

@@ -26,6 +26,7 @@ class StudentTrainingAssessmentTest(CacheResetTest):
         Create a submission.
         """
         submission = sub_api.create_submission(STUDENT_ITEM, ANSWER)
+        training_api.create_student_training_workflow(submission['uuid'])
         self.submission_uuid = submission['uuid']
 
     def test_training_workflow(self):
@@ -102,7 +103,7 @@ class StudentTrainingAssessmentTest(CacheResetTest):
         # This will need to create the student training workflow and the first item
         # NOTE: we *could* cache the rubric model to reduce the number of queries here,
         # but we're selecting it by content hash, which is indexed and should be plenty fast.
-        with self.assertNumQueries(6):
+        with self.assertNumQueries(4):
             training_api.get_training_example(self.submission_uuid, RUBRIC, EXAMPLES)
 
         # Without assessing the first training example, try to retrieve a training example.
@@ -121,6 +122,7 @@ class StudentTrainingAssessmentTest(CacheResetTest):
 
     def test_submitter_is_finished_num_queries(self):
         # Complete the first training example
+        training_api.create_student_training_workflow(self.submission_uuid)
         training_api.get_training_example(self.submission_uuid, RUBRIC, EXAMPLES)
         training_api.assess_training_example(self.submission_uuid, EXAMPLES[0]['options_selected'])
 
@@ -321,6 +323,7 @@ class StudentTrainingAssessmentTest(CacheResetTest):
 
         """
         pre_submission = sub_api.create_submission(STUDENT_ITEM, ANSWER)
+        training_api.create_student_training_workflow(pre_submission['uuid'])
         for example in examples:
             training_api.get_training_example(pre_submission['uuid'], rubric, examples)
             training_api.assess_training_example(pre_submission['uuid'], example['options_selected'])

apps/openassessment/assessment/test/test_student_training_models.py

@@ -17,7 +17,7 @@ class StudentTrainingWorkflowTest(CacheResetTest):
     """
 
     @mock.patch.object(StudentTrainingWorkflow.objects, 'get')
-    @mock.patch.object(StudentTrainingWorkflow.objects, 'create')
+    @mock.patch.object(StudentTrainingWorkflow.objects, 'get_or_create')
     def test_create_workflow_integrity_error(self, mock_create, mock_get):
         # Simulate a race condition in which someone creates a workflow
         # after we check if it exists.  This will violate the database uniqueness
@@ -28,27 +28,25 @@ class StudentTrainingWorkflowTest(CacheResetTest):
         # The second time, we should get the workflow created by someone else
         mock_workflow = mock.MagicMock(StudentTrainingWorkflow)
         mock_get.side_effect = [
-            StudentTrainingWorkflow.DoesNotExist,
             mock_workflow
         ]
 
         # Expect that we retry and retrieve the workflow that someone else created
         submission = sub_api.create_submission(STUDENT_ITEM, ANSWER)
-        workflow = StudentTrainingWorkflow.get_or_create_workflow(submission['uuid'])
+        StudentTrainingWorkflow.create_workflow(submission['uuid'])
+        workflow = StudentTrainingWorkflow.get_workflow(submission['uuid'])
         self.assertEqual(workflow, mock_workflow)
 
-    @mock.patch.object(StudentTrainingWorkflowItem.objects, 'get')
     @mock.patch.object(StudentTrainingWorkflowItem.objects, 'create')
-    def test_create_workflow_item_integrity_error(self, mock_create, mock_get):
+    def test_create_workflow_item_integrity_error(self, mock_create):
         # Create a submission and workflow
         submission = sub_api.create_submission(STUDENT_ITEM, ANSWER)
-        workflow = StudentTrainingWorkflow.get_or_create_workflow(submission['uuid'])
+        workflow = StudentTrainingWorkflow.create_workflow(submission['uuid'])
 
         # Simulate a race condition in which someone creates a workflow item
         # after we check if it exists.
         mock_workflow_item = mock.MagicMock(StudentTrainingWorkflowItem)
         mock_create.side_effect = IntegrityError
-        mock_get.return_value = mock_workflow_item
 
         # Expect that we retry and retrieve the workflow item created by someone else
-        self.assertEqual(workflow.next_training_example(EXAMPLES), mock_workflow_item.training_example)
+        self.assertEqual(workflow.next_training_example(EXAMPLES), EXAMPLES[0])

apps/openassessment/workflow/api.py

@@ -8,7 +8,10 @@ import logging
 from django.db import DatabaseError
 
 from openassessment.assessment.api import peer as peer_api
-from openassessment.assessment.errors import PeerAssessmentError
+from openassessment.assessment.api import student_training as training_api
+from openassessment.assessment.errors import (
+    PeerAssessmentError, StudentTrainingInternalError
+)
 from submissions import api as sub_api
 from .models import AssessmentWorkflow, AssessmentWorkflowStep
 from .serializers import AssessmentWorkflowSerializer
@@ -140,6 +143,12 @@ def create_workflow(submission_uuid, steps):
         status = AssessmentWorkflow.STATUS.self
     elif steps[0] == "training":
         status = AssessmentWorkflow.STATUS.training
+        try:
+            training_api.create_student_training_workflow(submission_uuid)
+        except StudentTrainingInternalError as err:
+            err_msg = u"Could not create assessment workflow: {}".format(err)
+            logger.exception(err_msg)
+            raise AssessmentWorkflowInternalError(err_msg)
 
     try:
         workflow = AssessmentWorkflow.objects.create(

apps/openassessment/workflow/test/test_api.py

@@ -56,7 +56,7 @@ class TestAssessmentWorkflowApi(CacheResetTest):
     def test_update_peer_workflow(self):
         submission = sub_api.create_submission(ITEM_1, "Shoot Hot Rod")
         workflow = workflow_api.create_workflow(submission["uuid"], ["training", "peer"])
-        StudentTrainingWorkflow.get_or_create_workflow(submission_uuid=submission["uuid"])
+        StudentTrainingWorkflow.create_workflow(submission_uuid=submission["uuid"])
         requirements = {
             "training": {
                 "num_required": 2

apps/openassessment/xblock/test/test_student_training.py

@@ -232,7 +232,7 @@ class StudentTrainingRenderTest(StudentTrainingAssessTest):
         self._assert_path_and_context(xblock, expected_template, expected_context)
 
     @scenario('data/student_training.xml', user_id="Plato")
-    @patch.object(StudentTrainingWorkflow, "get_or_create_workflow")
+    @patch.object(StudentTrainingWorkflow, "get_workflow")
     def test_internal_error(self, xblock, mock_workflow):
         mock_workflow.side_effect = DatabaseError("Oh no.")
         xblock.create_submission(xblock.get_student_item_dict(), self.SUBMISSION)