-
Notifications
You must be signed in to change notification settings - Fork 252
Construct searches using the search builder directly from the controller. #3855
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
base: main
Are you sure you want to change the base?
Changes from all commits
e315fb9
22eac79
1e67e18
2408af1
4a5863b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,8 +26,8 @@ def search_service | |
|
|
||
| # This method may be overridden to customize search behavior. | ||
| # @return [Blacklight::Solr::Response] the solr response object | ||
| def retrieve_search_results | ||
| search_service.search_results | ||
| def retrieve_search_results(params: nil) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we change this kwarg to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just matching the argument name to the repository for consistency. |
||
| search_service.search_results(params: params || search_builder.with(search_state).rows(search_state.per_page).page(search_state.page)) | ||
| end | ||
|
|
||
| # This method may be overridden to customize search behavior. | ||
|
|
@@ -42,6 +42,18 @@ def retrieve_documents(ids) | |
| search_service.fetch(Array(ids)) | ||
| end | ||
|
|
||
| # @return [Blacklight::SearchBuilder] | ||
| def search_builder | ||
| klass = blacklight_config.search_builder_class | ||
|
|
||
| if klass.initialize_supports_blacklight_config_parameter? | ||
| klass.new(self, blacklight_config: blacklight_config) | ||
| else | ||
| # deprecated behavior for implementations that don't support the new initializer signature. | ||
| klass.new(self) | ||
| end | ||
| end | ||
|
|
||
| # Override this method on the class that includes Blacklight::Searchable to provide more context to the search service if necessary. | ||
| # For example, if your search builder needs to be aware of the current user, override this method to return a hash including the current user. | ||
| # Then the search builder could use some property about the current user to construct a constraint on the search. | ||
|
|
||
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 conflates search results (documents) from facet results. Can we keep these two concerns separated?
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.
Is there any value in doing that? I find the current state incredibly confusing and it's not clear to me what behavior the seam is intending to provide that isn't better suited for the search builder.
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 that it is confusing, and having one method handle the one case makes it less so.