-
Notifications
You must be signed in to change notification settings - Fork 41
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
hacky way to keep unpermitted params out of text range facet links #298
Conversation
20e74ee
to
fe21b13
Compare
fe21b13
to
5846791
Compare
OK, I learned more about what's going on. Blacklight SearchState does have an allow-list, such that un-mentioned params are ignored. But we had to ADD our special range params into the allow-list, to get them available for use in the builder where we needed them.
But we want them ONLY in the #range_limit action -- we don't want them forwarded on when we generate facet links to typical catalog action. I actually can't see any better way to get that to happen then with the overide in FaceItemPresenter. BUT, two improvements as a result of this investigation:
|
It seems like there should be a better way -- I thought Blacklight search_state was only including allow-listed params in the first place but apparently not? Not sure what a better way would be or if it would require Blacklight architectural changes, but this works for now. Regression test included that was red without this change.
123d442
to
be56b23
Compare
@seanaery after this one, I'll release a beta2 -- and hope to be done! |
This looks to me like a reasonable way to address the issue, even if it is imperfect. Thanks for figuring it out. The test is helpful and I like that this isolates the params the plugin needs to permit within the |
It seems like there should be a better way -- I thought Blacklight search_state was only including allow-listed params in the first place but apparently not? Not sure what a better way would be or if it would require Blacklight architectural changes, but this works for now. Regression test included that was red without this change.
Closes #296