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

Use taxon children when searching classification #3168

Merged
merged 1 commit into from
May 14, 2019

Conversation

fkoessler
Copy link
Contributor

Description

In current implementation, eligible? checks the configured taxons and all descendants and actionable? only seems to check against the taxons themselves. This leads to buggy behavior. See: #1409 (comment)

This commit modifies actionable? and have it check against configured taxons and all descendants in order to have a coherent behavior.

Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Hey @fkoessler, thanks a lot for submitting this PR! Could you please add some specs to cover this change? 🙏

@fkoessler
Copy link
Contributor Author

Hello @aitbw , thanks for reviewing. I added some specs that cover the change. Let me know if I can improve the PR.

Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Some minor typos to correct and we should be done ☺️

core/spec/models/spree/promotion/rules/taxon_spec.rb Outdated Show resolved Hide resolved
core/spec/models/spree/promotion/rules/taxon_spec.rb Outdated Show resolved Hide resolved
core/spec/models/spree/promotion/rules/taxon_spec.rb Outdated Show resolved Hide resolved
core/spec/models/spree/promotion/rules/taxon_spec.rb Outdated Show resolved Hide resolved
@aitbw
Copy link
Contributor

aitbw commented Apr 10, 2019

Thanks @fkoessler! 👌 could you please squash your commits into one? That should be enough to finish this PR 😸

Promotion rule is now actionnable if product has a child
taxon of taxon rule
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.

Thanks, this is great! Do you think this can impact performances negatively since we now could potentially have way more database queries?

@fkoessler
Copy link
Contributor Author

There will be a performance overhead since actionable? now calls to #rule_taxon_ids_with_children. I'm not sure if it is significant or not.

actionable? will be called on each line_item of the order, see Spree::Promotion #line_item_actionable?
The performance overhead will probably depend on the depth of the taxonomy, the number of line_items in the order. Should this be benchmarked?
Performance improvements from #2258 are kept.

In any case, the taxon promotion rule won't work as expected until eligible? and actionable? are made coherent.

Let me know what you think about this.

@kennyadsl
Copy link
Member

I'm fine with a performance regression if we are making things work as expected. The overhead will probably be higher depending on how many nested taxons you have and if the promotion is auto-applied (without the promo code), but in that case, I'd left to each store to fix eventual performance issues on their own. Thanks for the accurate explanation.

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.

👍 Thanks!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

I am okay with this! Thanks @fkoessler!

@kennyadsl kennyadsl merged commit 4955822 into solidusio:master May 14, 2019
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.

5 participants