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
Show Hyrax::PcdmCollection resources in search results #5515
Conversation
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.
This looks really good. I just have one question. I'm going to go ahead and approve because I am good with your decision on the question either way, and even lean a bit toward the way it is now.
@@ -303,6 +303,14 @@ def clean_active_fedora_repository | |||
Valkyrie::IndexingAdapter.find(adapter_name).wipe! | |||
end | |||
|
|||
# Configure blacklight to use the valkyrie solr index | |||
config.around(:example, index_adapter: :solr_index) do |example| |
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.
Nice to have this in place. It will help with future tests that need the valkyrie core to be primary.
@@ -0,0 +1,63 @@ | |||
# frozen_string_literal: true | |||
RSpec.describe 'searching', index_adapter: :solr_index do | |||
let(:user) { create :user } |
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.
First fully valkyrie feature test. 💯
@@ -50,6 +50,8 @@ attributes: | |||
multiple: true | |||
form: | |||
primary: false | |||
index_keys: | |||
- "description_tesim" |
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.
- "description_tesim" | |
- "description_sim" | |
- "description_tesim" |
Should this have the string version as well? It is pretty common to include both, but the string stores as the exact string typically used for exact matching. I don't imagine that someone would be looking for an exact match for the entire description.
String stores a word/sentence as an exact string without performing tokenization etc. Commonly useful for storing exact matches, e.g, for facetting.
Text typically performs tokenization, and secondary processing (such as lower-casing etc.). Useful for all scenarios when we want to match part of a sentence.
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.
From what I can tell description was only every indexed as _tesim
. (https://github.com/samvera/hyrax/blob/main/app/indexers/hyrax/basic_metadata_indexer.rb#L7)
Fixes #5463
Journey to getting Hyrax::PcdmCollection resources to show in search results:
Hyrax::CollectionName
to force Rails to resolve view partials tohyrax/collections/collection
. Without this change Rails was looking inhyrax/hyrax/pcdm_collections/pcdm_collection
when attempting to render a SolrDocument withhas_model_ssim: Hyrax::PcdmCollection
(See https://github.com/samvera/hyrax/blob/main/lib/hyrax/active_fedora_dummy_model.rb#L53)catalog/_index_headers_list_collection.html.erb
tocatalog/_index_headers_list_hyrax_pcdm_collection.html.erb
so Blacklight could find it. This seemed simpler than overriding Blacklight's helper which looks for partials to render (See https://github.com/projectblacklight/blacklight/blob/v6.24.0/app/helpers/blacklight/render_partials_helper.rb#L128-L146)hyrax-test
core.basic_metadata.yaml
to have the description field be indexed like it is in ActiveFedora work and collection models since the feature test finds the collection by its description.@samvera/hyrax-code-reviewers