Requiring files for a work to move to the approved state

Also adds decorator for the work to encapsulate the changes and messages, becuase the show can be rendered from the AASM error capture too

app/controllers/works_controller.rb

@@ -67,16 +67,12 @@ def new_submission
   def show
     @work = Work.find(params[:id])
     @work.reload_snapshots
-    @changes = WorkActivity.changes_for_work(@work.id)
-    @messages = WorkActivity.messages_for_work(@work.id)
+    @work_decorator = WorkDecorator.new(@work, current_user)
 
     respond_to do |format|
       format.html do
         # Ensure that the Work belongs to a Collection
-        @collection = @work.collection
-        raise(Work::InvalidCollectionError, "The Work #{@work.id} does not belong to any Collection") unless @collection
-
-        @can_curate = current_user.can_admin?(@collection)
+        raise(Work::InvalidCollectionError, "The Work #{@work.id} does not belong to any Collection") unless @work_decorator.collection
         @work.mark_new_notifications_as_read(current_user.id)
       end
       format.json { render json: @work.to_json }
@@ -345,6 +341,7 @@ def error_action
       elsif action_name == "validate"
         :edit
       else
+        @work_decorator = WorkDecorator.new(@work, current_user)
         :show
       end
     end

app/decorators/work_decorator.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+class WorkDecorator
+  attr_reader :work, :changes, :messages, :can_curate, :current_user
+
+  delegate :collection, to: :work
+
+  def initialize(work, current_user)
+    @work = work
+    @current_user = current_user
+    @changes =  WorkActivity.changes_for_work(work.id)
+    @messages = WorkActivity.messages_for_work(work.id)
+    @can_curate = current_user&.can_admin?(collection)
+  end
+end

app/models/work.rb

@@ -186,6 +186,9 @@ def valid_to_approve(user)
     unless user.has_role? :collection_admin, collection
       errors.add :base, "Unauthorized to Approve"
     end
+    if pre_curation_uploads_fast.empty? && post_curation_uploads.empty?
+      errors.add :base, "Uploads must be present for a work to be approved"
+    end
     errors.count == 0
   end
 

app/views/works/_work_activity_messages.html.erb

@@ -1,9 +1,9 @@
 <div>
-  <% if @messages.size == 0 && !@work.submission_notes.present? %>
+  <% if @work_decorator.messages.size == 0 && !@work.submission_notes.present? %>
     No messages
   <% end %>
   <ul class="no-beads work-messages">
-    <% @messages.sort_by(&:created_at).reverse.each do |activity| %>
+    <% @work_decorator.messages.sort_by(&:created_at).reverse.each do |activity| %>
       <li class="activity-history-item">
         <%= activity.to_html.html_safe %>
       </li>

app/views/works/_work_activity_provenance.html.erb

@@ -1,10 +1,10 @@
 <div>
   <h2>Change History</h2>
-  <% if @changes.size == 0 %>
+  <% if @work_decorator.changes.size == 0 %>
     No activity
   <% end %>
   <ul class="beads">
-    <% @changes.sort_by(&:created_at).each do |activity| %>
+    <% @work_decorator.changes.sort_by(&:created_at).each do |activity| %>
       <li class="activity-history-item">
         <%= activity.to_html.html_safe %>
       </li>

app/views/works/show.html.erb

@@ -70,12 +70,14 @@
   }
 </style>
 
+<%= render 'form_errors' %>
+
 <div class="row">
   <div class="col-8">
     <% if @work.editable_in_current_state?(current_user)  %>
       <%= link_to("Edit", edit_work_path(@work, wizard: @work.draft?), class: "btn btn-primary") %>
     <% end %>
-    <% if @work.awaiting_approval? && current_user.has_role?(:collection_admin, @collection) %>
+    <% if @work.awaiting_approval? && current_user.has_role?(:collection_admin, @work_decorator.collection) %>
       <%= button_to("Approve Dataset", approve_work_path(@work), class: "btn btn-secondary", method: :post) %>
     <% elsif @work.approved? %>
       <%= button_to("Withdraw Dataset", withdraw_work_path(@work), class: "btn btn-secondary", method: :post) %>
@@ -110,10 +112,10 @@
 
       <dt>Curator</dt>
       <dd>
-        <% if @can_curate %>
+        <% if @work_decorator.can_curate %>
           <select id="curator_select">
             <option value="no-one">(no one)</option>
-            <% @collection.administrators.each do |user| %>
+            <% @work_decorator.collection.administrators.each do |user| %>
               <option value="<%= user.id %>" <%= @work.curator_user_id == user.id ? "selected" : "" %>><%= user.display_name_safe %></option>
             <% end %>
           </select>
