diff --git a/.rubocop.yml b/.rubocop.yml index 8a7262e20..6d7e5bf98 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -17,7 +17,7 @@ AllCops: Layout/IndentationConsistency: EnforcedStyle: indented_internal_methods - + Layout/LineLength: Exclude: - "spec/system/data_migration/*" @@ -89,3 +89,6 @@ RSpec/ExampleLength: Style/NumericPredicate: Enabled: false + +Style/Next: + Enabled: false diff --git a/app/controllers/works_wizard_controller.rb b/app/controllers/works_wizard_controller.rb index e7f73a919..967bf2db0 100644 --- a/app/controllers/works_wizard_controller.rb +++ b/app/controllers/works_wizard_controller.rb @@ -61,10 +61,10 @@ def file_upload end # POST /works/1/upload-files-wizard (called via Uppy) - # this puts the files into AWS, but does not capture anything in the Upload Snapshot. THat occurs when the user hits next def upload_files @work = Work.find(params[:id]) - params["files"].each { |file| upload_file(file) } + upload_service = WorkUploadsEditService.new(@work, current_user) + upload_service.update_precurated_file_list(params["files"], []) end # POST /works/1/file_upload @@ -220,14 +220,4 @@ def redirect_aasm_error(transition_error_message) redirect_to work_create_new_submission_path(@work), notice: transition_error_message, params: end end - - def upload_file(file) - key = @work.s3_query_service.upload_file(io: file.to_io, filename: file.original_filename, size: file.size) - if key.blank? - # TODO: Can we tell uppy there was a failure instead of just logging the error here - # you can render json: {}, status: 500 but how do you tell one file failed instead of all of them - Rails.logger.error("Error uploading #{file.original_filename} to work #{@work.id}") - Honeybadger.notify("Error uploading #{file.original_filename} to work #{@work.id}") - end - end end diff --git a/app/jobs/attach_file_to_work_job.rb b/app/jobs/attach_file_to_work_job.rb deleted file mode 100644 index 6c098438b..000000000 --- a/app/jobs/attach_file_to_work_job.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -class AttachFileToWorkJob < ApplicationJob - queue_as :default - - def perform(file_path:, file_name:, size:, background_upload_snapshot_id:) - @file_path = file_path - @file_name = file_name - @size = size - - @background_upload_snapshot_id = background_upload_snapshot_id - - File.open(file_path) do |file| - unless work.s3_query_service.upload_file(io: file.to_io, filename: file_name, size: @size) - raise "An error uploading #{file_name} was encountered for work #{work}" - end - end - File.delete(file_path) - - background_upload_snapshot.with_lock do - background_upload_snapshot.reload - background_upload_snapshot.mark_complete(file_name, work.s3_query_service.last_response.etag.delete('"')) - background_upload_snapshot.save! - end - end - - private - - def background_upload_snapshot - @background_upload_snapshot ||= BackgroundUploadSnapshot.find(@background_upload_snapshot_id) - end - - def work - @work ||= background_upload_snapshot.work - end -end diff --git a/app/services/work_uploads_edit_service.rb b/app/services/work_uploads_edit_service.rb index 836e1cec0..2a54c6528 100644 --- a/app/services/work_uploads_edit_service.rb +++ b/app/services/work_uploads_edit_service.rb @@ -53,14 +53,30 @@ def delete_uploads(deleted_files) def add_uploads(added_files) return if added_files.empty? + # Update the upload snapshot to reflect the files the user wants to add... last_snapshot = work.upload_snapshots.first snapshot = BackgroundUploadSnapshot.new(work:) snapshot.store_files(added_files, pre_existing_files: last_snapshot&.files, current_user: @current_user) snapshot.save + + # ...adds the file to AWS directly and mark them as complete in the snapshot added_files.map do |file| - new_path = "/tmp/#{file.original_filename}" - FileUtils.mv(file.path, new_path) - AttachFileToWorkJob.perform_later(file_path: new_path, file_name: file.original_filename, size: file.size, background_upload_snapshot_id: snapshot.id) + if upload_file(file) + snapshot.mark_complete(file.original_filename, work.s3_query_service.last_response.etag.delete('"')) + snapshot.save! + File.delete(file.path) # delete the local copy + end + end + end + + def upload_file(file) + key = work.s3_query_service.upload_file(io: file.to_io, filename: file.original_filename, size: file.size) + if key.blank? + Rails.logger.error("Error uploading #{file.original_filename} to work #{work.id}") + Honeybadger.notify("Error uploading #{file.original_filename} to work #{work.id}") + false + else + true end end end diff --git a/spec/controllers/works_controller_spec.rb b/spec/controllers/works_controller_spec.rb index f307aec49..95c929235 100644 --- a/spec/controllers/works_controller_spec.rb +++ b/spec/controllers/works_controller_spec.rb @@ -157,7 +157,6 @@ before do stub_request(:put, /#{bucket_url}/).to_return(status: 200) allow(fake_s3_service).to receive(:client_s3_files).and_return([file1]) - allow(AttachFileToWorkJob).to receive(:perform_later) end it "handles the update page" do @@ -185,12 +184,8 @@ background_snapshot = BackgroundUploadSnapshot.last expect(background_snapshot.work).to eq(work) expect(background_snapshot.files.map { |file| file["user_id"] }.uniq).to eq([user.id]) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, - size: 92, - file_name: "us_covid_2019.csv", - file_path: anything - ) + expect(background_snapshot.files.first["filename"]).to eq "10.34770/123-abc/1us_covid_2019.csv" + expect(background_snapshot.files.first["upload_status"]).to eq "complete" end end @@ -222,7 +217,6 @@ # This is utilized for active record to send the file to S3 stub_request(:put, /#{bucket_url}/).to_return(status: 200) allow(fake_s3_service).to receive(:client_s3_files).and_return([file1, file2]) - allow(AttachFileToWorkJob).to receive(:perform_later) end it "handles the update page" do @@ -250,18 +244,11 @@ background_snapshot = BackgroundUploadSnapshot.last expect(background_snapshot.work).to eq(work) expect(background_snapshot.files.map { |file| file["user_id"] }.uniq).to eq([user.id]) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, - size: 92, - file_name: "us_covid_2019.csv", - file_path: anything - ) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, - size: 114, - file_name: "us_covid_2020.csv", - file_path: anything - ) + + file1 = background_snapshot.files.find { |file| file["filename"] == "10.34770/123-abc/1us_covid_2019.csv" } + expect(file1["upload_status"]).to eq "complete" + file2 = background_snapshot.files.find { |file| file["filename"] == "10.34770/123-abc/1us_covid_2020.csv" } + expect(file2["upload_status"]).to eq "complete" end end @@ -536,7 +523,6 @@ fake_s3_service stub_request(:put, /#{bucket_url}/).to_return(status: 200) stub_request(:delete, /#{bucket_url}/).to_return(status: 200) - allow(AttachFileToWorkJob).to receive(:perform_later) sign_in user end @@ -557,14 +543,12 @@ background_snapshot = BackgroundUploadSnapshot.last expect(background_snapshot.work).to eq(work) expect(background_snapshot.files.map { |file| file["user_id"] }.uniq).to eq([user.id]) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, size: 114, - file_name: uploaded_files.first.original_filename, file_path: anything - ) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, size: 92, - file_name: uploaded_files.last.original_filename, file_path: anything - ) + + file1 = background_snapshot.files.find { |file| file["filename"].include?(uploaded_files.first.original_filename) } + expect(file1["upload_status"]).to eq "complete" + + file2 = background_snapshot.files.find { |file| file["filename"].include?(uploaded_files.last.original_filename) } + expect(file2["upload_status"]).to eq "complete" end end diff --git a/spec/requests/works_spec.rb b/spec/requests/works_spec.rb index 48e1ce4a7..51806a191 100644 --- a/spec/requests/works_spec.rb +++ b/spec/requests/works_spec.rb @@ -161,7 +161,6 @@ # This is utilized for active record to send the file to S3 stub_request(:put, /#{bucket_url}/).to_return(status: 200) allow(fake_s3_service).to receive(:client_s3_files).and_return([], [file1, file2]) - allow(AttachFileToWorkJob).to receive(:perform_later) stub_ark patch work_url(work), params: params @@ -175,18 +174,10 @@ background_snapshot = BackgroundUploadSnapshot.last expect(background_snapshot.work).to eq(work) expect(background_snapshot.files.map { |file| file["user_id"] }.uniq).to eq([user.id]) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, - size: 92, - file_name: "us_covid_2019.csv", - file_path: anything - ) - expect(AttachFileToWorkJob).to have_received(:perform_later).with( - background_upload_snapshot_id: background_snapshot.id, - size: 114, - file_name: "us_covid_2020.csv", - file_path: anything - ) + expect(background_snapshot.files.first["filename"]).to eq "10.34770/123-abc/1us_covid_2019.csv" + expect(background_snapshot.files.first["upload_status"]).to eq "complete" + expect(background_snapshot.files.second["filename"]).to eq "10.34770/123-abc/1us_covid_2020.csv" + expect(background_snapshot.files.second["upload_status"]).to eq "complete" end end diff --git a/spec/services/work_upload_edit_service_spec.rb b/spec/services/work_upload_edit_service_spec.rb index 6fbb6c459..1a6fcce7f 100644 --- a/spec/services/work_upload_edit_service_spec.rb +++ b/spec/services/work_upload_edit_service_spec.rb @@ -80,9 +80,8 @@ upload_service = described_class.new(work, user) updated_work = nil expect { updated_work = upload_service.update_precurated_file_list(added_files, deleted_files) }.to change { BackgroundUploadSnapshot.count }.by 1 - perform_enqueued_jobs - expect(updated_work.pre_curation_uploads.map(&:filename).sort).to eq([s3_file1.key, s3_file2.key].sort) + expect(updated_work.pre_curation_uploads.map(&:filename).sort).to eq([s3_file1.key, s3_file2.key, s3_file3.key].sort) expect(fake_s3_service).not_to have_received(:delete_s3_object) # it logs the addition (and no delete)