From 3e9b48ecf209eacaa556398fca7bddecd9852743 Mon Sep 17 00:00:00 2001 From: Justin Coyne Date: Wed, 1 May 2019 09:56:49 -0500 Subject: [PATCH] Extract the registration request from the registration service --- .rubocop_todo.yml | 39 ++--- app/models/registration_request.rb | 103 ++++++++++++ app/services/registration_service.rb | 175 ++++++++------------- spec/services/registration_service_spec.rb | 38 +++-- 4 files changed, 204 insertions(+), 151 deletions(-) create mode 100644 app/models/registration_request.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index ad17da3238..738e9dd10c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2019-05-02 14:26:35 -0500 using RuboCop version 0.65.0. +# on 2019-05-07 10:46:45 -0700 using RuboCop version 0.65.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -29,27 +29,27 @@ Lint/UriEscapeUnescape: - 'app/controllers/sdr_controller.rb' - 'spec/controllers/sdr_controller_spec.rb' -# Offense count: 22 +# Offense count: 23 Metrics/AbcSize: - Max: 122 + Max: 94 # Offense count: 3 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 139 + Max: 124 -# Offense count: 4 +# Offense count: 5 Metrics/CyclomaticComplexity: - Max: 27 + Max: 11 # Offense count: 27 # Configuration parameters: CountComments, ExcludedMethods. Metrics/MethodLength: - Max: 63 + Max: 43 -# Offense count: 3 +# Offense count: 4 Metrics/PerceivedComplexity: - Max: 27 + Max: 11 # Offense count: 9 Naming/AccessorMethodName: @@ -155,7 +155,7 @@ RSpec/ExpectInHook: - 'spec/services/publish_metadata_service_spec.rb' - 'spec/services/registration_service_spec.rb' -# Offense count: 127 +# Offense count: 111 # Configuration parameters: AssignmentOnly. RSpec/InstanceVariable: Exclude: @@ -178,11 +178,6 @@ RSpec/MessageSpies: - 'spec/services/registration_service_spec.rb' - 'spec/services/version_service_spec.rb' -# Offense count: 2 -# Configuration parameters: AggregateFailuresByDefault. -RSpec/MultipleExpectations: - Max: 11 - # Offense count: 22 RSpec/NestedGroups: Max: 5 @@ -261,7 +256,7 @@ Style/ConditionalAssignment: Exclude: - 'spec/support/foxml_helper.rb' -# Offense count: 13 +# Offense count: 12 Style/Documentation: Exclude: - 'spec/**/*' @@ -275,7 +270,6 @@ Style/Documentation: - 'app/models/dor/service_item.rb' - 'app/models/dor/update_marc_record_service.rb' - 'app/services/dublin_core_service.rb' - - 'app/services/registration_service.rb' - 'config/application.rb' - 'config/initializers/okcomputer.rb' @@ -363,17 +357,14 @@ Style/StringLiterals: # Offense count: 3 # Cop supports --auto-correct. -# Configuration parameters: EnforcedStyle, MinSize. +# Configuration parameters: . # SupportedStyles: percent, brackets Style/SymbolArray: - Exclude: - - 'app/controllers/versions_controller.rb' - - 'app/services/release_tags.rb' - - 'lib/tasks/rspec.rake' + EnforcedStyle: percent + MinSize: 4 -# Offense count: 3 +# Offense count: 2 # Cop supports --auto-correct. Style/ZeroLengthPredicate: Exclude: - 'app/models/dor/update_marc_record_service.rb' - - 'app/services/registration_service.rb' diff --git a/app/models/registration_request.rb b/app/models/registration_request.rb new file mode 100644 index 0000000000..6acedad4b9 --- /dev/null +++ b/app/models/registration_request.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'uuidtools' + +# This represents the request a user could make to register an object +class RegistrationRequest + # @param [Hash{Symbol => various}] params + # @option params [String] :object_type required + # @option params [String] :label required + # @option params [String] :admin_policy required + # @option params [String] :metadata_source + # @option params [String] :rights + # @option params [String] :collection + # @option params [Hash{String => String}] :source_id Primary ID from another system, max one key/value pair! + # @option params [Hash] :other_ids including :uuid if known + # @option params [String] :pid Fully qualified PID if you don't want one generated for you + # @option params [Integer] :workflow_priority] + # @option params [Array] :seed_datastream datastream_names (only 'descMetadata' is a permitted value) + # @option params [Array] :initiate_workflow workflow_ids + # @option params [Array] :tags + def initialize(params) + @params = params + end + + # Raises Dor::ParameterError if the parameters aren't valid + def validate! + %i[object_type label admin_policy].each do |required_param| + raise Dor::ParameterError, "#{required_param.inspect} must be specified in call to register_object" unless params[required_param] + end + raise Dor::ParameterError, 'label cannot be empty to call register_object' if params[:label].empty? && %w[label none].include?(metadata_source) + + raise Dor::ParameterError, "Unknown item type: '#{object_type}'" if item_class.nil? + + raise Dor::ParameterError, "Unknown value for seed_datastream: '#{params[:seed_datastream]}'" if params[:seed_datastream] && params[:seed_datastream] != ['descMetadata'] + + raise ArgumentError, ":source_id Hash can contain at most 1 pair: recieved #{source_id.size}" if source_id.size > 1 + + raise Dor::ParameterError, "Unknown rights setting '#{rights}' when calling #{name}.register_object" if rights && rights != 'default' && !Dor::RightsMetadataDS.valid_rights_type?(rights) + end + + def pid + params[:pid] + end + + # TODO: Move to validate + def rights + params[:rights] + end + + def initiate_workflow + Array(params[:initiate_workflow]) + end + + def workflow_priority + params[:workflow_priority] ? params[:workflow_priority].to_i : 0 + end + + def object_type + params[:object_type] + end + + def admin_policy + params[:admin_policy] + end + + def item_class + Dor.registered_classes[object_type] + end + + def metadata_source + params[:metadata_source] + end + + def seed_desc_metadata + params[:seed_datastream] && params[:seed_datastream] == ['descMetadata'] + end + + def label + params[:label].length > 254 ? params[:label][0, 254] : params[:label] + end + + def source_id + params[:source_id] || {} + end + + def other_ids + hash = params[:other_ids] || {} + hash[:uuid] ||= UUIDTools::UUID.timestamp_create.to_s + hash + end + + def tags + params[:tags] || [] + end + + def collection + params[:collection] + end + + private + + attr_reader :params +end diff --git a/app/services/registration_service.rb b/app/services/registration_service.rb index c2fa0e71ff..8ea556c13c 100644 --- a/app/services/registration_service.rb +++ b/app/services/registration_service.rb @@ -1,10 +1,54 @@ # frozen_string_literal: true -require 'uuidtools' - +# Registers an object. Creates a skeleton object in Fedora with a Druid. class RegistrationService class << self - # @TODO: Why isn't all this logic in, for example, Dor::Item.create? or Dor::Base.create? or Dor::Creatable.create? + # @param [Hash] params + # @see register_object similar but different + def create_from_request(params) + other_ids = Array(params[:other_id]).map do |id| + if id =~ /^symphony:(.+)$/ + "#{$1.length < 14 ? 'catkey' : 'barcode'}:#{$1}" + else + id + end + end + + if params[:label] == ':auto' + params.delete(:label) + params.delete('label') + metadata_id = Dor::MetadataService.resolvable(other_ids).first + params[:label] = Dor::MetadataService.label_for(metadata_id) + end + + dor_params = { + pid: params[:pid], + admin_policy: params[:admin_policy], + content_model: params[:model], + label: params[:label], + object_type: params[:object_type], + other_ids: ids_to_hash(other_ids), + parent: params[:parent], + source_id: ids_to_hash(params[:source_id]), + tags: params[:tag] || [], + seed_datastream: params[:seed_datastream], + initiate_workflow: Array(params[:initiate_workflow]) + Array(params[:workflow_id]), + rights: params[:rights], + metadata_source: params[:metadata_source], + collection: params[:collection], + workflow_priority: params[:workflow_priority] + } + dor_params.delete_if { |_k, v| v.nil? } + + request = RegistrationRequest.new(dor_params) + dor_obj = register_object(request) + pid = dor_obj.pid + location = URI.parse(Dor::Config.fedora.safeurl.sub(%r{/*$}, '/')).merge("objects/#{pid}").to_s + dor_params.dup.merge(location: location, pid: pid) + end + + private + # @TODO: these duplicate checks could be combined into 1 query # @param [String] pid an ID to check, if desired. If not passed (or nil), a new ID is minted @@ -31,68 +75,25 @@ def check_source_id(source_id_string) source_id_string end - # @param [Hash{Symbol => various}] params - # @option params [String] :object_type required - # @option params [String] :label required - # @option params [String] :admin_policy required - # @option params [String] :metadata_source - # @option params [String] :rights - # @option params [String] :collection - # @option params [Hash{String => String}] :source_id Primary ID from another system, max one key/value pair! - # @option params [Hash] :other_ids including :uuid if known - # @option params [String] :pid Fully qualified PID if you don't want one generated for you - # @option params [Integer] :workflow_priority] - # @option params [Array] :seed_datastream datastream_names (only 'descMetadata' is a permitted value) - # @option params [Array] :initiate_workflow workflow_ids - # @option params [Array] :tags - def register_object(params = {}) - %i[object_type label].each do |required_param| - raise Dor::ParameterError, "#{required_param.inspect} must be specified in call to #{name}.register_object" unless params[required_param] - end - metadata_source = params[:metadata_source] - raise Dor::ParameterError, "label cannot be empty to call #{name}.register_object" if params[:label].length < 1 && %w[label none].include?(metadata_source) - - object_type = params[:object_type] - item_class = Dor.registered_classes[object_type] - raise Dor::ParameterError, "Unknown item type: '#{object_type}'" if item_class.nil? - - seed_desc_metadata = false - if params[:seed_datastream] - raise Dor::ParameterError, "Unknown value for seed_datastream: '#{params[:seed_datastream]}'" if params[:seed_datastream] != ['descMetadata'] - - seed_desc_metadata = true - end - - label = params[:label] - source_id = params[:source_id] || {} - other_ids = params[:other_ids] || {} - tags = params[:tags] || [] - collection = params[:collection] + # @param [RegistrationRequest] RegistrationRequest + def register_object(request) + request.validate! # Check for sourceId conflict *before* potentially minting PID - source_id_string = check_source_id [source_id.keys.first, source_id[source_id.keys.first]].compact.join(':') - pid = unduplicated_pid(params[:pid]) + source_id_string = check_source_id [request.source_id.keys.first, request.source_id[request.source_id.keys.first]].compact.join(':') + pid = unduplicated_pid(request.pid) - raise ArgumentError, ":source_id Hash can contain at most 1 pair: recieved #{source_id.size}" if source_id.size > 1 - - rights = nil - if params[:rights] - rights = params[:rights] - raise Dor::ParameterError, "Unknown rights setting '#{rights}' when calling #{name}.register_object" unless rights == 'default' || Dor::RightsMetadataDS.valid_rights_type?(rights) - end - - other_ids[:uuid] = UUIDTools::UUID.timestamp_create.to_s if (other_ids.key?(:uuid) || other_ids.key?('uuid')) == false - apo_object = Dor.find(params[:admin_policy]) - new_item = item_class.new(pid: pid) - new_item.label = label.length > 254 ? label[0, 254] : label + apo_object = Dor.find(request.admin_policy) + new_item = request.item_class.new(pid: pid) + new_item.label = request.label.length > 254 ? request.label[0, 254] : request.label idmd = new_item.identityMetadata idmd.sourceId = source_id_string idmd.add_value(:objectId, pid) idmd.add_value(:objectCreator, 'DOR') - idmd.add_value(:objectLabel, label) - idmd.add_value(:objectType, object_type) - other_ids.each_pair { |name, value| idmd.add_otherId("#{name}:#{value}") } - tags.each { |tag| idmd.add_value(:tag, tag) } + idmd.add_value(:objectLabel, request.label) + idmd.add_value(:objectType, request.object_type) + request.other_ids.each_pair { |name, value| idmd.add_otherId("#{name}:#{value}") } + request.tags.each { |tag| idmd.add_value(:tag, tag) } new_item.admin_policy_object = apo_object apo_object.administrativeMetadata.ng_xml.xpath('/administrativeMetadata/relationships/*').each do |rel| @@ -104,18 +105,17 @@ def register_object(params = {}) end new_item.add_relationship short_predicate, rel['rdf:resource'] end - new_item.add_collection(collection) if collection - if rights && %w(item collection).include?(object_type) + new_item.add_collection(request.collection) if request.collection + if request.rights && %w(item collection).include?(request.object_type) rights_xml = apo_object.defaultObjectRights.ng_xml new_item.datastreams['rightsMetadata'].content = rights_xml.to_s - new_item.read_rights = rights unless rights == 'default' # already defaulted to default! + new_item.read_rights = request.rights unless request.rights == 'default' # already defaulted to default! end # create basic mods from the label - build_desc_metadata_from_label(new_item, label) if metadata_source == 'label' - RefreshMetadataAction.run(new_item) if seed_desc_metadata + build_desc_metadata_from_label(new_item, request.label) if request.metadata_source == 'label' + RefreshMetadataAction.run(new_item) if request.seed_desc_metadata - workflow_priority = params[:workflow_priority] ? params[:workflow_priority].to_i : 0 - initiate_workflow(workflows: Array(params[:initiate_workflow]), item: new_item, priority: workflow_priority) + initiate_workflow(workflows: request.initiate_workflow, item: new_item, priority: request.workflow_priority) new_item.class.ancestors.select { |x| x.respond_to?(:to_class_uri) && x != ActiveFedora::Base }.each do |parent_class| new_item.add_relationship(:has_model, parent_class.to_class_uri) @@ -125,51 +125,6 @@ def register_object(params = {}) new_item end - # @param [Hash] params - # @see register_object similar but different - def create_from_request(params) - other_ids = Array(params[:other_id]).map do |id| - if id =~ /^symphony:(.+)$/ - "#{$1.length < 14 ? 'catkey' : 'barcode'}:#{$1}" - else - id - end - end - - if params[:label] == ':auto' - params.delete(:label) - params.delete('label') - metadata_id = Dor::MetadataService.resolvable(other_ids).first - params[:label] = Dor::MetadataService.label_for(metadata_id) - end - - dor_params = { - pid: params[:pid], - admin_policy: params[:admin_policy], - content_model: params[:model], - label: params[:label], - object_type: params[:object_type], - other_ids: ids_to_hash(other_ids), - parent: params[:parent], - source_id: ids_to_hash(params[:source_id]), - tags: params[:tag] || [], - seed_datastream: params[:seed_datastream], - initiate_workflow: Array(params[:initiate_workflow]) + Array(params[:workflow_id]), - rights: params[:rights], - metadata_source: params[:metadata_source], - collection: params[:collection], - workflow_priority: params[:workflow_priority] - } - dor_params.delete_if { |_k, v| v.nil? } - - dor_obj = register_object(dor_params) - pid = dor_obj.pid - location = URI.parse(Dor::Config.fedora.safeurl.sub(%r{/*$}, '/')).merge("objects/#{pid}").to_s - dor_params.dup.merge(location: location, pid: pid) - end - - private - def ids_to_hash(ids) return nil if ids.nil? diff --git a/spec/services/registration_service_spec.rb b/spec/services/registration_service_spec.rb index eaa9647c3c..7011a49f34 100644 --- a/spec/services/registration_service_spec.rb +++ b/spec/services/registration_service_spec.rb @@ -12,6 +12,8 @@ end context '#register_object' do + subject(:register) { described_class.send(:register_object, request) } + before do allow(Dor::SuriService).to receive(:mint_id).and_return(@pid) allow(Dor::SearchService).to receive(:query_by_id).and_return([]) @@ -31,6 +33,8 @@ } end + let(:request) { RegistrationRequest.new(@params) } + let(:mock_collection) do coll = Dor::Collection.new allow(coll).to receive(:new?).and_return false @@ -140,21 +144,21 @@ it 'registering a duplicate PID' do @params[:pid] = @pid expect(Dor::SearchService).to receive(:query_by_id).with('druid:ab123cd4567').and_return([@pid]) - expect { described_class.register_object(@params) }.to raise_error(Dor::DuplicateIdError) + expect { register }.to raise_error(Dor::DuplicateIdError) end it 'registering a duplicate source ID' do expect(Dor::SearchService).to receive(:query_by_id).with('barcode:9191919191').and_return([@pid]) - expect { described_class.register_object(@params) }.to raise_error(Dor::DuplicateIdError) + expect { register }.to raise_error(Dor::DuplicateIdError) end it 'missing a required parameter' do @params.delete(:object_type) - expect { described_class.register_object(@params) }.to raise_error(Dor::ParameterError) + expect { register }.to raise_error(Dor::ParameterError) end context 'when seed_datastream is present and something other than descMetadata' do it 'raises an error' do @params[:seed_datastream] = ['invalid'] - expect { described_class.register_object(@params) }.to raise_error(Dor::ParameterError) + expect { register }.to raise_error(Dor::ParameterError) end end @@ -165,9 +169,9 @@ it 'and metadata_source is label or none' do @params[:metadata_source] = 'label' - expect { described_class.register_object(@params) }.to raise_error(Dor::ParameterError) + expect { register }.to raise_error(Dor::ParameterError) @params[:metadata_source] = 'none' - expect { described_class.register_object(@params) }.to raise_error(Dor::ParameterError) + expect { register }.to raise_error(Dor::ParameterError) end end end @@ -187,7 +191,7 @@ expect(Dor::Collection).to receive(:new).with(pid: @pid).and_return(@coll) @params[:rights] = 'stanford' @params[:object_type] = 'collection' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -203,7 +207,7 @@ end it 'creates the datastream' do - @obj = described_class.register_object(@params) + @obj = register expect(RefreshMetadataAction).to have_received(:run) end end @@ -215,7 +219,7 @@ describe 'object registration' do before do - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -239,7 +243,7 @@ before do @params[:collection] = 'druid:something' expect(Dor::Collection).to receive(:find).with('druid:something').and_return(mock_collection) - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -265,7 +269,7 @@ describe 'default' do before do @params[:rights] = 'default' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -277,7 +281,7 @@ describe 'world' do before do @params[:rights] = 'world' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -289,7 +293,7 @@ describe 'loc:music' do before do @params[:rights] = 'loc:music' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -301,7 +305,7 @@ describe 'stanford no-download' do before do @params[:rights] = 'stanford-nd' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -314,7 +318,7 @@ describe 'when passed metadata_source=label' do before do @params[:metadata_source] = 'label' - @obj = described_class.register_object(@params) + @obj = register end it_behaves_like 'common registration' @@ -333,7 +337,7 @@ it 'truncates label if >= 255 chars' do # expect(Dor.logger).to receive(:warn).at_least(:once) @params[:label] = 'a' * 256 - obj = described_class.register_object(@params) + obj = register expect(obj.label).to eq('a' * 254) end @@ -341,7 +345,7 @@ expect(Dor::CreateWorkflowService).to receive(:create_workflow).with(Dor::Item, name: 'digitizationWF', create_ds: false, priority: 50) @params[:workflow_priority] = 50 @params[:initiate_workflow] = 'digitizationWF' - described_class.register_object(@params) + register end end # context common cases end