-
Notifications
You must be signed in to change notification settings - Fork 124
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
replace ActiveFedora where with Valkyrized where #4910
Conversation
end | ||
|
||
def collection_type_gids_that_disallow_multiple_membership | ||
Hyrax::CollectionType.gids_that_do_not_allow_multiple_membership | ||
Hyrax::CollectionType.gids_that_do_not_allow_multiple_membership.map(&:to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests initially failed when refactoring for the new approach which required this change. But the final refactor really doesn't need this. So I set it back to what it was.
results = solr_service.query(query) | ||
ids = results.map(&:id) | ||
return query_service.find_many_by_ids(ids: ids) if use_valkyrie | ||
ids.map { |id| query_service.find_by_alternate_identifier(alternate_identifier: id.to_str, use_valkyrie: false) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the advantage of using the valkyrie query service?
would there be fewer risks if we stuck with the the existing ActiveFedora::Base#where
implementation in the case where what we want is to query AF objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to use AF where unless use_valkyrie is true.
7af0aa6
to
c7af5b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great pattern and helps create the inflection point. I "approve" this contingent on pending .map(&:to_s)
question
Results were expected to be an array, but before this commit, an enumeration was returned when using Valkyrie. This ensures that an array is returned whether working wtih AF or Valkyrie. Also removes need to convert to string per requested change during review.
Partially addresses Issue #3820
Establishes the pattern for how to replace calls to ActiveFedora::Base #where method.
Establishes a new service class that searches solr to get object IDs and then uses Hyrax.query_service get the objects. The method name that is the equivalent of the ActiveFedora where method is
.find_for_model_by_field_pairs
.This allows for other types of queries for solr to be constructed as needed at a later time.
@samvera/hyrax-code-reviewers