From 66704c8b8bb3fb089111f0708b7f88f15aa629bc Mon Sep 17 00:00:00 2001 From: Rob Kaufman Date: Tue, 12 Dec 2023 01:17:20 -0800 Subject: [PATCH] handle contributor creation properly, better multiple detection --- Gemfile.lock | 50 +++++------ .../valkyrie_object_factory_decorator.rb | 9 ++ app/matchers/concerns/has_ams_matchers.rb | 16 ++-- app/models/bulkrax/pbcore_xml_entry.rb | 2 + app/transactions/ams/container.rb | 4 + .../ams/steps/create_aapb_admin_data.rb | 8 -- .../ams/steps/handle_contributors.rb | 90 +++++++++++++++++++ app/transactions/ams/work_create.rb | 1 + app/transactions/ams/work_update.rb | 1 + 9 files changed, 136 insertions(+), 45 deletions(-) create mode 100644 app/factories/bulkrax/valkyrie_object_factory_decorator.rb create mode 100644 app/transactions/ams/steps/handle_contributors.rb diff --git a/Gemfile.lock b/Gemfile.lock index 02cc43de..f7576d56 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -10,7 +10,7 @@ GIT GIT remote: https://github.com/samvera-labs/bulkrax.git - revision: ed49dc2b366cd564ea9593aa60fdb6c63aa5aec9 + revision: ba7a071799b97fc3585448c93ad8ec246cc5de5e branch: hyrax-4-valkyrie-support specs: bulkrax (5.3.0) @@ -41,10 +41,10 @@ GIT GIT remote: https://github.com/samvera/hyrax.git - revision: a904e5a07040c148e4d6c3a9288d048efc8e42b9 + revision: 86453ea8f5d9614b9757db5a578bd13dd3ac28aa branch: double_combo specs: - hyrax (5.0.0.rc1) + hyrax (5.0.0.rc2) active-fedora (~> 14.0) almond-rails (~> 0.1) awesome_nested_set (~> 3.1) @@ -54,15 +54,14 @@ GIT browse-everything (>= 0.16, < 2.0) carrierwave (~> 1.0) clipboard-rails (~> 1.5) + concurrent-ruby (~> 1.0) connection_pool (~> 2.4) draper (~> 4.0) dry-container (~> 0.11) - dry-equalizer (~> 0.2) - dry-events (~> 1.0.0) + dry-events (~> 1.0, >= 1.0.1) dry-logic (~> 1.5) - dry-monads (~> 1.5) - dry-struct (~> 1.0) - dry-validation (~> 1.3) + dry-monads (~> 1.6) + dry-validation (~> 1.10) flipflop (~> 2.3) flot-rails (~> 0.0.6) font-awesome-rails (~> 4.2) @@ -98,7 +97,7 @@ GIT signet sprockets (~> 3.7) tinymce-rails (~> 5.10) - valkyrie (~> 3.0.1) + valkyrie (~> 3.1.1) view_component (~> 2.74.1) GIT @@ -324,7 +323,7 @@ GEM activerecord (>= 5.a) database_cleaner-core (~> 2.0.0) database_cleaner-core (2.0.1) - date (3.3.3) + date (3.3.4) declarative (0.0.20) deprecation (1.1.0) activesupport @@ -363,7 +362,6 @@ GEM dry-core (1.0.1) concurrent-ruby (~> 1.0) zeitwerk (~> 2.6) - dry-equalizer (0.3.0) dry-events (1.0.1) concurrent-ruby (~> 1.0) dry-core (~> 1.0, < 2) @@ -541,7 +539,7 @@ GEM rails-dom-testing (>= 1, < 3) railties (>= 4.2.0) thor (>= 0.14, < 2.0) - json (2.6.3) + json (2.7.1) json-canonicalization (0.3.3) json-ld (3.3.0) htmlentities (~> 4.3) @@ -631,7 +629,7 @@ GEM rb-fsevent (~> 0.9, >= 0.9.4) rb-inotify (~> 0.9, >= 0.9.7) logger (1.5.3) - loofah (2.21.3) + loofah (2.22.0) crass (~> 1.0.2) nokogiri (>= 1.12.0) mail (2.8.1) @@ -650,7 +648,7 @@ GEM mime-types-data (3.2023.0808) mini_magick (4.12.0) mini_mime (1.1.5) - mini_portile2 (2.8.4) + mini_portile2 (2.8.5) minitest (5.20.0) multi_json (1.15.0) multi_xml (0.6.0) @@ -662,21 +660,21 @@ GEM redic net-http-persistent (4.0.2) connection_pool (~> 2.2) - net-imap (0.3.7) + net-imap (0.4.8) date net-protocol net-pop (0.1.2) net-protocol - net-protocol (0.2.1) + net-protocol (0.2.2) timeout - net-smtp (0.3.3) + net-smtp (0.4.0) net-protocol - nio4r (2.5.9) + nio4r (2.7.0) noid (0.9.0) noid-rails (3.1.0) actionpack (>= 5.0.0, < 7.1) noid (~> 0.9) - nokogiri (1.15.4) + nokogiri (1.15.5) mini_portile2 (~> 2.8.2) racc (~> 1.4) oai (1.2.1) @@ -727,7 +725,7 @@ GEM nokogiri (~> 1.6) rails (>= 5.0, < 7.1) rdf - racc (1.7.1) + racc (1.7.3) rack (2.2.8) rack-linkeddata (3.3.0) linkeddata (~> 3.3) @@ -779,11 +777,11 @@ GEM rake (>= 12.2) thor (~> 1.0) rainbow (3.1.1) - rake (13.0.6) + rake (13.1.0) rb-fsevent (0.11.2) rb-inotify (0.10.1) ffi (~> 1.0) - rdf (3.3.0) + rdf (3.3.1) bcp47_spec (~> 0.2) link_header (~> 0.0, >= 0.0.8) rdf-aggregate-repo (3.3.0) @@ -1045,9 +1043,9 @@ GEM temple (0.10.2) terminal-table (3.0.2) unicode-display_width (>= 1.1.1, < 3) - thor (1.2.2) + thor (1.3.0) tilt (2.2.0) - timeout (0.4.0) + timeout (0.4.1) tinymce-rails (5.10.7.1) railties (>= 3.1.1) trailblazer-option (0.1.2) @@ -1068,7 +1066,7 @@ GEM unicode-display_width (2.4.2) unicode-types (1.8.0) validatable (1.6.7) - valkyrie (3.0.3) + valkyrie (3.1.1) activemodel activesupport dry-struct @@ -1119,7 +1117,7 @@ GEM psych (>= 3.3) rdf (~> 3.2, >= 3.2.9) rdf-xsd (~> 3.2) - zeitwerk (2.6.11) + zeitwerk (2.6.12) PLATFORMS ruby diff --git a/app/factories/bulkrax/valkyrie_object_factory_decorator.rb b/app/factories/bulkrax/valkyrie_object_factory_decorator.rb new file mode 100644 index 00000000..79669666 --- /dev/null +++ b/app/factories/bulkrax/valkyrie_object_factory_decorator.rb @@ -0,0 +1,9 @@ +module Bulkrax + module ValkyrieObjectFactoryDecorator + def permitted_attributes + attributes.keys.map(&:to_sym) + end + end +end + +Bulkrax::ValkyrieObjectFactory.prepend(Bulkrax::ValkyrieObjectFactoryDecorator) diff --git a/app/matchers/concerns/has_ams_matchers.rb b/app/matchers/concerns/has_ams_matchers.rb index a00ff6d0..b69a3882 100644 --- a/app/matchers/concerns/has_ams_matchers.rb +++ b/app/matchers/concerns/has_ams_matchers.rb @@ -13,7 +13,6 @@ def field_supported?(field) return true if field == "title" return true if field == "description" return true if field == "subject" - return true if field == "contributors" property_defined = factory_class.singleton_methods.include?(:properties) && factory_class.properties[field].present? @@ -39,12 +38,11 @@ def multiple?(field) return true if field == "description" return true if field == "subject" - field_supported?(field) && (multiple_field?(field) || factory_class.singleton_methods.include?(:properties) && factory_class&.properties&.[](field)&.[]("multiple")) - end - - def multiple_field?(field) - form_definition = schema_form_definitions[field.to_sym] - form_definition.nil? ? false : form_definition[:multiple] + if factory_class.respond_to?(:schema) + field_supported?(field) && valkyrie_multiple?(field) + else + field_supported?(field) && ar_multiple?(field) + end end def add_metadata(node_name, node_content, index = nil) @@ -53,7 +51,6 @@ def add_metadata(node_name, node_content, index = nil) object_name = get_object_name(name) || false # the "key" of an object property. e.g. { object_name: { alpha: 'beta' } } multiple = multiple?(name) # the property has multiple values. e.g. 'letters': ['a', 'b', 'c'] object_multiple = object_name && multiple?(object_name) # the property's value is an array of object(s) - next unless field_supported?(name) || (object_name && field_supported?(object_name)) if object_name @@ -119,7 +116,4 @@ def multiple_metadata(content, name = nil) # end # end - def schema_form_definitions - @schema_form_definitions ||= Hyrax::SimpleSchemaLoader.new.form_definitions_for(schema: factory_class.name.underscore.to_sym) - end end diff --git a/app/models/bulkrax/pbcore_xml_entry.rb b/app/models/bulkrax/pbcore_xml_entry.rb index 4dc00d04..54f5c774 100644 --- a/app/models/bulkrax/pbcore_xml_entry.rb +++ b/app/models/bulkrax/pbcore_xml_entry.rb @@ -54,11 +54,13 @@ def build_metadata bulkrax_importer_id = importer.id admin_data_gid = update_or_create_admin_data_gid(bulkrax_importer_id) + self.parsed_metadata["contributors"] = self.raw_metadata["contributors"] self.parsed_metadata['bulkrax_importer_id'] = bulkrax_importer_id self.parsed_metadata['admin_data_gid'] = admin_data_gid build_annotations(self.raw_metadata['annotations'], admin_data_gid) if self.raw_metadata['annotations'].present? end + self.parsed_metadata['label'] = nil if self.parsed_metadata['label'] == "[]" add_visibility add_rights_statement add_admin_set_id diff --git a/app/transactions/ams/container.rb b/app/transactions/ams/container.rb index 96cac9d5..83fa1023 100644 --- a/app/transactions/ams/container.rb +++ b/app/transactions/ams/container.rb @@ -5,6 +5,10 @@ class Container extend Dry::Container::Mixin namespace 'change_set' do |ops| + ops.register "handle_contributors" do + Ams::Steps::HandleContributors.new + end + ops.register "create_aapb_admin_data" do Ams::Steps::CreateAapbAdminData.new end diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index f864c378..d4c72aa5 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -10,7 +10,6 @@ class CreateAapbAdminData def call(change_set) case change_set.model when AssetResource - contributions = extract_contributions(change_set) add_title_types(change_set) add_description_types(change_set) add_date_types(change_set) @@ -199,13 +198,6 @@ def save_instantiation_aapb_admin_data(change_set) remove_instantiation_admin_data_from_env_attributes(change_set) end - def extract_contributions(change_set) - return [] unless change_set.fields.has_key?(:contributors) - - contributors = change_set.fields.delete(:contributors) || [] - contributors.select { |contributor| contributor unless contributor[:contributor].first.blank? } - end - def find_or_create_instantiation_admin_data(change_set) instantiation_admin_data_gid = change_set.model.instantiation_admin_data_gid || change_set.instantiation_admin_data_gid if instantiation_admin_data_gid diff --git a/app/transactions/ams/steps/handle_contributors.rb b/app/transactions/ams/steps/handle_contributors.rb new file mode 100644 index 00000000..9dca0d9d --- /dev/null +++ b/app/transactions/ams/steps/handle_contributors.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'dry/monads' + +module Ams + module Steps + class HandleContributors + include Dry::Monads[:result] + + attr_accessor :change_set, :user + def call(change_set, user: nil) + @change_set = change_set + @user = user + case change_set.model + when AssetResource + contributions = extract_contributions(change_set) + create_or_update_contributions(change_set, contributions) + end + + Success(change_set) + rescue NoMethodError => err + Failure([err.message, change_set]) + end + + private + + def extract_contributions(change_set) + return [] unless change_set.input_params.has_key?(:contributors) + + contributors = change_set.input_params.delete(:contributors) || [] + contributors.select { |contributor| contributor unless contributor['contributor'].first.blank? }.map(&:with_indifferent_access) + end + + def create_or_update_contributions(change_set, contributions) + if contributions&.first&.[]("contributor")&.present? + inserts = [] + destroys = [] + contributions.each do |param_contributor| + param_contributor[:contributor] = Array(param_contributor['contributor']) + param_contributor[:admin_set_id] = change_set['admin_set_id'] + param_contributor[:title] = change_set["title"] + + to_destroy = ActiveModel::Type::Boolean.new.cast(param_contributor['_destroy']) + if to_destroy + destroys << param_contributor[:id] + next + end + + + contributor = Contribution.find(param_contributor[:id]) if param_contributor[:id].present? + if contributor + param_contributor.delete(:id) + contributor.attributes.merge!(param_contributor) + contributor_resource = Hyrax.persister.save(resource: contributor) + Hyrax.publisher.publish('object.metadata.updated', object: contributor_resource, user: change_set.user) + inserts << contributor_resource.id + next + end + contribution_resource = Hyrax.persister.save(resource: ContributionResource.new(param_contributor.symbolize_keys)) + Hyrax.index_adapter.save(resource: contribution_resource) + Hyrax.publisher.publish('object.deposited', object: contribution_resource, user: user) + Hyrax::AccessControlList.copy_permissions(source: target_permissions, target: contribution_resource) + inserts << contribution_resource.id + end + + update_members(change_set, inserts, destroys) + end + end + + def update_members(change_set, inserts, destroys) + return if inserts.empty? && destroys.empty? + current_member_ids = change_set.member_ids.map(&:id) + inserts = inserts - current_member_ids + destroys = destroys & current_member_ids + change_set.member_ids += inserts.map { |id| Valkyrie::ID.new(id) } + change_set.member_ids -= destroys.map { |id| Valkyrie::ID.new(id) } + end + + ## + # @api private + # + # @note cache these per instance to avoid repeated lookups. + # + # @return [Hyrax::AccessControlList] permissions to set on created filesets + def target_permissions + @target_permissions ||= Hyrax::AccessControlList.new(resource: change_set) + end + end + end +end diff --git a/app/transactions/ams/work_create.rb b/app/transactions/ams/work_create.rb index 4bba9b8f..1ce569a7 100644 --- a/app/transactions/ams/work_create.rb +++ b/app/transactions/ams/work_create.rb @@ -6,6 +6,7 @@ class WorkCreate < Hyrax::Transactions::Transaction 'change_set.ensure_admin_set', 'change_set.set_user_as_depositor', 'change_set.create_aapb_admin_data', + 'change_set.handle_contributors', 'change_set.apply', 'work_resource.apply_permission_template', 'work_resource.save_acl', diff --git a/app/transactions/ams/work_update.rb b/app/transactions/ams/work_update.rb index 9da6f001..70bb4e85 100644 --- a/app/transactions/ams/work_update.rb +++ b/app/transactions/ams/work_update.rb @@ -3,6 +3,7 @@ module Ams class WorkUpdate < Hyrax::Transactions::Transaction DEFAULT_STEPS = ['change_set.create_aapb_admin_data', + 'change_set.handle_contributors', 'change_set.apply', 'work_resource.save_acl', 'work_resource.add_file_sets',