diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6807ba47c..fbc4e86b4 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -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(", ") @@ -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." diff --git a/app/controllers/works_wizard_controller.rb b/app/controllers/works_wizard_controller.rb index 2d726af18..8956cfd8a 100644 --- a/app/controllers/works_wizard_controller.rb +++ b/app/controllers/works_wizard_controller.rb @@ -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 @@ -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 @@ -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) @@ -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." diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 89fa07dc3..140b0fef5 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -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? diff --git a/app/models/work.rb b/app/models/work.rb index e8fbd0794..31955a3f6 100644 --- a/app/models/work.rb +++ b/app/models/work.rb @@ -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] def uploads return post_curation_uploads if approved? pre_curation_uploads end + # Retrieve the S3 file uploads named "README" + # @return [Array] + 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] + 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. diff --git a/app/services/work_validator.rb b/app/services/work_validator.rb index 3eaa5216c..d068185e8 100644 --- a/app/services/work_validator.rb +++ b/app/services/work_validator.rb @@ -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, @@ -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. Please upload one") end errors.count == 0 end @@ -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. Please upload one") if readme.blank? + errors.add(:base, "You must provide only one README file upload") if work.readme_uploads.length > 1 end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index e6654d71b..a99d49432 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -43,7 +43,7 @@ <% if flash[:notice] %>
- +
<% end %> diff --git a/spec/controllers/works_controller_spec.rb b/spec/controllers/works_controller_spec.rb index 95c929235..0bf6184d8 100644 --- a/spec/controllers/works_controller_spec.rb +++ b/spec/controllers/works_controller_spec.rb @@ -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. Please upload one, You must include at least one file. Please upload one. 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. Please upload one, You must include at least one file. Please upload one. Please contact the PDC Describe administrators for any assistance."]) # rubocop:enable Layout/LineLength end end @@ -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. Please upload one. 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 diff --git a/spec/controllers/works_wizard_controller_spec.rb b/spec/controllers/works_wizard_controller_spec.rb index 8c34eb46d..29b92e26e 100644 --- a/spec/controllers/works_wizard_controller_spec.rb +++ b/spec/controllers/works_wizard_controller_spec.rb @@ -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 @@ -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. Please upload one."]) + # rubocop:enable Layout/LineLength + end + end + end end end diff --git a/spec/requests/works_spec.rb b/spec/requests/works_spec.rb index 51806a191..8a0704334 100644 --- a/spec/requests/works_spec.rb +++ b/spec/requests/works_spec.rb @@ -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 @@ -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 diff --git a/spec/services/work_validator_spec.rb b/spec/services/work_validator_spec.rb index 704d4efe3..c0a6db179 100644 --- a/spec/services/work_validator_spec.rb +++ b/spec/services/work_validator_spec.rb @@ -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. Please upload one") end context "a migrated work" do diff --git a/spec/system/migrate_submission_spec.rb b/spec/system/migrate_submission_spec.rb index c24823ce3..407f2b267 100644 --- a/spec/system/migrate_submission_spec.rb +++ b/spec/system/migrate_submission_spec.rb @@ -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)]) diff --git a/spec/system/work_approve_spec.rb b/spec/system/work_approve_spec.rb index 670543871..9fe5c5cb1 100644 --- a/spec/system/work_approve_spec.rb +++ b/spec/system/work_approve_spec.rb @@ -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 @@ -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