Merge pull request #4880 from sul-dlss/fix_virtual_objects

app/services/constituent_service.rb

@@ -28,7 +28,7 @@ def add(constituent_druids:)
     return errors if errors.any?
 
     # Make sure the virtual object is open before making modifications
-    updated_virtual_object = if WorkflowStateService.active_version_wf?(druid: virtual_object.externalIdentifier, version: virtual_object.version)
+    updated_virtual_object = if WorkflowStateService.open?(druid: virtual_object.externalIdentifier, version: virtual_object.version)
                                virtual_object
                              else
                                VersionService.open(cocina_object: virtual_object,

app/services/item_query_service.rb

@@ -4,8 +4,6 @@
 class ItemQueryService
   class UncombinableItemError < RuntimeError; end
 
-  ALLOWED_WORKFLOW_STATES = %w[Accessioned Opened].freeze
-
   # @param [String] virtual_object a virtual_object druid
   # @param [Array] constituents a list of constituent druids
   # @return [Hash]
@@ -17,13 +15,15 @@ def self.validate_combinable_items(virtual_object:, constituents:)
     # check virtual_object for combinability
     begin
       find_combinable_item(virtual_object)
+      check_open(virtual_object)
     rescue UncombinableItemError => e
       errors[virtual_object] << e.message
     end
 
     # check constituents for combinability and whether they are already virtual objects
     constituents.each do |druid|
       check_virtual(druid)
+      check_accessioned(druid)
       find_combinable_item(druid)
     rescue UncombinableItemError => e
       errors[virtual_object] << e.message
@@ -32,13 +32,12 @@ def self.validate_combinable_items(virtual_object:, constituents:)
     errors
   end
 
-  # @raise [UncombinableItemError] if the item is dark, citation_only, or not modifiable
+  # @raise [UncombinableItemError] if the item is not an item, dark, or citation_only
   def self.find_combinable_item(druid)
     new(id: druid).item do |item|
       raise UncombinableItemError, "Item #{item.externalIdentifier} is not an item" unless item.dro?
       raise UncombinableItemError, "Item #{item.externalIdentifier} is dark" if item.access.view == 'dark'
       raise UncombinableItemError, "Item #{item.externalIdentifier} is citation-only" if item.access.view == 'citation-only'
-      raise UncombinableItemError, "Item #{item.externalIdentifier} is not in the accessioned or opened workflow state" unless current_workflow_state(item).in?(ALLOWED_WORKFLOW_STATES)
     end
   end
 
@@ -50,13 +49,19 @@ def self.check_virtual(druid)
   end
   private_class_method :check_virtual
 
-  def self.current_workflow_state(item)
-    WorkflowClientFactory
-      .build
-      .status(druid: item.externalIdentifier, version: item.version)
-      .display_simplified
+  # @raise [UncombinableItemError] if the items is not open or openable
+  def self.check_open(druid)
+    new(id: druid).item do |item|
+      raise UncombinableItemError, "Item #{item.externalIdentifier} is not open or openable" unless VersionService.open?(druid: item.externalIdentifier, version: item.version) || VersionService.can_open?(druid: item.externalIdentifier, version: item.version)
+    end
+  end
+
+  # @raise [UncombinableItemError] if the item is dark, citation_only, or not modifiable
+  def self.check_accessioned(druid)
+    new(id: druid).item do |item|
+      raise UncombinableItemError, "Item #{item.externalIdentifier} has not been accessioned" unless WorkflowStateService.accessioned?(druid: item.externalIdentifier, version: item.version)
+    end
   end
-  private_class_method :current_workflow_state
 
   # @param [String] id - The id of the item
   def initialize(id:)

app/services/workflow_state_service.rb

@@ -7,8 +7,12 @@ def self.accessioning?(...)
     new(...).accessioning?
   end
 
-  def self.active_version_wf?(...)
-    new(...).active_version_wf?
+  def self.open?(...)
+    new(...).open?
+  end
+
+  def self.accessioned?(...)
+    new(...).accessioned?
   end
 
   def initialize(druid:, version:)
@@ -53,13 +57,6 @@ def accessioning?
     false
   end
 
-  def active_version_wf?
-    return true if workflow_client.active_lifecycle(druid:, milestone_name: 'opened', version: version.to_s)
-
-    # Note that this will return false for version 1, since there is no versionWF.
-    false
-  end
-
   # @return [Boolean] true if the object has previously been accessioned.
   def accessioned?
     return true if workflow_client.lifecycle(druid:, milestone_name: 'accessioned')

spec/services/constituent_service_spec.rb

@@ -17,7 +17,7 @@
       allow(ItemQueryService).to receive_messages(find_combinable_item: virtual_object, validate_combinable_items: item_errors)
       allow(VersionService).to receive_messages(open: virtual_object)
       allow(VersionService).to receive(:close)
-      allow(WorkflowStateService).to receive(:active_version_wf?).and_return(open_for_versioning)
+      allow(WorkflowStateService).to receive(:open?).and_return(open_for_versioning)
       allow(ResetContentMetadataService).to receive(:reset).and_return(virtual_object)
       allow(UpdateObjectService).to receive(:update)
       allow(CocinaObjectStore).to receive(:find).and_return(mock_item)
@@ -30,7 +30,7 @@
 
       it 'returns hash with errors' do
         expect(service.add(constituent_druids:)).to eq(item_errors)
-        expect(WorkflowStateService).not_to have_received(:active_version_wf?)
+        expect(WorkflowStateService).not_to have_received(:open?)
         expect(VersionService).not_to have_received(:open)
         expect(VersionService).not_to have_received(:close)
         expect(ResetContentMetadataService).not_to have_received(:reset)

spec/services/item_query_service_spec.rb

@@ -10,68 +10,17 @@
   let(:cocina_object) do
     build(:dro, id: druid).new(access: { download: 'none' }.merge(access))
   end
-  let(:workflow_state) { 'Accessioned' }
-  let(:workflow_client) { instance_double(Dor::Workflow::Client, status: status_client) }
-  let(:status_client) { instance_double(Dor::Workflow::Client::Status, display_simplified: workflow_state) }
   let(:created_at) { Time.zone.now }
   let(:updated_at) { Time.zone.now }
   let(:cocina_object_with_metadata) { Cocina::Models.with_metadata(cocina_object, '1', created: created_at, modified: updated_at) }
 
   before do
     allow(CocinaObjectStore).to receive(:find).with(druid).and_return(cocina_object_with_metadata)
-    allow(WorkflowClientFactory).to receive(:build).and_return(workflow_client)
+    allow(VersionService).to receive(:open?).and_return(true)
+    allow(WorkflowStateService).to receive(:accessioned?).and_return(true)
   end
 
   describe '.find_combinable_item' do
-    context 'with item in accessioned state' do
-      let(:workflow_state) { 'Accessioned' }
-
-      it 'returns the item' do
-        expect(service.find_combinable_item(druid)).to match_cocina_object_with(cocina_object.to_h)
-      end
-    end
-
-    context 'with item in opened state' do
-      let(:workflow_state) { 'Opened' }
-
-      it 'returns the item' do
-        expect(service.find_combinable_item(druid)).to match_cocina_object_with(cocina_object.to_h)
-      end
-    end
-
-    context 'with item in registered state' do
-      let(:workflow_state) { 'Registered' }
-
-      it 'raises an error' do
-        expect { service.find_combinable_item(druid) }.to raise_error(
-          described_class::UncombinableItemError,
-          "Item #{druid} is not in the accessioned or opened workflow state"
-        )
-      end
-    end
-
-    context 'with item in accessioning state' do
-      let(:workflow_state) { 'In Accessioning' }
-
-      it 'raises an error' do
-        expect { service.find_combinable_item(druid) }.to raise_error(
-          described_class::UncombinableItemError,
-          "Item #{druid} is not in the accessioned or opened workflow state"
-        )
-      end
-    end
-
-    context 'with item in unknown state' do
-      let(:workflow_state) { 'Unknown Status' }
-
-      it 'raises an error' do
-        expect { service.find_combinable_item(druid) }.to raise_error(
-          described_class::UncombinableItemError,
-          "Item #{druid} is not in the accessioned or opened workflow state"
-        )
-      end
-    end
-
     context 'with dark item' do
       let(:access) { { view: 'dark' } }
 
@@ -178,19 +127,22 @@
         allow(described_class).to receive(:find_combinable_item)
         allow(described_class).to receive(:find_combinable_item).with(druid).and_raise(described_class::UncombinableItemError, "Item #{druid} is dark")
         allow(described_class).to receive(:check_virtual)
+        allow(described_class).to receive(:check_accessioned)
       end
 
       it 'returns an error' do
         expect(service.validate_combinable_items(virtual_object: druid, constituents: constituent_druids)).to eq(
           'druid:bc123df4567' => ['Item druid:bc123df4567 is dark']
         )
+        expect(described_class).to have_received(:check_virtual).with('druid:xh235dd9059')
       end
     end
 
     context 'when all items are combinable' do
       before do
         allow(described_class).to receive(:find_combinable_item)
         allow(described_class).to receive(:check_virtual)
+        allow(described_class).to receive(:check_accessioned)
       end
 
       it 'returns an empty hash otherwise' do
@@ -204,6 +156,7 @@
       before do
         allow(described_class).to receive(:find_combinable_item)
         allow(described_class).to receive(:check_virtual)
+        allow(described_class).to receive(:check_accessioned)
       end
 
       it 'returns an error' do
@@ -275,4 +228,64 @@
       end
     end
   end
+
+  describe '.check_open' do
+    context 'with item in opened state' do
+      before do
+        allow(VersionService).to receive_messages(open?: true, can_open?: false)
+      end
+
+      it 'returns the item' do
+        expect(service.check_open(druid)).to match_cocina_object_with(cocina_object.to_h)
+      end
+    end
+
+    context 'with item in openable state' do
+      before do
+        allow(VersionService).to receive_messages(open?: false, can_open?: true)
+      end
+
+      it 'returns the item' do
+        expect(service.check_open(druid)).to match_cocina_object_with(cocina_object.to_h)
+      end
+    end
+
+    context 'with item not in open state or openable' do
+      before do
+        allow(VersionService).to receive_messages(open?: false, can_open?: false)
+      end
+
+      it 'raises an error' do
+        expect { service.check_open(druid) }.to raise_error(
+          described_class::UncombinableItemError,
+          "Item #{druid} is not open or openable"
+        )
+      end
+    end
+  end
+
+  describe '.check_accessioned' do
+    context 'when item has been accessioned' do
+      before do
+        allow(WorkflowStateService).to receive(:accessioned?).and_return(true)
+      end
+
+      it 'raises an error' do
+        expect { service.check_accessioned(druid) }.not_to raise_error
+      end
+    end
+
+    context 'when item has not been accessioned' do
+      before do
+        allow(WorkflowStateService).to receive(:accessioned?).and_return(false)
+      end
+
+      it 'raises an error' do
+        expect { service.check_accessioned(druid) }.to raise_error(
+          described_class::UncombinableItemError,
+          "Item #{druid} has not been accessioned"
+        )
+      end
+    end
+  end
 end

spec/services/workflow_state_service_spec.rb

@@ -97,29 +97,6 @@
     end
   end
 
-  describe '.active_version_wf?' do
-    context 'when there is an active versionWF' do
-      before do
-        allow(workflow_client).to receive(:active_lifecycle).and_return(Time.current)
-      end
-
-      it 'returns true' do
-        expect(workflow_state).to be_active_version_wf
-        expect(workflow_client).to have_received(:active_lifecycle).with(druid:, milestone_name: 'opened', version: '1')
-      end
-    end
-
-    context 'when there is not an active versionWF' do
-      before do
-        allow(workflow_client).to receive(:active_lifecycle).and_return(nil)
-      end
-
-      it 'returns false' do
-        expect(workflow_state).not_to be_active_version_wf
-      end
-    end
-  end
-
   describe '.accessioning?' do
     context 'when there is an active accessioningWF' do
       before do