fix sis batch creation race condition

fixes #9610

test plan: since this is a race condition manual testing is difficult,
when uploading a lot of large sis batches none should fail to process
because of a nil attachment (and stickiness/batch mode settings should
always apply).

Change-Id: Iee9d1f48d8f9ab147409361ac19ccbca9df4fe97
Reviewed-on: https://gerrit.instructure.com/12478
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>

app/controllers/sis_imports_api_controller.rb

@@ -143,22 +143,21 @@ class SisImportsApiController < ApplicationController
         end
       end
 
-      batch = SisBatch.create_with_attachment(@account, params[:import_type], file_obj)
-      if batch_mode_term
-        batch.batch_mode = true
-        batch.batch_mode_term = batch_mode_term
-      end
+      batch = SisBatch.create_with_attachment(@account, params[:import_type], file_obj) do |batch|
+        if batch_mode_term
+          batch.batch_mode = true
+          batch.batch_mode_term = batch_mode_term
+        end
 
-      batch.options ||= {}
-      if params[:override_sis_stickiness].to_i > 0
-        batch.options[:override_sis_stickiness] = true
-        [:add_sis_stickiness, :clear_sis_stickiness].each do |option|
-          batch.options[option] = true if params[option].to_i > 0
+        batch.options ||= {}
+        if params[:override_sis_stickiness].to_i > 0
+          batch.options[:override_sis_stickiness] = true
+          [:add_sis_stickiness, :clear_sis_stickiness].each do |option|
+            batch.options[option] = true if params[option].to_i > 0
+          end
         end
       end
 
-      batch.save!
-
       unless Setting.get('skip_sis_jobs_account_ids', '').split(',').include?(@account.global_id.to_s)
         batch.process
       end

app/models/sis_batch.rb

@@ -43,11 +43,14 @@ class SisBatch < ActiveRecord::Base
       }
   end
 
+  # If you are going to change any settings on the batch before it's processed,
+  # do it in the block passed into this method, so that the changes are saved
+  # before the batch is marked created and eligible for processing.
   def self.create_with_attachment(account, import_type, attachment)
     batch = SisBatch.new
     batch.account = account
     batch.progress = 0
-    batch.workflow_state = :created
+    batch.workflow_state = :initializing
     batch.data = {:import_type => import_type}
     batch.save
 
@@ -56,15 +59,19 @@ class SisBatch < ActiveRecord::Base
     att.context = batch
     att.uploaded_data = attachment
     att.display_name = t :upload_filename, "sis_upload_%{id}.zip", :id => batch.id
-    att.save
+    att.save!
     Attachment.skip_scribd_submits(false)
     batch.attachment = att
-    batch.save
+
+    yield batch if block_given?
+    batch.workflow_state = :created
+    batch.save!
 
     batch
   end
 
   workflow do
+    state :initializing
     state :created
     state :importing
     state :imported

spec/models/sis_batch_spec.rb

@@ -65,6 +65,19 @@ describe SisBatch do
     create_csv_data(['abc']) { |batch| batch.attachment.position.should be_nil}
   end
 
+  it "should keep the batch in initializing state during create_with_attachment" do
+    batch = SisBatch.create_with_attachment(@account, 'instructure_csv', stub_file_data('test.csv', 'abc', 'text')) do |batch|
+      batch.attachment.should_not be_new_record
+      batch.workflow_state.should == 'initializing'
+      batch.options = { :override_sis_stickiness => true }
+    end
+
+    batch.workflow_state.should == 'created'
+    batch.should_not be_new_record
+    batch.changed?.should be_false
+    batch.options[:override_sis_stickiness].should == true
+  end
+
   describe ".process_all_for_account" do
     it "should process all non-processed batches for the account" do
       b1 = create_csv_data(['abc'])