Validate constituents before creating virtual object

Fixes #365

Includes:
* Add `ItemQueryService.validate_combinable_items`. This new class method takes an array of druids and makes sure each is combinable. If any are not, a hash of errors is created and returned.
* Validate all items to be combined before modifying any of them
* Return HTTP 422 when constituent service returns errors

app/controllers/objects_controller.rb

@@ -42,7 +42,10 @@ def update
     raise ActionController::ParameterMissing, 'constituent_ids must be an array' unless filtered_params[:constituent_ids]
 
     # Update the constituent relationship
-    ConstituentService.new(parent_druid: params[:id]).add(child_druids: filtered_params[:constituent_ids])
+    errors = ConstituentService.new(parent_druid: params[:id]).add(child_druids: filtered_params[:constituent_ids])
+
+    return render json: { errors: errors.to_json }, status: 422 if errors
+
     head :no_content
   end
 

app/services/constituent_service.rb

@@ -15,13 +15,21 @@ def initialize(parent_druid:)
   # Typically this is only called one time (with a list of all the pids) because
   # subsequent calls will erase the previous changes.
   # @param [Array<String>] child_druids the identifiers of the child objects
+  # @raises ActiveFedora::RecordInvalid if AF object validations fail on #save!
+  # @returns [NilClass, Hash] true if successful, hash of errors otherwise (if combinable validation fails)
   def add(child_druids:)
+    errors = ItemQueryService.validate_combinable_items([parent_druid] + child_druids)
+
+    return errors if errors.any?
+
     ResetContentMetadataService.new(item: parent).reset
 
     child_druids.each do |child_druid|
       add_constituent(child_druid: child_druid)
     end
     parent.save!
+
+    nil
   end
 
   private

app/services/item_query_service.rb

@@ -2,6 +2,8 @@
 
 # Responsible for retrieving information based on the given Dor::Item.
 class ItemQueryService
+  class UncombinableItemError < RuntimeError; end
+
   # @param [String] id - The id of the item
   # @param [#exists?, #find] item_relation - How we will query some of the related information
   def initialize(id:, item_relation: default_item_relation)
@@ -11,13 +13,26 @@ def initialize(id:, item_relation: default_item_relation)
 
   delegate :allows_modification?, to: :item
 
-  # @raises [RuntimeError] if the item is dark, citation_only, or not modifiable
+  # @param [Array] druids a list of druids
+  def self.validate_combinable_items(druids)
+    errors = {}
+
+    druids.each do |druid|
+      find_combinable_item(druid)
+    rescue UncombinableItemError => e
+      errors[druid] = e.message
+    end
+
+    errors
+  end
+
+  # @raises [UncombinableItemError] if the item is dark, citation_only, or not modifiable
   def self.find_combinable_item(druid)
     query_service = ItemQueryService.new(id: druid)
     query_service.item do |item|
-      raise "Item #{item.pid} is not open for modification" unless query_service.allows_modification?
-      raise "Item #{item.pid} is dark" if item.rightsMetadata.dra_object.dark?
-      raise "Item #{item.pid} is citation_only" if item.rightsMetadata.dra_object.citation_only?
+      raise UncombinableItemError, "Item #{item.pid} is not open for modification" unless query_service.allows_modification?
+      raise UncombinableItemError, "Item #{item.pid} is dark" if item.rightsMetadata.dra_object.dark?
+      raise UncombinableItemError, "Item #{item.pid} is citation_only" if item.rightsMetadata.dra_object.citation_only?
     end
   end
 

spec/requests/virtual_merge_spec.rb

@@ -10,7 +10,7 @@
   let(:child2_id) { 'druid:child2' }
 
   let(:object) { Dor::Item.new(pid: parent_id) }
-  let(:service) { instance_double(ConstituentService, add: true) }
+  let(:service) { instance_double(ConstituentService, add: nil) }
 
   before do
     allow(Dor).to receive(:find).and_return(object)
@@ -50,4 +50,20 @@
       expect(json['errors'][0]['detail']).to eq 'param is missing or the value is empty: constituent_ids must be an array'
     end
   end
+
+  context 'when constituent_ids contain objects that are not combinable' do
+    before do
+      allow(service).to receive(:add).and_return(child2_id => "Item #{child2_id} is not open for modification")
+    end
+
+    it 'renders an error' do
+      put "/v1/objects/#{parent_id}",
+          params: { constituent_ids: [child1_id, child2_id] },
+          headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).to have_received(:add).with(child_druids: [child1_id, child2_id])
+      expect(response).to be_unprocessable
+      json = JSON.parse(response.body)
+      expect(json['errors']).to eq '{"druid:child2":"Item druid:child2 is not open for modification"}'
+    end
+  end
 end

