Skip to content

Commit

Permalink
Ensures that one and only one README file is uploaded and that there …
Browse files Browse the repository at this point in the history
…is one or more files uploaded for any given Work
  • Loading branch information
jrgriffiniii committed May 24, 2024
1 parent f735794 commit 5d1fc4e
Show file tree
Hide file tree
Showing 12 changed files with 141 additions and 18 deletions.
4 changes: 2 additions & 2 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def new_session_path(_scope)
#
# @return [String] a combined error message for the work and transition error
#
def message_from_assm_error(aasm_error:, work:)
def message_from_aasm_error(aasm_error:, work:)
message = aasm_error.message
if work.errors.count > 0
message = work.errors.to_a.join(", ")
Expand Down Expand Up @@ -99,7 +99,7 @@ def prepare_decorators_for_work_form(work)
def rescue_aasm_error
yield
rescue AASM::InvalidTransition => error
message = message_from_assm_error(aasm_error: error, work: @work)
message = message_from_aasm_error(aasm_error: error, work: @work)

Honeybadger.notify("Invalid #{@work.current_transition}: #{error.message} errors: #{message}")
transition_error_message = "We apologize, the following errors were encountered: #{message}. Please contact the PDC Describe administrators for any assistance."
Expand Down
26 changes: 24 additions & 2 deletions app/controllers/works_wizard_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ def review
# PATCH /works/1/validate-wizard
def validate
@work.submission_notes = params["submission_notes"]

if params[:save_only] == "true"
@work.save
render :review
Expand Down Expand Up @@ -139,11 +140,19 @@ def readme_uploaded
end
end

def files_param
params["files"]
end

# Uploads the README file, called by Uppy.
# POST /works/1/readme-uploaded-payload
def readme_uploaded_payload
raise StandardError("Only one README file can be uploaded.") if files_param.empty?
raise StandardError("Only one README file can be uploaded.") if files_param.length > 1

readme_file = files_param.first
readme = Readme.new(@work, current_user)
readme_file = params["files"].first

readme_error = readme.attach(readme_file)
if readme_error.nil?
render plain: readme.file_name
Expand All @@ -158,6 +167,19 @@ def file_location_url
end
helper_method :file_location_url

def rescue_aasm_error
yield
rescue AASM::InvalidTransition => error
message = message_from_aasm_error(aasm_error: error, work: @work)

Honeybadger.notify("Invalid #{@work.current_transition}: #{error.message} errors: #{message}")
transition_error_message = "We apologize, the following errors were encountered: #{message}."
@errors = [transition_error_message]

# redirect_to root_url, notice: transition_error_message
redirect_aasm_error(transition_error_message)
end

private

def edit_helper(view_name, redirect_url)
Expand Down Expand Up @@ -204,7 +226,7 @@ def readme_file_param
patch_params[:readme_file]
end

def rescue_aasm_error
def rescue_aasm_error_dis
super
rescue StandardError => generic_error
redirect_to root_url, notice: "We apologize, an error was encountered: #{generic_error.message}. Please contact the PDC Describe administrators."
Expand Down
7 changes: 7 additions & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ def html_tag_attributes
{ lang: I18n.locale }
end

def flash_notice
value = flash[:notice]
# rubocop:disable Rails/OutputSafety
value.html_safe
# rubocop:enable Rails/OutputSafety
end

def pre_curation_uploads_file_name(file:)
value = file.filename.to_s
return if value.blank?
Expand Down
14 changes: 14 additions & 0 deletions app/models/work.rb
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,26 @@ def current_transition
aasm.current_event.to_s.humanize.delete("!")
end

# Retrieve the S3 file uploads associated with the Work
# @return [Array<S3File>]
def uploads
return post_curation_uploads if approved?

pre_curation_uploads
end

# Retrieve the S3 file uploads named "README"
# @return [Array<S3File>]
def readme_uploads
uploads.select { |s3_file| s3_file.filename.include?("README") }
end

# Retrieve the S3 file uploads which are research artifacts proper (not README or other files providing metadata/documentation)
# @return [Array<S3File>]
def artifact_uploads
uploads.reject { |s3_file| s3_file.filename.include?("README") }
end

