Require README for a work to be completed (#1777)

app/controllers/works_controller.rb

@@ -221,6 +221,14 @@ def upload_files
     upload_service.update_precurated_file_list(params["files"], [])
   end
 
+  # Validates that the work is ready to be approved
+  # GET /works/1/validate
+  def validate
+    @work = Work.find(params[:id])
+    @work.complete_submission!(current_user)
+    redirect_to user_path(current_user)
+  end
+
   private
 
     # Extract the Work ID parameter

app/controllers/works_wizard_controller.rb

@@ -104,7 +104,8 @@ def review
   end
 
   # Validates that the work is ready to be approved
-  # GET /works/1/validate
+  # POST /works/1/validate-wizard
+  # PATCH /works/1/validate-wizard
   def validate
     @work.submission_notes = params["submission_notes"]
     if params[:save_only] == "true"

app/models/work.rb

@@ -19,7 +19,7 @@ class InvalidGroupError < ::ArgumentError; end
 
   alias state_history user_work
 
-  delegate :valid_to_submit, :valid_to_draft, :valid_to_approve, to: :work_validator
+  delegate :valid_to_submit, :valid_to_draft, :valid_to_approve, :valid_to_complete, to: :work_validator
 
   include AASM
 
@@ -32,7 +32,7 @@ class InvalidGroupError < ::ArgumentError; end
     end
 
     event :complete_submission do
-      transitions from: :draft, to: :awaiting_approval, guard: :valid_to_submit
+      transitions from: :draft, to: :awaiting_approval, guard: :valid_to_complete
     end
 
     event :request_changes do

app/services/work_validator.rb

@@ -27,6 +27,11 @@ def valid_to_draft(*)
     errors.count == 0
   end
 
+  def valid_to_complete(*args)
+    validate_files
+    valid_to_submit(args)
+  end
+
   def valid_to_submit(*args)
     valid_to_draft(args)
     validate_metadata
@@ -44,6 +49,7 @@ def valid_to_approve(user)
     unless user.has_role? :group_admin, group
       errors.add :base, "Unauthorized to Approve"
     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"
     end
@@ -151,4 +157,10 @@ def validate_metadata
       errors.add(:base, "Must provide a Version number") if resource.version_number.blank?
       validate_related_objects
     end
+
+    def validate_files
+      return if @work.resource.migrated
+      readme = Readme.new(work, nil)
+      errors.add(:base, "Must provide a README") if readme.blank?
+    end
 end

app/views/works_wizard/review.html.erb

@@ -6,7 +6,7 @@
     it more discoverable and reusable. Recommendations will be available in the "unfinished submissions"
     section of this form within 5-10 business days.</p>
 
-  <%= form_with(model: @work, url: work_validate_path(@work)) do |form| %>
+  <%= form_with(model: @work, url: work_validate_wizard_path(@work)) do |form| %>
     <textarea id="submission_notes" name="submission_notes" cols="80", rows="10"
       placeholder="(optional) Type here to leave a message for the curators"><%= @work.submission_notes %></textarea>
     <div class="actions">

config/routes.rb

@@ -45,8 +45,9 @@
   get "works/:id/review", to: "works_wizard#review", as: :work_review
   post "works/:id/review", to: "works_wizard#review"
   patch "works/:id/review", to: "works_wizard#review"
-  post "works/:id/validate", to: "works_wizard#validate", as: :work_validate
-  patch "works/:id/validate", to: "works_wizard#validate"
+  post "works/:id/validate-wizard", to: "works_wizard#validate", as: :work_validate_wizard
+  patch "works/:id/validate-wizard", to: "works_wizard#validate"
+  post "works/:id/validate", to: "works#validate", as: :work_validate
   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"

spec/controllers/works_controller_spec.rb

@@ -693,7 +693,7 @@
 
     describe "#approve" do
       before do
-        stub_s3 data: [FactoryBot.build(:s3_file)]
+        stub_s3 data: [FactoryBot.build(:s3_readme), FactoryBot.build(:s3_file)]
         allow(Work).to receive(:find).with(work.id).and_return(work)
         allow(Work).to receive(:find).with(work.id.to_s).and_return(work)
         allow(work).to receive(:publish_precurated_files).and_return(true)
@@ -742,13 +742,14 @@
 
       context "no files attached" do
         it "handles aproval errors" do
+          fake_s3_service = stub_s3 data: [FactoryBot.build(:s3_readme)]
           work.complete_submission!(user)
-          stub_s3 data: []
+          allow(fake_s3_service).to receive(:client_s3_files).and_return([])
           sign_in curator
           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: Uploads must be present for a work to be approved. Please contact the PDC Describe administrators for any assistance."])
+          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."])
           # rubocop:enable Layout/LineLength
         end
       end
@@ -1271,4 +1272,23 @@
       expect(assigns[:errors]).to eq(["Contributor: Type cannot be nil"])
     end
   end
+
+  describe "#validate" do
+    it "validates a work and errors when there is no readme" do
+      stub_s3
+      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.")
+    end
+
+    it "validates a work completes it when there are no errors" do
+      stub_s3 data: [FactoryBot.build(:s3_readme)]
+      sign_in user
+      post :validate, params: { id: work.id }
+      expect(response).to redirect_to(user_path(user))
+      expect(controller.flash[:notice]).to be_nil
+      expect(work.reload).to be_awaiting_approval
+    end
+  end
 end

spec/controllers/works_wizard_controller_spec.rb

@@ -334,7 +334,7 @@
 
     describe "#validate" do
       before do
-        stub_s3
+        stub_s3 data: [FactoryBot.build(:s3_readme)]
         sign_in user
       end
 

spec/models/upload_snapshot_spec.rb

@@ -54,12 +54,18 @@
 
       let(:fake_s3_service_pre) { stub_s3(data: [file1]) }
       let(:fake_s3_service_post) { stub_s3(data: []) }
+      let(:readme) { FactoryBot.build(:s3_readme) }
 
       before do
-        allow(S3QueryService).to receive(:new).and_return(fake_s3_service_pre, fake_s3_service_post)
+        fake_s3_service_post
+        fake_s3_service_pre
+
+        allow(S3QueryService).to receive(:new).with(instance_of(Work), "precuration").and_return(fake_s3_service_pre)
+        allow(S3QueryService).to receive(:new).with(instance_of(Work), "postcuration").and_return(fake_s3_service_post)
         allow(fake_s3_service_pre.client).to receive(:head_object).with(bucket: "example-post-bucket", key: work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
         allow(fake_s3_service_post).to receive(:bucket_name).and_return("example-post-bucket")
         allow(fake_s3_service_pre).to receive(:bucket_name).and_return("example-pre-bucket")
+        allow(fake_s3_service_pre).to receive(:client_s3_files).and_return([readme], [readme, file1])
         stub_ark
         stub_datacite_doi
 
@@ -83,12 +89,18 @@
 
       let(:fake_s3_service_pre) { stub_s3(data: [file2]) }
       let(:fake_s3_service_post) { stub_s3(data: [file2]) }
+      let(:readme) { FactoryBot.build(:s3_readme) }
 
       before do
-        allow(S3QueryService).to receive(:new).and_return(fake_s3_service_pre, fake_s3_service_post)
+        fake_s3_service_post
+        fake_s3_service_pre
+
+        allow(S3QueryService).to receive(:new).with(instance_of(Work), "precuration").and_return(fake_s3_service_pre)
+        allow(S3QueryService).to receive(:new).with(instance_of(Work), "postcuration").and_return(fake_s3_service_post)
         allow(fake_s3_service_pre.client).to receive(:head_object).with(bucket: "example-post-bucket", key: work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
         allow(fake_s3_service_post).to receive(:bucket_name).and_return("example-post-bucket")
         allow(fake_s3_service_pre).to receive(:bucket_name).and_return("example-pre-bucket")
+        allow(fake_s3_service_pre).to receive(:client_s3_files).and_return([readme], [readme, file2])
         stub_ark
         stub_datacite_doi
 

spec/models/work_spec.rb

@@ -26,6 +26,7 @@
     fixture_file_upload("us_covid_2019.csv", "text/csv")
   end
   let(:s3_file) { FactoryBot.build :s3_file, filename: "us_covid_2019.csv", work: }
+  let(:readme) { FactoryBot.build(:s3_readme) }
 
   before do
     stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
@@ -168,23 +169,22 @@
 
     let(:file1) { FactoryBot.build(:s3_file, filename: "#{work.doi}/#{work.id}/us_covid_2019.csv", work:, size: 1024) }
     let(:file2) { FactoryBot.build(:s3_file, filename: "#{work.doi}/#{work.id}/us_covid_2019_2.csv", work:, size: 2048) }
+    let(:readme) { FactoryBot.build(:s3_readme) }
 
-    let(:post_curation_data_profile) { { objects: [file1, file2] } }
-    let(:pre_curated_data_profile) { { objects: [] } }
-
-    let(:fake_s3_service_pre) { stub_s3(data: [file1, file2]) }
-    let(:fake_s3_service_post) { stub_s3 }
+    let(:fake_s3_service_pre) { stub_s3 }
+    let(:fake_s3_service_post) { stub_s3(data: [file1, file2]) }
 
     before do
       # initialize so the next allows happen after the stubs
       fake_s3_service_post
       fake_s3_service_pre
 
-      allow(S3QueryService).to receive(:new).with(work, "postcuration").and_return(fake_s3_service_post)
-      allow(S3QueryService).to receive(:new).with(work, "precuration").and_return(fake_s3_service_pre)
+      allow(S3QueryService).to receive(:new).with(instance_of(Work), "precuration").and_return(fake_s3_service_pre)
+      allow(S3QueryService).to receive(:new).with(instance_of(Work), "postcuration").and_return(fake_s3_service_post)
       allow(fake_s3_service_pre.client).to receive(:head_object).with(bucket: "example-post-bucket", key: work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
       allow(fake_s3_service_post).to receive(:bucket_name).and_return("example-post-bucket")
       allow(fake_s3_service_pre).to receive(:bucket_name).and_return("example-pre-bucket")
+      allow(fake_s3_service_pre).to receive(:client_s3_files).and_return([readme], [readme, file1, file2])
     end
 
     it "approves works and records the change history" do
@@ -199,8 +199,10 @@
       it "transfers the files to the AWS Bucket" do
         stub_datacite_doi
         work.approve!(curator_user)
-        work.reload
+        work_id = work.id
+        work = Work.find(work_id)
 
+        expect(work).to be_approved
         expect(fake_s3_service_pre).to have_received(:publish_files).once
         expect(fake_s3_service_post).to have_received(:bucket_name).once
         expect(fake_s3_service_pre.client).to have_received(:head_object).once
@@ -218,10 +220,8 @@
       end
 
       context "when the pre curation bucket is empty" do
-        let(:fake_s3_service_pre) { stub_s3(data: []) }
-
         before do
-          allow(fake_s3_service_pre.client).to receive(:head_object).with(bucket: "example-post-bucket", key: work.s3_object_key).and_return(true)
+          allow(fake_s3_service_pre).to receive(:client_s3_files).and_return([])
         end
 
         it "raises an error" do
@@ -232,9 +232,10 @@
 
       context "when the Work is under active embargo" do
         subject(:work) { FactoryBot.create(:awaiting_approval_work, doi: "10.34770/123-abc", embargo_date:) }
+        let(:work_after_approve) { Work.find(work.id) }
 
         let(:embargo_date) { Date.parse("2033-09-14") }
-        let(:json) { JSON.parse(work.to_json) }
+        let(:json) { JSON.parse(work_after_approve.to_json) }
         let(:uploaded_file) do
           fixture_file_upload("us_covid_2019.csv", "text/csv")
         end
@@ -258,7 +259,7 @@
         end
 
         it "does not serialize the files in JSON" do
-          expect(work.post_curation_uploads).not_to be_empty
+          expect(work_after_approve.post_curation_uploads).not_to be_empty
 
           expect(json).to include("files")
           expect(json["files"]).to be_empty
@@ -267,9 +268,10 @@
 
       context "when the Work is under an expired embargo" do
         subject(:work) { FactoryBot.create(:awaiting_approval_work, doi: "10.34770/123-abc", embargo_date:) }
+        let(:work_after_approve) { Work.find(work.id) }
 
         let(:embargo_date) { Date.parse("2023-09-14") }
-        let(:json) { JSON.parse(work.to_json) }
+        let(:json) { JSON.parse(work_after_approve.to_json) }
         let(:uploaded_file) do
           fixture_file_upload("us_covid_2019.csv", "text/csv")
         end
@@ -284,11 +286,10 @@
 
           stub_datacite_doi
           work.approve!(curator_user)
-          work.reload
         end
 
         it "does serialize the files in JSON" do
-          expect(work.post_curation_uploads).not_to be_empty
+          expect(work_after_approve.post_curation_uploads).not_to be_empty
 
           expect(json).to include("files")
           expect(json["files"]).not_to be_empty
@@ -335,7 +336,7 @@
     end
 
     before do
-      fake_s3_service = stub_s3(data: [s3_file])
+      fake_s3_service = stub_s3(data: [readme, s3_file])
       allow(fake_s3_service.client).to receive(:head_object).with(bucket: "example-bucket", key: work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
     end
 
@@ -564,7 +565,9 @@
   end
 
   describe "#complete_submission" do
+    let(:fake_s3_service) { stub_s3 data: [readme, s3_file] }
     let(:awaiting_approval_work) do
+      fake_s3_service
       work = FactoryBot.create :draft_work
       work.complete_submission!(user)
       work
@@ -581,7 +584,6 @@
 
     it "transitions from awaiting_approval to approved" do
       stub_datacite_doi
-      fake_s3_service = stub_s3(data: [s3_file])
       allow(fake_s3_service.client).to receive(:head_object).with(bucket: "example-bucket", key: awaiting_approval_work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
 
       awaiting_approval_work.approve!(curator_user)
@@ -624,13 +626,20 @@
   end
 
   describe "#approve" do
-    let(:fake_s3_service_pre) { stub_s3(data: [s3_file]) }
-    let(:fake_s3_service_post) { stub_s3(data: [s3_file]) }
+    let(:fake_s3_service_pre) { stub_s3(data: [readme, s3_file]) }
+    let(:fake_s3_service_post) { stub_s3(data: [readme, s3_file]) }
 
     let(:approved_work) do
       work = FactoryBot.create :awaiting_approval_work, doi: "10.34770/123-abc"
       stub_request(:put, "https://api.datacite.org/dois/10.34770/123-abc")
-      allow(S3QueryService).to receive(:new).and_return(fake_s3_service_pre, fake_s3_service_post)
+
+      # initialize so the next allows happen after the stubs
+      fake_s3_service_post
+      fake_s3_service_pre
+
+      allow(S3QueryService).to receive(:new).with(instance_of(Work), "precuration").and_return(fake_s3_service_pre)
+      allow(S3QueryService).to receive(:new).with(instance_of(Work), "postcuration").and_return(fake_s3_service_post)
+
       allow(fake_s3_service_pre.client).to receive(:head_object).with(bucket: "example-post-bucket", key: work.s3_object_key).and_raise(Aws::S3::Errors::NotFound.new("blah", "error"))
       allow(fake_s3_service_post).to receive(:bucket_name).and_return("example-post-bucket")
       allow(fake_s3_service_pre).to receive(:bucket_name).and_return("example-pre-bucket")
@@ -1072,6 +1081,7 @@
 
   describe "delete" do
     it "cleans up all the related objects" do
+      work.resource.migrated = true
       work.complete_submission!(user)
       expect { work.destroy }.to change { Work.count }.by(-1)
                                                       .and change { UserWork.count }.by(-1)

spec/models/work_state_spec.rb

@@ -3,6 +3,9 @@
 
 RSpec.describe "Work state transions", type: :model do
   let(:curator_user) { FactoryBot.create :user, groups_to_admin: [work.group] }
+  before do
+    stub_s3 data: [FactoryBot.build(:s3_readme)]
+  end
 
   {
     none_work: :draft!,
@@ -32,7 +35,7 @@
 
     it "Creates a work activity notification for the curator & the user when approved" do
       allow(work).to receive(:publish)
-      stub_s3 data: [FactoryBot.build(:s3_file)]
+      stub_s3 data: [FactoryBot.build(:s3_readme), FactoryBot.build(:s3_file)]
       expect do
         work.approve!(curator_user)
       end.to change { WorkActivity.count }.by(2)