Change logic determining whether item is publishable

Fixes #5715

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|

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

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

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

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

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

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)

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