centralize the list of collection classes #5207

elrayle · 2021-10-19T13:35:01Z

This centralizes the list of collection classes to Hyrax.config.collection_classes.

Places where any collection class could be input or processing occurs across all collection classes were updated to use Hyrax.config.collection_classes. Remaining single references to ::Collection were updated to Hyrax.config.collection_class. These changes allow for almost all remaining uses of ::Collection to be removed.

Exceptions:

work_form which is only used with ActiveFedora
PermissionTemplate #collection which is deprecated and only works with ::Collection
Hyrax::Collections::MigrationService which performs a migration dependent on reading in the collection as a ::Collection

This PR does not update CollectionAbiility as it is being updated in PR #5206. Whichever PR is merged first, there will be a small fix required to get abilities to reference the collection_classes config instead of creating the list in the CollectionAbility class.

@samvera/hyrax-code-reviewers

no-reply · 2021-10-21T18:46:58Z

i think this is a good goal, but maybe needs some more consideration.

in some cases (e.g. on the search side) i think we want/need to continue to find indexed data under model ::Collection. in part this is because Wings indexes to the Solr core used by ActiveFedora using that Collection model. but it also seems nicer to downstream apps opting into valkyrie anyway---they may not want to force a large scale re-index as a prerequisite to going live with a config switch.

