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.

Sign up for GitHub

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

Jump to bottom

Use Variant Searcher for Autocomplete #4197

Merged
merged 2 commits into from
Feb 15, 2022
Merged

Use Variant Searcher for Autocomplete #4197

merged 2 commits into from
Feb 15, 2022

Conversation

adammathys
Copy link
Member

@adammathys adammathys commented Oct 27, 2021 •
edited
Loading

Description

Update /api/variants to be able to search either using the configurable variant_search_class or Ransack. This makes it possible for users to more easily override the search behaviour of variantAutocomplete based on their needs. (e.g.: If they're using Globalize and need to search the translated fields instead of just the base columns that won't be in use.)

The existing behaviour is maintained to ensure the API is completely backwards compatible.

Ref #4175

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@adammathys
Allow using the variant_search_class in API
42fbafa 
Our admin has already mostly moved over to using the new search class in
a few places. This adds it as an option to the API index endpoint.

After this change, we'll be able to update our autocomplete JS logic to
use the new search object instead of the hardcoded ransack queries it
currently does. Eventually allowing us a much easier configuration
interface we can use to ensure the variant searching works for all users
who wan't to tweak it to include other search fields. (e.g.: Globalize
wants to search different translated columns.)

The existing behaviour is also easy enough to keep in, which prevents us
from having to make a breaking change.
@adammathys
Update variantAutocomplete to use new search param
e9d5db1 
Instead of trying to generate some hardcoded Ransack search values, we
can use the APIs ability to use our Search::Variant class instead.
jarednorman
jarednorman approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally fine with merging this in a minor release since it preserves the old behaviour even if you have weird overrides.

@waiting-for-dev
Copy link
Contributor

Excellent idea, but I'm not entirely sure about the implementation. What about creating another variant_search_class delegating to ransack and letting users choose which one to use in the API controller? It should default to the ransack one to keep current behavior.

@adammathys
Copy link
Member Author

@waiting-for-dev I'm not sure I follow. The default variant_search_class already uses Ransack under the hood. The difference between this change and existing behaviour is that we allow passing in a single search term and letting the variant_search_class decide how to craft the appropriate Ransack parameters to use for that, instead of having those parameters hardcoded in JavaScript. (Which makes them impossible to easily modify.)

@waiting-for-dev
Copy link
Contributor

@adammathys, sorry, I overlooked that the current variant_search_class is already delegating to ransack.

It's nice the extra flexibility that this PR adds to the variant autocomplete code for JS. And it makes sense to have the API controller perform both fuzzy and conventional search (as the backend needs the fuzzy one while the API request needs the traditional one).

My comment was more about creating a second variant_search_class for the traditional type of search, so just extracting the ransack forwarding without further processing. So, I'd have two types of configurable variant classes: variant_search_class and variant_autocomplete_search_class, and would apply them not only to the backend as it's done now but also in the API index action.

However, that would broaden this PR's scope as we're discussing a more concrete need. So, I'm ok with merging this straightaway.

@jarednorman jarednorman merged commit a6c2cee into solidusio:master Feb 15, 2022
@jarednorman jarednorman deleted the adammathys/variant-searcher-autocomplete branch February 15, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarednorman jarednorman jarednorman approved these changes

@waiting-for-dev waiting-for-dev waiting-for-dev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adammathys @waiting-for-dev @jarednorman