@@ -216,7 +218,7 @@
       <% end %>
 
       <dt>Collection</dt>
-      <dd><%= @collection&.title %></dd>
+      <dd><%= @work_decorator.collection&.title %></dd>
 
       <% if @work.resource.funders.count > 0 %>
         <dt>Funders</dt>

spec/controllers/works_controller_spec.rb

@@ -887,8 +887,8 @@
       it "renders the workshow page" do
         get :show, params: { id: work.id }
         expect(response).to render_template("show")
-        expect(assigns[:changes]).to eq([])
-        expect(assigns[:messages]).to eq([])
+        expect(assigns[:work_decorator].changes).to eq([])
+        expect(assigns[:work_decorator].messages).to eq([])
       end
 
       context "when the work has changes and messages" do
@@ -900,8 +900,8 @@
         it "renders the workshow page" do
           get :show, params: { id: work.id }
           expect(response).to render_template("show")
-          expect(assigns[:changes].map(&:message)).to eq(["Hello System"])
-          expect(assigns[:messages].map(&:message)).to eq(["Hello World"])
+          expect(assigns[:work_decorator].changes.map(&:message)).to eq(["Hello System"])
+          expect(assigns[:work_decorator].messages.map(&:message)).to eq(["Hello World"])
         end
       end
     end
@@ -1023,7 +1023,7 @@
 
     describe "#approve" do
       before do
-        stub_s3
+        stub_s3 data: [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)
@@ -1067,6 +1067,17 @@
         end
       end
 
+      context "no files attached" do
+        it "handles aproval errors" do
+          work.complete_submission!(user)
+          stub_s3 data: []
+          sign_in curator
+          post :approve, params: { id: work.id }
+          expect(response.status).to be 422
+          expect(assigns[:errors]).to eq(["Cannot Approve: Uploads must be present for a work to be approved"])
+        end
+      end
+
       context "work submitter is trying to approve" do
         let(:user) { FactoryBot.create(:princeton_submitter) }
 

spec/models/work_spec.rb

@@ -491,6 +491,7 @@
       let(:user) { FactoryBot.create(:princeton_submitter) }
 
       it "can not transition from awaitng_approval to approved" do
+        stub_s3 data: [FactoryBot.build(:s3_file)]
         expect { awaiting_approval_work.approve!(user) }.to raise_error AASM::InvalidTransition
         expect(awaiting_approval_work.reload.state).to eq("awaiting_approval")
       end

spec/models/work_state_spec.rb

@@ -32,6 +32,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)]
       expect do
         work.approve!(curator_user)
       end.to change { WorkActivity.count }.by(2)

spec/system/authz_super_admin_spec.rb

@@ -70,7 +70,7 @@
 
     it "should be able to approve a work" do
       stub_datacite_doi
-      stub_s3
+      stub_s3 data: [FactoryBot.build(:s3_file)]
       work = FactoryBot.create :awaiting_approval_work
 
       work.save!

spec/system/migrate_submission_spec.rb

@@ -121,7 +121,7 @@
       work.save!
       work.reload
 
-      stub_s3
+      stub_s3 data: [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)

spec/system/work_approve_spec.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+require "rails_helper"
+
+RSpec.describe "Work Approval", type: :system do
+  let(:work) { FactoryBot.create(:awaiting_approval_work) }
+  let!(:curator) { FactoryBot.create(:user, collections_to_admin: [work.collection]) }
+
+  before do
+    stub_s3
+    stub_ark
+    stub_datacite_doi
+  end
+  context "No uploads, can not approve" do
+    it "produces and saves a valid datacite record", js: true do
+      sign_in curator
+      visit(user_path(curator))
+      expect(page).to have_content curator.display_name
+      click_link work.title
+      expect(page).to have_content(work.doi)
+      click_on "Approve Dataset"
+      expect(page).to have_content("Uploads must be present for a work to be approved")
+    end
+  end
+end

spec/views/works/work_activity_messages.html.erb_spec.rb

@@ -4,6 +4,7 @@
 describe "Messages" do
   let(:user) { FactoryBot.create :user }
   let(:work) { FactoryBot.create :draft_work }
+  let(:work_decorator) { WorkDecorator.new(work, user) }
   let(:partial) { "works/work_activity_messages" }
   let(:older) do
     WorkActivity.add_work_activity(work.id, "older", user.id,
@@ -16,56 +17,60 @@
 
   it "handles no messages" do
     assign(:work, work)
-    assign(:messages, [])
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to include("No messages")
   end
 
   it "handles unknown user" do
     assign(:work, work)
-    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", nil,
-      activity_type: WorkActivity::MESSAGE)])
+    WorkActivity.add_work_activity(work.id, "message!", nil, activity_type: WorkActivity::MESSAGE)
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to include("Unknown user outside the system")
   end
 
   it "handles submission notes" do
     work.submission_notes = "submission note!"
     assign(:work, work)
