Fixes Wizard delay in displaying uploaded files (#1762)

* Testing uploading files to AWS right away (instead of as a background job)

* Quick hack to add files directly to AWS

* Undo previous hack

* Added an explicit endpoint to the Wizard controlller to upload files. Moved the logic to upload a file to the controller instead of my hack via the Readme class.

* Removing intermidiary upload snapshot

* Allowing the upload snapshot reload to have a user who is responsible

* Remove additional delete note

* Assign the work to an acutal work model

* Fixing tests and rubocop violation

* Doing some cleanup

---------

Co-authored-by: Carolyn Cole <[email protected]>

app/controllers/works_controller.rb

@@ -214,6 +214,7 @@ def bibtex
     send_data bibtex, filename: "#{citation.bibtex_id}.bibtex", type: "text/plain", disposition: "attachment"
   end
 
+  # POST /works/1/upload-files (called via Uppy)
   def upload_files
     @work = Work.find(params[:id])
     upload_service = WorkUploadsEditService.new(@work, current_user)

app/controllers/works_wizard_controller.rb

@@ -60,13 +60,20 @@ def file_upload
     @work_decorator = WorkDecorator.new(@work, current_user)
   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) }
+  end
+
   # POST /works/1/file_upload
   def file_uploaded
     upload_service = WorkUploadsEditService.new(@work, current_user)
     # By the time we hit this endpoint files have been uploaded by Uppy submmitting POST requests
-    # to /works/1/upload-files therefore we only need to handle deleted files here.
-    @work = upload_service.update_precurated_file_list([], deleted_files_param)
-    @work.reload_snapshots
+    # to /works/1/upload-files-wizard therefore we only need to delete files here and update the upload snapshot.
+    @work = upload_service.snapshot_uppy_and_delete_files(deleted_files_param)
+
     prepare_decorators_for_work_form(@work)
     if params[:save_only] == "true"
       render :file_upload
@@ -198,5 +205,14 @@ 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
-# rubocop:enable Metrics/ClassLength

app/models/work.rb

@@ -436,8 +436,9 @@ def past_snapshots
   end
 
   # Build or find persisted UploadSnapshot models for this Work
+  # @param [integer] user_id optional user to assign the snapshot to
   # @return [UploadSnapshot]
-  def reload_snapshots
+  def reload_snapshots(user_id: nil)
     work_changes = []
     s3_files = pre_curation_uploads_fast
     s3_filenames = s3_files.map(&:filename)
@@ -453,7 +454,7 @@ def reload_snapshots
       new_snapshot = UploadSnapshot.new(work: self, url: s3_query_service.prefix)
       new_snapshot.store_files(s3_files)
       new_snapshot.save!
-      WorkActivity.add_work_activity(id, work_changes.to_json, nil, activity_type: WorkActivity::FILE_CHANGES)
+      WorkActivity.add_work_activity(id, work_changes.to_json, user_id, activity_type: WorkActivity::FILE_CHANGES)
     end
   end
 

app/services/work_uploads_edit_service.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 class WorkUploadsEditService
-  attr_reader :work, :s3_service
+  attr_reader :work, :s3_service, :current_user
 
   def initialize(work, current_user)
     @work = work
@@ -19,6 +19,20 @@ def update_precurated_file_list(added_files, deleted_files)
     work
   end
 
+  # Delete any files the user has decided not to keep and
+  #  add all files that were uploaded in the backgroud via uppy and any files deleted to an upload snapshot
+  #
+  # @param [Array] deleted_files files that exist in AWS that should be removed
+  def snapshot_uppy_and_delete_files(deleted_files)
+    deleted_files.each do |filename|
+      s3_service.delete_s3_object(filename)
+    end
+
+    # assigns all backgroun changes and deletes to the current user
+    work.reload_snapshots(user_id: current_user.id)
+    work
+  end
+
   def find_post_curation_uploads(upload_keys: [])
     return [] unless work.approved? && !upload_keys.empty?
     work.post_curation_uploads.select { |upload| upload_keys.include?(upload.key) }
@@ -31,7 +45,7 @@ def delete_uploads(deleted_files)
 
       deleted_files.each do |filename|
         s3_service.delete_s3_object(filename)
-        work.track_change(:deleted, filename)
+        work.track_change(:removed, filename)
       end
       work.log_file_changes(@current_user.id)
     end

