Skip to content

Commit

Permalink
Fixes provenance bug in the Wizard (#1806)
Browse files Browse the repository at this point in the history
  • Loading branch information
hectorcorrea authored May 9, 2024
1 parent 01a2bce commit 0fe77f8
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 96 deletions.
5 changes: 4 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ AllCops:

Layout/IndentationConsistency:
EnforcedStyle: indented_internal_methods

Layout/LineLength:
Exclude:
- "spec/system/data_migration/*"
Expand Down Expand Up @@ -89,3 +89,6 @@ RSpec/ExampleLength:

Style/NumericPredicate:
Enabled: false

Style/Next:
Enabled: false
14 changes: 2 additions & 12 deletions app/controllers/works_wizard_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
36 changes: 0 additions & 36 deletions app/jobs/attach_file_to_work_job.rb

This file was deleted.

22 changes: 19 additions & 3 deletions app/services/work_uploads_edit_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
42 changes: 13 additions & 29 deletions spec/controllers/works_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
17 changes: 4 additions & 13 deletions spec/requests/works_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
3 changes: 1 addition & 2 deletions spec/services/work_upload_edit_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 0fe77f8

Please sign in to comment.