diff --git a/app/models/data_update_script.rb b/app/models/data_update_script.rb index e43f9efd0..fec92bf7c 100644 --- a/app/models/data_update_script.rb +++ b/app/models/data_update_script.rb @@ -51,8 +51,12 @@ class DataUpdateScript < ApplicationRecord update!(finished_at: Time.current, status: :succeeded) end - def mark_as_failed! - update!(finished_at: Time.current, status: :failed) + def mark_as_failed!(err) + update!( + finished_at: Time.current, + status: :failed, + error: "#{err.class}: #{err.message}", + ) end def file_path diff --git a/app/views/admin/data_update_scripts/index.html.erb b/app/views/admin/data_update_scripts/index.html.erb index e248df16c..c522490c9 100644 --- a/app/views/admin/data_update_scripts/index.html.erb +++ b/app/views/admin/data_update_scripts/index.html.erb @@ -14,8 +14,11 @@ <%= script.id %> <%= script.file_name %> - + <%= script.status %> + <% if script.error.present? %> +
<%= script.error %>
+ <% end %> <%= script.created_at %> <%= script.run_at %> diff --git a/app/workers/data_update_worker.rb b/app/workers/data_update_worker.rb index af3153e70..9786789b9 100644 --- a/app/workers/data_update_worker.rb +++ b/app/workers/data_update_worker.rb @@ -21,7 +21,7 @@ class DataUpdateWorker script.mark_as_finished! log_status(script) rescue StandardError => e - script.mark_as_failed! + script.mark_as_failed!(e) log_status(script) Honeybadger.notify(e, context: { script_id: script.id }) @@ -31,7 +31,7 @@ class DataUpdateWorker status = script.status file_name = script.file_name - logger_destination = status == :failed ? :error : :info + logger_destination = status.to_sym == :failed ? :error : :info Rails.logger.public_send( logger_destination, "time=#{Time.current.rfc3339}, script=#{file_name}, status=#{status}", diff --git a/db/migrate/20210121102114_add_error_to_data_update_script.rb b/db/migrate/20210121102114_add_error_to_data_update_script.rb new file mode 100644 index 000000000..8e053d30d --- /dev/null +++ b/db/migrate/20210121102114_add_error_to_data_update_script.rb @@ -0,0 +1,5 @@ +class AddErrorToDataUpdateScript < ActiveRecord::Migration[6.0] + def change + add_column :data_update_scripts, :error, :text + end +end diff --git a/db/schema.rb b/db/schema.rb index 17b574305..cf47b1cfd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_01_11_151630) do +ActiveRecord::Schema.define(version: 2021_01_21_102114) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -433,6 +433,7 @@ ActiveRecord::Schema.define(version: 2021_01_11_151630) do create_table "data_update_scripts", force: :cascade do |t| t.datetime "created_at", null: false + t.text "error" t.string "file_name" t.datetime "finished_at" t.datetime "run_at" diff --git a/spec/models/data_update_script_spec.rb b/spec/models/data_update_script_spec.rb index 01036db03..4e845c6ae 100644 --- a/spec/models/data_update_script_spec.rb +++ b/spec/models/data_update_script_spec.rb @@ -20,7 +20,7 @@ RSpec.describe DataUpdateScript do it "creates new DataUpdateScripts from files" do expect do described_class.scripts_to_run - end.to change(described_class, :count).by(1) + end.to change(described_class, :count).by(2) end it "returns scripts that need to be run" do @@ -52,18 +52,24 @@ RSpec.describe DataUpdateScript do expect(described_class.scripts_to_run?).to be(true) end - it "returns false if there are only working scripts" do + it "returns true if there are multiple files on disk" do create(:data_update_script, status: :working) + expect(described_class.scripts_to_run?).to be(true) + end + + it "returns false if there are only working scripts" do + create_list(:data_update_script, 2, status: :working) expect(described_class.scripts_to_run?).to be(false) end it "returns false if there are only succeeded scripts" do - create(:data_update_script, status: :succeeded) + create_list(:data_update_script, 2, status: :succeeded) + expect(described_class.scripts_to_run?).to be(false) end it "returns false if there are only failed scripts" do - create(:data_update_script, status: :failed) + create_list(:data_update_script, 2, status: :failed) expect(described_class.scripts_to_run?).to be(false) end end @@ -95,8 +101,9 @@ RSpec.describe DataUpdateScript do test_script = create(:data_update_script) expect(test_script.finished_at).to be_nil expect(test_script).to be_enqueued - test_script.mark_as_failed! + test_script.mark_as_failed!(StandardError.new("error")) expect(test_script).to be_failed + expect(test_script.error).to eq("StandardError: error") expect(test_script.finished_at).not_to be_nil end end diff --git a/spec/support/fixtures/data_update_scripts/20210120141757_failing_data_update_test_script.rb b/spec/support/fixtures/data_update_scripts/20210120141757_failing_data_update_test_script.rb new file mode 100644 index 000000000..c81652ac8 --- /dev/null +++ b/spec/support/fixtures/data_update_scripts/20210120141757_failing_data_update_test_script.rb @@ -0,0 +1,7 @@ +module DataUpdateScripts + class FailingDataUpdateTestScript + def run + some_error.to_s + end + end +end diff --git a/spec/workers/data_update_worker_spec.rb b/spec/workers/data_update_worker_spec.rb index 7e9f11462..1a0d4039d 100644 --- a/spec/workers/data_update_worker_spec.rb +++ b/spec/workers/data_update_worker_spec.rb @@ -13,7 +13,7 @@ RSpec.describe DataUpdateWorker, type: :worker do it "runs scripts that need running" do expect do worker.perform - end.to change(DataUpdateScript, :count).by(1) + end.to change(DataUpdateScript, :count).by(2) end it "will not run a script that has already been run" do @@ -26,21 +26,28 @@ RSpec.describe DataUpdateWorker, type: :worker do it "updates DataUpdateScript model" do expect do worker.perform - end.to change(DataUpdateScript, :count).by(1) + end.to change(DataUpdateScript, :count).by(2) - dus = DataUpdateScript.last - expect(dus.finished_at).not_to be_nil - expect(dus).to be_succeeded - expect(dus.run_at).not_to be_nil + successsful_dus = DataUpdateScript.find_by(status: :succeeded) + expect(successsful_dus.finished_at).not_to be_nil + expect(successsful_dus.run_at).not_to be_nil + expect(successsful_dus.error).to be_nil + + failed_dus = DataUpdateScript.find_by(status: :failed) + expect(failed_dus.finished_at).not_to be_nil + expect(failed_dus.run_at).not_to be_nil + expect(failed_dus.run_at).not_to be_nil + expect(failed_dus.error).not_to be_nil end it "logs data to stdout", :aggregate_failures do allow(Rails.logger).to receive(:info) + allow(Rails.logger).to receive(:error) worker.perform - statuses.each do |status| - expect(Rails.logger).to have_received(:info).once.with(/#{status}/) - end + expect(Rails.logger).to have_received(:info).twice.with(/working/) + expect(Rails.logger).to have_received(:info).once.with(/succeeded/) + expect(Rails.logger).to have_received(:error).once.with(/failed/) end it "logs data to Datadog", :aggregate_failures do