-
Notifications
You must be signed in to change notification settings - Fork 257
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
Do not add facet limit unless it is known. #1989
Do not add facet limit unless it is known. #1989
Conversation
6d71659
to
82f5462
Compare
REF BL-712 REF projectblacklight/blacklight#1989 ## Issue Description After some QA testing, Jackie and I were talking further about the persistence of filters between Articles and the Solr menu options, and realized that there were some issues with this. For instance, even when there is a field match (author, topic, resource type), each index uses different vocabularies such that the values frequently don't match, leading to no results. We also felt that it might be confusing if some filters persisted (ex. date) while others didn't. So as a result, we'd like to scale this back and have just the basic query persist between Articles and the other options. Even in cases where the user has switched the basic query for "All Fields" to something else (ex. Title), it will revert back to All Fields. The relationship between Books, Journals, and More will remain as is, however. ## Implementation Description This issue is a result of the upstream code being promiscuous with respect to allowing any facet like user parameter to be turned into a facet filter for the query. Thus, it is not possible to have multiple facet url parameters that should not be shared with all searches. By adding a check at the search build level that skips adding a facet if the facet field in the parameter is not known (via configuration), then multiple searches can co-exist in the same URL and not clash. An upstream PR has been submitted to fix this issue at the root, hopefully it will be accepted.
REF BL-712 REF projectblacklight/blacklight#1989 ## Issue Description After some QA testing, Jackie and I were talking further about the persistence of filters between Articles and the Solr menu options, and realized that there were some issues with this. For instance, even when there is a field match (author, topic, resource type), each index uses different vocabularies such that the values frequently don't match, leading to no results. We also felt that it might be confusing if some filters persisted (ex. date) while others didn't. So as a result, we'd like to scale this back and have just the basic query persist between Articles and the other options. Even in cases where the user has switched the basic query for "All Fields" to something else (ex. Title), it will revert back to All Fields. The relationship between Books, Journals, and More will remain as is, however. ## Implementation Description This issue is a result of the upstream code being promiscuous with respect to allowing any facet like user parameter to be turned into a facet filter for the query. Thus, it is not possible to have multiple facet url parameters that should not be shared with all searches. By adding a check at the search build level that skips adding a facet if the facet field in the parameter is not known (via configuration), then multiple searches can co-exist in the same URL and not clash. An upstream PR has been submitted to fix this issue at the root, hopefully it will be accepted.
d5ba820
to
2c0de14
Compare
@dkinzer this is just sanity checking then, right? If the facet isn't defined, i.e. a bad config/typo in facet setting, then don't add it to the query? |
@cdmo effectively yes. If we are passing some facets over URL params the app shouldn't just pass these to solr backend if they are not actually defined / configured at the controller level. |
eacdab9
to
7854cb6
Compare
7854cb6
to
b03929b
Compare
I think these changes are effectively in-place via other refactors, but I have rebased against |
This commit updates the code that adds facet limits from user supplied parameters so that facets are only added if they have been configured as facet fields. This is done to allow for multiple faceted searches to be defined in the same URL while still keeping the facets separate.
b03929b
to
a9031a1
Compare
spec/features/search_spec.rb
Outdated
|
||
it "sanitizes searches with unconfigured facet parameters" do | ||
visit root_path f: { missing_s: [1] } | ||
expect(page).not_to have_text "Missing 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.
I think this is a useful thing to check, but I'm wary of "page doesn't have text" tests - high likelihood of false passes. I wonder if we could now re-implement this as a ConstraintsComponent test?
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.
@dkinzer I've added a spec against the ConstraintsComponent that exercises this expectation in context. Okay to remove this feature spec?
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.
Sounds good.
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.
Adding specs to verify facet params are sanitized before passing to solr
Waiting on CI to merge, thanks for this work @dkinzer |
* Verify search builder not adding facet limit unless it is configured. This commit updates the code that adds facet limits from user supplied parameters so that facets are only added if they have been configured as facet fields. This is done to allow for multiple faceted searches to be defined in the same URL while still keeping the facets separate. * verify constraints component does not render unconfigured facets Co-authored-by: Benjamin Armintor <armintor@gmail.com>
REF: #1990
This commit updates the code that adds facet limits from user supplied
parameters so that facets are only added if they have been configured
as facet fields. This is done to allow for multiple faceted searches to
be defined in the same URL while still keeping the facets separate.