Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use configured adminset model in search builders #5423

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Jan 31, 2022

Fixes #5408

Instead of hardcoding the admin set class, use the configured Hyrax.config.admin_set_model.


FEEDBACK REQUESTED:

@no-reply @cjcolvar

This works for pure Valkyrie and pure ActiveFedora apps. It will not work for any apps that may have some solr docs with model AdminSet and some with model Hyrax::AdministrativeSet.

OPTIONS:

  • OPTION 1: Use the change in this PR. If this is the long term solution, then once apps start to migrate to Valkyrie, this would lead to a need to add a data migration for solr docs to update all docs with model AdminSet to have model Hyrax::AdministrativeSet instead.
  • OPTION 2: Update solr query to allow either model. Not a simple query. See notes below. Seems like the best to prevent the need to migrate solr docs once Valkyrie migrations start in the community. (Maybe migrations would be expected anyway?)
  • OPTION 3: Accept the PR as it is now and create an issue addressing OPTION 2's OR query or creating a migration script for solr docs. Fixes the functionality required for the MVP while acknowledging the need for a long term plan for how to deal with or prevent the need for solr doc migrations.
  • Other options?

Seems like there should be a way to include an OR that allows for either model. I have not been able to determine the way to write that query. See Issue #5408 for more information on the current query and attempts to make the query work with both models.

I confirmed that with this fix, the admin sets are in the list of collections on Dashboard -> My (and All) Collections for nurax-pg.

@samvera/hyrax-code-reviewers

@elrayle elrayle mentioned this pull request Jan 31, 2022
67 tasks
@elrayle
Copy link
Contributor Author

elrayle commented Jan 31, 2022

After extensive discussion in Slack, it was decided to go with Option 3, accepting the PR as is and revisiting the question once a migration path is defined for moving from ActiveFedora to non-ActiveFedora Valkyrie persisters (e.g. postgres).

For documentation, @no-reply suggested trying the following query, which did return both AdminSets and Hyrax::AdministrativeSets. But there seem to be items returned for the My -> Collections tab that included admin sets and collections created by other users. The query seems to be on the right track, but either it or the way it was used for the various search builders involved needs additional work to return the correct results for the current user.

        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(has_model: ::AdminSet.to_s, creator: current_user_key)
        ]

@elrayle elrayle marked this pull request as ready for review January 31, 2022 22:30
@elrayle
Copy link
Contributor Author

elrayle commented Jan 31, 2022

More information from the Slack conversation in case we come back to this later:

From @cjcolvar

When looking at this with @elrayle last week, I think one thing complicating things is that Hyrax.config.admin_set_model for the valkyrie case is Hyrax::AdministrativeSet which doesn't seem to play well with the raw query that construct_query_for_rel generates. An alternative is construct_query which defaults to a field query but could be configured to use terms instead which does seem to work.
https://github.com/samvera/active_fedora/blob/511ee837cd9a461021e53d5e20f362b634de39a8/lib/active_fedora/solr_query_builder.rb#L35

@no-reply no-reply merged commit a778e23 into main Jan 31, 2022
@no-reply no-reply deleted the dashboard_list_adminsets branch January 31, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hyrax::AdministrativeSets fail to be listed on Dashboard->Collections
2 participants