# Returns the list of files for the work with some basic information about each of them.
# This method is much faster than `uploads` because it does not return the actual S3File
# objects to the client, instead it returns just a few selected data elements.
Expand Down
6 changes: 4 additions & 2 deletions app/services/work_validator.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true
class WorkValidator
include Rails.application.routes.url_helpers
attr_reader :work

delegate :errors, :metadata, :resource, :ark, :doi, :user_entered_doi, :id, :group,
Expand Down Expand Up @@ -51,7 +52,7 @@ def valid_to_approve(user)
end
validate_files
if pre_curation_uploads.empty? && post_curation_uploads.empty?
errors.add :base, "Uploads must be present for a work to be approved"
errors.add(:base, "You must include at least one file. <a href='#{work_file_upload_path(work)}'>Please upload one</a>")
end
errors.count == 0
end
Expand Down Expand Up @@ -161,6 +162,7 @@ def validate_metadata
def validate_files
return if @work.resource.migrated
readme = Readme.new(work, nil)
errors.add(:base, "Must provide a README") if readme.blank?
errors.add(:base, "You must include a README. <a href='#{work_readme_select_path(work)}'>Please upload one</a>") if readme.blank?
errors.add(:base, "You must provide only one README file upload") if work.readme_uploads.length > 1
end
end
2 changes: 1 addition & 1 deletion app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
<% if flash[:notice] %>
<div class="row">
<div class="col">
<div class="alert alert-warning" role="alert" ><%= flash[:notice] %></div>
<div class="alert alert-warning" role="alert" ><%= flash_notice %></div>
</div>
</div>
<% end %>
Expand Down
21 changes: 17 additions & 4 deletions spec/controllers/works_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -725,15 +725,26 @@
end

context "no files attached" do
it "handles aproval errors" do
fake_s3_service = stub_s3 data: [FactoryBot.build(:s3_readme)]
let(:data) do
[
FactoryBot.build(:s3_readme),
FactoryBot.build(:s3_file)
]
end
let(:fake_s3_service) { stub_s3(data:) }
before do
fake_s3_service
work.complete_submission!(user)
allow(fake_s3_service).to receive(:client_s3_files).and_return([])
sign_in curator
end
it "handles aproval errors" do
post :approve, params: { id: work.id }
expect(response.status).to be 302
# rubocop:disable Layout/LineLength
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: Must provide a README, Uploads must be present for a work to be approved. Please contact the PDC Describe administrators for any assistance."])
# binding.pry
# "We apologize, the following errors were encountered: You must include a README. <a href='/works/1/readme-select'>Please upload one</a>, You must include at least one file. <a href='/works/1/file-upload'>Please upload one</a>. Please contact the PDC Describe administrators for any assistance."
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: You must include a README. <a href='#{work_readme_select_path(work)}'>Please upload one</a>, You must include at least one file. <a href='#{work_file_upload_path(work)}'>Please upload one</a>. Please contact the PDC Describe administrators for any assistance."])
# rubocop:enable Layout/LineLength
end
end
Expand Down Expand Up @@ -1263,7 +1274,9 @@
sign_in user
post :validate, params: { id: work.id }
expect(response).to redirect_to(edit_work_path(work))
expect(controller.flash[:notice]).to eq("We apologize, the following errors were encountered: Must provide a README. Please contact the PDC Describe administrators for any assistance.")
# rubocop:disable Layout/LineLength
expect(controller.flash[:notice]).to eq("We apologize, the following errors were encountered: You must include a README. <a href='#{work_readme_select_path(work)}'>Please upload one</a>. Please contact the PDC Describe administrators for any assistance.")
# rubocop:enable Layout/LineLength
end

it "validates a work completes it when there are no errors" do
Expand Down
64 changes: 63 additions & 1 deletion spec/controllers/works_wizard_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@
expect(response.status).to be 302
expect(work.reload).to be_draft
# rubocop:disable Layout/LineLength
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: Must provide a description. Please contact the PDC Describe administrators for any assistance."])
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: Must provide a description."])
# rubocop:enable Layout/LineLength
end
end
Expand All @@ -368,5 +368,67 @@
end
end
end

context "a work with multiple README files" do
describe "#validate" do
before do
stub_s3(data: [
FactoryBot.build(:s3_readme),
FactoryBot.build(:s3_readme)
])
sign_in(user)
end

