Skip to content
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

Improve how adv search handles incoming params #3278

Conversation

mamrey
Copy link
Contributor

@mamrey mamrey commented Aug 15, 2024

closes #3237, #3238

Changes

  • excludes adv search form parameters from hidden search fields to prevent duplicate query params
  • pre selects facet appropriate facet checkbox only when receiving inclusive filter parameters
    • implements check using new FacetCheckboxItemPresenter that overrides #selected?
  • increases spec coverage when prepopulating adv search form
  • adds specs for new presenter

Questions

I have a couple lingering questions that would help clarify my understanding of the expected behavior of the adv search and provide additional context for a review:

Currently, incoming exclusive filter parameters will pre-check the corresponding facet checkbox. This PR prevents this behavior, and now any incoming exclusive filters will continue to be added as a hidden field. What is the desired behavior around incoming exclusive filters? In my opinion, the facet checkboxes represent inclusive filters, so it's a bit misleading to have them pre selected with an incoming exclusive filter. Any thoughts on this?

Currently, we exclude incoming inclusive parameters from the displayed constraints on the adv search page. This PR does not change this, but is this desired behavior?

@jcoyne jcoyne added this to the 8.x milestone Aug 21, 2024
- use rspec `is_expected` syntax to simplify tests
@tampakis
Copy link
Member

Currently, we exclude incoming inclusive parameters from the displayed constraints on the adv search page. This PR does not change this, but is this desired behavior?

I think the inclusive facet parameters should also carry over. Could we track adding that capability in a separate issue?

@mamrey mamrey requested a review from jcoyne August 22, 2024 16:58
@jcoyne jcoyne dismissed their stale review August 23, 2024 15:42

resoved issue

@cgalarza
Copy link
Contributor

Currently, we exclude incoming inclusive parameters from the displayed constraints on the adv search page. This PR does not change this, but is this desired behavior?

I think the inclusive facet parameters should also carry over. Could we track adding that capability in a separate issue?

@mamrey and I discussed this and we think we should only display parameters that are not already selected on the form in the displayed constraints at the top of the advanced search form. The inclusive facet parameters will be selected on the advanced search form so there isn't a need to display them again in the displayed constraints. @mamrey is going to write up a ticket explaining this issue in more detail.

@cgalarza cgalarza requested review from cgalarza and removed request for jcoyne August 27, 2024 17:56
@tampakis tampakis merged commit 69373f2 into projectblacklight:main Aug 27, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Submitting an advanced search with existing parameters duplicates parameters in request
4 participants