diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 85230a84..a969f683 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -1,5 +1,9 @@ class UploadsController < ApplicationController def create + if empty_documents.any? + return render json: { result: "error", message: empty_documents_message }, status: :unprocessable_content + end + blobs = document_params.map do |document| ActiveStorage::Blob.create_and_upload!(**document) end @@ -28,7 +32,7 @@ def destroy private def document_params - params.require(:documents).map do |document| + documents.map do |document| { io: document, filename: derived_filename(document), @@ -37,15 +41,23 @@ def document_params end end + def documents + @documents ||= params.require(:documents) + end + + def empty_documents + @empty_documents ||= documents.select { |document| document.size.zero? } + end + + def empty_documents_message + names = empty_documents.map { |document| original_filename(document) }.to_sentence + + "Nothing was uploaded because #{names} #{empty_documents.one? ? "is" : "are"} empty (0 bytes). " \ + "Check the file on your computer and try again." + end + def derived_filename(document) - filename = - if document.respond_to?(:original_filename) - document.original_filename - elsif document.respond_to?(:filename) - document.filename.to_s - else - "unnamed" - end + filename = original_filename(document) name = File.basename(filename, ".*") ext = File.extname(filename).delete_prefix(".") @@ -54,4 +66,14 @@ def derived_filename(document) [ name.parameterize(separator: "_"), ext ].compact.join("."), ].join("_") end + + def original_filename(document) + if document.respond_to?(:original_filename) + document.original_filename + elsif document.respond_to?(:filename) + document.filename.to_s + else + "unnamed" + end + end end diff --git a/app/javascript/controllers/upload_controller.js b/app/javascript/controllers/upload_controller.js index 3306ef2f..65e9c1cf 100644 --- a/app/javascript/controllers/upload_controller.js +++ b/app/javascript/controllers/upload_controller.js @@ -7,6 +7,7 @@ export default class extends Controller { "filesContainer", "hiddenField", "submitButton", + "errorMessage", ]; uploadFile(event) { @@ -15,6 +16,7 @@ export default class extends Controller { this.submitButtonTarget.disabled = true; this.submitButtonTarget.classList.add("disabled:opacity-75"); this.submitButtonTarget.value = "Uploading..."; + this.showError(""); const filesInput = this.filesInputTarget; let files = Array.from(filesInput.files); @@ -40,15 +42,28 @@ export default class extends Controller { .then((data) => { if (data.result === "success") { this.filesContainerTarget.innerHTML += data.html; - filesInput.value = ""; + } else { + this.showError(data.message || "The upload failed. Please try again."); } - + }) + .catch(() => { + this.showError("The upload failed. Please try again."); + }) + .finally(() => { + filesInput.value = ""; this.submitButtonTarget.disabled = false; this.submitButtonTarget.classList.remove("disabled:opacity-75"); this.submitButtonTarget.value = "Save Topic"; }); } + showError(message) { + if (!this.hasErrorMessageTarget) return; + + this.errorMessageTarget.textContent = message; + this.errorMessageTarget.hidden = !message; + } + removeFile(event) { event.preventDefault(); diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb index 51d9a113..945e52a1 100644 --- a/app/jobs/application_job.rb +++ b/app/jobs/application_job.rb @@ -5,6 +5,10 @@ class ApplicationJob < ActiveJob::Base # Most jobs are safe to ignore if the underlying records are no longer available # discard_on ActiveJob::DeserializationError + # Solid Queue lives in its own database, so a job enqueued inside a transaction + # can be picked up before the records it needs are committed. + self.enqueue_after_transaction_commit = true + around_perform do |job, block| start_time = Time.current Rails.logger.debug "[Job Start] #{job.class.name} ID=#{job.job_id} Args=#{job.arguments}" diff --git a/app/jobs/documents_sync_job.rb b/app/jobs/documents_sync_job.rb index 5d9d8e4a..8d219bfd 100644 --- a/app/jobs/documents_sync_job.rb +++ b/app/jobs/documents_sync_job.rb @@ -1,4 +1,6 @@ class DocumentsSyncJob < ApplicationJob + retry_on AzureFileShares::Errors::ApiError, Timeout::Error, wait: :polynomially_longer, attempts: 5 + def perform(topic_id:, document_id:, action:, share: ENV["AZURE_STORAGE_SHARE_NAME"]) return if ENV["AZURE_MEDIA_FILES_SYNC_DISABLED"].present? diff --git a/app/services/file_worker.rb b/app/services/file_worker.rb index ee18e34d..299d5653 100644 --- a/app/services/file_worker.rb +++ b/app/services/file_worker.rb @@ -1,6 +1,8 @@ class FileWorker UPLOAD_TIMEOUT = 300 + class EmptyFileError < StandardError; end + def initialize(share:, name:, path:, file:, new_path: nil) @share = share @name = name @@ -28,7 +30,8 @@ def copy def send_file validate_filename - return if file.blank? + # Skipping quietly left the file listed in File.csv but missing from Azure + raise EmptyFileError, "#{name} is empty and was not uploaded to #{path}" if file.blank? create_subdirs(path) diff --git a/app/views/topics/_form.html.erb b/app/views/topics/_form.html.erb index cd4557a6..eb00bdb9 100644 --- a/app/views/topics/_form.html.erb +++ b/app/views/topics/_form.html.erb @@ -140,6 +140,8 @@ + +
diff --git a/spec/fixtures/files/empty.pdf b/spec/fixtures/files/empty.pdf new file mode 100644 index 00000000..e69de29b diff --git a/spec/jobs/documents_sync_job_spec.rb b/spec/jobs/documents_sync_job_spec.rb index a0cf804a..5402338a 100644 --- a/spec/jobs/documents_sync_job_spec.rb +++ b/spec/jobs/documents_sync_job_spec.rb @@ -6,6 +6,44 @@ let(:document) { topic.documents.first } let(:file_name) { document.filename } + describe "enqueueing" do + let(:job_args) { { topic_id: topic.id, document_id: document.id, action: "update" } } + + it "waits for the surrounding transaction to commit" do + expect { + ActiveRecord::Base.transaction do + expect { described_class.perform_later(**job_args) }.not_to have_enqueued_job(described_class) + end + }.to have_enqueued_job(described_class).with(**job_args) + end + + it "is dropped when the surrounding transaction rolls back" do + expect { + ActiveRecord::Base.transaction do + described_class.perform_later(**job_args) + + raise ActiveRecord::Rollback + end + }.not_to have_enqueued_job(described_class) + end + end + + describe "retries" do + let(:job_args) { { topic_id: topic.id, document_id: document.id, action: "update" } } + + it "retries when Azure returns an API error" do + allow(FileManager).to receive(:new).and_raise(AzureFileShares::Errors::ApiError.new("ServerBusy")) + + expect { described_class.perform_now(**job_args) }.to have_enqueued_job(described_class).with(**job_args) + end + + it "retries when the Azure request times out" do + allow(FileManager).to receive(:new).and_raise(Timeout::Error) + + expect { described_class.perform_now(**job_args) }.to have_enqueued_job(described_class).with(**job_args) + end + end + describe "#perform" do let(:file_worker) { instance_double(FileWorker) } diff --git a/spec/requests/uploads/create_spec.rb b/spec/requests/uploads/create_spec.rb index b442190f..02da1edf 100644 --- a/spec/requests/uploads/create_spec.rb +++ b/spec/requests/uploads/create_spec.rb @@ -42,5 +42,26 @@ expect(ActiveStorage::Blob.last.filename.to_s).to eq("[skillrx_internal_upload]_who_was_there.pdf") end end + + context "when one of the files is empty" do + let(:document_params) do + [ + Rack::Test::UploadedFile.new( + Rails.root.join("spec", "fixtures", "files", "dummy.pdf"), + "application/pdf", + original_filename: "dummy.pdf" + ), + Rack::Test::UploadedFile.new(StringIO.new(""), "application/pdf", original_filename: "empty.pdf"), + ] + end + + it "rejects the upload and names the empty file" do + post uploads_url, params: { documents: document_params } + + expect(response).to have_http_status(:unprocessable_content) + expect(JSON.parse(response.body)).to include("result" => "error", "message" => /empty\.pdf/) + expect(ActiveStorage::Blob.count).to eq(0) + end + end end end diff --git a/spec/services/file_worker_spec.rb b/spec/services/file_worker_spec.rb index 97d7ed48..479f295e 100644 --- a/spec/services/file_worker_spec.rb +++ b/spec/services/file_worker_spec.rb @@ -70,7 +70,7 @@ expect { worker.send }.to raise_error(AzureFileShares::Errors::ApiError) end - it "does not upload blank files" do + it "refuses to upload blank files and fails loudly" do worker_with_blank_file = described_class.new( share: "skillrx-test", file: "", @@ -80,7 +80,7 @@ expect(files).not_to receive(:upload_file) - worker_with_blank_file.send + expect { worker_with_blank_file.send }.to raise_error(FileWorker::EmptyFileError, /#{name}/) end end diff --git a/spec/system/upload_management_spec.rb b/spec/system/upload_management_spec.rb index f82be403..f83a96f3 100644 --- a/spec/system/upload_management_spec.rb +++ b/spec/system/upload_management_spec.rb @@ -26,6 +26,14 @@ click_button("remove-button-1") expect(page).not_to have_text("logo_ruby_for_good.png") end + + it "explains why an empty file was not uploaded" do + page.attach_file(Rails.root.join("spec/fixtures/files/empty.pdf")) do + page.find("#documents").click + end + expect(page).to have_text("empty.pdf is empty (0 bytes)") + expect(page).to have_button("Save Topic", disabled: false) + end end context "when updating a topic " do