diff --git a/.rubocop.yml b/.rubocop.yml index f08dee44a..7c702ccbf 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,7 +1,9 @@ # Bixby coding conventions +require: rubocop-factory_bot + # Added AllCops config for further modification AllCops: - TargetRubyVersion: 2.6 + TargetRubyVersion: 2.7 DisabledByDefault: true DisplayCopNames: true Exclude: diff --git a/Gemfile b/Gemfile index d8f99f28f..ebfe5fb1c 100644 --- a/Gemfile +++ b/Gemfile @@ -34,4 +34,5 @@ end group :lint do gem 'bixby' + gem 'rubocop-factory_bot', require: false end diff --git a/app/controllers/bulkrax/importers_controller.rb b/app/controllers/bulkrax/importers_controller.rb index cb762e0a6..c44fa60de 100644 --- a/app/controllers/bulkrax/importers_controller.rb +++ b/app/controllers/bulkrax/importers_controller.rb @@ -241,7 +241,7 @@ def importer_params end def list_external_sets - url = params[:base_url] || (@harvester ? @harvester.base_url : nil) + url = params[:base_url] || @harvester&.base_url setup_client(url) if url.present? @sets = [['All', 'all']] diff --git a/app/factories/bulkrax/object_factory.rb b/app/factories/bulkrax/object_factory.rb index ca0ba791a..02454ad37 100644 --- a/app/factories/bulkrax/object_factory.rb +++ b/app/factories/bulkrax/object_factory.rb @@ -28,15 +28,16 @@ class ObjectFactory # rubocop:disable Metrics/ClassLength class_attribute :transformation_removes_blank_hash_values, default: false define_model_callbacks :save, :create - attr_reader :attributes, :object, :source_identifier_value, :klass, :replace_files, :update_files, :work_identifier, :related_parents_parsed_mapping, :importer_run_id + attr_reader :attributes, :object, :source_identifier_value, :klass, :replace_files, :update_files, :work_identifier, :work_identifier_search_field, :related_parents_parsed_mapping, :importer_run_id # rubocop:disable Metrics/ParameterLists - def initialize(attributes:, source_identifier_value:, work_identifier:, related_parents_parsed_mapping: nil, replace_files: false, user: nil, klass: nil, importer_run_id: nil, update_files: false) + def initialize(attributes:, source_identifier_value:, work_identifier:, work_identifier_search_field:, related_parents_parsed_mapping: nil, replace_files: false, user: nil, klass: nil, importer_run_id: nil, update_files: false) @attributes = ActiveSupport::HashWithIndifferentAccess.new(attributes) @replace_files = replace_files @update_files = update_files @user = user || User.batch_user @work_identifier = work_identifier + @work_identifier_search_field = work_identifier_search_field @related_parents_parsed_mapping = related_parents_parsed_mapping @source_identifier_value = source_identifier_value @klass = klass || Bulkrax.default_work_type.constantize @@ -103,12 +104,7 @@ def find_or_create end def search_by_identifier - # TODO(alishaevn): return the proper `work_index` value below - # ref: https://github.com/samvera-labs/bulkrax/issues/866 - # ref:https://github.com/samvera-labs/bulkrax/issues/867 - # work_index = ::ActiveFedora.index_field_mapper.solr_name(work_identifier, :facetable) - work_index = work_identifier - query = { work_index => + query = { work_identifier_search_field => source_identifier_value } # Query can return partial matches (something6 matches both something6 and something68) # so we need to weed out any that are not the correct full match. But other items might be diff --git a/app/models/concerns/bulkrax/import_behavior.rb b/app/models/concerns/bulkrax/import_behavior.rb index c095ab568..eea56afac 100644 --- a/app/models/concerns/bulkrax/import_behavior.rb +++ b/app/models/concerns/bulkrax/import_behavior.rb @@ -179,6 +179,7 @@ def factory @factory ||= of.new(attributes: self.parsed_metadata, source_identifier_value: identifier, work_identifier: parser.work_identifier, + work_identifier_search_field: parser.work_identifier_search_field, related_parents_parsed_mapping: parser.related_parents_parsed_mapping, replace_files: replace_files, user: user, diff --git a/app/parsers/bulkrax/application_parser.rb b/app/parsers/bulkrax/application_parser.rb index 9c5a1280f..3cc390ea7 100644 --- a/app/parsers/bulkrax/application_parser.rb +++ b/app/parsers/bulkrax/application_parser.rb @@ -81,6 +81,13 @@ def work_identifier @work_identifier ||= get_field_mapping_hash_for('source_identifier')&.keys&.first&.to_sym || :source end + # @return [Symbol] the solr property of the source_identifier. Used for searching. + # defaults to work_identifier value + "_sim" + # @see #work_identifier + def work_identifier_search_field + @work_identifier_search_field ||= get_field_mapping_hash_for('source_identifier')&.values&.first&.[]('search_field')&.first&.to_s || "#{work_identifier}_sim" + end + # @return [String] def generated_metadata_mapping @generated_metadata_mapping ||= 'generated' diff --git a/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb b/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb index 36dfcbfc6..0e3d0d747 100644 --- a/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb +++ b/lib/generators/bulkrax/templates/config/initializers/bulkrax.rb @@ -47,7 +47,7 @@ # (For more info on importing relationships, see Bulkrax Wiki: https://github.com/samvera-labs/bulkrax/wiki/Configuring-Bulkrax#parent-child-relationship-field-mappings) # # # e.g. to add the required source_identifier field - # # config.field_mappings["Bulkrax::CsvParser"]["source_id"] = { from: ["old_source_id"], source_identifier: true } + # # config.field_mappings["Bulkrax::CsvParser"]["source_id"] = { from: ["old_source_id"], source_identifier: true, search_field: 'source_id_sim' } # If you want Bulkrax to fill in source_identifiers for you, see below # To duplicate a set of mappings from one parser to another diff --git a/lib/tasks/bulkrax_tasks.rake b/lib/tasks/bulkrax_tasks.rake index b465aef3c..f0a741cb7 100644 --- a/lib/tasks/bulkrax_tasks.rake +++ b/lib/tasks/bulkrax_tasks.rake @@ -124,7 +124,7 @@ namespace :bulkrax do end def make_new_exports - Bulkrax::Exporter.all.each { |e| Bulkrax::ExporterJob.perform_later(e.id) } + Bulkrax::Exporter.find_each { |e| Bulkrax::ExporterJob.perform_later(e.id) } rescue => e puts "(#{e.message})" end diff --git a/spec/factories/bulkrax/object_factory_spec.rb b/spec/factories/bulkrax/object_factory_spec.rb index ce807ac73..5a46ddb8e 100644 --- a/spec/factories/bulkrax/object_factory_spec.rb +++ b/spec/factories/bulkrax/object_factory_spec.rb @@ -1,29 +1,4 @@ # frozen_string_literal: true -require 'rails_helper' - -RSpec.describe Bulkrax::ObjectFactory do - # Why aren't there more tests? In part because so much of the ObjectFactory require that we boot - # up Fedora and SOLR; something that remains non-desirous due to speed. - describe "#transform_attributes" do - context 'default behavior' do - it "does not empty arrays that only have empty values" do - attributes = { empty_array: ["", ""], empty_string: "", filled_array: ["A", "B"], filled_string: "A" } - factory = described_class.new(attributes: attributes, source_identifier_value: 123, work_identifier: "filled_string") - factory.base_permitted_attributes = %i[empty_array empty_string filled_array filled_string] - expect(factory.send(:transform_attributes)).to eq(attributes.stringify_keys) - end - end - - context 'when :transformation_removes_blank_hash_values = true' do - it "empties arrays that only have empty values" do - attributes = { empty_array: ["", ""], empty_string: "", filled_array: ["A", "B"], filled_string: "A" } - factory = described_class.new(attributes: attributes, source_identifier_value: 123, work_identifier: "filled_string") - factory.base_permitted_attributes = %i[empty_array empty_string filled_array filled_string] - factory.transformation_removes_blank_hash_values = true - expect(factory.send(:transform_attributes)) - .to eq({ empty_array: [], empty_string: nil, filled_array: ["A", "B"], filled_string: "A" }.stringify_keys) - end - end - end -end +# NOTE: do not put any specs here. FactoryBot autoloads every ruby file in spec/factories so even through it better +# mirrors the app/ path, we put the ObjectFactory specs in spec/models/bulkrax/object_factory_spec.rb diff --git a/spec/factories/bulkrax_object_factories.rb b/spec/factories/bulkrax_object_factories.rb index 0e49a0e42..59e116351 100644 --- a/spec/factories/bulkrax_object_factories.rb +++ b/spec/factories/bulkrax_object_factories.rb @@ -6,7 +6,8 @@ new( attributes: {}, source_identifier_value: :source_identifier, - work_identifier: :source + work_identifier: :source, + work_identifier_search_field: 'source_sim' ) end end diff --git a/spec/models/bulkrax/object_factory_spec.rb b/spec/models/bulkrax/object_factory_spec.rb index efe191aa0..2c888feab 100644 --- a/spec/models/bulkrax/object_factory_spec.rb +++ b/spec/models/bulkrax/object_factory_spec.rb @@ -5,11 +5,42 @@ module Bulkrax # NOTE: Unable to put this file in spec/factories/bulkrax (where it would mirror the path in app/) because # (presumably) FactoryBot autoloads all files in spec/factories, which would always run this spec. + # Why aren't there more tests? In part because so much of the ObjectFactory require that we boot + # up Fedora and SOLR; something that remains non-desirous due to speed. + RSpec.describe ObjectFactory do subject(:object_factory) { build(:object_factory) } describe 'is capable of looking up records dynamically' do include_examples 'dynamic record lookup' end + + describe "#transform_attributes" do + context 'default behavior' do + it "does not empty arrays that only have empty values" do + attributes = { empty_array: ["", ""], empty_string: "", filled_array: ["A", "B"], filled_string: "A" } + factory = described_class.new(attributes: attributes, + source_identifier_value: 123, + work_identifier: "filled_string", + work_identifier_search_field: 'filled_string_sim') + factory.base_permitted_attributes = %i[empty_array empty_string filled_array filled_string] + expect(factory.send(:transform_attributes)).to eq(attributes.stringify_keys) + end + end + + context 'when :transformation_removes_blank_hash_values = true' do + it "empties arrays that only have empty values" do + attributes = { empty_array: ["", ""], empty_string: "", filled_array: ["A", "B"], filled_string: "A" } + factory = described_class.new(attributes: attributes, + source_identifier_value: 123, + work_identifier: "filled_string", + work_identifier_search_field: 'filled_string_sim') + factory.base_permitted_attributes = %i[empty_array empty_string filled_array filled_string] + factory.transformation_removes_blank_hash_values = true + expect(factory.send(:transform_attributes)) + .to eq({ empty_array: [], empty_string: nil, filled_array: ["A", "B"], filled_string: "A" }.stringify_keys) + end + end + end end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 386ec8ec2..d0d48acd3 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -69,7 +69,7 @@ end # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures - config.fixture_path = "#{::Rails.root}/spec/fixtures" + config.fixture_path = Rails.root.join('spec', 'fixtures') # If you're not using ActiveRecord, or you'd prefer not to run each of your # examples within a transaction, remove the following line or assign false