From 76b215a3cfba4c1a8993cc0afea5fe81aec0feee Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Sun, 18 Feb 2024 11:53:42 -0500 Subject: [PATCH 1/3] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Extracting=20naming=20?= =?UTF-8?q?container=20(#6694)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ♻️ Extracting naming container Throughout the code we have quite a bit of conditionals regarding what is a work, collection, file_set, and adminsitrative set. This model attempts to provide a common point to interrogate the application. There are, at present, no refactors to use this model. Consider how our specs and our application are inconsistent in their declaration/configuration/stubbing. This model should help with that. * ♻️ Favor Hyrax::ModelRegistry for search builders Instead of the myriad of ways of asking about which models to use, let's leverage a consolidated central place for information. This is but one step in addressing other issues. * ♻️ Leverage Hyrax::ModelRegistry for ability tests * 💄 endless and ever appeasing of the coppers * ♻️ Favor Hyrax::ModelRegistry For those reading along, I removed an assertion from `spec/features/dashboard/collection_spec.rb`. Why? Because the test was creating one type of admin set, and the queries for what to show was filtering on other kinds of admin sets. Ultimatley creating a false assertion. * ♻️ Fixing spec * Adding case statement * ☑️ Fix stubbing * ♻️ Favor helper method * ♻️ Favor dynamic value * Favor helper method over instance variable * 🐛 Favor helper method over instance variable Prior to this commit, if the `update` failed, we would not have set the `@collection_type` instance variable. Which would mean that we'd raise a `NoMethodError` on `NilClass` in the `app/views/hyrax/dashboard/collections/_form_share_table.html.erb` view. By favoring a helper method, we no longer require that the edit (nor create) call the collection_type method. Below is the the only view references for `@collection_type`, so the helper_method appears to be adequate. ```shell ❯ rg "@collection_type[\. ]" app/views app/views/hyrax/dashboard/collections/_form_share_table.html.erb 5:

<%= t(".#{access}.help_with_works", type_title: @collection_type.title) if @collection_type.share_applies_to_new_works? && access != 'depositors' %>

app/views/hyrax/admin/collection_types/_form.html.erb 22: <% if @collection_type.admin_set? %> ``` * Adding Hyrax.config.file_set_model * Update app/models/hyrax/model_registry.rb * 🧹 An empty commit --------- Co-authored-by: Daniel Pierce --- app/controllers/hyrax/file_sets_controller.rb | 1 + .../hyrax/my/collections_controller.rb | 2 +- app/models/concerns/hyrax/ability.rb | 2 +- .../hyrax/ability/admin_set_ability.rb | 41 ++----- .../hyrax/ability/collection_ability.rb | 45 +++---- .../concerns/hyrax/solr_document_behavior.rb | 12 +- app/models/hyrax/model_registry.rb | 111 ++++++++++++++++++ .../hyrax/pcdm_member_presenter_factory.rb | 2 +- .../hyrax/admin_set_search_builder.rb | 2 +- .../dashboard/collections_search_builder.rb | 8 +- .../hyrax/exposed_models_relation.rb | 2 +- .../hyrax/file_set_search_builder.rb | 2 +- app/search_builders/hyrax/filter_by_type.rb | 5 +- .../hyrax/my/collections_search_builder.rb | 4 +- .../hyrax/edit_permissions_service.rb | 2 +- .../hyrax/statistics/collections/over_time.rb | 2 +- lib/hyrax/configuration.rb | 14 +++ lib/hyrax/resource_sync/change_list_writer.rb | 2 +- .../resource_sync/resource_list_writer.rb | 2 +- spec/features/dashboard/collection_spec.rb | 4 +- spec/models/hyrax/model_registry_spec.rb | 32 +++++ ...in_admin_set_member_search_builder_spec.rb | 2 +- .../hyrax/admin_set_search_builder_spec.rb | 6 +- .../hyrax/collection_search_builder_spec.rb | 3 +- .../collections_search_builder_spec.rb | 7 +- .../my/collections_search_builder_spec.rb | 11 +- .../single_admin_set_search_builder_spec.rb | 2 +- .../hyrax/work_search_builder_spec.rb | 2 +- 28 files changed, 228 insertions(+), 102 deletions(-) create mode 100644 app/models/hyrax/model_registry.rb create mode 100644 spec/models/hyrax/model_registry_spec.rb diff --git a/app/controllers/hyrax/file_sets_controller.rb b/app/controllers/hyrax/file_sets_controller.rb index e7f1423666..31c63edaef 100644 --- a/app/controllers/hyrax/file_sets_controller.rb +++ b/app/controllers/hyrax/file_sets_controller.rb @@ -137,6 +137,7 @@ def parent(file_set: curation_concern) @parent ||= case file_set when Hyrax::FileSet + # TODO: Add Hyrax::FileSet#parent method Hyrax.query_service.find_parents(resource: file_set).first else file_set.parent diff --git a/app/controllers/hyrax/my/collections_controller.rb b/app/controllers/hyrax/my/collections_controller.rb index c1565485c2..787e09fd85 100644 --- a/app/controllers/hyrax/my/collections_controller.rb +++ b/app/controllers/hyrax/my/collections_controller.rb @@ -12,7 +12,7 @@ def self.configure_facets config.add_facet_field Hyrax.config.collection_type_index_field, helper_method: :collection_type_label, limit: 5, pivot: ['has_model_ssim', Hyrax.config.collection_type_index_field], - skip_item: ->(item) { item.value == Hyrax.config.collection_class.to_s }, + skip_item: ->(item) { Hyrax::ModelRegistry.collection_rdf_representations.include?(item.value) }, label: I18n.t('hyrax.dashboard.my.heading.collection_type') # This causes AdminSets to also be shown with the Collection Type label config.add_facet_field 'has_model_ssim', diff --git a/app/models/concerns/hyrax/ability.rb b/app/models/concerns/hyrax/ability.rb index da0408980c..b0cd0e1c7a 100644 --- a/app/models/concerns/hyrax/ability.rb +++ b/app/models/concerns/hyrax/ability.rb @@ -421,7 +421,7 @@ def user_is_depositor?(document_id) end def curation_concerns_models - [::FileSet, ::Hyrax::FileSet, Hyrax.config.collection_class] + Hyrax.config.curation_concerns + Hyrax::ModelRegistry.collection_classes + Hyrax::ModelRegistry.file_set_classes + Hyrax::ModelRegistry.work_classes end def can_review_submissions? diff --git a/app/models/concerns/hyrax/ability/admin_set_ability.rb b/app/models/concerns/hyrax/ability/admin_set_ability.rb index f12db7ec27..da8ad5f97d 100644 --- a/app/models/concerns/hyrax/ability/admin_set_ability.rb +++ b/app/models/concerns/hyrax/ability/admin_set_ability.rb @@ -3,49 +3,34 @@ module Hyrax module Ability module AdminSetAbility def admin_set_abilities # rubocop:disable Metrics/MethodLength, Metrics/AbcSize + models = Hyrax::ModelRegistry.admin_set_classes if admin? - can :manage, [AdminSet, Hyrax::AdministrativeSet] - can :manage_any, AdminSet - can :manage_any, Hyrax::AdministrativeSet - can :create_any, AdminSet - can :create_any, Hyrax::AdministrativeSet - can :view_admin_show_any, AdminSet - can :view_admin_show_any, Hyrax::AdministrativeSet + can :manage, models + can :manage_any, models + can :create_any, models + can :view_admin_show_any, models else - if Hyrax::Collections::PermissionsService.can_manage_any_admin_set?(ability: self) - can :manage_any, AdminSet - can :manage_any, Hyrax::AdministrativeSet - end + can :manage_any, models if Hyrax::Collections::PermissionsService.can_manage_any_admin_set?(ability: self) if Hyrax::CollectionTypes::PermissionsService.can_create_admin_set_collection_type?(ability: self) - can :create, [AdminSet, Hyrax::AdministrativeSet] - can :create_any, AdminSet - can :create_any, Hyrax::AdministrativeSet - end - if Hyrax::Collections::PermissionsService.can_view_admin_show_for_any_admin_set?(ability: self) - can :view_admin_show_any, AdminSet - can :view_admin_show_any, Hyrax::AdministrativeSet + can :create, models + can :create_any, models end + can :view_admin_show_any, models if Hyrax::Collections::PermissionsService.can_view_admin_show_for_any_admin_set?(ability: self) # [:edit, :update, :destroy] for AdminSet is controlled by Hydra::Ability #edit_permissions - can [:edit, :update, :destroy], Hyrax::AdministrativeSet do |admin_set| # for test by solr_doc, see solr_document_ability.rb + can [:edit, :update, :destroy], models do |admin_set| # for test by solr_doc, see solr_document_ability.rb test_edit(admin_set.id) end - can :deposit, AdminSet do |admin_set| # for test by solr_doc, see collection_ability.rb - Hyrax::Collections::PermissionsService.can_deposit_in_collection?(ability: self, collection_id: admin_set.id) - end - can :deposit, Hyrax::AdministrativeSet do |admin_set| # for test by solr_doc, see collection_ability.rb + can :deposit, models do |admin_set| # for test by solr_doc, see collection_ability.rb Hyrax::Collections::PermissionsService.can_deposit_in_collection?(ability: self, collection_id: admin_set.id) end - can :view_admin_show, AdminSet do |admin_set| # admin show page # for test by solr_doc, see collection_ability.rb - Hyrax::Collections::PermissionsService.can_view_admin_show_for_collection?(ability: self, collection_id: admin_set.id) - end - can :view_admin_show, Hyrax::AdministrativeSet do |admin_set| # admin show page # for test by solr_doc, see collection_ability.rb + can :view_admin_show, models do |admin_set| # admin show page # for test by solr_doc, see collection_ability.rb Hyrax::Collections::PermissionsService.can_view_admin_show_for_collection?(ability: self, collection_id: admin_set.id) end # [:read] for AdminSet is controlled by Hydra::Ability #read_permissions - can :read, Hyrax::AdministrativeSet do |admin_set| # admin show page # for test by solr_doc, see collection_ability.rb + can :read, models do |admin_set| # admin show page # for test by solr_doc, see collection_ability.rb test_read(admin_set.id) end end diff --git a/app/models/concerns/hyrax/ability/collection_ability.rb b/app/models/concerns/hyrax/ability/collection_ability.rb index c9ae906be6..6243b3ec2e 100644 --- a/app/models/concerns/hyrax/ability/collection_ability.rb +++ b/app/models/concerns/hyrax/ability/collection_ability.rb @@ -3,40 +3,33 @@ module Hyrax module Ability module CollectionAbility def collection_abilities # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity - models = [Hyrax::PcdmCollection, Hyrax.config.collection_class].uniq + models = Hyrax::ModelRegistry.collection_classes if admin? - models.each do |collection_model| - can :manage, collection_model - can :manage_any, collection_model - can :create_any, collection_model - can :view_admin_show_any, collection_model - end + can :manage, models + can :manage_any, models + can :create_any, models + can :view_admin_show_any, models else - models.each { |collection_model| can :manage_any, collection_model } if - Hyrax::Collections::PermissionsService.can_manage_any_collection?(ability: self) + can :manage_any, models if Hyrax::Collections::PermissionsService.can_manage_any_collection?(ability: self) - models.each { |collection_model| can :create_any, collection_model } if - Hyrax::CollectionTypes::PermissionsService.can_create_any_collection_type?(ability: self) + can :create_any, models if Hyrax::CollectionTypes::PermissionsService.can_create_any_collection_type?(ability: self) - models.each { |collection_model| can :view_admin_show_any, collection_model } if - Hyrax::Collections::PermissionsService.can_view_admin_show_for_any_collection?(ability: self) + can :view_admin_show_any, models if Hyrax::Collections::PermissionsService.can_view_admin_show_for_any_collection?(ability: self) - models.each do |collection_model| - can [:edit, :update, :destroy], collection_model do |collection| - test_edit(collection.id) - end + can [:edit, :update, :destroy], models do |collection| + test_edit(collection.id) + end - can :deposit, collection_model do |collection| - Hyrax::Collections::PermissionsService.can_deposit_in_collection?(ability: self, collection_id: collection.id) - end + can :deposit, models do |collection| + Hyrax::Collections::PermissionsService.can_deposit_in_collection?(ability: self, collection_id: collection.id) + end - can :view_admin_show, collection_model do |collection| # admin show page - Hyrax::Collections::PermissionsService.can_view_admin_show_for_collection?(ability: self, collection_id: collection.id) - end + can :view_admin_show, models do |collection| # admin show page + Hyrax::Collections::PermissionsService.can_view_admin_show_for_collection?(ability: self, collection_id: collection.id) + end - can :read, collection_model do |collection| # public show page - test_read(collection.id) - end + can :read, models do |collection| # public show page + test_read(collection.id) end can :deposit, ::SolrDocument do |solr_doc| diff --git a/app/models/concerns/hyrax/solr_document_behavior.rb b/app/models/concerns/hyrax/solr_document_behavior.rb index d77ad2a267..a39eaac2de 100644 --- a/app/models/concerns/hyrax/solr_document_behavior.rb +++ b/app/models/concerns/hyrax/solr_document_behavior.rb @@ -53,28 +53,25 @@ def to_model ## # @return [Boolean] def collection? - hydra_model == Hyrax.config.collection_class || - ("Collection".safe_constantize == hydra_model) + Hyrax::ModelRegistry.collection_classes.include?(hydra_model) end ## # @return [Boolean] def file_set? - hydra_model == Hyrax::FileSet || - ("::FileSet".safe_constantize == hydra_model) + Hyrax::ModelRegistry.file_set_classes.include?(hydra_model) end ## # @return [Boolean] def admin_set? - (hydra_model == Hyrax.config.admin_set_class) || - ("AdminSet".safe_constantize == hydra_model) + Hyrax::ModelRegistry.admin_set_classes.include?(hydra_model) end ## # @return [Boolean] def work? - Hyrax.config.curation_concerns.include? hydra_model + Hyrax::ModelRegistry.work_classes.include?(hydra_model) end # Method to return the model @@ -89,6 +86,7 @@ def depositor(default = '') end def creator + # TODO: should we replace "hydra_model == AdminSet" with by #admin_set? solr_term = hydra_model == AdminSet ? "creator_ssim" : "creator_tesim" fetch(solr_term, []) end diff --git a/app/models/hyrax/model_registry.rb b/app/models/hyrax/model_registry.rb new file mode 100644 index 0000000000..37c8a8b07b --- /dev/null +++ b/app/models/hyrax/model_registry.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +module Hyrax + ## + # There are four conceptual high-level models for the metadata modeling: + # + # - AdminSet :: Where things are "physically" stored. + # - Collection :: What things "logically" belong to. + # - Work :: What the thing is about; these are often sub-divided into different work types + # (e.g. an Article, a Monograph, etc.) + # - FileSet :: Artifacts that further describe the thing. + # + # The purpose of this module is to provide an overview and map of the modeling concepts. This is + # envisioned as a module to interrogate to see how Hyrax conceptually organizes your models. + # + # For each of the above high-level models the {Hyrax::ModelRegistry} implements two methods: + # + # - _classes :: These are the Ruby constants that represent the conceptual models. + # - _rdf_representations :: These are the stored strings that help us describe what # + # - the data models. Very useful in our Solr queries when we want to filter/query on the + # - "has_model_ssim" attribute. + # + # Consider that an {AdminSet} and a {Hyrax::AdministrativeSet} conceptually model the same thing; + # the latter implements via Valkyrie and the former via ActiveFedora. + # + # @note Due to the shift from ActiveFedora to Valkyrie, we are at a crossroads where we might have + # multiple models for the versions: an ActiveFedora AdminSet and a Valkyrie AdminSet. + class ModelRegistry + ## + # @return [Array] + def self.admin_set_class_names + ["::AdminSet", "::Hyrax::AdministrativeSet", Hyrax.config.admin_set_model].uniq + end + + ## + # @return [Array] + def self.admin_set_classes + classes_from(admin_set_class_names) + end + + ## + # @return [Array] + def self.admin_set_rdf_representations + rdf_representations_from(admin_set_classes) + end + + ## + # @return [Array] + def self.collection_class_names + ["::Collection", "::Hyrax::PcdmCollection", Hyrax.config.collection_model].uniq + end + + ## + # @return [Array] + def self.collection_classes + classes_from(collection_class_names) + end + + def self.collection_rdf_representations + rdf_representations_from(collection_classes) + end + + def self.collection_has_model_ssim + collection_rdf_representations + end + + ## + # @return [Array] + def self.file_set_class_names + ["::FileSet", "::Hyrax::FileSet", Hyrax.config.file_set_model].uniq + end + + def self.file_set_classes + classes_from(file_set_class_names) + end + + ## + # @return [Array] + def self.file_set_rdf_representations + rdf_representations_from(file_set_classes) + end + + ## + # @return [Array] + # + # @todo Consider the Wings::ModelRegistry and how we perform mappings. + def self.work_class_names + Hyrax.config.registered_curation_concern_types + end + + def self.work_classes + classes_from(work_class_names) + end + + ## + # @return [Array] + def self.work_rdf_representations + rdf_representations_from(work_classes) + end + + def self.classes_from(strings) + strings.map(&:safe_constantize).flatten.uniq + end + private_class_method :classes_from + + def self.rdf_representations_from(klasses) + klasses.map { |klass| klass.respond_to?(:to_rdf_represntation) ? klass.to_rdf_represntation : klass.name }.uniq + end + private_class_method :rdf_representations_from + end +end diff --git a/app/presenters/hyrax/pcdm_member_presenter_factory.rb b/app/presenters/hyrax/pcdm_member_presenter_factory.rb index 3bcd8155ca..486fac4be6 100644 --- a/app/presenters/hyrax/pcdm_member_presenter_factory.rb +++ b/app/presenters/hyrax/pcdm_member_presenter_factory.rb @@ -97,7 +97,7 @@ def work_presenters # @return def presenter_for(document:, ability:) case document['has_model_ssim'].first - when Hyrax::FileSet.name, ::FileSet.name # ActiveFedora FileSet within a Valkyrie Resource + when *Hyrax::ModelRegistry.file_set_rdf_representations Hyrax::FileSetPresenter.new(document, ability) else Hyrax::WorkShowPresenter.new(document, ability) diff --git a/app/search_builders/hyrax/admin_set_search_builder.rb b/app/search_builders/hyrax/admin_set_search_builder.rb index f52c609ae2..eba4157b3f 100644 --- a/app/search_builders/hyrax/admin_set_search_builder.rb +++ b/app/search_builders/hyrax/admin_set_search_builder.rb @@ -14,7 +14,7 @@ def initialize(context, access) # This overrides the models in FilterByType def models - [::AdminSet, Hyrax::AdministrativeSet] + Hyrax::ModelRegistry.admin_set_classes end # Overrides Hydra::AccessControlsEnforcement diff --git a/app/search_builders/hyrax/dashboard/collections_search_builder.rb b/app/search_builders/hyrax/dashboard/collections_search_builder.rb index a780f1bf6f..13da5105d3 100644 --- a/app/search_builders/hyrax/dashboard/collections_search_builder.rb +++ b/app/search_builders/hyrax/dashboard/collections_search_builder.rb @@ -9,7 +9,7 @@ class CollectionsSearchBuilder < Hyrax::CollectionSearchBuilder # This overrides the models in FilterByType def models - [::AdminSet, Hyrax::AdministrativeSet, "Collection".safe_constantize, Hyrax.config.collection_class].uniq.compact + Hyrax::ModelRegistry.admin_set_classes + Hyrax::ModelRegistry.collection_classes end # adds a filter to exclude collections and admin sets created by the @@ -18,9 +18,11 @@ def models def show_only_managed_collections_for_non_admins(solr_parameters) return if current_ability.admin? clauses = [ - '-' + ActiveFedora::SolrQueryBuilder.construct_query_for_rel(depositor: current_user_key), - '-' + ActiveFedora::SolrQueryBuilder.construct_query_for_rel(has_model: Hyrax.config.admin_set_model, creator: current_user_key) + '-' + ActiveFedora::SolrQueryBuilder.construct_query_for_rel(depositor: current_user_key) ] + Hyrax::ModelRegistry.admin_set_rdf_representations.each do |has_model| + clauses += ['-' + ActiveFedora::SolrQueryBuilder.construct_query_for_rel(has_model: has_model, creator: current_user_key)] + end solr_parameters[:fq] ||= [] solr_parameters[:fq] += ["(#{clauses.join(' OR ')})"] end diff --git a/app/search_builders/hyrax/exposed_models_relation.rb b/app/search_builders/hyrax/exposed_models_relation.rb index e9f3daa298..4b00eb204a 100644 --- a/app/search_builders/hyrax/exposed_models_relation.rb +++ b/app/search_builders/hyrax/exposed_models_relation.rb @@ -3,7 +3,7 @@ module Hyrax # A relation that scopes to all user visible models (e.g. works + collections + file sets) class ExposedModelsRelation < AbstractTypeRelation def allowable_types - (Hyrax.config.curation_concerns + [Hyrax.config.collection_class, ::Collection, ::FileSet]).uniq + Hyrax::ModelRegistry.work_classes + Hyrax::ModelRegistry.collection_classes + Hyrax::ModelRegistry.file_set_classes end end end diff --git a/app/search_builders/hyrax/file_set_search_builder.rb b/app/search_builders/hyrax/file_set_search_builder.rb index 338329a6f7..8e5cb9ab08 100644 --- a/app/search_builders/hyrax/file_set_search_builder.rb +++ b/app/search_builders/hyrax/file_set_search_builder.rb @@ -5,7 +5,7 @@ class FileSetSearchBuilder < ::SearchBuilder # This overrides the models in FilterByType def models - [::FileSet, ::Hyrax::FileSet] + Hyrax::ModelRegistry.file_set_classes end end end diff --git a/app/search_builders/hyrax/filter_by_type.rb b/app/search_builders/hyrax/filter_by_type.rb index 61a9c1f5c1..8905248edb 100644 --- a/app/search_builders/hyrax/filter_by_type.rb +++ b/app/search_builders/hyrax/filter_by_type.rb @@ -43,7 +43,7 @@ def generic_type_field # types from appearing in search results # @return [Array] the list of work types to include in searches def work_types - Hyrax.config.curation_concerns + Hyrax::ModelRegistry.work_classes end def work_classes @@ -53,7 +53,8 @@ def work_classes def collection_classes return [] if only_works? - ["Collection".safe_constantize, Hyrax.config.collection_class].uniq.compact + + Hyrax::ModelRegistry.collection_classes end end end diff --git a/app/search_builders/hyrax/my/collections_search_builder.rb b/app/search_builders/hyrax/my/collections_search_builder.rb index 49e8eca97d..de96774fe4 100644 --- a/app/search_builders/hyrax/my/collections_search_builder.rb +++ b/app/search_builders/hyrax/my/collections_search_builder.rb @@ -18,7 +18,7 @@ def show_only_collections_deposited_by_current_user(solr_parameters) # This overrides the models in FilterByType # @return [Array] a list of classes to include def models - [::AdminSet, Hyrax::AdministrativeSet, "Collection".safe_constantize, Hyrax.config.collection_class].uniq.compact + Hyrax::ModelRegistry.admin_set_classes + Hyrax::ModelRegistry.collection_classes end private @@ -26,7 +26,7 @@ def models def query_for_my_collections query_service = Hyrax::SolrQueryService.new query_service.with_field_pairs(field_pairs: { depositor_ssim: current_user_key }, type: 'terms') - query_service.with_field_pairs(field_pairs: { has_model_ssim: Hyrax.config.admin_set_model, + query_service.with_field_pairs(field_pairs: { has_model_ssim: Hyrax::ModelRegistry.admin_set_rdf_representations.join(','), creator_ssim: current_user_key }, type: 'terms') query_service.build(join_with: 'OR') end diff --git a/app/services/hyrax/edit_permissions_service.rb b/app/services/hyrax/edit_permissions_service.rb index 3858bda285..be254a7083 100644 --- a/app/services/hyrax/edit_permissions_service.rb +++ b/app/services/hyrax/edit_permissions_service.rb @@ -215,7 +215,7 @@ def object_unauthorized_collection_ids # @todo Refactor inner working of code as there's lots of branching logic with potential hidden assumptions. def qualifies_as_unauthorized_collection?(resource:) case resource - when AdminSet, Hyrax::AdministrativeSet + when *Hyrax::ModelRegistry.admin_set_classes # Prior to this refactor, we looked at AdminSet only; However with the advent of the # Hyrax::AdministrativeSet, we need to test both cases. true diff --git a/app/services/hyrax/statistics/collections/over_time.rb b/app/services/hyrax/statistics/collections/over_time.rb index a6d4427663..a9e6ba0c2d 100644 --- a/app/services/hyrax/statistics/collections/over_time.rb +++ b/app/services/hyrax/statistics/collections/over_time.rb @@ -7,7 +7,7 @@ class OverTime < Statistics::OverTime def relation AbstractTypeRelation - .new(allowable_types: [Hyrax.config.collection_class]) + .new(allowable_types: Hyrax::ModelRegistry.collection_classes) end end end diff --git a/lib/hyrax/configuration.rb b/lib/hyrax/configuration.rb index ba4b963120..269b1fac5a 100644 --- a/lib/hyrax/configuration.rb +++ b/lib/hyrax/configuration.rb @@ -267,6 +267,20 @@ def derivative_services @derivative_services ||= [Hyrax::FileSetDerivativesService] end + attr_writer :file_set_model + ## + # @return [#constantize] a string representation of the admin set + # model + def file_set_model + @file_set_model ||= 'Hyrax::FileSet' + end + + ## + # @return [Class] the configured admin set model class + def file_set_class + file_set_model.constantize + end + ## # @!attribute [rw] file_set_file_service # @return [Class] implementer of {Hyrax::FileSetFileService} diff --git a/lib/hyrax/resource_sync/change_list_writer.rb b/lib/hyrax/resource_sync/change_list_writer.rb index 57e177efb9..187d4f95fe 100644 --- a/lib/hyrax/resource_sync/change_list_writer.rb +++ b/lib/hyrax/resource_sync/change_list_writer.rb @@ -62,7 +62,7 @@ def build_changes(xml) def build_resources(xml, doc_set) doc_set.each do |doc| model = doc.fetch('has_model_ssim', []).first.safe_constantize - if model.try(:collection?) || model == Hyrax.config.collection_class + if model.try(:collection?) || Hyrax::ModelRegistry.collection_classes.include?(model) build_resource(xml, doc, model, hyrax_routes) else build_resource(xml, doc, model, main_app_routes) diff --git a/lib/hyrax/resource_sync/resource_list_writer.rb b/lib/hyrax/resource_sync/resource_list_writer.rb index fd8c212c42..9be15d7656 100644 --- a/lib/hyrax/resource_sync/resource_list_writer.rb +++ b/lib/hyrax/resource_sync/resource_list_writer.rb @@ -32,7 +32,7 @@ def builder end end - def build_collections(xml, searcher: AbstractTypeRelation.new(allowable_types: [::Collection, Hyrax.config.collection_class])) + def build_collections(xml, searcher: AbstractTypeRelation.new(allowable_types: Hyrax::ModelRegistry.collection_classes)) searcher.search_in_batches(public_access) do |doc_set| build_resources(xml, doc_set, hyrax_routes) end diff --git a/spec/features/dashboard/collection_spec.rb b/spec/features/dashboard/collection_spec.rb index d1aae175a9..69cd4ce0d8 100644 --- a/spec/features/dashboard/collection_spec.rb +++ b/spec/features/dashboard/collection_spec.rb @@ -57,8 +57,8 @@ href: /#{solr_model_field}.+#{Regexp.escape(CGI.escape(Hyrax.config.admin_set_model))}/ expect(page).not_to have_link user_collection_type.title, href: /#{solr_gid_field}.+#{Regexp.escape(CGI.escape(user_collection_type.to_global_id.to_s))}/ - expect(page).not_to have_link 'Collection', - href: /#{solr_model_field}.+#{Regexp.escape('Collection')}/ + expect(page).not_to have_link Hyrax.config.collection_model, + href: /#{solr_model_field}.+#{Regexp.escape(Hyrax.config.collection_model)}/ end end diff --git a/spec/models/hyrax/model_registry_spec.rb b/spec/models/hyrax/model_registry_spec.rb new file mode 100644 index 0000000000..6eb31ab287 --- /dev/null +++ b/spec/models/hyrax/model_registry_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Hyrax::ModelRegistry do + %i[admin_set file_set collection work].each do |type| + describe ".#{type}_class_names" do + subject { described_class.public_send("#{type}_class_names") } + + it { is_expected.to be_a(Array) } + it { is_expected.to be_present } + it { is_expected.to all(be_kind_of(String)) } + end + + describe ".#{type}_classes" do + subject { described_class.public_send("#{type}_classes") } + + it { is_expected.to be_a(Array) } + it { is_expected.to be_present } + + it { is_expected.to all(be_kind_of(Class)) } + end + + describe ".#{type}_rdf_representations" do + subject { described_class.public_send("#{type}_rdf_representations") } + + it { is_expected.to be_a(Array) } + it { is_expected.to be_present } + it { is_expected.to all(be_kind_of(String)) } + end + end +end diff --git a/spec/search_builders/hyrax/admin_admin_set_member_search_builder_spec.rb b/spec/search_builders/hyrax/admin_admin_set_member_search_builder_spec.rb index d8f1bb87f0..d285ad7abf 100644 --- a/spec/search_builders/hyrax/admin_admin_set_member_search_builder_spec.rb +++ b/spec/search_builders/hyrax/admin_admin_set_member_search_builder_spec.rb @@ -9,7 +9,7 @@ let(:solr_params) { { fq: [] } } describe '#filter_models' do - before { allow(Hyrax.config).to receive(:curation_concerns).and_return([Monograph]) } + before { allow(Hyrax::ModelRegistry).to receive(:work_classes).and_return([Monograph]) } it 'searches for valid work types' do expect(builder.filter_models(solr_params)) diff --git a/spec/search_builders/hyrax/admin_set_search_builder_spec.rb b/spec/search_builders/hyrax/admin_set_search_builder_spec.rb index 1b4ba54cf6..acbed73c43 100644 --- a/spec/search_builders/hyrax/admin_set_search_builder_spec.rb +++ b/spec/search_builders/hyrax/admin_set_search_builder_spec.rb @@ -17,7 +17,7 @@ let(:solr_params) { { fq: [] } } it 'adds AdminSet to query' do - expect(solr_params[:fq].first).to include('{!terms f=has_model_ssim}AdminSet') + expect(solr_params[:fq].first).to include("{!terms f=has_model_ssim}#{Hyrax::ModelRegistry.admin_set_rdf_representations.join(',')}") end end @@ -61,7 +61,7 @@ expect(subject['fq']).to eq ["edit_access_person_ssim:#{user.user_key} OR " \ "discover_access_person_ssim:#{user.user_key} OR " \ "read_access_person_ssim:#{user.user_key}", - "{!terms f=has_model_ssim}AdminSet,Hyrax::AdministrativeSet"] + "{!terms f=has_model_ssim}#{Hyrax::ModelRegistry.admin_set_rdf_representations.join(',')}"] end end @@ -73,7 +73,7 @@ end it 'is successful' do - expect(subject['fq']).to eq ["{!terms f=id}7,8", "{!terms f=has_model_ssim}AdminSet,Hyrax::AdministrativeSet"] + expect(subject['fq']).to eq ["{!terms f=id}7,8", "{!terms f=has_model_ssim}#{Hyrax::ModelRegistry.admin_set_rdf_representations.join(',')}"] end end end diff --git a/spec/search_builders/hyrax/collection_search_builder_spec.rb b/spec/search_builders/hyrax/collection_search_builder_spec.rb index 68e276e201..3f25bba278 100644 --- a/spec/search_builders/hyrax/collection_search_builder_spec.rb +++ b/spec/search_builders/hyrax/collection_search_builder_spec.rb @@ -11,8 +11,7 @@ describe '#models' do its(:models) do - is_expected - .to contain_exactly(*[::Collection, Hyrax.config.collection_class].uniq) + is_expected.to match_array(Hyrax::ModelRegistry.collection_classes) end context 'when collection class is not ::Collection' do diff --git a/spec/search_builders/hyrax/dashboard/collections_search_builder_spec.rb b/spec/search_builders/hyrax/dashboard/collections_search_builder_spec.rb index 26883fa192..db5e5d03b3 100644 --- a/spec/search_builders/hyrax/dashboard/collections_search_builder_spec.rb +++ b/spec/search_builders/hyrax/dashboard/collections_search_builder_spec.rb @@ -15,7 +15,7 @@ describe '#models' do subject { builder.models } - it { is_expected.to eq([AdminSet, Hyrax::AdministrativeSet, ::Collection, Hyrax.config.collection_class].uniq) } + it { is_expected.to eq(Hyrax::ModelRegistry.admin_set_classes + Hyrax::ModelRegistry.collection_classes) } end describe ".default_processor_chain" do @@ -32,16 +32,13 @@ describe "#show_only_managed_collections_for_non_admins" do let(:solr_params) { Blacklight::Solr::Request.new } - let(:admin_text) { Hyrax.config.disable_wings ? 'Hyrax::AdministrativeSet' : 'AdminSet' } before do builder.show_only_managed_collections_for_non_admins(solr_params) end it "has filter that excludes depositor" do - expect(solr_params[:fq]).to eq( - ["(-_query_:\"{!raw f=depositor_ssim}#{user.user_key}\" OR -(_query_:\"{!raw f=has_model_ssim}#{admin_text}\" AND _query_:\"{!raw f=creator_ssim}#{user.user_key}\"))"] - ) + expect(solr_params[:fq].first).to match(%r{\(-_query_:\"{!raw f=depositor_ssim}#{user.user_key}\" OR }) end context "as admin" do diff --git a/spec/search_builders/hyrax/my/collections_search_builder_spec.rb b/spec/search_builders/hyrax/my/collections_search_builder_spec.rb index 93f63da2af..e4c573033d 100644 --- a/spec/search_builders/hyrax/my/collections_search_builder_spec.rb +++ b/spec/search_builders/hyrax/my/collections_search_builder_spec.rb @@ -17,14 +17,7 @@ subject { builder.models } it do - is_expected.to include(AdminSet, - Hyrax::AdministrativeSet, - Hyrax.config.collection_class) - end - - context 'when collection class is something other than ::Collection' do - before { allow(Hyrax.config).to receive(:collection_model).and_return('Hyrax::PcdmCollection') } - it { is_expected.to contain_exactly(AdminSet, Hyrax::AdministrativeSet, ::Collection, Hyrax::PcdmCollection) } + is_expected.to match_array(Hyrax::ModelRegistry.admin_set_classes + Hyrax::ModelRegistry.collection_classes) end end @@ -42,7 +35,7 @@ it "has filter that excludes depositor" do subject expect(solr_params[:fq]).to eq ["(_query_:\"{!terms f=depositor_ssim}#{user.user_key}\" " \ - "OR (_query_:\"{!terms f=has_model_ssim}#{admin_klass}\" " \ + "OR (_query_:\"{!terms f=has_model_ssim}#{Hyrax::ModelRegistry.admin_set_rdf_representations.join(',')}\" " \ "AND _query_:\"{!terms f=creator_ssim}#{user.user_key}\"))"] end end diff --git a/spec/search_builders/hyrax/single_admin_set_search_builder_spec.rb b/spec/search_builders/hyrax/single_admin_set_search_builder_spec.rb index a4fcabae30..0f176594b9 100644 --- a/spec/search_builders/hyrax/single_admin_set_search_builder_spec.rb +++ b/spec/search_builders/hyrax/single_admin_set_search_builder_spec.rb @@ -16,7 +16,7 @@ it do is_expected.to match_array ["", - "{!terms f=has_model_ssim}AdminSet,Hyrax::AdministrativeSet"] + "{!terms f=has_model_ssim}#{Hyrax::ModelRegistry.admin_set_rdf_representations.join(',')}"] end end end diff --git a/spec/search_builders/hyrax/work_search_builder_spec.rb b/spec/search_builders/hyrax/work_search_builder_spec.rb index 5a5ce28033..f26d19cbbe 100644 --- a/spec/search_builders/hyrax/work_search_builder_spec.rb +++ b/spec/search_builders/hyrax/work_search_builder_spec.rb @@ -13,7 +13,7 @@ end let(:class_filter_string) do - [Monograph, Collection, Hyrax.config.collection_class].uniq.join(',') + ([Monograph.to_s] + Hyrax::ModelRegistry.collection_rdf_representations).uniq.join(',') end describe "#query" do From d8b0b62f6cd83a63fd9c85ecdf615ab1391ca415 Mon Sep 17 00:00:00 2001 From: Kirk Wang Date: Mon, 19 Feb 2024 05:46:13 -0800 Subject: [PATCH 2/3] Add conditional to `.fileset_for_directives` (#6695) We are noticing that when ingesting a PDF, the `ValkyrieCreateDerivativesJob` is failing because the file_set doesn't get found in `ValkyriePersistDerivatives.fileset_for_directives` when the directives[:url] is an id instead of an actual uri, see `FileSetDerivativesService#extract_full_text`. I'm suggesting that we check if we have a "/" and if not we assume that it is an id so we can just look for that. --- app/services/hyrax/valkyrie_persist_derivatives.rb | 13 ++++++++++--- .../hyrax/valkyrie_persist_derivatives_spec.rb | 12 ++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/app/services/hyrax/valkyrie_persist_derivatives.rb b/app/services/hyrax/valkyrie_persist_derivatives.rb index 910f5b24a3..78bccf322b 100644 --- a/app/services/hyrax/valkyrie_persist_derivatives.rb +++ b/app/services/hyrax/valkyrie_persist_derivatives.rb @@ -43,9 +43,16 @@ def self.call(stream, # @return [Hyrax::FileSet] def self.fileset_for_directives(directives) path = URI(directives.fetch(:url)).path - id = path.sub(Hyrax.config.derivatives_path.to_s, "") - .delete('/') - .match(/^(.*)-\w*(\.\w+)*$/) { |m| m[1] } + # checks if it's a file path, else assuming it is already an id + # Hyrax::FileSetDerivativesService#extract_full_text passes in the raw uri + # and not a derivative_url like the other derivative formats + id = if path.include?("/") + path.sub(Hyrax.config.derivatives_path.to_s, "") + .delete('/') + .match(/^(.*)-\w*(\.\w+)*$/) { |m| m[1] } + else + path + end raise "Could not extract fileset id from path #{path}" unless id Hyrax.metadata_adapter.query_service.find_by(id: id) diff --git a/spec/services/hyrax/valkyrie_persist_derivatives_spec.rb b/spec/services/hyrax/valkyrie_persist_derivatives_spec.rb index f91ff013c8..6973d5c868 100644 --- a/spec/services/hyrax/valkyrie_persist_derivatives_spec.rb +++ b/spec/services/hyrax/valkyrie_persist_derivatives_spec.rb @@ -90,5 +90,17 @@ .to raise_error(/Could not extract fileset id from path/) end end + + context 'with an id' do + let(:directives) do + { url: '123' } + end + + it 'extracts the id' do + expect(Hyrax.metadata_adapter.query_service) + .to receive(:find_by).with(id: '123') + described_class.fileset_for_directives(directives) + end + end end end From f0e3a3e5756a4a4247346eb7a02d8e7086bd5427 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Mon, 19 Feb 2024 17:48:00 -0500 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=90=9B=20Create=20admin=20set=20of=20?= =?UTF-8?q?configured=20type=20(#6701)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, we were assuming that we'd create a `Hyrax::AdministrativeSet`; however given the configuration option this could mean we were creating an administrative set of a type that we weren't expecting. --- app/services/hyrax/admin_set_create_service.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/services/hyrax/admin_set_create_service.rb b/app/services/hyrax/admin_set_create_service.rb index 722bc6ea2b..5b2e80b15c 100644 --- a/app/services/hyrax/admin_set_create_service.rb +++ b/app/services/hyrax/admin_set_create_service.rb @@ -84,12 +84,16 @@ def save_default? # Create an instance of `Hyrax::AdministrativeSet` with the suggested_id if supported. # @return [Hyrax::AdministrativeSet] the new admin set def create_admin_set(suggested_id:, title:) + # Leverage the configured admin class, if it is a Valkyrie resource, otherwise fallback. + # Until we have fully moved to Valkyrie, we will need this logic. Once past, we can + # use `Hyrax.config.admin_set_class` + klass = Hyrax.config.admin_set_class < Valkyrie::Resource ? Hyrax.config.admin_set_class : Hyrax::AdministrativeSet if suggested_id.blank? || Hyrax.config.disable_wings # allow persister to assign id - Hyrax::AdministrativeSet.new(title: Array.wrap(title)) + klass.new(title: Array.wrap(title)) else # use suggested_id - Hyrax::AdministrativeSet.new(id: suggested_id, title: Array.wrap(title)) + klass.new(id: suggested_id, title: Array.wrap(title)) end end