ensure sis batch restore failure is recorded
currently if a sis batch fails to restore due to an exception, the job just vanishes into limbo. add a `restore_failed` workflow_state and ensure the job gets put into it on failure test plan: specs flag=none closes FOO-4506 Change-Id: I8ef4006d4fb78e19813fd8f8412a19982358fd51 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/352631 Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: August Thornton <august@instructure.com> QA-Review: Jeremy Stanley <jeremy@instructure.com> Product-Review: Jeremy Stanley <jeremy@instructure.com>
This commit is contained in:
parent
dda7dae26d
commit
87dea2e11b
|
@ -155,7 +155,7 @@ class Progress < ActiveRecord::Base
|
|||
@progress.message = "Unexpected error, ID: #{er_id || "unknown"}"
|
||||
@progress.save
|
||||
@progress.fail
|
||||
@context.fail_with_error!(error) if @context.respond_to?(:fail_with_error!)
|
||||
object.fail_with_error!(error) if object.respond_to?(:fail_with_error!)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -128,6 +128,7 @@ class SisBatch < ActiveRecord::Base
|
|||
state :restoring
|
||||
state :partially_restored
|
||||
state :restored
|
||||
state :restore_failed
|
||||
end
|
||||
|
||||
def process
|
||||
|
@ -209,12 +210,7 @@ class SisBatch < ActiveRecord::Base
|
|||
|
||||
import_scheme[:callback].call(self)
|
||||
rescue => e
|
||||
reload # might have failed trying to save
|
||||
self.data ||= {}
|
||||
self.data[:error_message] = e.to_s
|
||||
self.data[:stack_trace] = "#{e}\n#{e.backtrace.join("\n")}"
|
||||
self.workflow_state = "failed"
|
||||
save
|
||||
fail_with_error!(e)
|
||||
end
|
||||
|
||||
def abort_batch
|
||||
|
@ -350,7 +346,7 @@ class SisBatch < ActiveRecord::Base
|
|||
|
||||
self.diffing_threshold_exceeded = false
|
||||
|
||||
self.data[:diffed_against_sis_batch_id] = previous_batch.id
|
||||
data[:diffed_against_sis_batch_id] = previous_batch.id
|
||||
|
||||
self.generated_diff = Attachment.create_data_attachment(
|
||||
self,
|
||||
|
@ -376,8 +372,8 @@ class SisBatch < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def download_zip
|
||||
@data_file = if self.data[:file_path]
|
||||
File.open(self.data[:file_path], "rb")
|
||||
@data_file = if data[:file_path]
|
||||
File.open(data[:file_path], "rb")
|
||||
else
|
||||
attachment.open(integrity_check: true)
|
||||
end
|
||||
|
@ -389,7 +385,7 @@ class SisBatch < ActiveRecord::Base
|
|||
@data_file = nil
|
||||
return self if workflow_state == "aborted"
|
||||
|
||||
if batch_mode? && import_finished && !self.data[:running_immediately]
|
||||
if batch_mode? && import_finished && !data[:running_immediately]
|
||||
# in batch mode, there's still a lot of work left to do, and it needs to be done in a separate job
|
||||
# from the last ParallelImporter or a failed job will retry that bit of the import (and not the batch cleanup!)
|
||||
save!
|
||||
|
@ -416,7 +412,7 @@ class SisBatch < ActiveRecord::Base
|
|||
save!
|
||||
InstStatsd::Statsd.increment("sis_batch_completed", tags: { failed: @has_errors })
|
||||
|
||||
if !self.data[:running_immediately] && account.sis_batches.needs_processing.exists?
|
||||
if !data[:running_immediately] && account.sis_batches.needs_processing.exists?
|
||||
self.class.queue_job_for_account(account) # check if there's anything that needs to be run
|
||||
end
|
||||
end
|
||||
|
@ -433,6 +429,19 @@ class SisBatch < ActiveRecord::Base
|
|||
end
|
||||
end
|
||||
|
||||
def fail_with_error!(error)
|
||||
reload # might have failed trying to save; also ensure workflow_state is up to date
|
||||
self.data ||= {}
|
||||
self.data[:error_message] = error&.to_s
|
||||
self.data[:stack_trace] = "#{error}\n#{error&.backtrace&.join("\n")}"
|
||||
self.workflow_state = if %w[restoring restored partially_restored restore_failed].include?(workflow_state)
|
||||
"restore_failed"
|
||||
else
|
||||
"failed"
|
||||
end
|
||||
save!
|
||||
end
|
||||
|
||||
def statistics
|
||||
stats = {}
|
||||
stats[:total_state_changes] = roll_back_data.count
|
||||
|
@ -907,6 +916,7 @@ class SisBatch < ActiveRecord::Base
|
|||
end
|
||||
|
||||
def restore_states_for_batch(restore_progress = nil, batch_mode: false, undelete_only: false, unconclude_only: false)
|
||||
capture_job_id
|
||||
restore_progress&.start
|
||||
update_attribute(:workflow_state, "restoring")
|
||||
roll_back = roll_back_data
|
||||
|
|
|
@ -122,6 +122,21 @@ describe SisBatch do
|
|||
expect(InstStatsd::Statsd).to have_received(:increment).with("sis_batch_restored", tags:)
|
||||
end
|
||||
|
||||
it "captures job failures on restore" do
|
||||
batch = process_csv_data([%(user_id,login_id,status
|
||||
user_1,user_1,active)])
|
||||
|
||||
expect(Delayed::Worker).to receive(:current_job).at_least(:once).and_return(double("Delayed::Job", id: 789))
|
||||
expect_any_instance_of(SisBatch).to receive(:roll_back_data) { raise "no roll back data for you" }
|
||||
batch.restore_states_later
|
||||
run_jobs
|
||||
|
||||
batch.reload
|
||||
expect(batch.workflow_state).to eq "restore_failed"
|
||||
expect(batch.data[:error_message]).to eq "no roll back data for you"
|
||||
expect(batch.job_ids).to include 789
|
||||
end
|
||||
|
||||
it "creates new linked observer enrollments when restoring enrollments" do
|
||||
course = @account.courses.create!(name: "one", sis_source_id: "c1", workflow_state: "available")
|
||||
user = user_with_managed_pseudonym(account: @account, sis_user_id: "u1")
|
||||
|
|
Loading…
Reference in New Issue