Save Data Update Script errors to database and show them on http://localhost:3000/admin/data_update_scripts (#12348)

* feat: add an error column to the data update script

* feat: save the error to the error field

* feat: save the error when the script fails

* feat: show the script error on the data update script page

* chore: pass the error to mark_as_failed instead of having its own function

* refactor: use presence

* test: ensure that we test an error case of a data update script

* chore: rename errorneous to failing

* test: update some specs, working on the others

* chore: update tests now that there are two files

* chore: change error from a string to a text to allow for more char

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

app/views/admin/data_update_scripts/index.html.erb

@@ -14,8 +14,11 @@
         <tr>
           <td class="whitespace-nowrap"><%= script.id %></td>
           <td><%= script.file_name %></td>
-          <td class="whitespace-nowrap justify-content-center">
+          <td class="justify-content-center">
             <%= script.status %>
+            <% if script.error.present? %>
+              <div class="fs-xs"><%= script.error %></div>
+            <% end %>
           </td>
           <td><%= script.created_at %></td>
           <td><%= script.run_at %></td>

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}",

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

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"

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

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

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