From 133bc3077c7ccc8bb3336327a930bcacb1f84b57 Mon Sep 17 00:00:00 2001 From: jrgriffiniii <1443986+jrgriffiniii@users.noreply.github.com> Date: Thu, 19 Sep 2024 09:56:36 -0400 Subject: [PATCH] Ensures that empty files and nil user IDs for WorkActivity objects raise ArgumentErrors --- app/models/background_upload_snapshot.rb | 14 +++++- app/models/work.rb | 1 - app/models/work_activity.rb | 1 + .../work_state_transition_notification.rb | 2 + .../models/background_upload_snapshot_spec.rb | 47 +++++++++++++++---- 5 files changed, 52 insertions(+), 13 deletions(-) diff --git a/app/models/background_upload_snapshot.rb b/app/models/background_upload_snapshot.rb index 235671951..8112fd1d9 100644 --- a/app/models/background_upload_snapshot.rb +++ b/app/models/background_upload_snapshot.rb @@ -2,9 +2,12 @@ class BackgroundUploadSnapshot < UploadSnapshot def store_files(uploaded_files, pre_existing_files: [], current_user: nil) save # needed so I can point to this snapshot in the files to distinguish new files from existing ones froma past snapshot + + raise(ArgumentError, "Failed to resolve the user ID from #{self}") if current_user.nil? + user_id = current_user&.id self.files = uploaded_files.map do |file| { "filename" => prefix_filename(file.original_filename), - "upload_status" => "started", user_id: current_user&.id, snapshot_id: id } + "upload_status" => "started", user_id:, snapshot_id: id } end files.concat pre_existing_files if pre_existing_files.present? end @@ -38,7 +41,14 @@ def finalize_upload new_files.each do |file| work.track_change(:added, file["filename"]) end - work.log_file_changes(new_files.first["user_id"]) + + raise(ArgumentError, "Upload failed with empty files.") if new_files.empty? + first_new_file = new_files.first + + user_id = first_new_file["user_id"] + raise(ArgumentError, "Failed to resolve the user ID from #{first_new_file['filename']}") if user_id.nil? + + work.log_file_changes(user_id) end def prefix_filename(filename) diff --git a/app/models/work.rb b/app/models/work.rb index 980b5a364..475c36602 100644 --- a/app/models/work.rb +++ b/app/models/work.rb @@ -265,7 +265,6 @@ def add_message(message, current_user_id) def add_provenance_note(date, note, current_user_id, change_label = "") WorkActivity.add_work_activity(id, { note:, change_label: }.to_json, current_user_id, activity_type: WorkActivity::PROVENANCE_NOTES, created_at: date) - # WorkActivity.add_work_activity(id, note, current_user_id, activity_type: WorkActivity::PROVENANCE_NOTES, created_at: date) end def log_changes(resource_compare, current_user_id) diff --git a/app/models/work_activity.rb b/app/models/work_activity.rb index 2d6ad9cd5..723f17438 100644 --- a/app/models/work_activity.rb +++ b/app/models/work_activity.rb @@ -267,6 +267,7 @@ def user_refernces(text_in) "#{at_uid}" end else + Rails.logger.warn("Failed to extract the user ID from #{uid}") UNKNOWN_USER end end diff --git a/app/models/work_state_transition_notification.rb b/app/models/work_state_transition_notification.rb index 37da9b487..cd813f33a 100644 --- a/app/models/work_state_transition_notification.rb +++ b/app/models/work_state_transition_notification.rb @@ -23,6 +23,8 @@ def initialize(work, current_user_id) @work_title = work.title @notification = notification_for_transition @id = work.id + + raise(NotImplementedError, "Invalid user ID provided.") if current_user_id.nil? @current_user_id = current_user_id end diff --git a/spec/models/background_upload_snapshot_spec.rb b/spec/models/background_upload_snapshot_spec.rb index 1396a91f2..3f0dd43eb 100644 --- a/spec/models/background_upload_snapshot_spec.rb +++ b/spec/models/background_upload_snapshot_spec.rb @@ -6,6 +6,7 @@ let(:work) { FactoryBot.create(:approved_work) } let(:uploaded_file1) { fixture_file_upload("us_covid_2019.csv", "text/csv") } let(:uploaded_file2) { fixture_file_upload("us_covid_2020.csv", "text/csv") } + let(:current_user) { work.created_by_user } describe "#count" do it "only counts the bacground uploads" do @@ -16,10 +17,12 @@ end describe "#store_files" do + let(:current_user) { work.created_by_user } + it "lists filenames associated with the snapshot" do - background_upload_snapshot.store_files([uploaded_file1, uploaded_file2]) - expect(background_upload_snapshot.files).to eq([{ "filename" => "#{work.prefix}us_covid_2019.csv", "upload_status" => "started", "user_id" => nil, "snapshot_id" => 123 }, - { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "started", "user_id" => nil, "snapshot_id" => 123 }]) + background_upload_snapshot.store_files([uploaded_file1, uploaded_file2], current_user:) + expect(background_upload_snapshot.files).to eq([{ "filename" => "#{work.prefix}us_covid_2019.csv", "upload_status" => "started", "snapshot_id" => 123, "user_id" => current_user.id }, + { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "started", "user_id" => current_user.id, "snapshot_id" => 123 }]) expect(background_upload_snapshot.existing_files).to eq([]) expect(background_upload_snapshot.upload_complete?).to be_falsey end @@ -39,26 +42,50 @@ describe "#mark_complete" do it "changes the status" do allow(Honeybadger).to receive(:notify) - background_upload_snapshot.store_files([uploaded_file1, uploaded_file2]) + background_upload_snapshot.store_files([uploaded_file1, uploaded_file2], current_user:) expect(work.work_activity.count).to eq(0) background_upload_snapshot.mark_complete(uploaded_file2.original_filename, "checksumabc123") expect(work.work_activity.count).to eq(0) - expect(background_upload_snapshot.files).to eq([{ "filename" => "#{work.prefix}us_covid_2019.csv", "upload_status" => "started", "user_id" => nil, "snapshot_id" => 123 }, - { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => nil, "checksum" => "checksumabc123", + expect(background_upload_snapshot.files).to eq([{ "filename" => "#{work.prefix}us_covid_2019.csv", "upload_status" => "started", "user_id" => current_user.id, "snapshot_id" => 123 }, + { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => current_user.id, "checksum" => "checksumabc123", "snapshot_id" => 123 }]) expect(background_upload_snapshot.upload_complete?).to be_falsey - expect(background_upload_snapshot.existing_files).to eq([{ "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => nil, "checksum" => "checksumabc123", + # rubocop:disable Layout/LineLength + expect(background_upload_snapshot.existing_files).to eq([{ "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => current_user.id, "checksum" => "checksumabc123", "snapshot_id" => 123 }]) background_upload_snapshot.mark_complete(uploaded_file1.original_filename, "checksumdef456") expect(background_upload_snapshot.upload_complete?).to be_truthy expect(background_upload_snapshot.existing_files).to eq([{ "filename" => "#{work.prefix}us_covid_2019.csv", "upload_status" => "complete", - "user_id" => nil, "checksum" => "checksumdef456", "snapshot_id" => 123 }, - { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => nil, "checksum" => "checksumabc123", + "user_id" => current_user.id, "checksum" => "checksumdef456", "snapshot_id" => 123 }, + { "filename" => "#{work.prefix}us_covid_2020.csv", "upload_status" => "complete", "user_id" => current_user.id, "checksum" => "checksumabc123", "snapshot_id" => 123 }]) + # rubocop:enable Layout/LineLength expect(work.work_activity.count).to eq(1) expect(work.work_activity.first.message).to eq("[{\"action\":\"added\",\"filename\":\"#{work.prefix}us_covid_2019.csv\"}"\ ",{\"action\":\"added\",\"filename\":\"#{work.prefix}us_covid_2020.csv\"}]") - expect(work.work_activity.first.created_by_user_id).to eq(nil) + expect(work.work_activity.first.created_by_user_id).to eq(current_user.id) + end + end + + describe "#finalize_upload" do + context "when no files are associated with the work" do + it "raises an exception" do + expect { background_upload_snapshot.finalize_upload }.to raise_error(ArgumentError, "Upload failed with empty files.") + end + end + + context "when the file is not associated with a user" do + let(:uploaded_file1) { fixture_file_upload("us_covid_2019.csv", "text/csv") } + let(:files) do + [ + uploaded_file1 + ] + end + + it "raises an error" do + pattern = Regexp.escape("Failed to resolve the user ID from ") + expect { background_upload_snapshot.store_files(files) }.to raise_error(ArgumentError, /#{pattern}/) + end end end end