Do not allow a submitter to approve a work (#366)

Only allow a user that has collection_admin to approve a work

Co-authored-by: Bess Sadler <bess.sadler@princeton.edu>

app/controllers/works_controller.rb

@@ -283,8 +283,12 @@ def pre_curation_uploads_param
     def rescue_aasm_error
       yield
     rescue AASM::InvalidTransition => error
-      logger.warn("Invalid #{@work.current_transition}: #{error.message}")
-      @errors = ["Cannot #{@work.current_transition}: #{error.message}"]
+      message = error.message
+      if @work.errors.count > 0
+        message = @work.errors.to_a.join(", ")
+      end
+      logger.warn("Invalid #{@work.current_transition}: #{error.message} errors: #{message}")
+      @errors = ["Cannot #{@work.current_transition}: #{message}"]
       render :show, status: :unprocessable_entity
     end
 end

app/models/work.rb

@@ -32,7 +32,7 @@ class Work < ApplicationRecord
     end
 
     event :approve do
-      transitions from: :awaiting_approval, to: :approved, guard: :valid_to_submit, after: :publish
+      transitions from: :awaiting_approval, to: :approved, guard: :valid_to_approve, after: :publish
     end
 
     event :withdraw do
@@ -178,6 +178,14 @@ def valid_to_submit
     errors.count == 0
   end
 
+  def valid_to_approve(user)
+    valid_to_submit
+    unless user.has_role? :collection_admin, collection
+      errors.add :base, "Unauthorized to Approve"
+    end
+    errors.count == 0
+  end
+
   def title
     resource.main_title
   end

app/views/works/show.html.erb

@@ -41,7 +41,7 @@
 
 <div>
   <%= link_to("Edit", edit_work_path(@work), class: "btn btn-primary") %>
-  <% if @work.awaiting_approval? %>
+  <% if @work.awaiting_approval? && current_user.has_role?(:collection_admin, @work.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) %>

spec/controllers/works_controller_spec.rb

@@ -9,7 +9,7 @@
     stub_datacite(host: "api.datacite.org", body: datacite_register_body(prefix: "10.34770"))
   end
   let(:user) { FactoryBot.create(:user) }
-  let(:curator) { FactoryBot.create(:user) }
+  let(:curator) { FactoryBot.create(:user, collections_to_admin: [collection]) }
   let(:collection) { Collection.first }
   let(:resource) { FactoryBot.build :resource }
   let(:work) { FactoryBot.create(:draft_work) }
@@ -553,16 +553,16 @@
           post :completed, params: { id: work.id }
           expect(response.status).to be 422
           expect(work.reload).to be_draft
-          expect(assigns[:errors]).to eq(["Cannot Complete submission: Event 'complete_submission' cannot transition from 'draft'. Failed callback(s): [:valid_to_submit]."])
+          expect(assigns[:errors]).to eq(["Cannot Complete submission: Must provide a description"])
         end
       end
     end
 
     describe "#approve" do
       it "handles aprovals" do
-        sign_in user
         work.complete_submission!(user)
         stub_datacite_doi
+        sign_in curator
         post :approve, params: { id: work.id }
         expect(response.status).to be 302
         expect(response.location).to eq "http://test.host/works/#{work.id}"
@@ -571,7 +571,7 @@
 
       context "invalid response from doi publish" do
         before do
-          sign_in user
+          sign_in curator
           work.complete_submission!(user)
           stub_datacite_doi(result: Failure(Faraday::Response.new(Faraday::Env.new)))
         end
@@ -588,13 +588,27 @@
 
       context "work not completed" do
         it "handles aproval errors" do
-          sign_in user
+          sign_in curator
           post :approve, params: { id: work.id }
           expect(response.status).to be 422
           expect(work.reload).to be_draft
           expect(assigns[:errors]).to eq(["Cannot Approve: Event 'approve' cannot transition from 'draft'."])
         end
       end
+
+      context "work submitter is trying to approve" do
+        let(:user) { FactoryBot.create(:princeton_submitter) }
+
+        it "handles aproval errors" do
+          sign_in user
+          work.complete_submission!(user)
+          stub_datacite_doi
+          post :approve, params: { id: work.id }
+          expect(response.status).to be 422
+          expect(work.reload).to be_awaiting_approval
+          expect(assigns[:errors]).to eq(["Cannot Approve: Unauthorized to Approve"])
+        end
+      end
     end
 
     describe "#withdraw" do

spec/models/work_spec.rb

@@ -82,7 +82,7 @@
   it "approves works and records the change history" do
     stub_datacite_doi
     work.complete_submission!(user)
-    work.approve!(user)
+    work.approve!(curator_user)
     expect(work.state_history.first.state).to eq "approved"
     expect(work.reload.state).to eq("approved")
   end
@@ -119,7 +119,7 @@
       work.save
       work.complete_submission!(user)
       stub_datacite_doi
-      work.approve!(user)
+      work.approve!(curator_user)
       expect(Ark).to have_received(:update).exactly(1).times
     end
 
@@ -128,7 +128,7 @@
       work.save
       work.complete_submission!(user)
       stub_datacite_doi
-      work.approve!(user)
+      work.approve!(curator_user)
       expect(Ark).to have_received(:update).exactly(0).times
     end
   end
@@ -342,7 +342,7 @@
     end
 
     it "can not transition from draft to approved" do
-      expect { draft_work.approve!(user) }.to raise_error AASM::InvalidTransition
+      expect { draft_work.approve!(curator_user) }.to raise_error AASM::InvalidTransition
     end
 
     it "can not transition from draft to tombsotne" do
@@ -368,10 +368,19 @@
 
     it "transitions from awaiting_approval to approved" do
       stub_datacite_doi
-      awaiting_approval_work.approve!(user)
+      awaiting_approval_work.approve!(curator_user)
       expect(awaiting_approval_work.reload.state).to eq("approved")
     end
 
+    context "submitter user" do
+      let(:user) { FactoryBot.create(:princeton_submitter) }
+
+      it "can not transition from awaitng_approval to approved" do
+        expect { awaiting_approval_work.approve!(user) }.to raise_error AASM::InvalidTransition
+        expect(awaiting_approval_work.reload.state).to eq("awaiting_approval")
+      end
+    end
+
     it "can not transition from awaiting_approval to tombsotne" do
       expect { awaiting_approval_work.remove!(user) }.to raise_error AASM::InvalidTransition
     end
@@ -385,7 +394,7 @@
     let(:approved_work) do
       work = FactoryBot.create :draft_work
       work.complete_submission!(user)
-      work.approve!(user)
+      work.approve!(curator_user)
       work
     end
 
@@ -498,7 +507,7 @@
     end
 
     it "can not be approved from none" do
-      expect { work.approve!(user) }.to raise_error AASM::InvalidTransition
+      expect { work.approve!(curator_user) }.to raise_error AASM::InvalidTransition
     end
 
     it "can not be maked ready for review from none" do

spec/system/authz_submitter_spec.rb

@@ -62,5 +62,12 @@
       expect(page).not_to have_content "Update Collection"
       expect(current_path).to eq "/collections"
     end
+
+    it "should not be able to approve a work" do
+      work = FactoryBot.create :completed_work
+      sign_in submitter1
+      visit work_path(work)
+      expect(page).not_to have_button "Approve Dataset"
+    end
   end
 end