Delayed job outcome submission race condition fix

This moves the submission creation to a separate method, so it can be called from a delayed job after the attachment is downloaded refs: RD-3449 Test plan: Makse sure LTI Cloud assignment submissions work Change-Id: Ib28cf43fc7ece4e05bd925f67f9ce43181fa6910 Reviewed-on: https://gerrit.instructure.com/101763 Tested-by: Jenkins Reviewed-by: Cody Cutrer <[email protected]> Reviewed-by: Benjamin Porter <[email protected]> Reviewed-by: Nathan Mills <[email protected]> QA-Review: Caleb Guanzon <[email protected]> Product-Review: Caleb Guanzon <[email protected]>
kazhuyo · Feb 22, 2017 · 869ed6c · 869ed6c
1 parent 6c9abf4
commit 869ed6c
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 20 deletions.
diff --git a/lib/basic_lti/basic_outcomes.rb b/lib/basic_lti/basic_outcomes.rb
@@ -225,12 +225,6 @@ def handle_replaceResult(_tool, _course, assignment, user)
             user: user
           )
 
-          job_options = {
-            :priority => Delayed::LOW_PRIORITY,
-            :max_attempts => 1,
-            :n_strand => 'file_download'
-          }
-          attachment.send_later_enqueue_args(:clone_url, job_options, url, 'rename', true)
           submission_hash[:attachments] = [attachment]
           submission_hash[:submission_type] = 'online_upload'
         elsif (launch_url = result_data_launch_url)
@@ -253,6 +247,7 @@ def handle_replaceResult(_tool, _course, assignment, user)
         elsif !text && !url && !launch_url
           error_message = I18n.t('lib.basic_lti.no_score', "No score given")
         end
+
         if error_message
           self.code_major = 'failure'
           self.description = error_message
@@ -269,21 +264,16 @@ def handle_replaceResult(_tool, _course, assignment, user)
           self.code_major = 'failure'
           self.description = I18n.t('lib.basic_lti.no_points_possible', 'Assignment has no points possible.')
         else
-          if submission_hash[:submission_type].present? && submission_hash[:submission_type] != 'external_tool'
-            @submission = assignment.submit_homework(user, submission_hash.clone)
-          end
-
-          if new_score || raw_score
-            submission_hash[:grade] = (new_score >= 1 ? "pass" : "fail") if assignment.grading_type == "pass_fail"
-            submission_hash[:grader_id] = -_tool.id
-            @submission = assignment.grade_student(user, submission_hash).first
-          end
-
-          if @submission
-            @submission.save
+          if attachment
+            job_options = {
+              :priority => Delayed::LOW_PRIORITY,
+              :max_attempts => 1,
+              :n_strand => 'file_download'
+            }
+
+            send_later_enqueue_args(:fetch_attachment_and_save_submission, job_options, url, attachment, _tool, submission_hash, assignment, user, new_score, raw_score)
           else
-            self.code_major = 'failure'
-            self.description = I18n.t('lib.basic_lti.no_submission_created', 'This outcome request failed to create a new homework submission.')
+            create_homework_submission _tool, submission_hash, assignment, user, new_score, raw_score
           end
         end
 
@@ -292,6 +282,25 @@ def handle_replaceResult(_tool, _course, assignment, user)
         true
       end
 
+      def create_homework_submission(_tool, submission_hash, assignment, user, new_score, raw_score)
+        if submission_hash[:submission_type].present? && submission_hash[:submission_type] != 'external_tool'
+          @submission = assignment.submit_homework(user, submission_hash.clone)
+        end
+
+        if new_score || raw_score
+          submission_hash[:grade] = (new_score >= 1 ? "pass" : "fail") if assignment.grading_type == "pass_fail"
+          submission_hash[:grader_id] = -_tool.id
+          @submission = assignment.grade_student(user, submission_hash).first
+        end
+
+        if @submission
+          @submission.save
+        else
+          self.code_major = 'failure'
+          self.description = I18n.t('lib.basic_lti.no_submission_created', 'This outcome request failed to create a new homework submission.')
+        end
+      end
+
       def handle_deleteResult(tool, _course, assignment, user)
         assignment.grade_student(user, :grade => nil, grader_id: -tool.id)
         self.body = "<deleteResultResponse />"
@@ -313,6 +322,11 @@ def handle_readResult(tool, course, assignment, user)
         true
       end
 
+      def fetch_attachment_and_save_submission(url, attachment, _tool, submission_hash, assignment, user, new_score, raw_score)
+        attachment.clone_url(url, 'rename', true)
+        create_homework_submission _tool, submission_hash, assignment, user, new_score, raw_score
+      end
+
       def submission_score
         if @submission.try(:graded?)
           raw_score = @submission.assignment.score_to_grade_percent(@submission.score)

diff --git a/spec/lib/basic_lti/basic_outcomes_spec.rb b/spec/lib/basic_lti/basic_outcomes_spec.rb
@@ -348,6 +348,8 @@ def gen_source_id(t: tool, c: @course, a: assignment, u: @user)
         xml.css('resultScore').remove
         xml.at_css('text').replace('<documentName>face.doc</documentName><downloadUrl>http://example.com/download</downloadUrl>')
         BasicLTI::BasicOutcomes.process_request(tool, xml)
+        expect(Delayed::Job.strand_size('file_download')).to be > 0
+        run_jobs
         expect(submission.reload.versions.count).to eq 2
         expect(submission.attachments.count).to eq 1
         expect(submission.attachments.first.display_name).to eq "face.doc"