From 4ef5717285c00d93202227b9d1a9bb6985240847 Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Wed, 4 Dec 2024 17:40:08 -0500 Subject: [PATCH 1/3] Detect that embargo state changed when determining if permissions changed --- .../works_controller_behavior_override.rb | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb b/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb index 4da855707..82166f105 100644 --- a/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb +++ b/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb @@ -26,6 +26,31 @@ def available_admin_sets Hyrax::AdminSetSelectionPresenter.new(admin_sets: admin_sets) end + private + # [hyc-override] Capture whether the work had an embargo before changes are saved + def save_permissions + @original_embargo_state = curation_concern.under_embargo? + @saved_permissions = + case curation_concern + when ActiveFedora::Base + curation_concern.permissions.map(&:to_hash) + else + Hyrax::AccessControl.for(resource: curation_concern).permissions + end + end + + # [hyc-override] Return true if the permissions have changed or the embargo state has changed + def permissions_changed? + perms_changed = @saved_permissions != + case curation_concern + when ActiveFedora::Base + curation_concern.permissions.map(&:to_hash) + else + Hyrax::AccessControl.for(resource: curation_concern).permissions + end + perms_changed || @original_embargo_state != curation_concern.under_embargo? + end + # [hyc-override] Special permissions for admins indicating they aren't constrained by the admin set class AdminPermissionTemplate < Hyrax::PermissionTemplate def release_no_delay? From 9bfea8cccb456119bab917b289d000e10ff0ae7d Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Thu, 5 Dec 2024 10:01:16 -0500 Subject: [PATCH 2/3] Make the overrides minimal. Add unit tests --- .../works_controller_behavior_override.rb | 19 ++----- .../hyrax/works_controller_behavior_spec.rb | 50 +++++++++++++++++-- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb b/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb index 82166f105..400442acd 100644 --- a/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb +++ b/app/overrides/controllers/concerns/hyrax/works_controller_behavior_override.rb @@ -28,27 +28,16 @@ def available_admin_sets private # [hyc-override] Capture whether the work had an embargo before changes are saved + alias_method :original_save_permissions, :save_permissions def save_permissions + original_save_permissions @original_embargo_state = curation_concern.under_embargo? - @saved_permissions = - case curation_concern - when ActiveFedora::Base - curation_concern.permissions.map(&:to_hash) - else - Hyrax::AccessControl.for(resource: curation_concern).permissions - end end # [hyc-override] Return true if the permissions have changed or the embargo state has changed + alias_method :original_permissions_changed?, :permissions_changed? def permissions_changed? - perms_changed = @saved_permissions != - case curation_concern - when ActiveFedora::Base - curation_concern.permissions.map(&:to_hash) - else - Hyrax::AccessControl.for(resource: curation_concern).permissions - end - perms_changed || @original_embargo_state != curation_concern.under_embargo? + original_permissions_changed? || @original_embargo_state != curation_concern.under_embargo? end # [hyc-override] Special permissions for admins indicating they aren't constrained by the admin set diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb index 58d62cd81..89b0ea735 100644 --- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb +++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb @@ -10,6 +10,12 @@ self.curation_concern_type = General end + before do + ActiveFedora::Cleaner.clean! + Blacklight.default_index.connection.delete_by_query('*:*') + Blacklight.default_index.connection.commit + end + describe '#available_admin_sets' do context 'with a logged in user' do before { sign_in user } @@ -26,9 +32,6 @@ let(:workflow) { Sipity::Workflow.find_by!(name: 'default', permission_template: permission_template) } before do - ActiveFedora::Cleaner.clean! - Blacklight.default_index.connection.delete_by_query('*:*') - Blacklight.default_index.connection.commit Hyrax::PermissionTemplateAccess.create(permission_template: permission_template, agent_type: 'user', agent_id: user.user_key, @@ -61,4 +64,45 @@ end end end + + describe '#permissions_changed?' do + let(:user) { FactoryBot.create(:user) } + let(:work) { + General.new(title: ['test work']) + } + + context 'with no new permissions or embargo' do + it 'returns false' do + allow(work).to receive(:under_embargo?).and_return(false) + controller.instance_variable_set(:@curation_concern, work) + + controller.send(:save_permissions) + + expect(controller.send(:permissions_changed?)).to be false + end + end + + context 'with an embargo added' do + it 'returns true' do + allow(work).to receive(:under_embargo?).and_return(true) + controller.instance_variable_set(:@curation_concern, work) + + controller.send(:save_permissions) + + expect(controller.send(:permissions_changed?)).to be true + end + end + + context 'with new permissions but no new embargo' do + it 'returns true' do + allow(work).to receive(:under_embargo?).and_return(false) + allow(controller).to receive(:original_permissions_changed?).and_return(true) + controller.instance_variable_set(:@curation_concern, work) + + controller.send(:save_permissions) + + expect(controller.send(:permissions_changed?)).to be true + end + end + end end From 473b7e994adc393752a5337cd9da8db6d516f4c7 Mon Sep 17 00:00:00 2001 From: Ben Pennell Date: Thu, 5 Dec 2024 15:22:42 -0500 Subject: [PATCH 3/3] Fix test and rubocop --- .../concerns/hyrax/works_controller_behavior_spec.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb index 89b0ea735..71b60c4c6 100644 --- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb +++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb @@ -68,8 +68,8 @@ describe '#permissions_changed?' do let(:user) { FactoryBot.create(:user) } let(:work) { - General.new(title: ['test work']) - } + General.new(title: ['test work']) + } context 'with no new permissions or embargo' do it 'returns false' do @@ -84,11 +84,13 @@ context 'with an embargo added' do it 'returns true' do - allow(work).to receive(:under_embargo?).and_return(true) + allow(work).to receive(:under_embargo?).and_return(false) controller.instance_variable_set(:@curation_concern, work) controller.send(:save_permissions) + allow(work).to receive(:under_embargo?).and_return(true) + expect(controller.send(:permissions_changed?)).to be true end end