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

Exclude canceled orders in the #usage_count of promotions and promotion codes #4123

Conversation

ikraamg
Copy link
Contributor

@ikraamg ikraamg commented Jun 28, 2021

Description

This change is extracted from a clients project. I thought it might be useful to have this on Solidus because excluding the count of canceled orders in #usage-count allows for a more accurate count(and limit) of promotions and promotion_codes.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

@ikraamg ikraamg force-pushed the exclude-canceled-orders-from-promotioncount branch from 299396d to b71e450 Compare June 28, 2021 11:28
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.

Nice catch, the CI will be green once we address #4110

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

I think the feature change makes sense, but I see a couple of issues, the first one is related to large datasets (see the other comment), the second one is we're changing the original behavior, so I'm wondering if we should give the possibility to keep the old one (for example via a config flag) or if the change is so common sense that we should consider the original behavior almost a bug.

core/app/models/spree/promotion.rb Outdated Show resolved Hide resolved
@ikraamg ikraamg force-pushed the exclude-canceled-orders-from-promotioncount branch from 62130e4 to 1745930 Compare July 5, 2021 09:18
@spaghetticode
Copy link
Member

@ikraamg thanks for removing the orders subquery 🙏

I'm not sure how picky we want to be with the behavior change, which is now moved here and here. I will discuss this with the rest of the core team during the next call and let you know.

@ikraamg ikraamg force-pushed the exclude-canceled-orders-from-promotioncount branch from 1745930 to 631ef0b Compare July 7, 2021 15:39
This allows for a more accurate count of promotion/promotion_code usage.

The exclusion was kept as an optional boolean within`Adjustment`
`#in_completed_orders` to keep backward compatibility and allow
 for canceled orders to be excluded within a single query when required.
@ikraamg ikraamg force-pushed the exclude-canceled-orders-from-promotioncount branch from 631ef0b to 843df0b Compare July 7, 2021 15:58
@spaghetticode spaghetticode merged commit abdd013 into solidusio:master Jul 14, 2021
@spaghetticode spaghetticode deleted the exclude-canceled-orders-from-promotioncount branch July 14, 2021 14:13
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