it "renders the error message" do
work.save
post :validate, params: { id: work.id }
expect(response).to redirect_to(edit_work_wizard_path(work))
expect(response.status).to be 302
expect(work.reload).to be_draft
# rubocop:disable Layout/LineLength
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: You must provide only one README file upload."])
# rubocop:enable Layout/LineLength
end
end
end

context "a work with one README but no uploaded files" do
describe "#validate" do
before do
stub_s3(data: [
FactoryBot.build(:s3_readme)
])
sign_in(user)
post :validate, params: { id: work.id }
work.reload
end

it "advances the Work from the 'draft' state to the 'awaiting_approval' state" do
expect(response).to redirect_to("/works/#{work.id}/complete")
expect(response.status).to be 302
expect(work.state).to eq("awaiting_approval")
end
end
end

context "a work with no README files" do
describe "#validate" do
before do
stub_s3(data: [])
sign_in(user)
end

it "renders the error message" do
work.save
post :validate, params: { id: work.id }
expect(response).to redirect_to(edit_work_wizard_path(work))
expect(response.status).to be 302
expect(work.reload).to be_draft
# rubocop:disable Layout/LineLength
expect(assigns[:errors]).to eq(["We apologize, the following errors were encountered: You must include a README. <a href='#{work_readme_select_path(work)}'>Please upload one</a>."])
# rubocop:enable Layout/LineLength
end
end
end
end
end
4 changes: 2 additions & 2 deletions spec/requests/works_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@

expect(response.status).to eq(200)
# rubocop:disable Layout/LineLength
error_message = ERB::Util.html_escape("We apologize, the following errors were encountered: Event 'approve' cannot transition from 'draft'. Please contact the PDC Describe administrators for any assistance.")
error_message = "We apologize, the following errors were encountered: Event 'approve' cannot transition from 'draft'. Please contact the PDC Describe administrators for any assistance."
# rubocop:enable Layout/LineLength
expect(response.body).to include(error_message)
end
Expand All @@ -328,7 +328,7 @@

expect(response.status).to eq(200)
# rubocop:disable Layout/LineLength
error_message = ERB::Util.html_escape("We apologize, the following errors were encountered: Event 'approve' cannot transition from 'draft'. Please contact the PDC Describe administrators for any assistance.")
error_message = "We apologize, the following errors were encountered: Event 'approve' cannot transition from 'draft'. Please contact the PDC Describe administrators for any assistance."
# rubocop:enable Layout/LineLength
expect(response.body).to include(error_message)
end
Expand Down
4 changes: 3 additions & 1 deletion spec/services/work_validator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@
validator = WorkValidator.new(work)
expect(validator.valid?).to be_truthy # we can still save, we just can not transition to awaiting approval
expect(validator.valid_to_complete).to be_falsey
expect(work.errors.full_messages).to eq(["Must provide a README"])
expect(work.errors.full_messages).not_to be_empty
full_message = work.errors.full_messages[0]
expect(full_message).to eq("You must include a README. <a href='/works/#{work.id}/readme-select'>Please upload one</a>")
end

context "a migrated work" do
Expand Down
2 changes: 1 addition & 1 deletion spec/system/migrate_submission_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@
fill_in "title_main", with: title
click_on "Create"
click_on "Complete"
expect(page).to have_content "Must provide a README"
expect(page).to have_content "You must include a README."

# fake that the user put a file up in globus
allow(fake_s3_service).to receive(:client_s3_files).and_return([FactoryBot.build(:s3_readme)])
Expand Down
5 changes: 3 additions & 2 deletions spec/system/work_approve_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
expect(page).to have_content(work.doi)
click_on "Approve Dataset"
page.driver.browser.switch_to.alert.accept
expect(page).to have_content("Uploads must be present for a work to be approved")
expect(page).to have_content("You must include at least one file. Please upload one.")
end

it "does not display warning if user cancels approval", js: true do
Expand All @@ -29,8 +29,9 @@
click_link work.title
expect(page).to have_content(work.doi)
click_on "Approve Dataset"
sleep(0.5)
page.driver.browser.switch_to.alert.dismiss
expect(page).to_not have_content("Uploads must be present for a work to be approved")
expect(page).not_to have_content("You must include at least one file. Please upload one.")
end
end
end

0 comments on commit 5d1fc4e

Please sign in to comment.