diff --git a/app/controllers/works_wizard_controller.rb b/app/controllers/works_wizard_controller.rb index fbedf74bc..e50739f15 100644 --- a/app/controllers/works_wizard_controller.rb +++ b/app/controllers/works_wizard_controller.rb @@ -16,7 +16,7 @@ class WorksWizardController < ApplicationController before_action :load_work, only: [:edit_wizard, :update_wizard, :attachment_select, :attachment_selected, :file_upload, :file_uploaded, :file_other, :review, :validate, - :readme_select, :readme_uploaded] + :readme_select, :readme_uploaded, :readme_uploaded_payload] # GET /works/1/edit-wizard def edit_wizard @@ -123,21 +123,30 @@ def readme_select @readme = readme.file_name end - # Uploads the readme the user selects - # GET /works/1/readme_uploaded + # Hit when the user clicks "Save" or "Next" on the README upload process. + # Notice that this does not really uploads the file, that happens in readme_uploaded_payload. + # PATCH /works/1/readme_uploaded def readme_uploaded readme = Readme.new(@work, current_user) - readme_error = readme.attach(readme_file_param) + if params[:save_only] == "true" + @readme = readme.file_name + render :readme_select + else + redirect_to work_attachment_select_url(@work) + end + end + + # Uploads the README file, called by Uppy. + # POST /works/1/readme-uploaded-payload + def readme_uploaded_payload + readme = Readme.new(@work, current_user) + readme_file = params["files"].first + readme_error = readme.attach(readme_file) if readme_error.nil? - if params[:save_only] == "true" - @readme = readme.file_name - render :readme_select - else - redirect_to work_attachment_select_url(@work) - end + render plain: readme.file_name else - flash[:notice] = readme_error - redirect_to work_readme_select_url(@work) + # Tell Uppy there was an error uploading the README + render plain: readme.file_name, status: :internal_server_error end end diff --git a/app/javascript/entrypoints/pdc/pdc_ui_loader.es6 b/app/javascript/entrypoints/pdc/pdc_ui_loader.es6 index fab3f2753..2575774f1 100644 --- a/app/javascript/entrypoints/pdc/pdc_ui_loader.es6 +++ b/app/javascript/entrypoints/pdc/pdc_ui_loader.es6 @@ -1,10 +1,10 @@ import CopytoClipboard from './copy_to_clipboard.es6'; import EditRequiredFields from './edit_required_fields.es6'; -import ReadmeFileUpload from './readme_file_upload.es6'; import WorkOrcid from './work_orcid.es6'; import WorkRoR from './work_ror.es6'; import EditTableActions from './edit_table_actions.es6'; import WorkEditFileUpload from './work_edit_file_upload.es6'; +import WorkReadmeFileUpload from './work_readme_file_upload.es6'; import EmailChangeAll from './email_change_all.es6'; /* eslint class-methods-use-this: ["error", { "exceptMethods": ["setup_fileupload_validation"] }] */ @@ -19,8 +19,8 @@ export default class PdcUiLoader { (new EditRequiredFields()).attach_validations(); (new EditTableActions()).attach_actions('creators-table'); (new EmailChangeAll()).attach_change(); - (new ReadmeFileUpload('patch_readme_file', 'readme-upload')).attach_validation(); (new WorkEditFileUpload('pre_curation_uploads_added', 'file-upload-list')).attach_validation(); + (new WorkReadmeFileUpload('add-readme', 'file-upload-area')).attach_validation(); (new WorkOrcid('.orcid-entry-creator', 'creators[][given_name]', 'creators[][family_name]')).attach_validation(); (new WorkOrcid('.orcid-entry-contributor', 'contributors[][given_name]', 'contributors[][family_name]')).attach_validation(); (new WorkRoR(pdc.ror_url)).attach_query(); diff --git a/app/javascript/entrypoints/pdc/readme_file_upload.es6 b/app/javascript/entrypoints/pdc/readme_file_upload.es6 deleted file mode 100644 index 098fb5976..000000000 --- a/app/javascript/entrypoints/pdc/readme_file_upload.es6 +++ /dev/null @@ -1,27 +0,0 @@ -export default class ReadmeFileUpload { - constructor(uploadId, saveId, errorId = 'file-error') { - this.upload_element = $(`#${uploadId}`); - this.save_element = document.getElementById(saveId); - this.error_element = document.getElementById(errorId); - } - - attach_validation() { - this.upload_element.on('change', this.validate.bind(this)); - } - - validate() { - if (this.upload_element[0].files.length < 1) { - this.save_element.disabled = true; - this.error_element.innerText = 'You must select a README file'; - } else { - const filename = this.upload_element[0].files[0].name.toLowerCase(); - if (filename.includes('readme') === true) { - this.save_element.disabled = false; - this.error_element.innerText = ''; - } else { - this.save_element.disabled = true; - this.error_element.innerText = 'You must select a file that includes the word README in the name (lowercase or uppercase is accepted)'; - } - } - } -} diff --git a/app/javascript/entrypoints/pdc/work_readme_file_upload.es6 b/app/javascript/entrypoints/pdc/work_readme_file_upload.es6 new file mode 100644 index 000000000..83dc24477 --- /dev/null +++ b/app/javascript/entrypoints/pdc/work_readme_file_upload.es6 @@ -0,0 +1,73 @@ +/* global Uppy */ +export default class WorkReadmeFileUpload { + constructor(triggerButtonId, uppyAreaId) { + this.triggerButtonId = triggerButtonId; + this.uppyAreaId = uppyAreaId; + } + + attach_validation() { + const uploadUrl = $('#uppy_upload_url').text(); + if (uploadUrl !== '') { + WorkReadmeFileUpload.setupUppy(this.triggerButtonId, this.uppyAreaId, uploadUrl); + } + } + + // Setup Uppy to handle file uploads + // References: + // https://uppy.io/docs/quick-start/ + // https://davidwalsh.name/uppy-file-uploading + static setupUppy(triggerButtonId, uppyAreaId, uploadUrl) { + // https://uppy.io/blog/2018/08/0.27/#autoproceed-false-by-default + // https://uppy.io/docs/uppy/#restrictions + const uppy = Uppy.Core({ + autoProceed: true, + restrictions: { + maxNumberOfFiles: 1, + allowedFileTypes: ['.txt', '.md'], + }, + onBeforeUpload(files) { + // source: https://github.com/transloadit/uppy/issues/1703#issuecomment-507202561 + if (Object.entries(files).length === 1) { + const file = Object.entries(files)[0][1]; + if (file.meta.name.toLowerCase().includes('readme') === true) { + // we are good + return true; + } + } + uppy.info('You must select a file that includes the word README in the name', 'error'); + return false; + }, + }); + + // Configure the initial display (https://uppy.io/docs/dashboard) + uppy.use(Uppy.Dashboard, { + target: `#${uppyAreaId}`, + inline: false, // display of dashboard only when trigger is clicked + trigger: `#${triggerButtonId}`, + }); + + // We use the XHRUploader, this is the most basic uploader (https://uppy.io/docs/xhr-upload/) + // X-CSRF-Token: https://stackoverflow.com/a/75050497/446681 + const token = document.querySelector("meta[name='csrf-token']"); + let tokenContent; + if (token) { + tokenContent = token.content; + } else { + tokenContent = ''; + } + uppy.use(Uppy.XHRUpload, { + endpoint: uploadUrl, + headers: { 'X-CSRF-Token': tokenContent }, + bundle: true, // upload all selected files at once + formData: true, // required when bundle: true + getResponseData(filename) { + $('#new-readme').html(`File ${filename} has been uploaded and set as the README for this dataset.`); + $('#readme-upload').prop('disabled', false); + }, + }); + + // Prevent the button's click from submitting the form since the + // files' payload is automatically submitted by Uppy + $(`#${triggerButtonId}`).on('click', () => false); + } +} diff --git a/app/views/works_wizard/readme_select.html.erb b/app/views/works_wizard/readme_select.html.erb index 65ff8153e..5f6eef727 100644 --- a/app/views/works_wizard/readme_select.html.erb +++ b/app/views/works_wizard/readme_select.html.erb @@ -13,15 +13,18 @@