on the flip side, i think we've encountered some other cases where we want to include Hyrax::PcdmCollection in the config regardless of settings. you've covered this case here, but i think maybe this blanket approach is too blunt (or at least that's a possible interpretation of some of the failing tests?).

so far i've only been able to approach this case-by-case, but if we could understand the whole problem space, i think that would help move this forward.

elrayle · 2021-10-29T01:51:53Z

@no-reply See what you think about the new approach to determining the list of collection classes.

WAS

   def collection_classes
      [collection_model.constantize, Hyrax::PcdmCollection].uniq
    end

NEW APPROACH

   def collection_classes
      klasses = [collection_class, Hyrax::PcdmCollection]
      klasses << ::Collection if defined? ::Collection
      klasses.uniq
    end

no-reply · 2021-10-29T04:29:01Z

i think #5229 clears the remaining failing test in this PR.

lib/hyrax/configuration.rb

spec/search_builders/hyrax/work_search_builder_spec.rb

app/views/hyrax/collections/_media_display.html.erb

app/helpers/hyrax/file_set_helper.rb

elrayle · 2021-10-29T14:33:47Z

spec/search_builders/hyrax/admin_admin_set_member_search_builder_spec.rb

@@ -13,7 +13,7 @@

    it 'searches for valid work types' do
      expect(builder.filter_models(solr_params))
-        .to contain_exactly("{!terms f=has_model_ssim}Monograph,#{Hyrax.config.collection_class}")
+        .to contain_exactly("{!terms f=has_model_ssim}Monograph,#{Hyrax.collection_classes.join(',')}")


@no-reply This line is modified in PR #5229. Since this test passes with the change here, I'm not sure how the test passes in 5229 without including collection classes. Any thoughts on this?

#5229 is designed to make this pass in the context of #5169. it still tests the same behavior, just using a different approach that tests separately for each class we want to see included.

This centralizes the list of collection classes to `Hyrax.config.collection_classes`.

Places where any collection class could be input or processing occurs across all collection classes were updated to use `Hyrax.config.collection_classes`. Remaining single references to `::Collection` were updated to `Hyrax.config.collection_class`. These changes allow for almost all remaining uses of `::Collection` to be removed. Exceptions: * work_form which is only used with ActiveFedora * `PermissionTemplate #collection` which is deprecated and only works with `::Collection` * `Hyrax::Collections::MigrationService` which performs a migration dependent on reading in the collection as a `::Collection`

stale · 2022-01-09T01:30:31Z

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

cjcolvar

This looks like a good change to me. Is it still relevant? How bad of a rebase will it be to bring this up to date?

cjcolvar · 2022-02-09T21:20:31Z

app/views/hyrax/collections/_list_collections.html.erb

@@ -3,7 +3,7 @@
  <td></td>
  <td>
    <div class="media">
-      <span class="<%= Hyrax::ModelIcon.css_class_for(::Collection) %> collection-icon-small pull-left"></span>
+      <span class="<%= Hyrax::ModelIcon.css_class_for(Hyrax.config.collection_class) %> collection-icon-small pull-left"></span>


Should this be changed like the change in app/views/hyrax/collections/_media_display.html.erb?

Suggested change

<span class="<%= Hyrax::ModelIcon.css_class_for(Hyrax.config.collection_class) %> collection-icon-small pull-left"></span>

<span class="<%= collection_model_icon %> collection-icon-small pull-left"></span>

cjcolvar · 2022-02-09T21:21:44Z

app/helpers/hyrax/collections_helper.rb

@@ -24,6 +24,13 @@ def available_parent_collections_data(collection:)
      end.to_json
    end

+    ##
+    # @since 3.1.0


This should be updated to 3.4.0?

Suggested change

# @since 3.1.0

# @since 3.4.0

stale · 2022-04-17T01:17:38Z

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

elrayle marked this pull request as draft October 19, 2021 13:35

elrayle force-pushed the col_classes branch from 4e6ee70 to 78f6490 Compare October 19, 2021 13:50

elrayle mentioned this pull request Oct 29, 2021

search for legacy collections in statistics OverTime #5230

Closed

elrayle force-pushed the col_classes branch from ed369e2 to 5b230a2 Compare October 29, 2021 01:45

no-reply reviewed Oct 29, 2021

View reviewed changes

lib/hyrax/configuration.rb Outdated Show resolved Hide resolved

no-reply reviewed Oct 29, 2021

View reviewed changes

spec/search_builders/hyrax/work_search_builder_spec.rb Outdated Show resolved Hide resolved

no-reply reviewed Oct 29, 2021

View reviewed changes

app/views/hyrax/collections/_media_display.html.erb Outdated Show resolved Hide resolved

no-reply reviewed Oct 29, 2021

View reviewed changes

app/helpers/hyrax/file_set_helper.rb Outdated Show resolved Hide resolved

elrayle force-pushed the col_classes branch 3 times, most recently from 30be1c0 to bbedb91 Compare October 29, 2021 13:23

elrayle marked this pull request as ready for review October 29, 2021 14:30

elrayle commented Oct 29, 2021

View reviewed changes

elrayle mentioned this pull request Oct 29, 2021

test AdminAdminSetMemberSearchBuilder for collection search #5229

Merged

elrayle added 4 commits October 29, 2021 15:21

centralize the list of collection classes

6c2f8a2

This centralizes the list of collection classes to `Hyrax.config.collection_classes`.

update search builders to use config.collection_classes

ac2cb62

use helper for collection model’s icon

b13ebb4

elrayle force-pushed the col_classes branch from a424945 to b13ebb4 Compare October 29, 2021 19:23

stale bot added the stale label Jan 9, 2022

elrayle removed the stale label Jan 12, 2022

cjcolvar reviewed Feb 9, 2022

View reviewed changes

stale bot added the stale label Apr 17, 2022

stale bot closed this May 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

centralize the list of collection classes #5207

centralize the list of collection classes #5207

elrayle commented Oct 19, 2021

no-reply commented Oct 21, 2021

elrayle commented Oct 29, 2021

no-reply commented Oct 29, 2021

elrayle Oct 29, 2021

no-reply Oct 29, 2021

stale bot commented Jan 9, 2022

cjcolvar left a comment

cjcolvar Feb 9, 2022

cjcolvar Feb 9, 2022

stale bot commented Apr 17, 2022

	<span class="<%= Hyrax::ModelIcon.css_class_for(Hyrax.config.collection_class) %> collection-icon-small pull-left"></span>
	<span class="<%= collection_model_icon %> collection-icon-small pull-left"></span>

centralize the list of collection classes #5207

centralize the list of collection classes #5207

Conversation

elrayle commented Oct 19, 2021

no-reply commented Oct 21, 2021

elrayle commented Oct 29, 2021

no-reply commented Oct 29, 2021

elrayle Oct 29, 2021

Choose a reason for hiding this comment

no-reply Oct 29, 2021

Choose a reason for hiding this comment

stale bot commented Jan 9, 2022

cjcolvar left a comment

Choose a reason for hiding this comment

cjcolvar Feb 9, 2022

Choose a reason for hiding this comment

cjcolvar Feb 9, 2022

Choose a reason for hiding this comment

stale bot commented Apr 17, 2022