From 447f18e08085721fdcfa44648231d09a34cd0265 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 18 Jan 2024 10:50:40 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=81=20Add=20Transaction=20for=20Cleani?= =?UTF-8?q?ng=20Up=20Split=20Pages?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Valkyrie leverages transactions instead of the actor stack; as such we need to mirror the actor stack behavior as a transaction (or listener). In this case, we should use a transaction. Related to: - https://github.com/scientist-softserv/iiif_print/issues/312 --- .../actors/file_set_actor_decorator.rb | 2 +- .../iiif_print_container_decorator.rb | 32 +++++++++++++++++ ...nditionally_destroy_children_from_split.rb | 31 +++++++++++++++++ lib/iiif_print.rb | 18 ++++++++-- lib/iiif_print/data/work_derivatives.rb | 2 +- lib/iiif_print/engine.rb | 3 ++ lib/iiif_print/persistence_layer.rb | 28 ++++++++++++--- .../active_fedora_adapter.rb | 18 ++++++++++ .../persistence_layer/valkyrie_adapter.rb | 6 ++++ .../destroy_pdf_child_works_service.rb | 17 ++-------- .../transactions/container_decorator_spec.rb | 6 ++++ .../iiif_print_container_decorator_spec.rb | 21 ++++++++++++ ...onally_destroy_children_from_split_spec.rb | 34 +++++++++++++++++++ 13 files changed, 195 insertions(+), 23 deletions(-) create mode 100644 app/transactions/hyrax/transactions/iiif_print_container_decorator.rb create mode 100644 app/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split.rb create mode 100644 spec/transactions/hyrax/transactions/container_decorator_spec.rb create mode 100644 spec/transactions/hyrax/transactions/iiif_print_container_decorator_spec.rb create mode 100644 spec/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split_spec.rb diff --git a/app/actors/iiif_print/actors/file_set_actor_decorator.rb b/app/actors/iiif_print/actors/file_set_actor_decorator.rb index 98d48965..1b7b3122 100644 --- a/app/actors/iiif_print/actors/file_set_actor_decorator.rb +++ b/app/actors/iiif_print/actors/file_set_actor_decorator.rb @@ -48,7 +48,7 @@ def destroy # we destroy the children before the file_set, because we need the parent relationship IiifPrint::SplitPdfs::DestroyPdfChildWorksService.conditionally_destroy_spawned_children_of( file_set: file_set, - work: file_set.parent + work: IiifPrint.parent_for(file_set) ) # and now back to your regularly scheduled programming super diff --git a/app/transactions/hyrax/transactions/iiif_print_container_decorator.rb b/app/transactions/hyrax/transactions/iiif_print_container_decorator.rb new file mode 100644 index 00000000..1b5dd0ae --- /dev/null +++ b/app/transactions/hyrax/transactions/iiif_print_container_decorator.rb @@ -0,0 +1,32 @@ +module Hyrax + module Transactions + ## + # This decorator does the following: + # + # - Prepend the {ConditionallyDestroyChildrenFromSplit} transaction to the "file_set.destroy" + # step. The prependment corresponds to the behavior for + # {IiifPrint::Actors::FileSetActorDecorator#destroy} + # + # For more information about adjusting transactions, see + # [Transitioning workshop solution for adding transaction](https://github.com/samvera-labs/transitioning-to-valkyrie-workshop/commit/bcab2bb8f65078e88395c68f72be00e7ffad57ec) + # + # @see https://github.com/samvera/hyrax/blob/f875d61dc87229cf1f05eb2bb6d414b5ef314616/lib/hyrax/transactions/container.rb + class IiifPrintContainerDecorator + extend Dry::Container::Mixin + + namespace 'file_set' do |ops| + ops.register 'iiif_print_conditionally_destroy_spawned_children' do + Steps::ConditionallyDestroyChildrenFromSplit.new + end + ops.register 'destroy' do + Hyrax::Transactions::FileSetDestroy.new( + steps: (['file_set.iiif_print_conditionally_destroy_spawned_children'] + + Hyrax::Transactions::FileSetDestroy::DEFAULT_STEPS) + ) + end + end + end + end +end + +Hyrax::Transactions::Container.merge(Hyrax::Transactions::IiifPrintContainerDecorator) diff --git a/app/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split.rb b/app/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split.rb new file mode 100644 index 00000000..20ddf9a5 --- /dev/null +++ b/app/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split.rb @@ -0,0 +1,31 @@ +module Hyrax + module Transactions + module Steps + ## + # For a FileSet that is a PDF, we need to delete any works and file_sets that are the result of + # splitting that PDF into constituent images of each page of the PDF. This is responsible for + # that work. + class ConditionallyDestroyChildrenFromSplit + include Dry::Monads[:result] + + ## + # @param resource [Hyrax::FileSet] + def call(resource) + return Failure(:resource_not_persisted) unless resource.persisted? + + parent = IiifPrint.persistence_adapter.parent_for(resource) + return Success(true) unless parent + + # We do not care about the results of this call; as it is conditionally looking for things + # to destroy. + IiifPrint::SplitPdfs::DestroyPdfChildWorksService.conditionally_destroy_spawned_children_of( + file_set: resource, + work: parent + ) + + Success(true) + end + end + end + end +end diff --git a/lib/iiif_print.rb b/lib/iiif_print.rb index c5499f86..7b3dbe7e 100644 --- a/lib/iiif_print.rb +++ b/lib/iiif_print.rb @@ -44,8 +44,22 @@ def self.config(&block) end class << self - delegate :skip_splitting_pdf_files_that_end_with_these_texts, :persistence_adapter, to: :config - delegate :parent_for, :grandparent_for, :solr_construct_query, :solr_query, :solr_name, :clean_for_tests!, to: :persistence_adapter + delegate( + :persistence_adapter, + :skip_splitting_pdf_files_that_end_with_these_texts, + to: :config + ) + + delegate( + :clean_for_tests!, + :destroy_children_split_from, + :grandparent_for, + :solr_construct_query, + :solr_name, + :solr_query, + :parent_for, + to: :persistence_adapter + ) end DEFAULT_MODEL_CONFIGURATION = { diff --git a/lib/iiif_print/data/work_derivatives.rb b/lib/iiif_print/data/work_derivatives.rb index 3ab70bee..86ad596d 100644 --- a/lib/iiif_print/data/work_derivatives.rb +++ b/lib/iiif_print/data/work_derivatives.rb @@ -239,7 +239,7 @@ def primary_file_path # of the first assigned file path for single-file work. work_file = parent return if work_file.nil? - work_files = work_file.parent + work_files = IiifPrint.parent_for(work_file) return if work_files.nil? work_files.assigned[0] else diff --git a/lib/iiif_print/engine.rb b/lib/iiif_print/engine.rb index 82131d65..04a5596f 100644 --- a/lib/iiif_print/engine.rb +++ b/lib/iiif_print/engine.rb @@ -11,7 +11,10 @@ module IiifPrint class Engine < ::Rails::Engine isolate_namespace IiifPrint + config.eager_load_paths += %W[#{config.root}/app/transactions] + initializer 'requires' do + require 'hyrax/transactions/iiif_print_container_decorator' require 'iiif_print/persistence_layer' require 'iiif_print/persistence_layer/active_fedora_adapter' if defined?(ActiveFedora) require 'iiif_print/persistence_layer/valkyrie_adapter' if defined?(Valkyrie) diff --git a/lib/iiif_print/persistence_layer.rb b/lib/iiif_print/persistence_layer.rb index 15b8a00f..c46b9ee1 100644 --- a/lib/iiif_print/persistence_layer.rb +++ b/lib/iiif_print/persistence_layer.rb @@ -9,17 +9,31 @@ module IiifPrint module PersistenceLayer # @abstract class AbstractAdapter + ## + # @param file_set [Object] + # @param work [Object] + # @param model [Class] The class name for which we'll split children. + def self.destroy_children_split_from(file_set:, work:, model:) + raise NotImplementedError, "#{self}.{__method__}" + end + ## # @abstract - def self.parent_for(*); end + def self.parent_for(*) + raise NotImplementedError, "#{self}.{__method__}" + end ## # @abstract - def self.grandparent_for(*); end + def self.grandparent_for(*) + raise NotImplementedError, "#{self}.{__method__}" + end ## # @abstract - def self.solr_field_query(*); end + def self.solr_field_query(*) + raise NotImplementedError, "#{self}.{__method__}" + end ## # @abstract @@ -30,11 +44,15 @@ def self.clean_for_tests! ## # @abstract - def self.solr_query(*args); end + def self.solr_query(*args) + raise NotImplementedError, "#{self}.{__method__}" + end ## # @abstract - def self.solr_name(*args); end + def self.solr_name(*args) + raise NotImplementedError, "#{self}.{__method__}" + end end end end diff --git a/lib/iiif_print/persistence_layer/active_fedora_adapter.rb b/lib/iiif_print/persistence_layer/active_fedora_adapter.rb index e6299fb8..e7ca2468 100644 --- a/lib/iiif_print/persistence_layer/active_fedora_adapter.rb +++ b/lib/iiif_print/persistence_layer/active_fedora_adapter.rb @@ -60,6 +60,24 @@ def self.solr_name(field_name) ::ActiveFedora.index_field_mapper.solr_name(field_name.to_s) end end + + ## + # @param file_set [Object] + # @param work [Object] + # @param model [Class] The class name for which we'll split children. + def self.destroy_children_split_from(file_set:, work:, model:) + # look first for children by the file set id they were split from + children = model.where(split_from_pdf_id: file_set.id) + if children.blank? + # find works where file name and work `to_param` are both in the title + children = model.where(title: file_set.label).where(title: work.to_param) + end + return if children.blank? + children.each do |rcd| + rcd.destroy(eradicate: true) + end + true + end end end end diff --git a/lib/iiif_print/persistence_layer/valkyrie_adapter.rb b/lib/iiif_print/persistence_layer/valkyrie_adapter.rb index 8617c404..105aa256 100644 --- a/lib/iiif_print/persistence_layer/valkyrie_adapter.rb +++ b/lib/iiif_print/persistence_layer/valkyrie_adapter.rb @@ -40,6 +40,12 @@ def self.solr_query(*args) def self.solr_name(field_name) Hyrax.config.index_field_mapper.solr_name(field_name.to_s) end + + ## + # @todo implement this logic + def self.destroy_children_split_from(file_set:, work:, model:) + super + end end end end diff --git a/lib/iiif_print/split_pdfs/destroy_pdf_child_works_service.rb b/lib/iiif_print/split_pdfs/destroy_pdf_child_works_service.rb index d50daf35..61edde61 100644 --- a/lib/iiif_print/split_pdfs/destroy_pdf_child_works_service.rb +++ b/lib/iiif_print/split_pdfs/destroy_pdf_child_works_service.rb @@ -12,21 +12,10 @@ def self.conditionally_destroy_spawned_children_of(file_set:, work:) return unless child_model return unless file_set.class.pdf_mime_types.include?(file_set.mime_type) + # NOTE: The IiifPrint::PendingRelationship is an ActiveRecord object; hence we don't need to + # leverage an adapter. IiifPrint::PendingRelationship.where(parent_id: work.id, file_id: file_set.id).find_each(&:destroy) - destroy_spawned_children(model: child_model, file_set: file_set, work: work) - end - - private_class_method def self.destroy_spawned_children(model:, file_set:, work:) - # look first for children by the file set id they were split from - children = model.where(split_from_pdf_id: file_set.id) - if children.blank? - # find works where file name and work `to_param` are both in the title - children = model.where(title: file_set.label).where(title: work.to_param) - end - return if children.blank? - children.each do |rcd| - rcd.destroy(eradicate: true) - end + IiifPrint.destroy_children_split_from(file_set: file_set, work: work, model: child_model) end end end diff --git a/spec/transactions/hyrax/transactions/container_decorator_spec.rb b/spec/transactions/hyrax/transactions/container_decorator_spec.rb new file mode 100644 index 00000000..0024a18a --- /dev/null +++ b/spec/transactions/hyrax/transactions/container_decorator_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe do +end diff --git a/spec/transactions/hyrax/transactions/iiif_print_container_decorator_spec.rb b/spec/transactions/hyrax/transactions/iiif_print_container_decorator_spec.rb new file mode 100644 index 00000000..aa6032b2 --- /dev/null +++ b/spec/transactions/hyrax/transactions/iiif_print_container_decorator_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Hyrax::Transactions::Container do + describe 'file_set.destroy' do + subject(:transaction_step) { described_class['file_set.destroy'] } + describe '#steps' do + subject { transaction_step.steps } + it { + is_expected.to match_array(["file_set.iiif_print_conditionally_destroy_spawned_children", + "file_set.remove_from_work", + "file_set.delete"]) + } + end + end + describe 'file_set.iiif_print_conditionally_destroy_spawned_children' do + subject(:transaction_step) { described_class['file_set.iiif_print_conditionally_destroy_spawned_children'] } + it { is_expected.to be_a Hyrax::Transactions::Steps::ConditionallyDestroyChildrenFromSplit } + end +end diff --git a/spec/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split_spec.rb b/spec/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split_spec.rb new file mode 100644 index 00000000..8696a3da --- /dev/null +++ b/spec/transactions/hyrax/transactions/steps/conditionally_destroy_children_from_split_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Hyrax::Transactions::Steps::ConditionallyDestroyChildrenFromSplit do + describe '#call' do + let(:file_set) { double(Hyrax::FileSet, persisted?: persisted) } + subject { described_class.new.call(file_set) } + + describe 'with an unsaved resource' do + let(:persisted) { false } + it { is_expected.to be_failure } + end + + describe 'with a saved resource' do + let(:persisted) { true } + before { expect(IiifPrint.persistence_adapter).to receive(:parent_for).and_return(parent) } + + context 'without a parent' do + let(:parent) { nil } + it { is_expected.to be_success } + end + + context 'with a parent' do + let(:parent) { double(Valkyrie::Resource) } + it do + expect(IiifPrint::SplitPdfs::DestroyPdfChildWorksService).to receive(:conditionally_destroy_spawned_children_of) + .with(file_set: file_set, work: parent) + is_expected.to be_success + end + end + end + end +end