From f4419caf5424f8000e519e7bd7c1e7d2ee36e9db Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Tue, 17 Sep 2024 12:49:14 -0600 Subject: [PATCH 1/2] 1578 - Adds a unique destroy? policy for file version membership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fileVersionMembershipPolicy used to just delegate to the destroy policy of the WorkVersionPolicy. However, we want a slight change – while we don't want to be able to delete the non-recent work versions, we want to be able to delete files even if they are associated with older versions. --- .../file_version_memberships_controller.rb | 2 +- .../file_version_membership_policy.rb | 13 +++++ .../file_version_membership_policy_spec.rb | 56 +++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/app/controllers/dashboard/file_version_memberships_controller.rb b/app/controllers/dashboard/file_version_memberships_controller.rb index 82f916581..4fd0afdfa 100644 --- a/app/controllers/dashboard/file_version_memberships_controller.rb +++ b/app/controllers/dashboard/file_version_memberships_controller.rb @@ -27,7 +27,7 @@ def update end def destroy - authorize(@work_version) + authorize(@file_version) @file_version.destroy respond_to do |format| format.html do diff --git a/app/policies/file_version_membership_policy.rb b/app/policies/file_version_membership_policy.rb index bb709c7f1..4bec2e921 100644 --- a/app/policies/file_version_membership_policy.rb +++ b/app/policies/file_version_membership_policy.rb @@ -12,4 +12,17 @@ def content? def work_version @work_version ||= record.work_version end + + def destroy? + return false if record.new_record? + return true if user.admin? + + !record.work_version.published? && editable? + end + + private + + def editable? + Pundit.policy(user, record.work_version.work).edit? + end end diff --git a/spec/policies/file_version_membership_policy_spec.rb b/spec/policies/file_version_membership_policy_spec.rb index 0339517bd..d1d63cef1 100644 --- a/spec/policies/file_version_membership_policy_spec.rb +++ b/spec/policies/file_version_membership_policy_spec.rb @@ -3,10 +3,13 @@ require 'rails_helper' RSpec.describe FileVersionMembershipPolicy, type: :policy do + subject { described_class } + let(:user) { instance_double 'User' } let(:file_version_membership) { instance_double 'FileVersionMembership', work_version: work_version } let(:work_version) { instance_double 'WorkVersion' } let(:mock_policy) { instance_spy 'WorkVersionPolicy' } + let(:admin) { build(:user, :admin) } describe 'download?' do before { allow(Pundit).to receive(:policy).with(user, work_version).and_return(mock_policy) } @@ -16,4 +19,57 @@ expect(mock_policy).to have_received(:download?) end end + + permissions :destroy? do + let(:regular_user) { build(:user) } + let(:edit_user) { build(:user) } + let(:proxy_user) { build(:user) } + let(:admin) { build(:user, :admin) } + let(:application) { build(:external_app) } + let (:work) { create(:work, proxy_depositor: proxy_user.actor, edit_users: [edit_user]) } + let(:work_version) { create(:work_version, :draft, work:) } + + context 'with a draft version that has NOT been persisted' do + let(:membership) { build(:file_version_membership) } + + it { is_expected.not_to permit(regular_user, membership) } + it { is_expected.not_to permit(proxy_user, membership) } + it { is_expected.not_to permit(edit_user, membership) } + it { is_expected.not_to permit(admin, membership) } + it { is_expected.not_to permit(application, membership) } + end + + context 'with the latest draft version' do + let(:membership) { create(:file_version_membership, work_version:) } + + it { is_expected.not_to permit(regular_user, membership) } + it { is_expected.to permit(proxy_user, membership) } + it { is_expected.to permit(edit_user, membership) } + it { is_expected.to permit(admin, membership) } + it { is_expected.to permit(application, membership) } + end + + context 'with a draft version that is NOT the latest' do + let(:work) { create(:work, versions_count: 2, proxy_depositor: proxy_user.actor, edit_users: [edit_user]) } + let (:work_version) { create(:work_version, work:) } + let(:membership) { create(:file_version_membership, work_version:) } + + it { is_expected.not_to permit(regular_user, membership) } + it { is_expected.to permit(proxy_user, membership) } + it { is_expected.to permit(edit_user, membership) } + it { is_expected.to permit(application, membership) } + it { is_expected.to permit(admin, membership) } + end + + context 'with a published version' do + let (:work_version) { create(:work_version, :published, work:) } + let(:membership) { create(:file_version_membership, work_version:) } + + it { is_expected.not_to permit(regular_user, membership) } + it { is_expected.not_to permit(edit_user, membership) } + it { is_expected.not_to permit(proxy_user, membership) } + it { is_expected.to permit(admin, membership) } + it { is_expected.to permit(application, membership) } + end + end end From 8cd63cd09661e6051366ae9cdd9f1782a7cbbf18 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Mon, 23 Sep 2024 09:45:39 -0600 Subject: [PATCH 2/2] 1578 - Wording change --- .../dashboard/file_version_memberships_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/dashboard/file_version_memberships_controller.rb b/app/controllers/dashboard/file_version_memberships_controller.rb index 4fd0afdfa..07a381dd3 100644 --- a/app/controllers/dashboard/file_version_memberships_controller.rb +++ b/app/controllers/dashboard/file_version_memberships_controller.rb @@ -32,7 +32,7 @@ def destroy respond_to do |format| format.html do redirect_to dashboard_form_files_path(@work_version), - notice: 'Work version was successfully destroyed.' + notice: 'File was successfully destroyed.' end format.json { head :no_content } end