Allow S3 synchronization on show of Object (#451)

Remove synchronization from validation
Remove S3 stubbing on every test and only have it when needed

app/controllers/works_controller.rb

@@ -65,6 +65,8 @@ def new_submission
   # When requested as .json, return the internal json resource
   def show
     @work = Work.find(params[:id])
+    # check if anything was added in S3 since we last viewed this object
+    @work.attach_s3_resources
     respond_to do |format|
       format.html do
         @can_curate = current_user.can_admin?(@work.collection)
@@ -119,6 +121,9 @@ def update
     end
 
     if @work.update(updates)
+      # pause to allow s3 time to remove the file completely
+      sleep(0.1) if work_params.key?(:deleted_uploads)
+
       if @wizard_mode
         redirect_to work_attachment_select_url(@work)
       else

app/models/work.rb

@@ -14,8 +14,6 @@ class Work < ApplicationRecord
 
   attr_accessor :user_entered_doi
 
-  validates_with Validators::WorkValidator
-
   include AASM
 
   aasm column: :state do
@@ -148,6 +146,7 @@ def works_mentioned_by_user_state(user, state)
   before_save do |work|
     # Ensure that the metadata JSON is persisted properly
     work.metadata = work.resource.to_json
+    work.save_pre_curation_uploads
   end
 
   after_save do |work|

app/views/works/_uploads_form.html.erb

@@ -36,7 +36,7 @@
                       <input id="work-deposit-uploads" name="work[replaced_uploads][<%= upload.id %>]" type="file" />
                     </td>
                     <td>
-                      <input id="work-deposit-uploads" name="work[deleted_uploads][<%= upload.key %>]" type="checkbox" value="1"/>
+                      <input id="work-uploads-<%= upload.id %>-delete" name="work[deleted_uploads][<%= upload.key %>]" type="checkbox" value="1"/>
                     </td>
                     <td></td>
                   </tr>

config/storage.yml

@@ -12,6 +12,9 @@ amazon: &amazon
   region: <%= S3QueryService.pre_curation_config.fetch(:region) %>
   bucket: <%= S3QueryService.pre_curation_config.fetch(:bucket) %>
 
+# development: &development
+#   <<: *amazon
+
 amazon_pre_curation:
   <% if Rails.env.development? %>
     <<: *development

spec/controllers/works_controller_spec.rb

@@ -29,6 +29,7 @@
 
     it "renders the resource json" do
       sign_in user
+      stub_s3
       get :show, params: { id: work.id, format: "json" }
       expect(response.content_type).to eq "application/json; charset=utf-8"
     end
@@ -485,7 +486,7 @@
           stub_request(:put, "https://api.datacite.org/dois/#{work.doi}").to_return(status: 200, body: "", headers: {})
           work.approve!(user)
 
-          work.valid?
+          work.attach_s3_resources
           sign_in user
         end
 

spec/models/work_spec.rb

@@ -123,7 +123,17 @@
     let(:uploaded_file2) do
       fixture_file_upload("us_covid_2019.csv", "text/csv")
     end
-    let(:s3_client) { @s3_client }
+    let(:s3_client) do
+      s3_client = instance_double(Aws::S3::Client)
+      pre_curated_data_profile = { objects: [] }
+      pre_curated_query_service = instance_double(S3QueryService)
+
+      allow(pre_curated_query_service).to receive(:data_profile).and_return(pre_curated_data_profile)
+      allow(pre_curated_query_service).to receive(:bucket_name).and_return("example-bucket")
+      allow(pre_curated_query_service).to receive(:client).and_return(s3_client)
+      allow(S3QueryService).to receive(:new).and_return(pre_curated_query_service)
+      s3_client
+    end
 
     before do
       stub_request(:delete, /#{attachment_url}/).to_return(status: 200)
@@ -622,15 +632,18 @@
       end
 
       it "persists S3 Bucket resources as ActiveStorage Attachments" do
+        # call the s3 reload and make sure no more files get added to the model
+        work.attach_s3_resources
+
         expect(work.pre_curation_uploads).not_to be_empty
         expect(work.pre_curation_uploads.length).to eq(2)
         expect(work.pre_curation_uploads.first).to be_a(ActiveStorage::Attachment)
         expect(work.pre_curation_uploads.first.key).to eq("#{work.doi}/#{work.id}/SCoData_combined_v1_2020-07_README.txt")
         expect(work.pre_curation_uploads.last).to be_a(ActiveStorage::Attachment)
         expect(work.pre_curation_uploads.last.key).to eq("#{work.doi}/#{work.id}/SCoData_combined_v1_2020-07_datapackage.json")
 
-        # call the s3 reload during validation and make sure no more files get added to the model
-        work.valid?
+        # call the s3 reload and make sure no more files get added to the model
+        work.attach_s3_resources
         expect(work.pre_curation_uploads.length).to eq(2)
       end
 
@@ -644,6 +657,7 @@
         end
 
         it "finds the blob and attaches it as an ActiveStorage Attachments" do
+          work.attach_s3_resources
           expect(work.pre_curation_uploads).not_to be_empty
           expect(work.pre_curation_uploads.length).to eq(2)
           expect(work.pre_curation_uploads.first).to be_a(ActiveStorage::Attachment)

spec/services/s3_query_service_spec.rb

@@ -17,14 +17,14 @@
           etag: "\"008eec11c39e7038409739c0160a793a\"",
           key: s3_key1,
           last_modified: s3_last_modified1,
-          size: 10_759,
+          size: s3_size1,
           storage_class: "STANDARD"
         },
         {
           etag: "\"7bd3d4339c034ebc663b990657714688\"",
           key: s3_key2,
           last_modified: s3_last_modified2,
-          size: 12_739,
+          size: s3_size2,
           storage_class: "STANDARD"
         },
         {

spec/support/s3_query_service_specs.rb

@@ -1,63 +1,12 @@
 # frozen_string_literal: true
 
-RSpec.configure do |config|
-  config.before(:each) do
-    @pre_curated_data_profile = {
-      objects: []
-    }
-    @s3_client = instance_double(Aws::S3::Client)
-    @pre_curated_query_service = instance_double(S3QueryService)
-
-    allow(@pre_curated_query_service).to receive(:data_profile).and_return(@pre_curated_data_profile)
-    allow(@pre_curated_query_service).to receive(:bucket_name).and_return("example-bucket")
-    allow(@pre_curated_query_service).to receive(:client).and_return(@s3_client)
-    allow(S3QueryService).to receive(:new).with(anything, true).and_return(@pre_curated_query_service)
-
-    # @todo remove this
-    @post_curated_data_profile_disable = {
-      objects: [
-        {
-          key: "test_object1",
-          size: 12,
-          etag: "6805f2cfc46c0f04559748bb039d69ae",
-          last_modified: Time.parse("Thu, 15 Dec 2016 01:19:41 GMT")
-        },
-        {
-          key: "test_object2",
-          size: 12,
-          etag: "6805f2cfc46c0f04559748bb039d69ae",
-          last_modified: Time.parse("Thu, 15 Dec 2016 01:19:41 GMT")
-        }
-      ]
-    }
-
-    @post_curated_data_profile = {
-      objects: [
-        S3File.new(
-          filename: "test_object1",
-          last_modified: Time.parse("Thu, 15 Dec 2016 01:19:41 GMT"),
-          size: 12,
-          checksum: "6805f2cfc46c0f04559748bb039d69ae"
-        ),
-        S3File.new(
-          filename: "test_object2",
-          last_modified: Time.parse("Thu, 15 Dec 2016 01:19:41 GMT"),
-          size: 12,
-          checksum: "6805f2cfc46c0f04559748bb039d69ae"
-        )
-      ]
-    }
-
-    @post_curated_query_service = instance_double(S3QueryService)
-    stub_request(:get, "https://example-bucket.s3.amazonaws.com/test_object1").to_return(status: 200, body: "test_content")
-    stub_request(:get, "https://example-bucket.s3.amazonaws.com/test_object2").to_return(status: 200, body: "test_content")
-
-    allow(@post_curated_query_service).to receive(:data_profile).and_return(@post_curated_data_profile)
-    allow(@post_curated_query_service).to receive(:bucket_name).and_return("example-bucket")
-    allow(@post_curated_query_service).to receive(:client).and_return(@s3_client)
-    allow(S3QueryService).to receive(:new).with(anything, false).and_return(@post_curated_query_service)
-  end
+def stub_s3(data: [])
+  fake_s3_query = double(S3QueryService, data_profile: { objects: data, ok: true })
+  allow(S3QueryService).to receive(:new).and_return(fake_s3_query)
+  fake_s3_query
+end
 
+RSpec.configure do |config|
   config.before(:each, mock_s3_query_service: false) do
     @s3_bucket_url = "https://example-bucket.s3.amazonaws.com/"
 

spec/system/bitklavier_form_submission_spec.rb

@@ -19,6 +19,7 @@
     stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
     stub_request(:get, "https://handle.stage.datacite.org/10.34770/r75s-9j74")
       .to_return(status: 200, body: "", headers: {})
+    stub_s3
   end
   context "migrate record from dataspace" do
     it "produces and saves a valid datacite record" do

spec/system/external_ids_spec.rb

@@ -6,6 +6,7 @@
 
   before do
     stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
+    stub_s3
     # Make the screen larger so the save button is alway on screen.  This avoids random `Element is not clickable` errors
     page.driver.browser.manage.window.resize_to(2000, 2000)
   end

spec/system/migrate_submission_spec.rb

@@ -36,6 +36,7 @@
   context "happy path" do
     before do
       stub_request(:get, "https://handle.stage.datacite.org/10.34770/123-abc").to_return(status: 200, body: "", headers: {})
+      stub_s3
     end
 
     it "produces and saves a valid datacite record" do
@@ -65,6 +66,7 @@
     before do
       stub_request(:get, "https://handle.stage.datacite.org/10.34770/123-abc").to_return(status: 200, body: "", headers: {})
       stub_request(:get, "https://handle.stage.datacite.org/10.34770/123-ab").to_return(status: 404, body: "", headers: {})
+      stub_s3
     end
 
     it "returns the user to the new page so they can recover from an error" do
@@ -105,6 +107,7 @@
     let(:datacite_stub) { stub_datacite_doi }
     let(:identifier) { @identifier } # from the mock_ezid_api
     before do
+      stub_s3
       datacite_stub # make sure the stub is created before we start the test
       Rails.configuration.update_ark_url = true
       allow(Honeybadger).to receive(:notify)

spec/system/work_edit_spec.rb

@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+require "rails_helper"
+
+RSpec.describe "Creating and updating works", type: :system, js: true, mock_s3_query_service: false do
+  let(:user) { FactoryBot.create(:princeton_submitter) }
+
+  before do
+    stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
+  end
+
+  let(:contents1) do
+    {
+      etag: "\"008eec11c39e7038409739c0160a793a\"",
+      key: "#{work.doi}/#{work.id}/us_covid_2019.csv",
+      last_modified: Time.parse("2022-04-21T18:29:40.000Z"),
+      size: 92,
+      storage_class: "STANDARD"
+    }
+  end
+
+  let(:contents2) do
+    {
+      etag: "\"7bd3d4339c034ebc663b990657714688\"",
+      key: "#{work.doi}/#{work.id}/us_covid_2020.csv",
+      last_modified: Time.parse("2022-04-21T19:29:40.000Z"),
+      size: 114,
+      storage_class: "STANDARD"
+    }
+  end
+
+  let(:s3_hash) { { contents: [contents1, contents2] } }
+  let(:s3_hash_after_delete) { { contents: [contents2] } }
+
+  context "when editing an existing draft Work with uploaded files" do
+    let(:work) { FactoryBot.create(:draft_work) }
+    let(:user) { work.created_by_user }
+
+    let(:uploaded_file1) do
+      fixture_file_upload("us_covid_2019.csv", "text/csv")
+    end
+    let(:uploaded_file2) do
+      fixture_file_upload("us_covid_2020.csv", "text/csv")
+    end
+    let(:bucket_url) do
+      "https://example-bucket.s3.amazonaws.com/"
+    end
+    let(:delete_url) { "#{bucket_url}#{work.doi}/#{work.id}/us_covid_2019.csv" }
+    before do
+      fake_aws_client = double(Aws::S3::Client)
+      fake_s3_resp = double(Aws::S3::Types::ListObjectsV2Output)
+      fake_aws_client.stub(:list_objects_v2).and_return(fake_s3_resp)
+      fake_s3_resp.stub(:to_h).and_return(s3_hash, s3_hash_after_delete)
+      s3 = S3QueryService.new(work)
+      allow(s3).to receive(:client).and_return(fake_aws_client)
+      allow(S3QueryService).to receive(:new).and_return(s3)
+
+      stub_request(:put, /#{bucket_url}/).to_return(status: 200)
+      stub_request(:delete, /#{delete_url}/).to_return(status: 200)
+      work.pre_curation_uploads.attach(uploaded_file1)
+      work.pre_curation_uploads.attach(uploaded_file2)
+      work.save
+
+      sign_in user
+      visit work_path(work)
+      visit edit_work_path(work)
+    end
+
+    it "allows users to delete one of the uploads" do
+      # Make the screen larger so the save button is alway on screen.  This avoids random `Element is not clickable` errors
+      page.driver.browser.manage.window.resize_to(2000, 2000)
+      expect(page).to have_content "Filename"
+      expect(page).to have_content "Created At"
+      expect(page).to have_content "Replace Upload"
+      expect(page).to have_content "Delete Upload"
+      expect(page).to have_content("us_covid_2019.csv")
+      expect(page).to have_content("us_covid_2020.csv")
+      check("work-uploads-#{work.pre_curation_uploads[0].id}-delete")
+      click_on "Save Work"
+      expect(page).to have_content("us_covid_2020.csv")
+      expect(page).not_to have_content("us_covid_2019.csv")
+      expect(a_request(:delete, delete_url)).to have_been_made
+    end
+  end
+end

spec/system/work_spec.rb

@@ -6,6 +6,7 @@
 
   before do
     stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
+    stub_s3
   end
 
   it "Prevents empty title", js: true do
@@ -30,7 +31,6 @@
   end
 
   it "Renders ORCID links for creators", js: true do
-    stub_s3
     resource = FactoryBot.build(:resource, creators: [PDCMetadata::Creator.new_person("Harriet", "Tubman", "1234-5678-9012-3456")])
     work = FactoryBot.create(:draft_work, resource: resource)