From a06ea55ee1f20f8a434597dd00ed6dc90bade3aa Mon Sep 17 00:00:00 2001 From: Brad Watson Date: Tue, 17 Oct 2023 23:55:40 -0500 Subject: [PATCH] Fixes the deletion of collections 500 (#6362). (#6367) * Fixes the deletion of collections 500 (#6362). * Reworks the logic into a Transaction::Step. * rubocop appeasement. * Transfers listener rspec to step rspec. * rspec tweak * rubocop tweak --- .../hyrax/batch_edits_controller.rb | 6 +-- .../hyrax/dashboard/collections_controller.rb | 5 ++- .../listeners/member_cleanup_listener.rb | 15 +------ lib/hyrax/transactions/collection_destroy.rb | 5 ++- lib/hyrax/transactions/container.rb | 5 +++ .../steps/remove_from_membership.rb | 45 +++++++++++++++++++ .../transactions/collection_destroy_spec.rb | 8 +++- .../steps/remove_from_membership_spec.rb | 41 +++++++++++++++++ .../listeners/member_cleanup_listener_spec.rb | 4 +- 9 files changed, 111 insertions(+), 23 deletions(-) create mode 100644 lib/hyrax/transactions/steps/remove_from_membership.rb create mode 100644 spec/hyrax/transactions/steps/remove_from_membership_spec.rb diff --git a/app/controllers/hyrax/batch_edits_controller.rb b/app/controllers/hyrax/batch_edits_controller.rb index e473d8212a..33fe227bb1 100644 --- a/app/controllers/hyrax/batch_edits_controller.rb +++ b/app/controllers/hyrax/batch_edits_controller.rb @@ -41,9 +41,9 @@ def destroy_collection batch.each do |doc_id| resource = Hyrax.query_service.find_by(id: Valkyrie::ID.new(doc_id)) transactions['collection_resource.destroy'] - .with_step_args('collection_resource.delete' => { user: current_user }) - .call(resource) - .value! + .with_step_args('collection_resource.delete' => { user: current_user }, + 'collection_resource.remove_from_membership' => { user: current_user }) + .call(resource).value! end flash[:notice] = "Batch delete complete" after_destroy_collection diff --git a/app/controllers/hyrax/dashboard/collections_controller.rb b/app/controllers/hyrax/dashboard/collections_controller.rb index 52e81d2a5f..436123e6c3 100644 --- a/app/controllers/hyrax/dashboard/collections_controller.rb +++ b/app/controllers/hyrax/dashboard/collections_controller.rb @@ -245,7 +245,10 @@ def update_valkyrie_collection end def valkyrie_destroy - if transactions['collection_resource.destroy'].call(@collection).success? + if transactions['collection_resource.destroy'] + .with_step_args('collection_resource.delete' => { user: current_user }, + 'collection_resource.remove_from_membership' => { user: current_user }) + .call(@collection).success? after_destroy(params[:id]) else after_destroy_error(params[:id]) diff --git a/app/services/hyrax/listeners/member_cleanup_listener.rb b/app/services/hyrax/listeners/member_cleanup_listener.rb index 6f36c15d58..dfad6e1539 100644 --- a/app/services/hyrax/listeners/member_cleanup_listener.rb +++ b/app/services/hyrax/listeners/member_cleanup_listener.rb @@ -13,20 +13,7 @@ def on_object_deleted(event); end # Called when 'collection.deleted' event is published # @param [Dry::Events::Event] event # @return [void] - def on_collection_deleted(event) - return unless event.payload.key?(:collection) # legacy callback - return if event[:collection].is_a?(ActiveFedora::Base) # handled by legacy code - - Hyrax.custom_queries.find_members_of(collection: event[:collection]).each do |resource| - resource.member_of_collection_ids -= [event[:collection].id] - Hyrax.persister.save(resource: resource) - Hyrax.publisher - .publish('collection.membership.updated', collection: event[:collection], user: event[:user]) - rescue StandardError - Hyrax.logger.warn "Failed to remove collection reference from #{work.class}:#{work.id} " \ - "during cleanup for collection: #{event[:collection]}. " - end - end + def on_collection_deleted(event); end end end end diff --git a/lib/hyrax/transactions/collection_destroy.rb b/lib/hyrax/transactions/collection_destroy.rb index 82ffc38d3c..603ff04725 100644 --- a/lib/hyrax/transactions/collection_destroy.rb +++ b/lib/hyrax/transactions/collection_destroy.rb @@ -9,8 +9,9 @@ module Transactions # @since 3.4.0 class CollectionDestroy < Transaction # TODO: Add step that checks if collection is empty for collections of types that require it - DEFAULT_STEPS = ['collection_resource.delete', - 'collection_resource.delete_acl'].freeze + DEFAULT_STEPS = ['collection_resource.delete_acl', + 'collection_resource.remove_from_membership', + 'collection_resource.delete'].freeze ## # @see Hyrax::Transactions::Transaction diff --git a/lib/hyrax/transactions/container.rb b/lib/hyrax/transactions/container.rb index af9605d550..c5eadeaad2 100644 --- a/lib/hyrax/transactions/container.rb +++ b/lib/hyrax/transactions/container.rb @@ -46,6 +46,7 @@ class Container # rubocop:disable Metrics/ClassLength require 'hyrax/transactions/steps/file_metadata_delete' require 'hyrax/transactions/steps/set_collection_type_gid' require 'hyrax/transactions/steps/remove_file_set_from_work' + require 'hyrax/transactions/steps/remove_from_membership' require 'hyrax/transactions/steps/save' require 'hyrax/transactions/steps/save_access_control' require 'hyrax/transactions/steps/save_collection_banner' @@ -215,6 +216,10 @@ class Container # rubocop:disable Metrics/ClassLength Steps::DeleteAccessControl.new end + ops.register 'remove_from_membership' do + Steps::RemoveFromMembership.new + end + ops.register 'save_acl' do Steps::SaveAccessControl.new end diff --git a/lib/hyrax/transactions/steps/remove_from_membership.rb b/lib/hyrax/transactions/steps/remove_from_membership.rb new file mode 100644 index 0000000000..51df869ad3 --- /dev/null +++ b/lib/hyrax/transactions/steps/remove_from_membership.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true +require 'dry/monads' + +module Hyrax + module Transactions + module Steps + ## + # Removes a collection from its members, returning a `Dry::Monads::Result` + # (`Success`|`Failure`). + # + # @see https://dry-rb.org/gems/dry-monads/1.0/result/ + class RemoveFromMembership + include Dry::Monads[:result] + + ## + # @params [#save] persister + def initialize(query_service: Hyrax.custom_queries, persister: Hyrax.persister, publisher: Hyrax.publisher) + @persister = persister + @query_service = query_service + @publisher = publisher + end + + ## + # @param [Valkyrie::Resource] resource + # @param [::User] the user resposible for the delete action + # + # @return [Dry::Monads::Result] + def call(collection, user: nil) + return Failure(:resource_not_persisted) unless collection.persisted? + return Failure(:user_not_present) if user.blank? + + @query_service.find_members_of(collection: collection).each do |member| + member.member_of_collection_ids -= [collection.id] + @persister.save(resource: member) + @publisher.publish('collection.membership.updated', collection: collection, user: user) + rescue StandardError + nil + end + + Success(collection) + end + end + end + end +end diff --git a/spec/hyrax/transactions/collection_destroy_spec.rb b/spec/hyrax/transactions/collection_destroy_spec.rb index 23e8fc9e23..91c68c0278 100644 --- a/spec/hyrax/transactions/collection_destroy_spec.rb +++ b/spec/hyrax/transactions/collection_destroy_spec.rb @@ -5,10 +5,16 @@ RSpec.describe Hyrax::Transactions::CollectionDestroy, valkyrie_adapter: :test_adapter do subject(:tx) { described_class.new } let(:resource) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:user) { FactoryBot.create(:user) } describe '#call' do it 'is a success' do - expect(tx.call(resource)).to be_success + expect( + tx + .with_step_args('collection_resource.delete' => { user: user }, + 'collection_resource.remove_from_membership' => { user: user }) + .call(resource) + ).to be_success end end end diff --git a/spec/hyrax/transactions/steps/remove_from_membership_spec.rb b/spec/hyrax/transactions/steps/remove_from_membership_spec.rb new file mode 100644 index 0000000000..bfe3577029 --- /dev/null +++ b/spec/hyrax/transactions/steps/remove_from_membership_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true +require 'spec_helper' +require 'hyrax/transactions' +require 'hyrax/specs/spy_listener' + +RSpec.describe Hyrax::Transactions::Steps::RemoveFromMembership, valkyrie_adapter: :test_adapter do + subject(:step) { described_class.new } + let(:user) { FactoryBot.create(:user) } + let(:collection) { FactoryBot.valkyrie_create(:hyrax_collection) } + let(:work) { FactoryBot.valkyrie_create(:monograph, member_of_collection_ids: [collection.id]) } + let(:spy_listener) { Hyrax::Specs::SpyListener.new } + + describe '#call' do + before do + work + Hyrax.publisher.subscribe(spy_listener) + end + after { Hyrax.publisher.unsubscribe(spy_listener) } + + it 'fails without a user' do + expect(step.call(collection)).to be_failure + end + + it 'gives success' do + expect(step.call(collection, user: user)).to be_success + end + + it 'removes collection references from member objects' do + expect { step.call(collection, user: user) } + .to change { Hyrax.custom_queries.find_members_of(collection: collection).size } + .from(1) + .to(0) + end + + it 'publishes events' do + expect { step.call(collection, user: user) } + .to change { spy_listener.collection_membership_updated&.payload } + .to match(collection: collection, user: user) + end + end +end diff --git a/spec/services/hyrax/listeners/member_cleanup_listener_spec.rb b/spec/services/hyrax/listeners/member_cleanup_listener_spec.rb index daccdd34a5..720ce03cab 100644 --- a/spec/services/hyrax/listeners/member_cleanup_listener_spec.rb +++ b/spec/services/hyrax/listeners/member_cleanup_listener_spec.rb @@ -36,14 +36,14 @@ work end - it 'removes collection references from member objects' do + xit 'removes collection references from member objects' do expect { listener.on_collection_deleted(event) } .to change { Hyrax.custom_queries.find_members_of(collection: event[:collection]).size } .from(1) .to(0) end - it 'publishes events' do + xit 'publishes events' do listener.on_collection_deleted(event) expect(spy_listener.collection_membership_updated&.payload) .to include(collection: collection, user: user)