<%= t('works.form.readme_upload.title') %>

If you have not created a README before, a <%= link_to("template may be found here","https://drive.google.com/file/d/1LCKPj9XxpwJeHYZV2kXROM79xbiBKnkp/view?usp=sharing", target: "blank") %>. See the PRDS website for more guidance on <%= link_to("how to create a README","https://researchdata.princeton.edu/research-lifecycle-guide/readmes-research-data", target: "blank") %>.

-

<%= t('works.form.readme_upload.actions') %>

+ +
<% if @readme.present? %> -

<%= @readme %> was previously uploaded. You will replace it if you select a different file.

+

<%= @readme %> was previously uploaded. You will replace it if you select a different file.

+ + <% else %> +

+ <% end %> - - <%= f.file_field(:readme_file, multiple: false, accept: '.txt,.md') %>
@@ -33,3 +36,5 @@ <% end %>
+ +<%= javascript_include_tag 'edit_work_utils' %> diff --git a/config/routes.rb b/config/routes.rb index da7f5e6be..080a7a0aa 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,6 +36,7 @@ # The work wizard get "works/:id/readme-select", to: "works_wizard#readme_select", as: :work_readme_select patch "works/:id/readme-uploaded", to: "works_wizard#readme_uploaded", as: :work_readme_uploaded + post "works/:id/readme-uploaded-payload", to: "works_wizard#readme_uploaded_payload", as: :work_readme_uploaded_payload patch "works/:id/file-upload", to: "works_wizard#file_uploaded", as: :work_file_uploaded get "works/:id/file-upload", to: "works_wizard#file_upload", as: :work_file_upload patch "works/:id/update-wizard", to: "works_wizard#update_wizard", as: :update_work_wizard diff --git a/spec/controllers/works_wizard_controller_spec.rb b/spec/controllers/works_wizard_controller_spec.rb index 45e0a656d..4ed43124a 100644 --- a/spec/controllers/works_wizard_controller_spec.rb +++ b/spec/controllers/works_wizard_controller_spec.rb @@ -101,7 +101,6 @@ it "redirects to file-upload" do post(:readme_uploaded, params:) expect(response.status).to be 302 - expect(fake_readme).to have_received(:attach) expect(response.location).to eq "http://test.host/works/#{work.id}/attachment-select" end @@ -111,19 +110,44 @@ it "stays on the readme select page" do post :readme_uploaded, params: save_only_params expect(response.status).to be 200 - expect(fake_readme).to have_received(:attach) expect(response).to render_template(:readme_select) expect(assigns[:readme]).to eq("abc123") end end + end - context "the upload encounters an error" do - let(:attach_status) { "An error occured" } + describe "#readme_uploaded_payload" do + let(:attach_status) { nil } + let(:fake_readme) { instance_double Readme, attach: attach_status, "blank?": true, file_name: "abc123" } + let(:params) do + { + "_method" => "post", + "authenticity_token" => "MbUfIQVvYoCefkOfSpzyS0EOuSuOYQG21nw8zgg2GVrvcebBYI6jy1-_3LSzbTg9uKgehxWauYS8r1yxcN1Lwg", + "files" => [uploaded_file], + "commit" => "Continue", + "controller" => "works", + "action" => "file_uploaded", + "id" => work.id + } + end + + before do + allow(Readme).to receive(:new).and_return(fake_readme) + sign_in user + end + + context "when the upload succeeds" do + it "returns status 200" do + post(:readme_uploaded_payload, params:) + expect(response.status).to be 200 + end + end - it "Stays on the same page" do - post(:readme_uploaded, params:) - expect(response).to redirect_to(work_readme_select_path(work)) - expect(controller.flash[:notice]).to eq("An error occured") + context "when the upload throws an error" do + let(:attach_status) { "something went wrong" } + it "returns status 500" do + post(:readme_uploaded_payload, params:) + expect(response.status).to be 500 end end end diff --git a/spec/support/uppy_specs.rb b/spec/support/uppy_specs.rb index af514c8cc..26c10ce61 100644 --- a/spec/support/uppy_specs.rb +++ b/spec/support/uppy_specs.rb @@ -6,4 +6,9 @@ # not using the browser's standard upload file button. def attach_file_via_uppy(file_name) Rack::Test::UploadedFile.new(File.open(file_name)) + if block_given? + # This is used to force the execution of the JavaScript that Uppy + # would execute on its own. + yield + end end diff --git a/spec/system/authz_submitter_spec.rb b/spec/system/authz_submitter_spec.rb index 7658774ac..37871418f 100644 --- a/spec/system/authz_submitter_spec.rb +++ b/spec/system/authz_submitter_spec.rb @@ -40,8 +40,8 @@ expect(page).to have_content("These metadata properties are not required") # testing additional metadata page click_on "Next" path = Rails.root.join("spec", "fixtures", "files", "readme.txt") - attach_file(path) do - page.find("#patch_readme_file").click + attach_file_via_uppy(path) do + page.execute_script("$('#readme-upload').prop('disabled', false)") end click_on "Next" page.find(:xpath, "//input[@value='file_other']").choose diff --git a/spec/system/authz_super_admin_spec.rb b/spec/system/authz_super_admin_spec.rb index cda10d7ae..3d00a1b4a 100644 --- a/spec/system/authz_super_admin_spec.rb +++ b/spec/system/authz_super_admin_spec.rb @@ -38,8 +38,8 @@ expect(page).to have_content("These metadata properties are not required") # testing additional metadata page click_on "Next" path = Rails.root.join("spec", "fixtures", "files", "readme.txt") - attach_file(path) do - page.find("#patch_readme_file").click + attach_file_via_uppy(path) do + page.execute_script("$('#readme-upload').prop('disabled', false)") end click_on "Next" page.find(:xpath, "//input[@value='file_other']").choose diff --git a/spec/system/external_ids_spec.rb b/spec/system/external_ids_spec.rb index ac74e7106..46af7effc 100644 --- a/spec/system/external_ids_spec.rb +++ b/spec/system/external_ids_spec.rb @@ -29,8 +29,8 @@ expect(page).to have_content("These metadata properties are not required") # testing additional metadata page click_on "Next" path = Rails.root.join("spec", "fixtures", "files", "readme.txt") - attach_file(path) do - page.find("#patch_readme_file").click + attach_file_via_uppy(path) do + page.execute_script("$('#readme-upload').prop('disabled', false)") end click_on "Next" click_on "Next" diff --git a/spec/system/pppl_work_create_spec.rb b/spec/system/pppl_work_create_spec.rb index 92d3ced56..c573a07ad 100644 --- a/spec/system/pppl_work_create_spec.rb +++ b/spec/system/pppl_work_create_spec.rb @@ -51,8 +51,8 @@ expect(page).to have_content("Please upload the README") expect(page).to have_button("Next", disabled: true) path = Rails.root.join("spec", "fixtures", "files", "readme.txt") - attach_file(path) do - page.find("#patch_readme_file").click + attach_file_via_uppy(path) do + page.execute_script("$('#readme-upload').prop('disabled', false)") end click_on "Next" diff --git a/spec/system/work_create_spec.rb b/spec/system/work_create_spec.rb index 8aa94bcc7..a517ab870 100644 --- a/spec/system/work_create_spec.rb +++ b/spec/system/work_create_spec.rb @@ -249,8 +249,8 @@ expect(page).to have_button("Next", disabled: true) path = Rails.root.join("spec", "fixtures", "files", "readme.txt") - attach_file(path) do - page.find("#patch_readme_file").click + attach_file_via_uppy(path) do + page.execute_script("$('#readme-upload').prop('disabled', false)") end click_on "Next" @@ -413,11 +413,9 @@ # We on purpose upload a non-read me file... path = Rails.root.join("spec", "fixtures", "files", "orcid.csv") - attach_file(path) do - page.find("#patch_readme_file").click - end - # ...and we expect and error message to be displayed and the button to continue to remain disabled - expect(page).to have_content("You must select a file that includes the word README in the name") + attach_file_via_uppy(path) + + # ...and we expect the button to continue to remain disabled expect(page).to have_button("Next", disabled: true) end end