-    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", user.id,
-      activity_type: WorkActivity::MESSAGE)])
+    WorkActivity.add_work_activity(work.id, "message!", user.id, activity_type: WorkActivity::MESSAGE)
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to include("submission note!")
   end
 
   it "handles message" do
     assign(:work, work)
-    assign(:messages, [WorkActivity.add_work_activity(work.id, "message!", user.id,
-      activity_type: WorkActivity::MESSAGE)])
+    WorkActivity.add_work_activity(work.id, "message!", user.id, activity_type: WorkActivity::MESSAGE)
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to include("message!")
     expect(rendered).to include("(@#{user.uid})")
   end
 
   it "handles notification" do
     assign(:work, work)
-    assign(:messages, [WorkActivity.add_work_activity(work.id, "notification!", user.id,
-      activity_type: WorkActivity::NOTIFICATION)])
+    WorkActivity.add_work_activity(work.id, "notification!", user.id, activity_type: WorkActivity::NOTIFICATION)
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to include("notification!")
     expect(rendered).to include("(@#{user.uid})")
   end
 
   it "shows newest message first, when array is in the same order" do
     assign(:work, work)
-    assign(:messages, [newer, older])
+    newer
+    older
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to match(/newer.*older/m)
   end
 
   it "still shows newest message first, when array is in the reverse order" do
     assign(:work, work)
-    assign(:messages, [older, newer])
+    older
+    newer
+    assign(:work_decorator, work_decorator)
     render(partial: partial)
     expect(rendered).to match(/newer.*older/m)
   end

spec/views/works/work_activity_provenance.html.erb_spec.rb

@@ -4,6 +4,7 @@
 describe "Change History, AKA Provenance" do
   let(:user) { FactoryBot.create :user }
   let(:work) { FactoryBot.create :draft_work }
+  let(:work_decorator) { WorkDecorator.new(work, user) }
   let(:partial) { "works/work_activity_provenance" }
   let(:older) do
     WorkActivity.add_work_activity(work.id, "older", user.id,
@@ -15,53 +16,59 @@
   end
 
   it "handles no activity" do
-    assign(:changes, [])
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("No activity")
   end
 
   it "handles metadata changes" do
-    assign(:changes, [WorkActivity.add_work_activity(work.id, JSON.dump({ a_field: [{ action: "changed", from: "old", to: "new" }] }), user.id,
-      activity_type: WorkActivity::CHANGES)])
+    WorkActivity.add_work_activity(work.id, JSON.dump({ a_field: [{ action: "changed", from: "old", to: "new" }] }), user.id,
+                                   activity_type: WorkActivity::CHANGES)
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("<del>old</del><ins>new</ins>")
   end
 
   it "handles file changes" do
-    assign(:changes, [WorkActivity.add_work_activity(work.id, { action: "added" }.to_json, user.id,
-      activity_type: WorkActivity::FILE_CHANGES)])
+    WorkActivity.add_work_activity(work.id, { action: "added" }.to_json, user.id, activity_type: WorkActivity::FILE_CHANGES)
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("Files Added:")
   end
 
   it "handles prov note" do
-    assign(:changes, [WorkActivity.add_work_activity(work.id, "note!", user.id,
-      activity_type: WorkActivity::PROVENANCE_NOTES)])
+    WorkActivity.add_work_activity(work.id, "note!", user.id, activity_type: WorkActivity::PROVENANCE_NOTES)
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("note!")
   end
 
   it "handles error" do
-    assign(:changes, [WorkActivity.add_work_activity(work.id, "error!", user.id,
-      activity_type: WorkActivity::DATACITE_ERROR)])
+    WorkActivity.add_work_activity(work.id, "error!", user.id, activity_type: WorkActivity::DATACITE_ERROR)
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("error!")
   end
 
   it "handles backdated prov note" do
-    assign(:changes, [older])
+    older
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to include("January 01, 2021 00:00 (backdated event created")
   end
 
   it "shows oldest change first, when array is in the same order" do
-    assign(:changes, [older, newer])
+    older
+    newer
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to match(/older.*newer/m)
   end
 
   it "still shows oldest change first, when array is in the reverse order" do
-    assign(:changes, [newer, older])
+    newer
+    older
+    assign(:work_decorator, work_decorator)
     render(partial: partial, locals: { can_add_provenance_note: false })
     expect(rendered).to match(/older.*newer/m)
   end