Merge pull request #379 from sul-dlss/batch_virtual_object_creation_a…

…rgo#1463

Allow virtual objects to be created in batches

app/controllers/objects_controller.rb

@@ -40,6 +40,7 @@ def create
     end
   end
 
+  # TODO: Remove this once Argo, in stage and prod, uses a version of dor-services-client that no longer hits this endpoint
   # Handles updates to the record.
   # Presently this only needs to handle the merge object use case.
   # Do this by providing: constituent_ids => ['druid:123', 'druid:345']

app/controllers/virtual_objects_controller.rb

@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+# Create virtual objects
+class VirtualObjectsController < ApplicationController
+  before_action :validate_params!, only: :create
+
+  # Create one or more virtual objects represented by JSON (see `#schema` below):
+  def create
+    errors = []
+
+    create_params[:virtual_objects].each do |virtual_object|
+      parent_id, child_ids = virtual_object.values_at(:parent_id, :child_ids)
+      # Update the constituent relationship between the parent and child druids
+      errors << ConstituentService.new(parent_druid: parent_id).add(child_druids: child_ids)
+    rescue ActiveFedora::ObjectNotFoundError, Rubydora::FedoraInvalidRequest => e
+      errors << { parent_id => [e.message] }
+    end
+
+    return render_error(errors: errors, status: :unprocessable_entity) if errors.any?
+
+    head :no_content
+  end
+
+  private
+
+  def create_params
+    params.to_unsafe_h
+  end
+
+  # Use dry-validation to validate params instead of Rails strong parameters.
+  # This gives us finer-grained validation.
+  def validate_params!
+    errors = schema.call(create_params).errors(full: true)
+    return render_error(errors: errors, status: :bad_request) if errors.any?
+  end
+
+  def render_error(errors:, status:)
+    render json: { errors: errors }, status: status
+  end
+
+  # {
+  #   virtual_objects: [
+  #     {
+  #       parent_id: 'a-non-empty-string',
+  #       child_ids: [
+  #         'another-non-empty-string',
+  #         ...
+  #       ]
+  #     },
+  #     ...
+  #   ]
+  # }
+  def schema
+    Dry::Schema.JSON do
+      required(:virtual_objects).value(:array, min_size?: 1).each do
+        hash do
+          required(:parent_id).filled(:string)
+          required(:child_ids).value(:array, min_size?: 1).each(:filled?)
+        end
+      end
+    end
+  end
+end

app/services/constituent_service.rb

@@ -18,16 +18,21 @@ def initialize(parent_druid:)
   # @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)
+    errors = ItemQueryService.validate_combinable_items(parent: parent_druid, children: child_druids)
 
     return errors if errors.any?
 
+    # Make sure the parent is open before making modifications
+    VersionService.open(parent) unless VersionService.open?(parent)
+
     ResetContentMetadataService.new(item: parent).reset
 
     child_druids.each do |child_druid|
       add_constituent(child_druid: child_druid)
     end
-    parent.save!
+
+    # NOTE: parent object is saved as part of closing the version
+    VersionService.close(parent)
 
     nil
   end
@@ -38,11 +43,16 @@ def add(child_druids:)
 
   def add_constituent(child_druid:)
     child = ItemQueryService.find_combinable_item(child_druid)
+    # Make sure the child is open before making modifications
+    VersionService.open(child) unless VersionService.open?(child)
+
     child.contentMetadata.ng_xml.search('//resource').each do |resource|
       parent.contentMetadata.add_virtual_resource(child.id, resource)
     end
     child.add_relationship :is_constituent_of, parent
-    child.save!
+
+    # NOTE: child object is saved as part of closing the version
+    VersionService.close(child)
   end
 
   def parent

app/services/item_query_service.rb

@@ -11,16 +11,16 @@ def initialize(id:, item_relation: default_item_relation)
     @item_relation = item_relation
   end
 
-  delegate :allows_modification?, to: :item
+  # @param [String] parent a parent druid
+  # @param [Array] children a list of child druids
+  # @return [Hash]
+  def self.validate_combinable_items(parent:, children:)
+    errors = Hash.new { |hash, key| hash[key] = [] }
 
-  # @param [Array] druids a list of druids
-  def self.validate_combinable_items(druids)
-    errors = {}
-
-    druids.each do |druid|
+    ([parent] + children).each do |druid|
       find_combinable_item(druid)
     rescue UncombinableItemError => e
-      errors[druid] = e.message
+      errors[parent] << e.message
     end
 
     errors
@@ -30,7 +30,7 @@ def self.validate_combinable_items(druids)
   def self.find_combinable_item(druid)
     query_service = ItemQueryService.new(id: druid)
     query_service.item do |item|
-      raise UncombinableItemError, "Item #{item.pid} is not open for modification" unless query_service.allows_modification?
+      raise UncombinableItemError, "Item #{item.pid} is not open or openable" unless VersionService.open?(item) || VersionService.can_open?(item)
       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

app/services/version_service.rb

@@ -10,6 +10,10 @@ def self.can_open?(work, opts = {})
     new(work).can_open?(opts)
   end
 
+  def self.open?(work)
+    new(work).open_for_versioning?
+  end
+
   def self.close(work, opts = {})
     new(work).close(opts)
   end