spec/services/constituent_service_spec.rb

@@ -3,15 +3,19 @@
 require 'rails_helper'
 
 RSpec.describe ConstituentService do
+  let(:parent_content) do
+    <<~XML
+      <contentMetadata objectId="druid:parent1" type="image">
+        <resource sequence="1" id="wrongthing_1" type="image">
+          <relationship type="alsoAvailableAs" objectId="druid:wrongthing"/>
+        </resource>
+      </contentMetadata>
+    XML
+  end
+
   let(:parent) do
     Dor::Item.new.tap do |item|
-      item.contentMetadata.content = <<~XML
-        <contentMetadata objectId="druid:parent1" type="image">
-          <resource sequence="1" id="wrongthing_1" type="image">
-            <relationship type="alsoAvailableAs" objectId="druid:wrongthing"/>
-          </resource>
-        </contentMetadata>
-      XML
+      item.contentMetadata.content = parent_content
     end
   end
 
@@ -90,31 +94,26 @@
 
     context 'when the parent is not combinable' do
       before do
-        allow(ItemQueryService).to receive(:find_combinable_item).with(parent.id).and_raise('nope')
+        allow(ItemQueryService).to receive(:validate_combinable_items).with([parent.id, child1.id, child2.id]).and_return(parent.id => 'nope')
       end
 
       it 'merges nothing' do
-        expect { add }.to raise_error(RuntimeError).and(not_change { parent.contentMetadata.content })
-
+        expect(add).to eq(parent.id => 'nope')
+        expect(parent.contentMetadata.content).to be_equivalent_to(parent_content)
         expect(child1.object_relations[:is_constituent_of]).to be_empty
+        expect(child2.object_relations[:is_constituent_of]).to be_empty
       end
     end
 
     context 'when a child is not combinable' do
       before do
-        allow(ItemQueryService).to receive(:find_combinable_item).with(child2.id).and_raise('not modifiable')
+        allow(ItemQueryService).to receive(:validate_combinable_items).with([parent.id, child1.id, child2.id]).and_return(child1.id => 'not modifiable message')
       end
 
-      it 'merges all the chidren before an error is encountered' do
-        expect { add }.to raise_error RuntimeError
-        expect(parent.contentMetadata.content).to be_equivalent_to <<~XML
-          <contentMetadata objectId="druid:parent1" type="image">
-            <resource sequence="1" id="parent1_1" type="image">
-              <relationship type="alsoAvailableAs" objectId="druid:child1"/>
-            </resource>
-          </contentMetadata>
-        XML
-        expect(child1.object_relations[:is_constituent_of]).to eq [parent]
+      it 'does not merge any children' do
+        expect(add).to eq(child1.id => 'not modifiable message')
+        expect(parent.contentMetadata.content).to be_equivalent_to(parent_content)
+        expect(child1.object_relations[:is_constituent_of]).to be_empty
         expect(child2.object_relations[:is_constituent_of]).to be_empty
       end
     end

spec/services/item_query_service_spec.rb

@@ -15,22 +15,25 @@
   describe '.find_combinable_item' do
     it 'raises error if object does not allow modification' do
       allow(item).to receive(:allows_modification?).and_return(false)
-      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(RuntimeError, 'Item druid:ab123cd4567 is not open for modification')
+      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(described_class::UncombinableItemError, 'Item druid:ab123cd4567 is not open for modification')
     end
+
     it 'raises error if object is dark' do
       dra = instance_double(Dor::RightsAuth, dark?: true, citation_only?: false)
       rights_ds = instance_double(Dor::RightsMetadataDS, dra_object: dra)
       allow(item).to receive(:rightsMetadata).and_return(rights_ds)
       allow(item).to receive(:allows_modification?).and_return(true)
-      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(RuntimeError, 'Item druid:ab123cd4567 is dark')
+      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(described_class::UncombinableItemError, 'Item druid:ab123cd4567 is dark')
     end
+
     it 'raises error if object is citation_only' do
       dra = instance_double(Dor::RightsAuth, dark?: false, citation_only?: true)
       rights_ds = instance_double(Dor::RightsMetadataDS, dra_object: dra)
       allow(item).to receive(:rightsMetadata).and_return(rights_ds)
       allow(item).to receive(:allows_modification?).and_return(true)
