From 4eb13daa05ed230e753275f961d385843e562bdd Mon Sep 17 00:00:00 2001 From: "Michael J. Giarlo" Date: Wed, 25 Sep 2024 14:41:59 -0700 Subject: [PATCH] Change logic determining whether item is publishable Fixes #5715 --- app/models/repository_object.rb | 7 ++++ app/services/member_service.rb | 37 +++++++--------- .../publish/metadata_transfer_service.rb | 4 +- app/services/virtual_object_service.rb | 34 ++++++--------- spec/models/repository_object_spec.rb | 30 +++++++++++++ spec/services/member_service_spec.rb | 42 ++++++++----------- .../publish/metadata_transfer_service_spec.rb | 4 +- spec/services/virtual_object_service_spec.rb | 38 +++++++---------- 8 files changed, 100 insertions(+), 96 deletions(-) diff --git a/app/models/repository_object.rb b/app/models/repository_object.rb index 869284233..c1a528d31 100644 --- a/app/models/repository_object.rb +++ b/app/models/repository_object.rb @@ -147,6 +147,13 @@ def closed? head_version == last_closed_version end + # When a collection object is published, publish the collection members that: + # * have a last closed version (meaning they are not Registered - they've been accessioned); and + # * there's cocina for that last closed version (meaning they've been closed at least once since we moved to the new version model) + def publishable? + last_closed_version.present? && last_closed_version.has_cocina? + end + # @return [String] xml representation of version metadata def version_xml Nokogiri::XML::Builder.new(encoding: 'UTF-8') do |xml| diff --git a/app/services/member_service.rb b/app/services/member_service.rb index edbad6fdd..2fc5944db 100644 --- a/app/services/member_service.rb +++ b/app/services/member_service.rb @@ -3,41 +3,32 @@ # Finds the members of a collection class MemberService # @param [String] druid the identifier of the collection - # @param [Boolean] only_published when true, restrict to only published items - # @param [Boolean] exclude_opened when true, exclude opened items + # @param [Boolean] publishable when true, restrict to publishable items only # @return [Array] the druids for the members of this collection - def self.for(druid, only_published: false, exclude_opened: false) - new(druid, only_published:, exclude_opened:).for + def self.for(druid, publishable: false) + new(druid, publishable:).for end - def initialize(druid, only_published: false, exclude_opened: false) + def initialize(druid, publishable: false) @druid = druid - @only_published = only_published - @exclude_opened = exclude_opened + @publishable = publishable end def for - RepositoryObject.currently_members_of_collection(druid).select(:external_identifier, :version, :head_version_id, :opened_version_id) - .then { |members| exclude_opened_members(members) } - .then { |members| only_published_members(members) } - .map(&:external_identifier) + RepositoryObject + .currently_members_of_collection(druid) + .select(:external_identifier, :version, :head_version_id, :opened_version_id, :last_closed_version_id) + .then { |members| publishable_members(members) } + .map(&:external_identifier) end private - attr_reader :druid, :only_published, :exclude_opened + attr_reader :druid, :publishable - def exclude_opened_members(members) - return members unless exclude_opened + def publishable_members(members) + return members unless publishable - members.reject(&:open?) - end - - def only_published_members(members) - return members unless only_published - - members.select do |member| - WorkflowStateService.published?(druid: member.external_identifier, version: member.version) - end + members.select(&:publishable?) end end diff --git a/app/services/publish/metadata_transfer_service.rb b/app/services/publish/metadata_transfer_service.rb index 67abcaf59..359549b87 100644 --- a/app/services/publish/metadata_transfer_service.rb +++ b/app/services/publish/metadata_transfer_service.rb @@ -37,14 +37,14 @@ def publish def republish_collection_members! Array.wrap( - MemberService.for(druid, exclude_opened: true, only_published: true) + MemberService.for(druid, publishable: true) ).each do |member_druid| PublishJob.set(queue: :publish_low).perform_later(druid: member_druid, background_job_result: BackgroundJobResult.create, workflow:, log_success: false) end end def republish_virtual_object_constituents! - VirtualObjectService.constituents(cocina_object, exclude_opened: true, only_published: true).each do |constituent_druid| + VirtualObjectService.constituents(cocina_object, publishable: true).each do |constituent_druid| PublishJob.set(queue: :publish_low).perform_later(druid: constituent_druid, background_job_result: BackgroundJobResult.create, workflow:, log_success: false) end end diff --git a/app/services/virtual_object_service.rb b/app/services/virtual_object_service.rb index 7f1603147..62462c4f7 100644 --- a/app/services/virtual_object_service.rb +++ b/app/services/virtual_object_service.rb @@ -7,43 +7,35 @@ def self.constituents(...) end # @param [Cocina::Models::DRO] cocina_dro the cocina model for the virtual object - # @param [Boolean] only_published when true, restrict to only published items - # @param [Boolean] exclude_opened when true, exclude opened items + # @param [Boolean] publishable when true, restrict to publishable items only # @return [Array] the druids for the constituents of this virtual object - def initialize(cocina_dro, only_published: false, exclude_opened: false) + def initialize(cocina_dro, publishable: false) @cocina_dro = cocina_dro - @only_published = only_published - @exclude_opened = exclude_opened + @publishable = publishable end def constituents return [] unless cocina_dro.dro? - RepositoryObject.joins(:head_version).where(external_identifier: constituent_druids).select(:external_identifier, :version, :head_version_id, :opened_version_id) - .then { |constituents| exclude_opened_constituents(constituents) } - .then { |constituents| only_published_constituents(constituents) } - .map(&:external_identifier) + RepositoryObject + .joins(:head_version) + .where(external_identifier: constituent_druids) + .select(:external_identifier, :version, :head_version_id, :opened_version_id, :last_closed_version_id) + .then { |constituents| publishable_constituents(constituents) } + .map(&:external_identifier) end private - attr_reader :cocina_dro, :only_published, :exclude_opened + attr_reader :cocina_dro, :publishable def constituent_druids cocina_dro.structural.hasMemberOrders.flat_map(&:members).uniq end - def exclude_opened_constituents(constituents) - return constituents unless exclude_opened + def publishable_constituents(constituents) + return constituents unless publishable - constituents.reject(&:open?) - end - - def only_published_constituents(constituents) - return constituents unless only_published - - constituents.select do |constituent| - WorkflowStateService.published?(druid: constituent.external_identifier, version: constituent.version) - end + constituents.select(&:publishable?) end end diff --git a/spec/models/repository_object_spec.rb b/spec/models/repository_object_spec.rb index 0e325af92..b992bf333 100644 --- a/spec/models/repository_object_spec.rb +++ b/spec/models/repository_object_spec.rb @@ -119,6 +119,36 @@ end end + describe '#publishable?' do + subject(:repository_object) { create(:repository_object, **attrs) } + + context 'without a last closed version' do + it 'returns false' do + expect(repository_object).not_to be_publishable + end + end + + context 'with a last closed version lacking cocina' do + before do + repository_object.update(last_closed_version: repository_object.versions.create!(version: 2, version_description: 'closed')) + end + + it 'returns false' do + expect(repository_object).not_to be_publishable + end + end + + context 'with a last closed version containing cocina' do + before do + repository_object.update(last_closed_version: repository_object.versions.create!(version: 2, version_description: 'closed', cocina_version: 1)) + end + + it 'returns true' do + expect(repository_object).to be_publishable + end + end + end + describe '#open_version!' do subject(:repository_object) { create(:repository_object, **attrs) } diff --git a/spec/services/member_service_spec.rb b/spec/services/member_service_spec.rb index ff774a99c..941692582 100644 --- a/spec/services/member_service_spec.rb +++ b/spec/services/member_service_spec.rb @@ -3,63 +3,57 @@ require 'rails_helper' RSpec.describe MemberService do - let(:members) { described_class.for(collection_druid, exclude_opened:, only_published:) } + let(:members) { described_class.for(collection_druid, publishable:) } let(:collection_druid) { 'druid:bc123df4567' } - let(:exclude_opened) { false } - let(:only_published) { false } + let(:publishable) { false } let(:repository_object1) { create(:repository_object) } let(:repository_object2) { create(:repository_object) } - - let(:workflow_client) { instance_double(Dor::Workflow::Client) } + let(:repository_object3) { create(:repository_object) } before do # Opened repository_object_version1 = create(:repository_object_version, version: 2, is_member_of: [collection_druid], repository_object: repository_object1) repository_object1.update!(head_version: repository_object_version1, opened_version: repository_object_version1) - # Closed + # Closed, with Cocina repository_object_version2 = create(:repository_object_version, version: 2, is_member_of: [collection_druid], repository_object: repository_object2, closed_at: Time.current) repository_object2.update!(head_version: repository_object_version2, last_closed_version: repository_object_version2, opened_version: nil) + # Closed, without Cocina + repository_object_version3 = create(:repository_object_version, version: 2, is_member_of: [collection_druid], repository_object: repository_object3, closed_at: Time.current, cocina_version: nil) + repository_object3.update!( + head_version: repository_object_version3, + last_closed_version: repository_object_version3, + opened_version: create(:repository_object_version, version: 3, is_member_of: [collection_druid], repository_object: repository_object3) + ) + # Not a member create(:repository_object) - allow(WorkflowClientFactory).to receive(:build).and_return(workflow_client) end describe '.for' do context 'when collection has members' do it 'returns members' do - expect(members).to contain_exactly(repository_object1.external_identifier, repository_object2.external_identifier) + expect(members).to contain_exactly(repository_object1.external_identifier, repository_object2.external_identifier, repository_object3.external_identifier) end end context 'when collection has no members' do + let(:members) { described_class.for('druid:cc123df4568') } + it 'returns no members' do - expect(described_class.for('druid:cc123df4568')).to be_empty + expect(members).to be_empty end end - context 'when excluding open members' do - let(:exclude_opened) { true } + context 'when only publishable members' do + let(:publishable) { true } it 'returns members' do expect(members).to eq [repository_object2.external_identifier] end end - - context 'when only published members' do - let(:only_published) { true } - - before do - allow(workflow_client).to receive(:lifecycle).with(druid: repository_object1.external_identifier, milestone_name: 'published', version: 2).and_return(true) - allow(workflow_client).to receive(:lifecycle).with(druid: repository_object2.external_identifier, milestone_name: 'published', version: 2).and_return(false) - end - - it 'returns members' do - expect(members).to eq [repository_object1.external_identifier] - end - end end end diff --git a/spec/services/publish/metadata_transfer_service_spec.rb b/spec/services/publish/metadata_transfer_service_spec.rb index 5cbd08fb5..c969f2c9e 100644 --- a/spec/services/publish/metadata_transfer_service_spec.rb +++ b/spec/services/publish/metadata_transfer_service_spec.rb @@ -39,7 +39,7 @@ it 'publishes the collection and its members' do described_class.publish(druid:) - expect(MemberService).to have_received(:for).with(druid, exclude_opened: true, only_published: true) + expect(MemberService).to have_received(:for).with(druid, publishable: true) expect(publish_job).to have_received(:perform_later).once.with(druid: member_druid, background_job_result: BackgroundJobResult, workflow: 'accessionWF', log_success: false) expect(PurlFetcher::Client::Publish).to have_received(:publish).with(cocina: public_cocina, file_uploads: {}, version: 1, must_version: false, version_date: closed_at) @@ -245,7 +245,7 @@ it 'publishes the virtual object and its constituents' do described_class.publish(druid:) - expect(VirtualObjectService).to have_received(:constituents).with(Cocina::Models::DROWithMetadata, exclude_opened: true, only_published: true) + expect(VirtualObjectService).to have_received(:constituents).with(Cocina::Models::DROWithMetadata, publishable: true) expect(publish_job).to have_received(:perform_later).once.with(druid: constituent_druid, background_job_result: BackgroundJobResult, workflow: 'accessionWF', log_success: false) expect(PurlFetcher::Client::Publish).to have_received(:publish).with(cocina: public_cocina, file_uploads: {}, version: 1, must_version: false, version_date: closed_at) diff --git a/spec/services/virtual_object_service_spec.rb b/spec/services/virtual_object_service_spec.rb index 23dd976a8..2db0fc9ae 100644 --- a/spec/services/virtual_object_service_spec.rb +++ b/spec/services/virtual_object_service_spec.rb @@ -4,9 +4,8 @@ RSpec.describe VirtualObjectService do describe '#constituents' do - let(:constituents) { described_class.constituents(cocina_object, only_published:, exclude_opened:).sort } - let(:only_published) { false } - let(:exclude_opened) { false } + let(:constituents) { described_class.constituents(cocina_object, publishable:).sort } + let(:publishable) { false } context 'when the virtual object is a DRO' do let(:cocina_object) do @@ -15,9 +14,9 @@ hasMemberOrders: [ { members: [ - 'druid:dj321gm8879', # published and closed - 'druid:vs491yc7072', # published and open - 'druid:bc778pm9866' # unpublished and closed + 'druid:dj321gm8879', # has a closed version with cocina, thus publishable + 'druid:vs491yc7072', # has a closed version without cocina, thus not publishable + 'druid:bc778pm9866' # lacks a closed version, thus not publishable ], viewingDirection: 'left-to-right' } @@ -27,12 +26,11 @@ end before do - create(:repository_object, :closed, external_identifier: 'druid:dj321gm8879') - create(:repository_object, external_identifier: 'druid:vs491yc7072') - create(:repository_object, :closed, external_identifier: 'druid:bc778pm9866') - allow(WorkflowStateService).to receive(:published?).with(druid: 'druid:dj321gm8879', version: 1).and_return(true) - allow(WorkflowStateService).to receive(:published?).with(druid: 'druid:vs491yc7072', version: 1).and_return(true) - allow(WorkflowStateService).to receive(:published?).with(druid: 'druid:bc778pm9866', version: 1).and_return(false) + create(:repository_object, :closed, external_identifier: 'druid:dj321gm8879').tap do |object| + object.last_closed_version.update!(cocina_version: 1) + end + create(:repository_object, :closed, external_identifier: 'druid:vs491yc7072') + create(:repository_object, external_identifier: 'druid:bc778pm9866') end context 'when not limited' do @@ -41,19 +39,11 @@ end end - context 'when only published' do - let(:only_published) { true } + context 'when limited to publishable constituents' do + let(:publishable) { true } - it 'returns the druids of the constituent objects' do - expect(constituents).to eq(['druid:dj321gm8879', 'druid:vs491yc7072']) - end - end - - context 'when only closed' do - let(:exclude_opened) { true } - - it 'returns the druids of the constituent objects' do - expect(constituents).to eq(['druid:bc778pm9866', 'druid:dj321gm8879']) + it 'returns the druids of the constituent objects that are publishable' do + expect(constituents).to eq(['druid:dj321gm8879']) end end end