config/routes.rb

@@ -20,6 +20,9 @@
       get 'catkey', to: 'marcxml#catkey'
     end
 
+    resources :virtual_objects, only: [:create]
+
+    # TODO: Remove :update once Argo, in stage and prod, uses a version of dor-services-client that no longer hits this endpoint
     resources :objects, only: [:create, :update, :show] do
       member do
         post 'publish'

spec/requests/batch_create_virtual_objects_spec.rb

@@ -0,0 +1,152 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'Batch creation of virtual objects' do
+  let(:payload) { { sub: 'argo' } }
+  let(:jwt) { JWT.encode(payload, Settings.dor.hmac_secret, 'HS256') }
+  let(:parent_id) { 'druid:mk420bs7601' }
+  let(:child1_id) { 'druid:child1' }
+  let(:child2_id) { 'druid:child2' }
+
+  let(:object) { Dor::Item.new(pid: parent_id) }
+  let(:service) { instance_double(ConstituentService, add: nil) }
+
+  before do
+    allow(Dor).to receive(:find).and_return(object)
+    allow(ConstituentService).to receive(:new).with(parent_druid: parent_id).and_return(service)
+  end
+
+  context 'when virtual_objects param is provided' do
+    it 'creates a virtual object out of the parent object and all child objects' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: parent_id, child_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_successful
+    end
+  end
+
+  context 'when virtual_objects contain objects that are not combinable' do
+    before do
+      allow(service).to receive(:add).and_return(parent_id => ["Item #{child2_id} is not open for modification"])
+    end
+
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: parent_id, child_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
+      expect(response.body).to eq '{"errors":[{"druid:mk420bs7601":["Item druid:child2 is not open for modification"]}]}'
+    end
+  end
+
+  context 'when virtual_objects param is not provided' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { title: 'New name' },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'virtual_objects is missing'
+    end
+  end
+
+  context 'when virtual_objects is not an array' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: child1_id },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'virtual_objects must be an array'
+    end
+  end
+
+  context 'when virtual_objects is empty array' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'parent_id is missing'
+    end
+  end
+
+  context 'when virtual_objects array lacks hashes defining parent_id' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ child_ids: ['foo'] }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'parent_id is missing'
+    end
+  end
+
+  context 'when virtual_objects array has a hash w/ an empty parent_id' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: '' }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'parent_id must be filled'
+    end
+  end
+
+  context 'when virtual_objects array lacks hashes defining child_ids' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: 'foo' }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'child_ids is missing'
+    end
+  end
+
+  context 'when virtual_objects array has a hash w/ child_ids not an array' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: 'foo', child_ids: '' }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq 'child_ids must be an array'
+    end
+  end
+
+  context 'when virtual_objects array has a hash w/ child_ids empty' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: 'foo', child_ids: [] }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq '0 must be filled'
+    end
+  end
+
+  context 'when virtual_objects array has a hash w/ child_ids containing empties' do
+    it 'renders an error' do
+      post '/v1/virtual_objects',
+           params: { virtual_objects: [{ parent_id: 'foo', child_ids: ['foo', 'bar', ''] }] },
+           headers: { 'X-Auth' => "Bearer #{jwt}" }
+      expect(service).not_to have_received(:add)
+      expect(response).to be_bad_request
+      json = JSON.parse(response.body)
+      expect(json['errors'][0]['text']).to eq '2 must be filled'
+    end
+  end
+end

spec/services/constituent_service_spec.rb

@@ -72,6 +72,9 @@
 
     context 'when the parent is open for modification' do
       before do
+        allow(VersionService).to receive(:open?).and_return(true)
+        allow(VersionService).to receive(:open)
+        allow(VersionService).to receive(:close)
         add
       end
 
@@ -89,16 +92,19 @@
         XML
         expect(child1.object_relations[:is_constituent_of]).to eq [parent]
         expect(child2.object_relations[:is_constituent_of]).to eq [parent]
+        expect(VersionService).to have_received(:open?).exactly(3).times
+        expect(VersionService).not_to have_received(:open)
+        expect(VersionService).to have_received(:close).exactly(3).times
       end
     end
 
     context 'when the parent is not combinable' do
       before do
-        allow(ItemQueryService).to receive(:validate_combinable_items).with([parent.id, child1.id, child2.id]).and_return(parent.id => 'nope')
+        allow(ItemQueryService).to receive(:validate_combinable_items).with(parent: parent.id, children: [child1.id, child2.id]).and_return(parent.id => ['that is a nope for child2'])
       end
 
       it 'merges nothing' do
-        expect(add).to eq(parent.id => 'nope')
+        expect(add).to eq(parent.id => ['that is a nope for child2'])
         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
@@ -107,7 +113,7 @@
 
     context 'when a child is not combinable' do
       before do
-        allow(ItemQueryService).to receive(:validate_combinable_items).with([parent.id, child1.id, child2.id]).and_return(child1.id => 'not modifiable message')
+        allow(ItemQueryService).to receive(:validate_combinable_items).with(parent: parent.id, children: [child1.id, child2.id]).and_return(child1.id => 'not modifiable message')
       end
 
       it 'does not merge any children' do