Merge pull request #6357 from samvera/remove_file_set_relatives_work_…

…del_6334

Removes FileSet related objects when work deleted (#6334).

app/controllers/concerns/hyrax/works_controller_behavior.rb

@@ -114,9 +114,9 @@ def destroy
         Hyrax.config.callback.run(:after_destroy, curation_concern.id, current_user, warn: false)
       else
         transactions['work_resource.destroy']
-          .with_step_args('work_resource.delete' => { user: current_user })
-          .call(curation_concern)
-          .value!
+          .with_step_args('work_resource.delete' => { user: current_user },
+                          'work_resource.delete_all_file_sets' => { user: current_user })
+          .call(curation_concern).value!
 
         title = Array(curation_concern.title).first
       end

app/controllers/hyrax/batch_edits_controller.rb

@@ -108,9 +108,9 @@ def destroy_batch
       batch.each do |id|
         resource = Hyrax.query_service.find_by(id: Valkyrie::ID.new(id))
         transactions['work_resource.destroy']
-          .with_step_args('work_resource.delete' => { user: current_user })
-          .call(resource)
-          .value!
+          .with_step_args('work_resource.delete' => { user: current_user },
+                          'work_resource.delete_all_file_sets' => { user: current_user })
+          .call(resource).value!
       end
       after_update
     end

app/services/hyrax/listeners/member_cleanup_listener.rb

@@ -8,20 +8,7 @@ class MemberCleanupListener
       # Called when 'object.deleted' event is published
       # @param [Dry::Events::Event] event
       # @return [void]
-      def on_object_deleted(event)
-        return unless event.payload.key?(:object) # legacy callback
-        return if event[:object].is_a?(ActiveFedora::Base) # handled by legacy code
-
-        Hyrax.custom_queries.find_child_file_sets(resource: event[:object]).each do |file_set|
-          Hyrax.persister.delete(resource: file_set)
-          Hyrax.publisher
-               .publish('object.deleted', object: file_set, id: file_set.id, user: event[:user])
-        rescue StandardError # we don't uncaught errors looping filesets
-          Hyrax.logger.warn "Failed to delete #{file_set.class}:#{file_set.id} " \
-                            "during cleanup for resource: #{event[:object]}. " \
-                            'This member may now be orphaned.'
-        end
-      end
+      def on_object_deleted(event); end
 
       # Called when 'collection.deleted' event is published
       # @param [Dry::Events::Event] event

app/services/hyrax/listeners/metadata_index_listener.rb

@@ -83,6 +83,17 @@ def on_collection_deleted(event)
         Hyrax.index_adapter.delete(resource: event[:collection])
       end
 
+      ##
+      # Remove the resource from the index.
+      #
+      # Called when 'file.metadata.deleted' event is published
+      # @param [Dry::Events::Event] event
+      # @return [void]
+      def on_file_metadata_deleted(event)
+        return unless resource? event.payload[:metadata]
+        Hyrax.index_adapter.delete(resource: event[:metadata])
+      end
+
       private
 
       def resource?(resource)

lib/hyrax/publisher.rb

@@ -137,6 +137,10 @@ class Publisher
     #     a system user.
     register_event('file.metadata.updated')
 
+    # @since 5.0.0
+    # @macro a_registered_event
+    register_event('file.metadata.deleted')
+
     # @since 5.0.0
     # @macro a_registered_event
     register_event('file.uploaded')

lib/hyrax/transactions/container.rb

@@ -25,6 +25,7 @@ class Container # rubocop:disable Metrics/ClassLength
       require 'hyrax/transactions/collection_create'
       require 'hyrax/transactions/collection_destroy'
       require 'hyrax/transactions/collection_update'
+      require 'hyrax/transactions/file_metadata_destroy'
       require 'hyrax/transactions/file_set_destroy'
       require 'hyrax/transactions/file_set_update'
       require 'hyrax/transactions/work_create'
@@ -38,8 +39,11 @@ class Container # rubocop:disable Metrics/ClassLength
       require 'hyrax/transactions/steps/change_depositor'
       require 'hyrax/transactions/steps/check_for_empty_admin_set'
       require 'hyrax/transactions/steps/delete_access_control'
+      require 'hyrax/transactions/steps/delete_all_file_metadata'
+      require 'hyrax/transactions/steps/delete_all_file_sets'
       require 'hyrax/transactions/steps/delete_resource'
       require 'hyrax/transactions/steps/ensure_admin_set'
+      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/save'
@@ -53,9 +57,6 @@ class Container # rubocop:disable Metrics/ClassLength
       require 'hyrax/transactions/steps/set_user_as_depositor'
       require 'hyrax/transactions/steps/update_work_members'
       require 'hyrax/transactions/steps/validate'
-      require 'hyrax/transactions/steps/delete_all_file_metadata'
-      require 'hyrax/transactions/steps/file_metadata_delete'
-      require 'hyrax/transactions/file_metadata_destroy'
 
       extend Dry::Container::Mixin
 
@@ -147,7 +148,7 @@ class Container # rubocop:disable Metrics/ClassLength
         end
 
         ops.register 'delete_all_file_metadata' do
-          Steps::DeleteAllFileMetadata.new(property: :file_ids)
+          Steps::DeleteAllFileMetadata.new
         end
 
         ops.register 'destroy' do
@@ -158,6 +159,10 @@ class Container # rubocop:disable Metrics/ClassLength
           Steps::RemoveFileSetFromWork.new
         end
 
+        ops.register 'delete_acl' do
+          Steps::DeleteAccessControl.new
+        end
+
         ops.register 'save_acl' do
           Steps::SaveAccessControl.new
         end
@@ -244,6 +249,10 @@ class Container # rubocop:disable Metrics/ClassLength
           Steps::DeleteResource.new
         end
 
+        ops.register 'delete_all_file_sets' do
+          Steps::DeleteAllFileSets.new
+        end
+
         ops.register 'destroy' do
           WorkDestroy.new
         end

lib/hyrax/transactions/file_set_destroy.rb

@@ -8,9 +8,10 @@ module Transactions
     #
     # @since 3.1.0
     class FileSetDestroy < Transaction
-      DEFAULT_STEPS = ['file_set.remove_from_work',
-                       'file_set.delete',
-                       'file_set.delete_all_file_metadata'].freeze
+      DEFAULT_STEPS = ['file_set.delete_all_file_metadata',
+                       'file_set.remove_from_work',
+                       'file_set.delete_acl',
+                       'file_set.delete'].freeze
 
       ##
       # @see Hyrax::Transactions::Transaction

lib/hyrax/transactions/steps/delete_all_file_metadata.rb

@@ -14,7 +14,7 @@ class DeleteAllFileMetadata
 
         ##
         # @params [#save] persister
-        def initialize(property:, query_service: Hyrax.query_service, persister: Hyrax.persister, publisher: Hyrax.publisher)
+        def initialize(property: :file_ids, query_service: Hyrax.query_service, persister: Hyrax.persister, publisher: Hyrax.publisher)
           @property = property
           @persister = persister
           @query_service = query_service
@@ -30,7 +30,10 @@ def call(resource)
           return Failure(:resource_not_persisted) unless resource.persisted?
 
           resource[@property].each do |file_id|
-            Hyrax::Transactions::Container['file_metadata.destroy'].call(@query_service.custom_queries.find_file_metadata_by(id: file_id))
+            return Failure[:failed_to_delete_file_metadata, file_id] unless
+              Hyrax::Transactions::Container['file_metadata.destroy']
+              .call(@query_service.custom_queries.find_file_metadata_by(id: file_id))
+              .success?
           rescue ::Ldp::Gone
             nil
           end

lib/hyrax/transactions/steps/delete_all_file_sets.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+require 'dry/monads'
+
+module Hyrax
+  module Transactions
+    module Steps
+      ##
+      # Deletes a resource's member FileSets from the persister, returning a `Dry::Monads::Result`
+      # (`Success`|`Failure`).
+      #
+      # @see https://dry-rb.org/gems/dry-monads/1.0/result/
+      class DeleteAllFileSets
+        include Dry::Monads[:result]
+
+        ##
+        # @params [#save] persister
+        def initialize(query_service: Hyrax.query_service, 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(resource, user: nil)
+          return Failure(:resource_not_persisted) unless resource.persisted?
+
+          @query_service.custom_queries.find_child_file_sets(resource: resource).each do |file_set|
+            return Failure[:failed_to_delete_file_set, file_set] unless
+              Hyrax::Transactions::Container['file_set.destroy']
+              .with_step_args('file_set.remove_from_work' => { user: user },
+                              'file_set.delete' => { user: user })
+              .call(file_set).success?
+          rescue ::Ldp::Gone
+            nil
+          end
+
+          Success(resource)
+        end
+      end
+    end
+  end
+end

lib/hyrax/transactions/steps/file_metadata_delete.rb

@@ -29,6 +29,7 @@ def call(resource)
           return Failure(:resource_not_persisted) unless resource.persisted?
 
           @persister.delete(resource: resource)
+          @publisher.publish('file.metadata.deleted', metadata: resource)
           Valkyrie::StorageAdapter.delete(id: resource.file_identifier)
 
           Success(resource)

lib/hyrax/transactions/work_destroy.rb

@@ -8,7 +8,8 @@ module Transactions
     #
     # @since 3.0.0
     class WorkDestroy < Transaction
-      DEFAULT_STEPS = ['work_resource.delete_acl',
+      DEFAULT_STEPS = ['work_resource.delete_all_file_sets',
+                       'work_resource.delete_acl',
                        'work_resource.delete'].freeze
 
       ##

spec/hyrax/transactions/file_set_destroy_spec.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 require 'spec_helper'
 require 'hyrax/transactions'
-require 'dry/container/stub'
 
 RSpec.describe Hyrax::Transactions::FileSetDestroy do
   subject(:transaction) { described_class.new }
@@ -10,6 +9,11 @@
   describe '#call' do
     let(:user) { FactoryBot.create(:user) }
 
+    before do
+      file_set.permission_manager.read_users = [user.user_key]
+      file_set.permission_manager.acl.save
+    end
+
     context 'without a user' do
       it 'is a failure' do
         expect(transaction.call(file_set)).to be_failure
@@ -28,6 +32,13 @@
         .to raise_error Valkyrie::Persistence::ObjectNotFoundError
     end
 
+    it 'deletes the access control resource' do
+      expect { transaction.with_step_args('file_set.remove_from_work' => { user: user }).call(file_set) }
+        .to change { Hyrax::AccessControl.for(resource: file_set).persisted? }
+        .from(true)
+        .to(false)
+    end
+
     context "with attached files" do
       let(:work) { FactoryBot.valkyrie_create(:hyrax_work, uploaded_files: [FactoryBot.create(:uploaded_file)], edit_users: [user]) }
       let(:file_set) { query_service.find_members(resource: work).first }

spec/hyrax/transactions/steps/delete_all_file_metadata_spec.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+require 'spec_helper'
+require 'hyrax/transactions'
+
+RSpec.describe Hyrax::Transactions::Steps::DeleteAllFileMetadata, valkyrie_adapter: :test_adapter, storage_adapter: :test_disk do
+  subject(:step) { described_class.new }
+  let(:file_set) { FactoryBot.valkyrie_create(:hyrax_file_set, :with_files) }
+
+  describe '#call' do
+    it 'gives success' do
+      expect(step.call(file_set)).to be_success
+    end
+
+    it 'destroys each file_set' do
+      file_ids = file_set.file_ids
+      step.call(file_set)
+      file_ids.each do |id|
+        expect { Hyrax.query_service.find_by(id: id) }.to raise_error Valkyrie::Persistence::ObjectNotFoundError
+      end
+    end
+
+    context 'with a resource that is not saved' do
+      let(:file_set) { FactoryBot.build(:hyrax_file_set) }
+
+      it 'is a failure' do
+        expect(step.call(file_set)).to be_failure
+      end
+    end
+  end
+end

spec/hyrax/transactions/steps/delete_all_file_sets_spec.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+require 'spec_helper'
+require 'hyrax/transactions'
+
+RSpec.describe Hyrax::Transactions::Steps::DeleteAllFileSets, valkyrie_adapter: :test_adapter do
+  subject(:step) { described_class.new }
+  let(:user)      { FactoryBot.create(:user) }
+  let(:work)      { FactoryBot.valkyrie_create(:hyrax_work, :with_member_file_sets) }
+
+  describe '#call' do
+    it 'fails without a user' do
+      expect(step.call(work)).to be_failure
+    end
+
+    it 'gives success' do
+      expect(step.call(work, user: user)).to be_success
+    end
+
+    it 'destroys each file_set' do
+      member_ids = work.member_ids
+      step.call(work, user: user)
+      member_ids.each do |id|
+        expect { Hyrax.query_service.find_by(id: id) }.to raise_error Valkyrie::Persistence::ObjectNotFoundError
+      end
+    end
+
+    context 'with a resource that is not saved' do
+      let(:work) { FactoryBot.build(:hyrax_work) }
+
+      it 'is a failure' do
+        expect(step.call(work)).to be_failure
+      end
+    end
+  end
+end

spec/hyrax/transactions/work_destroy_spec.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+require 'spec_helper'
+require 'hyrax/transactions'
+
+RSpec.describe Hyrax::Transactions::WorkDestroy do
+  subject(:transaction) { described_class.new }
+  let(:work)      { FactoryBot.valkyrie_create(:hyrax_work, read_users: [user], members: [file_set]) }
+  let(:file_set)  { FactoryBot.valkyrie_create(:hyrax_file_set) }
+
+  describe '#call' do
+    let(:user) { FactoryBot.create(:user) }
+
+    context 'without a user' do
+      it 'is a failure' do
+        expect(transaction.call(work)).to be_failure
+      end
+    end
+
+    it 'succeeds' do
+      expect(transaction.with_step_args('work_resource.delete_all_file_sets' => { user: user }).call(work))
+        .to be_success
+    end
+
+    it 'deletes the work' do
+      transaction.with_step_args('work_resource.delete_all_file_sets' => { user: user }).call(work)
+
+      expect { Hyrax.query_service.find_by(id: work.id) }
+        .to raise_error Valkyrie::Persistence::ObjectNotFoundError
+    end
+
+    it 'deletes the file set' do
+      transaction.with_step_args('work_resource.delete_all_file_sets' => { user: user }).call(work)
+
+      expect { Hyrax.query_service.find_by(id: file_set.id) }
+        .to raise_error Valkyrie::Persistence::ObjectNotFoundError
+    end
+
+    it 'deletes the access control resource' do
+      expect { transaction.with_step_args('work_resource.delete_all_file_sets' => { user: user }).call(work) }
+        .to change { Hyrax::AccessControl.for(resource: work).persisted? }
+        .from(true).to(false)
+    end
+
+    context "with attached files" do
+      let(:work) { FactoryBot.valkyrie_create(:hyrax_work, uploaded_files: [FactoryBot.create(:uploaded_file)], edit_users: [user]) }
+      let(:file_set) { query_service.find_members(resource: work).first }
+      let(:file_metadata) { query_service.custom_queries.find_file_metadata_by(id: file_set.file_ids.first) }
+      let(:uploaded) { storage_adapter.find_by(id: file_metadata.file_identifier) }
+      let(:storage_adapter) { Hyrax.storage_adapter }
+      let(:query_service) { Hyrax.query_service }
+      it "deletes them" do
+        file_metadata
+        transaction.with_step_args('work_resource.delete_all_file_sets' => { user: user }).call(work)
+
+        expect { Hyrax.query_service.find_by(id: file_metadata.id) }.to raise_error Valkyrie::Persistence::ObjectNotFoundError
+        expect { storage_adapter.find_by(id: uploaded.id) }.to raise_error Valkyrie::StorageAdapter::FileNotFound
+      end
+    end
+  end
+end