I897 Bulkrax readiness for Hyku 6 and Hyrax 4 & 5 (#898)

* 🧹 relocates transactions from inititalizer file Issue: - #897 Co-Authored-By: LaRita Robinson <[email protected]> * 🧹 Add specs for container.rb, relocate files Co-Authored-By: LaRita Robinson <[email protected]> * 🧹 normalize magic strings into constants for referencing later Convert the create_with_bulk_behavior and update_with_bulk_behavior to a constant; that way we can reference it in IiifPrint and document the “magic” string. Co-Authored-By: LaRita Robinson <[email protected]> * 🧹 correct camel case to constant notation for easier referencing Co-Authored-By: LaRita Robinson <[email protected]> * 💄 rubocop fixes Co-Authored-By: LaRita Robinson <[email protected]> * Update app/factories/bulkrax/valkyrie_object_factory.rb * Update spec/bulkrax/transactions/container_spec.rb * 🧹 Move container & steps Match Hyrax convention by using bulkrax/transactions. * restructure org to run specs locally receiving error when trying to run the entire spec suite due to restructuring files but not moving the spec file. * 🚧 WIP: Consolidate HasMatchers with HasMappingExt Remove HasMappingExt and consolidate logic within HasMatchers. HasMatchers should handle both cases, when objects are ActiveFedora vs Valkyrie. * 🧹 Fix Specs & add Valkyrie Specs * 🧹 Fix Rubocop complaint * 🧹 Address Valkyrie's determination of multiple? * 🧹 Address permitted attributes In Valkyrie, we use the schema to identify the permitted attributes. All allowed attributes should be on the schema, so no additional attributes should be required. Also add a fallback for permitted attributes in case an ActiveFedora model class goes through the ValkyrieObjectFactory. This supports the case where we want to always force a Valkyrie resource to be created, regardless of the model name given. * 🧹 Update TODO comment Adjust TODO message because referring to a handler that doesn't exist anywhere is confusing. We may need to register steps for file sets once the behavior is implemented. --------- Co-authored-by: LaRita Robinson <[email protected]> Co-authored-by: Jeremy Friesen <[email protected]> Co-authored-by: LaRita Robinson <[email protected]>
samvera · Jan 25, 2024 · 837ab8a · 837ab8a
1 parent a2cca06
commit 837ab8a
Show file tree

Hide file tree

Showing 12 changed files with 201 additions and 132 deletions.
diff --git a/app/factories/bulkrax/valkyrie_object_factory.rb b/app/factories/bulkrax/valkyrie_object_factory.rb
@@ -53,7 +53,7 @@ def create
       result = transaction
                .with_step_args(
           # "work_resource.add_to_parent" => {parent_id: @related_parents_parsed_mapping, user: @user},
-          "work_resource.add_bulkrax_files" => { files: get_s3_files(remote_files: attributes["remote_files"]), user: @user },
+          "work_resource.#{Bulkrax::Transactions::Container::ADD_BULKRAX_FILES}" => { files: get_s3_files(remote_files: attributes["remote_files"]), user: @user },
           "change_set.set_user_as_depositor" => { user: @user },
           "work_resource.change_depositor" => { user: @user },
           'work_resource.save_acl' => { permissions_params: [attrs.try('visibility') || 'open'].compact }
@@ -83,7 +83,7 @@ def update
 
       result = update_transaction
                .with_step_args(
-          "work_resource.add_bulkrax_files" => { files: get_s3_files(remote_files: attributes["remote_files"]), user: @user }
+          "work_resource.#{Bulkrax::Transactions::Container::ADD_BULKRAX_FILES}" => { files: get_s3_files(remote_files: attributes["remote_files"]), user: @user }
 
           # TODO: uncomment when we upgrade Hyrax 4.x
           # 'work_resource.save_acl' => { permissions_params: [attrs.try('visibility') || 'open'].compact }
@@ -109,15 +109,11 @@ def get_s3_files(remote_files: {})
     end
 
     ##
-    # TODO: What else fields are necessary: %i[id edit_users edit_groups read_groups work_members_attributes]?
-    # Regardless of what the Parser gives us, these are the properties we are prepared to accept.
+    # We accept attributes based on the model schema
     def permitted_attributes
-      Bulkrax::ValkyrieObjectFactory.schema_properties(klass) +
-        %i[
-          admin_set_id
-          title
-          visibility
-        ]
+      return Bulkrax::ValkyrieObjectFactory.schema_properties(klass) if klass.respond_to?(:schema)
+      # fallback to support ActiveFedora model name
+      klass.properties.keys.map(&:to_sym) + base_permitted_attributes
     end
 
     def apply_depositor_metadata(object, user)
@@ -164,13 +160,14 @@ def destroy_existing_files
 
     private
 
+    # TODO: Rename to create_transaction
     def transaction
-      Hyrax::Transactions::Container["work_resource.create_with_bulk_behavior"]
+      Hyrax::Transactions::Container["work_resource.#{Bulkrax::Transactions::Container::CREATE_WITH_BULK_BEHAVIOR}"]
     end
 
     # Customize Hyrax::Transactions::WorkUpdate transaction with bulkrax
     def update_transaction
-      Hyrax::Transactions::Container["work_resource.update_with_bulk_behavior"]
+      Hyrax::Transactions::Container["work_resource.#{Bulkrax::Transactions::Container::UPDATE_WITH_BULK_BEHAVIOR}"]
     end
 
     # Query child FileSet in the resource/object

diff --git a/app/models/concerns/bulkrax/has_matchers.rb b/app/models/concerns/bulkrax/has_matchers.rb
@@ -56,6 +56,10 @@ def add_metadata(node_name, node_content, index = nil)
       end
     end
 
+    def get_object_name(field)
+      mapping&.[](field)&.[]('object')
+    end
+
     def set_parsed_data(name, value)
       return parsed_metadata[name] = value unless multiple?(name)
 
@@ -125,9 +129,14 @@ def field_supported?(field)
 
       return false if excluded?(field)
       return true if supported_bulkrax_fields.include?(field)
-      property_defined = factory_class.singleton_methods.include?(:properties) && factory_class.properties[field].present?
 
-      factory_class.method_defined?(field) && (Bulkrax::ValkyrieObjectFactory.schema_properties(factory_class).include?(field) || property_defined)
+      if Bulkrax.object_factory == Bulkrax::ValkyrieObjectFactory
+        # used in cases where we have a Fedora object class but use the Valkyrie object factory
+        property_defined = factory_class.singleton_methods.include?(:properties) && factory_class.properties[field].present?
+        factory_class.method_defined?(field) && (property_defined || Bulkrax::ValkyrieObjectFactory.schema_properties(factory_class).include?(field))
+      else
+        factory_class.method_defined?(field) && factory_class.properties[field].present?
+      end
     end
 
     def supported_bulkrax_fields
@@ -144,6 +153,8 @@ def supported_bulkrax_fields
         ]
     end
 
+    ##
+    # Determine a multiple properties field
     def multiple?(field)
       @multiple_bulkrax_fields ||=
         %W[
@@ -157,25 +168,30 @@ def multiple?(field)
       return true if @multiple_bulkrax_fields.include?(field)
       return false if field == 'model'
 
-      if factory.class.respond_to?(:schema)
+      if Bulkrax.object_factory == Bulkrax::ValkyrieObjectFactory
         field_supported?(field) && valkyrie_multiple?(field)
       else
         field_supported?(field) && ar_multiple?(field)
       end
     end
 
+    def schema_form_definitions
+      @schema_form_definitions ||= ::SchemaLoader.new.form_definitions_for(factory_class.name.underscore.to_sym)
+    end
+
     def ar_multiple?(field)
       factory_class.singleton_methods.include?(:properties) && factory_class&.properties&.[](field)&.[]("multiple")
     end
 
     def valkyrie_multiple?(field)
       # TODO: there has got to be a better way. Only array types have 'of'
-      sym_field = field.to_sym
-      factory_class.schema.key(sym_field).respond_to?(:of) if factory_class.fields.include?(sym_field)
-    end
-
-    def get_object_name(field)
-      mapping&.[](field)&.[]('object')
+      if factory_class.respond_to?(:schema)
+        sym_field = field.to_sym
+        return true if factory_class.schema.key(sym_field).primitive == Array
+        false
+      else
+        ar_multiple?(field)
+      end
     end
 
     # Hyrax field to use for the given import field

diff --git a/app/transactions/bulkrax/transactions.rb b/app/transactions/bulkrax/transactions.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+require 'bulkrax/transactions/container'
+
+module Bulkrax
+  ##
+  # This is a parent module for DRY Transaction classes handling Bulkrax
+  # processes. Especially: transactions and steps for creating, updating, and
+  # destroying PCDM Objects are located here.
+  #
+  # @since 2.4.0
+  #
+  # @example
+  #   Bulkrax::Transaction::Container['transaction_name'].call(:input)
+  #
+  # @see https://dry-rb.org/gems/dry-transaction/
+  module Transactions
+  end
+end
diff --git a/app/transactions/bulkrax/transactions/container.rb b/app/transactions/bulkrax/transactions/container.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+require 'dry/container'
+
+module Bulkrax
+  module Transactions
+    class Container
+      extend Dry::Container::Mixin
+
+      ADD_BULKRAX_FILES = 'add_bulkrax_files'
+      CREATE_WITH_BULK_BEHAVIOR = 'create_with_bulk_behavior'
+      CREATE_WITH_BULK_BEHAVIOR_STEPS = begin
+        steps = Hyrax::Transactions::WorkCreate::DEFAULT_STEPS.dup
+        steps[steps.index("work_resource.add_file_sets")] = "work_resource.#{Bulkrax::Transactions::Container::ADD_BULKRAX_FILES}"
+        steps
+      end.freeze
+      UPDATE_WITH_BULK_BEHAVIOR = 'update_with_bulk_behavior'
+      UPDATE_WITH_BULK_BEHAVIOR_STEPS = begin
+        steps = Hyrax::Transactions::WorkUpdate::DEFAULT_STEPS.dup
+        steps[steps.index("work_resource.add_file_sets")] = "work_resource.#{Bulkrax::Transactions::Container::ADD_BULKRAX_FILES}"
+        steps
+      end.freeze
+
+      namespace "work_resource" do |ops|
+        ops.register CREATE_WITH_BULK_BEHAVIOR do
+          Hyrax::Transactions::WorkCreate.new(steps: CREATE_WITH_BULK_BEHAVIOR_STEPS)
+        end
+
+        ops.register UPDATE_WITH_BULK_BEHAVIOR do
+          Hyrax::Transactions::WorkUpdate.new(steps: UPDATE_WITH_BULK_BEHAVIOR_STEPS)
+        end
+
+        # TODO: Need to register step for uploads handler?
+        # ops.register "add_file_sets" do
+        #   Hyrax::Transactions::Steps::AddFileSets.new
+        # end
+
+        ops.register ADD_BULKRAX_FILES do
+          Bulkrax::Transactions::Steps::AddFiles.new
+        end
+      end
+    end
+  end
+end
+Hyrax::Transactions::Container.merge(Bulkrax::Transactions::Container)
diff --git a/...s/bulkrax/transactions/steps/add_files.rb → ...s/bulkrax/transactions/steps/add_files.rb b/...s/bulkrax/transactions/steps/add_files.rb → ...s/bulkrax/transactions/steps/add_files.rb
diff --git a/lib/bulkrax/engine.rb b/lib/bulkrax/engine.rb
@@ -5,6 +5,9 @@
 module Bulkrax
   class Engine < ::Rails::Engine
     isolate_namespace Bulkrax
+
+    config.eager_load_paths += %W[#{config.root}/app/transactions]
+
     initializer :append_migrations do |app|
       if !app.root.to_s.match(root.to_s) && app.root.join('db/migrate').children.none? { |path| path.fnmatch?("*.bulkrax.rb") }
         config.paths["db/migrate"].expanded.each do |expanded_path|
@@ -17,6 +20,7 @@ class Engine < ::Rails::Engine
       require 'bulkrax/persistence_layer'
       require 'bulkrax/persistence_layer/active_fedora_adapter' if defined?(ActiveFedora)
       require 'bulkrax/persistence_layer/valkyrie_adapter' if defined?(Valkyrie)
+      require 'bulkrax/transactions' if defined?(Hyrax::Transactions)
     end
 
     config.generators do |g|

diff --git a/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb b/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb
@@ -86,98 +86,3 @@
 if Object.const_defined?(:Hyrax) && ::Hyrax::DashboardController&.respond_to?(:sidebar_partials)
   Hyrax::DashboardController.sidebar_partials[:repository_content] << "hyrax/dashboard/sidebar/bulkrax_sidebar_additions"
 end
-
-# TODO: move outside of initializer?
-class BulkraxTransactionContainer
-  extend Dry::Container::Mixin
-
-  namespace "work_resource" do |ops|
-    ops.register "create_with_bulk_behavior" do
-      steps = Hyrax::Transactions::WorkCreate::DEFAULT_STEPS.dup
-      steps[steps.index("work_resource.add_file_sets")] = "work_resource.add_bulkrax_files"
-
-      Hyrax::Transactions::WorkCreate.new(steps: steps)
-    end
-
-    ops.register "update_with_bulk_behavior" do
-      steps = Hyrax::Transactions::WorkUpdate::DEFAULT_STEPS.dup
-      steps[steps.index("work_resource.add_file_sets")] = "work_resource.add_bulkrax_files"
-
-      Hyrax::Transactions::WorkUpdate.new(steps: steps)
-    end
-
-    # TODO: uninitialized constant BulkraxTransactionContainer::InlineUploadHandler
-    # ops.register "add_file_sets" do
-    #   Hyrax::Transactions::Steps::AddFileSets.new(handler: InlineUploadHandler)
-    # end
-
-    ops.register "add_bulkrax_files" do
-      Bulkrax::Transactions::Steps::AddFiles.new
-    end
-  end
-end
-Hyrax::Transactions::Container.merge(BulkraxTransactionContainer)
-
-# TODO: move outside of initializer?
-module HasMappingExt
-  ##
-  # Field of the model that can be supported
-  def field_supported?(field)
-    field = field.gsub("_attributes", "")
-
-    return false if excluded?(field)
-    return true if supported_bulkrax_fields.include?(field)
-
-    property_defined = factory_class.singleton_methods.include?(:properties) && factory_class.properties[field].present?
-
-    factory_class.method_defined?(field) && (Bulkrax::ValkyrieObjectFactory.schema_properties(factory_class).include?(field) || property_defined)
-  end
-
-  ##
-  # Determine a multiple properties field
-  def multiple?(field)
-    @multiple_bulkrax_fields ||=
-      %W[
-        file
-        remote_files
-        rights_statement
-        #{related_parents_parsed_mapping}
-        #{related_children_parsed_mapping}
-      ]
-
-    return true if @multiple_bulkrax_fields.include?(field)
-    return false if field == "model"
-
-    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?
-  end
-
-  # override: we want to directly infer from a property being multiple that we should split when it's a String
-  # def multiple_metadata(content)
-  #   return unless content
-
-  #   case content
-  #   when Nokogiri::XML::NodeSet
-  #     content&.content
-  #   when Array
-  #     content
-  #   when Hash
-  #     Array.wrap(content)
-  #   when String
-  #     String(content).strip.split(Bulkrax.multi_value_element_split_on)
-  #   else
-  #     Array.wrap(content)
-  #   end
-  # end
-
-  def schema_form_definitions
-    @schema_form_definitions ||= ::SchemaLoader.new.form_definitions_for(factory_class.name.underscore.to_sym)
-  end
-end
-[Bulkrax::HasMatchers, Bulkrax::HasMatchers.singleton_class].each do |mod|
-  mod.prepend HasMappingExt
-end
diff --git a/spec/bulkrax/entry_spec_helper_spec.rb b/spec/bulkrax/entry_spec_helper_spec.rb
@@ -22,20 +22,53 @@
         }
       end
 
-      let(:data) { { model: "Work", source_identifier: identifier, title: "If You Want to Go Far" } }
+      context 'when ActiveFedora object' do
+        let(:data) { { model: "Work", source_identifier: identifier, title: "If You Want to Go Far" } }
 
-      it { is_expected.to be_a(Bulkrax::CsvEntry) }
+        before do
+          allow(Bulkrax).to receive(:object_factory).and_return(Bulkrax::ObjectFactory)
+        end
 
-      it "parses metadata" do
-        entry.build_metadata
+        it { is_expected.to be_a(Bulkrax::CsvEntry) }
 
-        expect(entry.factory_class).to eq(Work)
-        {
-          "title" => ["If You Want to Go Far"],
-          "admin_set_id" => "admin_set/default",
-          "source" => [identifier]
-        }.each do |key, value|
-          expect(entry.parsed_metadata.fetch(key)).to eq(value)
+        it "parses metadata" do
+          entry.build_metadata
+
+          expect(entry.factory_class).to eq(Work)
+          {
+            "title" => ["If You Want to Go Far"],
+            "admin_set_id" => "admin_set/default",
+            "source" => [identifier]
+          }.each do |key, value|
+            expect(entry.parsed_metadata.fetch(key)).to eq(value)
+          end
+        end
+      end
+
+      context 'when using ValkyrieObjectFactory' do
+        ['Work', 'WorkResource'].each do |model_name|
+          context "for #{model_name}" do
+            let(:data) { { model: model_name, source_identifier: identifier, title: "If You Want to Go Far" } }
+
+            before do
+              allow(Bulkrax).to receive(:object_factory).and_return(Bulkrax::ValkyrieObjectFactory)
+            end
+
+            it { is_expected.to be_a(Bulkrax::CsvEntry) }
+
+            it "parses metadata" do
+              entry.build_metadata
+
+              expect(entry.factory_class).to eq(model_name.constantize)
+              {
+                "title" => ["If You Want to Go Far"],
+                "admin_set_id" => "admin_set/default",
+                "source" => [identifier]
+              }.each do |key, value|
+                expect(entry.parsed_metadata.fetch(key)).to eq(value)
+              end
+            end
+          end
         end
       end
     end

diff --git a/spec/test_app/app/models/work_resource.rb b/spec/test_app/app/models/work_resource.rb
@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class WorkResource < Hyrax::Work
+  include Hyrax::Schema(:basic_metadata)
+  include Hyrax::Schema(:work_resource)
+end
diff --git a/spec/test_app/config/metadata/work_resource.yaml b/spec/test_app/config/metadata/work_resource.yaml
@@ -0,0 +1,11 @@
+attributes:
+  source_identifier:
+    type: string
+    multiple: false
+    index_keys:
+      - "source_identifier_sim"
+      - "source_identifier_tesim"
+    form:
+      required: false
+      primary: false
+      multiple: false