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

Ensure coupon code times_used decrements on cancel #1031

Merged
merged 4 commits into from
Jun 26, 2020
Merged

Ensure coupon code times_used decrements on cancel #1031

merged 4 commits into from
Jun 26, 2020

Conversation

mattdavenport
Copy link
Contributor

To implement this cleanly in the observer (without instantiating new connection objects, fetching records, etc.) I added a parameter $decrement to updateCustomerCouponTimesUsed which should be a non-breaking change.

fixes #167

@mattdavenport
Ensure coupon code times_used decrements on cancel
022810b 
- Update rule name in config. Separate save calls.
- Fix typo in bind params
- Ensure coupon usage table update. Allow decrementing coupon usage.
- Fix observer definition
- Fix coupon codes on order cancellation
- Decrement `times_used` on coupon codes when an order payment is cancelled.
@sreichel
Copy link
Contributor

sreichel commented Jun 8, 2020

LGTM ... but can you please add some steps to reproduce?

@sreichel sreichel added bug Component: SalesRule Relates to Mage_SalesRule labels Jun 8, 2020
@mattdavenport
Copy link
Contributor Author

Sure thing!

  1. In Shopping Cart Price Rules, add a new rule with a "Specific Coupon" code with > 0 uses per coupon.
  2. In Actions tab, set a discount amount.
  3. Add a product to cart. Apply promo code in cart. Checkout.
  4. Upon checkout, it can be observed that times_used has been incremented in tables:
    • salesrule_coupon
    • salesrule_coupon_usage
    • salesrule_customer
  5. In admin panel, cancel the order.
  6. It can be observed that times_used has decremented to allow the coupon to be used again.

colinmollenhour
colinmollenhour approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the PR!

@colinmollenhour
Copy link
Member

@tomekjordan @seansan may be interested in reviewing

sreichel
sreichel previously approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

Tzzz .... uppercase FALSE .... :P

@sreichel sreichel added this to the Release 19.4.6 milestone Jun 26, 2020
colinmollenhour
colinmollenhour requested changes Jun 26, 2020
View reviewed changes
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

I actually prefer uppercase bools but unfortunately one of those PSR things doesn't agree with me.. 🤣

app/code/core/Mage/SalesRule/Model/Observer.php Outdated Show resolved Hide resolved
app/code/core/Mage/SalesRule/Model/Resource/Coupon/Usage.php Outdated Show resolved Hide resolved
app/code/core/Mage/SalesRule/Model/Resource/Coupon/Usage.php Outdated Show resolved Hide resolved
@colinmollenhour colinmollenhour merged commit ec31d8f into OpenMage:1.9.4.x Jun 26, 2020
This was referenced Jul 14, 2020
Merged
Merged
@henrykbrzoska
Copy link
Contributor

Hi, this logic is not obvious. Please reanalyse if used coupon should be reused.
IMO - shouldn't. Regardless of whether the order has been placed.

@sreichel
Copy link
Contributor

Hi, this logic is not obvious. Please reanalyse if used coupon should be reused.
IMO - shouldn't. Regardless of whether the order has been placed.

Can you please explain your concerns?

If I use a voucher but cancel the order, the voucher should still be available to me.

In what cases should the voucher no longer be valid?

@henrykbrzoska
Copy link
Contributor

henrykbrzoska commented Aug 19, 2020 •
edited
Loading

If I use a voucher but cancel the order, the voucher should still be available to me.

are you sure?
I think only about ecommerce logic. If i give someone 'ONE time' coupon it should be 'ONE time' usable.

I'm not against it. Just please analyze.

@sreichel
Copy link
Contributor

are you sure?
I think only about ecommerce logic. If i give someone 'ONE time' coupon it should be 'ONE time' usable.

Yeah, but maybe I'm missing something...

For example: A customer orders an article not in stock with a one-time-voucher. Since it takes too long, he cancels the order.

Should the voucher expire?

@henrykbrzoska
Copy link
Contributor

IMO yes. he should get another one if seller permit.

@sreichel
Copy link
Contributor

Well ... IMO it should not :)

Just did a quick search ...

  • mediamarkt/saturn voucher expires
  • adidas does not expire, even after full refund

I think its easy cover both needs ... add a config section and a check to observer. Time for a PR?

@henrykbrzoska
Copy link
Contributor

ok, approve. :)

I wanted to be careful

edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
@mattdavenport @edannenberg
[impr-OpenMage#1031] Coupon times_used field should decrease if a ord…
334d65f 
…er is canceled
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 24, 2020
@mattdavenport @edannenberg
[impr-OpenMage#1031] Coupon times_used field should decrease if a ord…
d8b4927 
…er is canceled
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 24, 2020
@drwilliams @edannenberg
[bug-OpenMage#1117] Duplicate entry when updating salesrule_coupon_usage
0dfa2c4 
* Remove $timesUsed > 0 check to prevent duplicate entry
* Prevent $timesUsed from going less than 0

refs: OpenMage#1031
@boesbo boesbo mentioned this pull request Oct 23, 2020
Merged
3 tasks
@spinsch spinsch mentioned this pull request Oct 26, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colinmollenhour colinmollenhour colinmollenhour approved these changes

@sreichel sreichel sreichel left review comments

Assignees
No one assigned
Labels
bug Component: SalesRule Relates to Mage_SalesRule
Projects
None yet
Milestone
Release 19.4.4 / 20.0.0
Development

Successfully merging this pull request may close these issues.

Canceled order - reuse coupon by the same customer
4 participants
@mattdavenport @sreichel @colinmollenhour @henrykbrzoska