handle contributor creation properly, better multiple detection

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

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)

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

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

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

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

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

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',

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',