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

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

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])

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

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

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

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

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

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

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)