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

Allow filtering option values by variant for the Option Value promotion rule #5200

Merged
merged 2 commits into from
Jul 4, 2023
Merged

Allow filtering option values by variant for the Option Value promotion rule #5200

merged 2 commits into from
Jul 4, 2023

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jun 30, 2023

Summary

This fixes some long-standing issues with the interaction between option Value Select and product Select on the Option Value Promotion Rule form.

Without this PR, the option values would not be scoped to the selected product, because option values didn't allow searching by variant product ID, and because the selector passed into the option value select would select elements anywhere on the page.

Luckily, jQuery accepts elements just as well as query strings.

You can test this with the sandbox: The Solidus hoodie does not have "blue" and "black" as option types, while the Solidus T-Shirt does.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@mamhoff mamhoff requested a review from a team as a code owner June 30, 2023 13:19
@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Jun 30, 2023
tvdeyen
tvdeyen approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice work.

@mamhoff mamhoff force-pushed the allow-filtering-option-values-by-variant branch from 75f0748 to 98510ec Compare June 30, 2023 13:30
@mamhoff
Fix filtering option values by variants_product_id
1c77559 
This fixes filtering option values by variants_product_id, making the
option value select in the option value promotion rule do the expected
thing.
@mamhoff
Option Values Rule: Correctly scope productSelect search
f72106b 
By passing an element instead of a selector, we can tell the
optionValueAutocomplete instance about the related productSelect.
@mamhoff mamhoff force-pushed the allow-filtering-option-values-by-variant branch from 98510ec to f72106b Compare July 3, 2023 10:27
@kennyadsl
Copy link
Member

@mamhoff Thanks a lot! I think I am missing some context here: it's not immediate to get looking at the files changed or reading the description, in which admin page can we see this in action?

@mamhoff
Copy link
Contributor Author

mamhoff commented Jul 3, 2023

in which admin page can we see this in action?

This is a promotion rule, so you'd have to add the "Option Value(s)" rule to an existing promotion on /admin/promotions/:promotion_id/edit. The functionality is somewhat obscure: The rule specifies that the order has to contain a product with option value x, and you first select the product, then the corresponding option values.

@codecov
Copy link

codecov bot commented Jul 3, 2023 •
edited

Codecov Report

Merging #5200 (f72106b) into main (2f59fa0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5200   +/-   ##
=======================================
  Coverage   88.67%   88.67%           
=======================================
  Files         564      564           
  Lines       13893    13894    +1     
=======================================
+ Hits        12319    12320    +1     
  Misses       1574     1574
Impacted Files Coverage Δ
core/app/models/spree/option_value.rb 100.00% <100.00%> (ø)

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

elia
elia approved these changes Jul 3, 2023
View reviewed changes
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

🐛 🔍

@kennyadsl kennyadsl added the type:bug Error, flaw or fault label Jul 3, 2023
kennyadsl
kennyadsl approved these changes Jul 3, 2023
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I consider this to be a bugfix, added the appropriate label. Thanks @mamhoff!

@kennyadsl kennyadsl merged commit 5f9c960 into solidusio:main Jul 4, 2023
13 checks passed
@elia elia changed the title Allow filtering option values by variant Allow filtering option values by variant for the Option Value promotion rule Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elia elia elia approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

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

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem type:bug Error, flaw or fault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mamhoff @kennyadsl @elia @tvdeyen @waiting-for-dev