app/views/works_wizard/file_upload.html.erb

@@ -10,7 +10,7 @@
         <%= render(partial: 'works/s3_resources', locals: { edit_mode: true, form: f }) %>
       </section>
       <div class="container-fluid deposit-uploads">
-        <span id="uppy_upload_url" class="hidden"><%= work_upload_files_path(@work) %></span>
+        <span id="uppy_upload_url" class="hidden"><%= work_wizard_upload_files_path(@work) %></span>
         <div id="file-upload-area"></div>
         <button id="add-files-button" class="btn btn-secondary" style="width: 200px;"><i class="bi bi-plus-circle"></i> Add Files</button>
         <div id="file-error" class="error_box"></div>

config/routes.rb

@@ -49,6 +49,7 @@
   get "works/:id/attachment-select", to: "works_wizard#attachment_select", as: :work_attachment_select
   post "works/:id/attachment-select", to: "works_wizard#attachment_selected", as: :work_attachment_selected
   patch "works/:id/attachment-select", to: "works_wizard#attachment_selected"
+  post "works/:id/upload-files-wizard", to: "works_wizard#upload_files", as: :work_wizard_upload_files
 
   get "works/:id/update-additional", to: "works_update_additional#update_additional", as: :work_update_additional
   patch "works/:id/update-additional", to: "works_update_additional#update_additional_save"

spec/services/work_upload_edit_service_spec.rb

@@ -88,7 +88,7 @@
       # it logs the addition (and no delete)
       activity_log = JSON.parse(updated_work.work_activity.first.message)
       expect(activity_log.find { |log| log["action"] == "added" && log["filename"].include?(s3_file3.filename_display) }).not_to be nil
-      expect(activity_log.find { |log| log["action"] == "deleted" }).to be nil
+      expect(activity_log.find { |log| log["action"] == "removed" }).to be nil
     end
   end
 
@@ -108,7 +108,7 @@
 
       # it logs the delete (and no additions)
       activity_log = JSON.parse(work.work_activity.first.message)
-      expect(activity_log.find { |log| log["action"] == "deleted" && log["filename"] == s3_data[0].filename }).not_to be nil
+      expect(activity_log.find { |log| log["action"] == "removed" && log["filename"] == s3_data[0].filename }).not_to be nil
       expect(activity_log.find { |log| log["action"] == "added" }).to be nil
     end
   end
@@ -132,7 +132,7 @@
       work_activities = work.work_activity
       expect(work_activities.count).to eq(2) # one for the deletes and one for the adds
       activity_log = work_activities.map { |work_activity| JSON.parse(work_activity.message) }.flatten
-      expect(activity_log.find { |log| log["action"] == "deleted" && log["filename"].include?(s3_file1.key) }).not_to be nil
+      expect(activity_log.find { |log| log["action"] == "removed" && log["filename"].include?(s3_file1.key) }).not_to be nil
       expect(activity_log.find { |log| log["action"] == "added" && log["filename"].include?("us_covid_2020.csv") }).not_to be nil
       expect(activity_log.find { |log| log["action"] == "added" && log["filename"].include?("orcid.csv") }).not_to be nil
     end
@@ -159,8 +159,8 @@
       work_activities = updated_work.work_activity
       expect(work_activities.count).to eq(2) # one for the deletes and one for the adds
       activity_log = work_activities.map { |work_activity| JSON.parse(work_activity.message) }.flatten
-      expect(activity_log.find { |log| log["action"] == "deleted" && log["filename"].include?("us_covid_2019.csv") }).not_to be nil
-      expect(activity_log.find { |log| log["action"] == "deleted" && log["filename"].include?("us_covid_2020.csv") }).not_to be nil
+      expect(activity_log.find { |log| log["action"] == "removed" && log["filename"].include?("us_covid_2019.csv") }).not_to be nil
+      expect(activity_log.find { |log| log["action"] == "removed" && log["filename"].include?("us_covid_2020.csv") }).not_to be nil
       expect(activity_log.find { |log| log["action"] == "added" && log["filename"].include?("us_covid_2020.csv") }).not_to be nil
       expect(activity_log.find { |log| log["action"] == "added" && log["filename"].include?("orcid.csv") }).not_to be nil
     end