-      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(RuntimeError, 'Item druid:ab123cd4567 is citation_only')
+      expect { service.find_combinable_item('ab123cd4567') }.to raise_error(described_class::UncombinableItemError, 'Item druid:ab123cd4567 is citation_only')
     end
+
     it 'returns item otherwise' do
       dra = instance_double(Dor::RightsAuth, dark?: false, citation_only?: false)
       rights_ds = instance_double(Dor::RightsMetadataDS, dra_object: dra)
@@ -39,4 +42,79 @@
       service.find_combinable_item('ab123cd4567')
     end
   end
+
+  describe '.validate_combinable_items' do
+    let(:item2) { instantiate_fixture('druid:xh235dd9059', Dor::Item) }
+    let(:item3) { instantiate_fixture('druid:hj097bm8879', Dor::Item) }
+    let(:dark_rights) { instance_double(Dor::RightsAuth, dark?: true, citation_only?: false) }
+    let(:citation_only_rights) { instance_double(Dor::RightsAuth, dark?: false, citation_only?: true) }
+    let(:permissive_rights) { instance_double(Dor::RightsAuth, dark?: false, citation_only?: false) }
+    let(:dark_rights_ds) { instance_double(Dor::RightsMetadataDS, dra_object: dark_rights) }
+    let(:citation_only_rights_ds) { instance_double(Dor::RightsMetadataDS, dra_object: citation_only_rights) }
+    let(:permissive_rights_ds) { instance_double(Dor::RightsMetadataDS, dra_object: permissive_rights) }
+
+    # Set defaults on all fixture objects and avoid making HTTP calls. Set defaults to the non-error cases.
+    before do
+      allow(Dor::Item).to receive(:find).with('druid:xh235dd9059').and_return(item2)
+      allow(Dor::Item).to receive(:find).with('druid:hj097bm8879').and_return(item3)
+      [item, item2, item3].each do |i|
+        allow(i).to receive(:allows_modification?).and_return(true)
+        allow(i).to receive(:rightsMetadata).and_return(permissive_rights_ds)
+      end
+    end
+
+    context 'when any objects do not allow modification' do
+      before do
+        allow(item).to receive(:allows_modification?).and_return(false)
+      end
+
+      it 'returns a single error if one object does not allow modification' do
+        expect(service.validate_combinable_items(['druid:ab123cd4567', 'druid:xh235dd9059', 'druid:hj097bm8879'])).to eq(
+          'druid:ab123cd4567' => 'Item druid:ab123cd4567 is not open for modification'
+        )
+      end
+    end
+
+    context 'when any objects are dark' do
+      before do
+        [item2, item3].each do |i|
+          allow(i).to receive(:rightsMetadata).and_return(dark_rights_ds)
+        end
+      end
+
+      it 'returns errors if any objects are dark' do
+        expect(service.validate_combinable_items(['druid:ab123cd4567', 'druid:xh235dd9059', 'druid:hj097bm8879'])).to eq(
+          'druid:hj097bm8879' => 'Item druid:hj097bm8879 is dark',
+          'druid:xh235dd9059' => 'Item druid:xh235dd9059 is dark'
+        )
+      end
+    end
+
+    context 'when any objects are citation-only' do
+      before do
+        [item, item3].each do |i|
+          allow(i).to receive(:rightsMetadata).and_return(citation_only_rights_ds)
+        end
+      end
+
+      it 'raises error if any objects are citation_only' do
+        expect(service.validate_combinable_items(['druid:ab123cd4567', 'druid:xh235dd9059', 'druid:hj097bm8879'])).to eq(
+          'druid:ab123cd4567' => 'Item druid:ab123cd4567 is citation_only',
+          'druid:hj097bm8879' => 'Item druid:hj097bm8879 is citation_only'
+        )
+      end
+    end
+
+    context 'when none of the error conditions exist' do
+      before do
+        [item, item2, item3].each do |i|
+          allow(i).to receive(:rightsMetadata).and_return(permissive_rights_ds)
+        end
+      end
+
+      it 'returns an empty hash otherwise' do
+        expect(service.validate_combinable_items(['druid:ab123cd4567', 'druid:xh235dd9059', 'druid:hj097bm8879'])).to eq({})
+      end
+    end
+  end
 end