[Moved] Improve variant and product autocomplete functions flexibility with Ransack

[RyanofWoods]

Summary

From this change, when initializing a productAutocomplete, a callback can be given to the searchCallback option. This callback should return a hash object that will be used to merge onto the existing parameters in the select2 ajax function. The callback also allows for one argument, which is the string typed in the select2 (term). Custom Ransack can be achieved by defining this option with the Ransack query on the q key.

$('#product-dropdown').productAutocomplete({
  multiple: false,
  searchCallback: (searchString) => ({
    q: {
      name_cont: searchString,
      available_on_not_null: true
    })
  }
})

Similar behavior was already existing on the variantAutocomplete, however accepted a simple hash object instead of a function, and so one didn't have access to the select2 search term, so this was improved.

$('#variant-dropdown).variantAutocomplete({
  searchCallback: (searchString) => ({
    q: {
      variant_search_term: null,
      sku_cont: searchString
    })
  }
})

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

• I have added automated tests to cover my changes.
  - [ ] I have attached screenshots to demo visual changes.
  - [ ] I have opened a PR to update the guides.
  - [ ] I have updated the README to account for my changes.

---

Autocompletes are still working:

Variant

Orders#index

Orders#edit

Product

Promotion Rules - Product

Promotion Rules - Option Values

[kennyadsl]

@RyanofWoods nice, can you please show us the way to use this custom query as well?

[RyanofWoods]

I added an example to the description @kennyadsl 👍 I also changed the option variable name to use lower camelcase
ransack_q_callback -> ransackQCallback

[waiting-for-dev]

Thanks, @RyanofWoods!! ❤️ A couple of questions/suggestions:

• Can you point us to the point where it's sent to the database? I'm worried about possible SQL injection attacks.
• Would it be possible to add a test for it?
• In the boy scout spirit, adding a few docs on top of the file would be nice.

[tvdeyen]

Loving that! Thanks. I have some doubts about the name and it would be nice to fix the variantAutocomplete as well :)

[RyanofWoods]

Thanks, @RyanofWoods!! ❤️ A couple of questions/suggestions:

• Can you point us to the point where it's sent to the database? I'm worried about possible XSS attacks.
• Would it be possible to add a test for it?
• In the boy scout spirit, adding a few docs on top of the file would be nice.

Thanks for your feedback @waiting-for-dev ❤️

I will happily add tests and docs for it after discussing some more 👍

Here's the endpoint it touches:

solidus/backend/app/controllers/spree/admin/search_controller.rb

Line 29 in d582f54

@products = Spree::Product.ransack(params[:q]).result

Security of this is a great point, this change would make it highly dependant on how well .ransack protects attacks. We could perhaps add a configuration such as allowed_product_autocomplete_rancsack_matchers, and then do param filters in the controller, WDYT?

[RyanofWoods]

On further inspection, if there would be any security issues with allowing full control of parameters to ransack, we would have this problem right now with the variantAutocomplete:

solidus/api/app/controllers/spree/api/variants_controller.rb

Lines 37 to 42 in d582f54

	if params[:variant_search_term]
	Spree::Config.variant_search_class.new(
	params[:variant_search_term], scope: scope
	).results.includes(include_list)
	else
	scope.includes(include_list).ransack(params[:q]).result

[waiting-for-dev]

Security of this is a great point, this change would make it highly dependant on how well .ransack protects attacks

Ok, it all boils down to the regular usage of Ransack, so we should be fine.

[codecov]

Codecov Report

Merging #4733 (d582f54) into master (efe2b3a) will not change coverage.
The diff coverage is n/a.

❗ Current head d582f54 differs from pull request most recent head c89a605. Consider uploading reports for the commit c89a605 to get more accurate results

@@           Coverage Diff           @@
##           master    #4733   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files         578      578           
  Lines       14654    14654           
=======================================
  Hits        12623    12623           
  Misses       2031     2031           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[RyanofWoods]

Specs are not getting actioned here @waiting-for-dev 🤔

[waiting-for-dev]

Specs are not getting actioned here @waiting-for-dev 🤔

Can you git commit --amend --allow-empty & git push -f? I've seen this kind of hiccup in the past 🤷

[RyanofWoods]

Done this twice now, but same thing @waiting-for-dev 🤷

[waiting-for-dev]

Done this twice now, but same thing @waiting-for-dev 🤷

For some reason, you are unauthorized to run the build. Let's investigate that further. For now, I re-run CI and we can see the results. BUT, same that master, it's failing because the image suddenly can't find the libvips package 🤷

[waiting-for-dev]

Great, @RyanofWoods, you're no longer unauthorized! 🥳 Just rebase from master to fix the CI errors.

[RyanofWoods]

Nope, happened again @waiting-for-dev, wondering it's something to do with force pushing or that I am pushing to my own fork 🤔

[RyanofWoods]

I couldn't get the CircleCI tests to trigger 🤷 So I made a new PR via the Nebulab fork which did the trick.
#4767

@tvdeyen @waiting-for-dev