In #3404821: Limit promotion to only apply once per order, the ability to apply the same promotion multiple times using unique coupons was removed.

https://git.drupalcode.org/project/commerce/-/commit/4a8e073085981f3a0d6...

      // The promotion might already be applied, make sure we don't apply the
      // same promotion more than once.
      if ($referenced_coupon->getPromotionId() == $coupon->getPromotionId()) {
        $form_state->setErrorByName($coupon_code_path, $this->t('The provided coupon code cannot be applied to your order.'));
        return;
      }

The introduction of this block of code has removed what previously worked as I do want users to be able to apply multiple single use unique coupon codes to an order.

In our case, each single use code gives up to $20 off the order. If a user uses 2 of them, they get $40 off etc. Trying to use the exact same coupon twice still rejects which is desired. More details on use case in #3414323-3: Promotion setting: Allow applying promotion more than once via coupon

It would be ideal if this was configurable like the other usage limits

Usage limits: Total allowed coupon uses per order.
Description: Number of times this promotion can be applied to an order using a different coupon.

Issue fork commerce-3414323

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ELC created an issue. See original summary.

jsacksick’s picture

jsacksick commented

The "obvious" usecase to me is supporting coupon codes belonging to different promotions. Supporting applying coupon codes belonging to the same promotion sounds like a bug to me...

elc’s picture

elc commented

Use case:

I have 2x promotions: "Fixed amount off the order subtotal"
- $50 off
- $20 off

Both require coupons. Unlimited uses per user, unlimited available.

I have 1000 unique codes on each which are given out. They each have single use set.

If a user has 4x $20 codes, they get $80 off their order.

If a user has 1x $50 code and 2x $20 codes, they get $90 off their order.

2x $50, $100 off their order.

jsacksick’s picture

jsacksick commented

I think this issue should be changed to a feature request, and we might need a new setting. Describing what it does might be a bit challenging as I doubt your usecase is a frequent one... I'm pretty sure in most cases the expectation is to NOT allow applying the same promotion multiple times.

elc’s picture

elc commented
Title: Regression: Limit promotion (via coupon) to only apply once per order » Promotion setting: Allow applying promotion more than once via coupon
Category: Bug report » Feature request
Issue summary: View changes

I've updated the the issue description into a FR.

For now I've just commented out the code with a patch to get the previous function working for users who have been complaining.

I have since had a request that it be possible to limit it to a number per order, so a similar setup as the other usage limits would be ideal. Setting to allow unlimited applications, or N applications per order.

hudsome’s picture

hudsome commented

+1

Would be nice to be able to use multiple coupons from a promotion on one order with the possibility to limit the amount of coupons per order.

jsacksick’s picture

jsacksick commented

Reasonable proposal I guess.
The current fields we have for limiting usage are named like "usage" and "usage_limit_customer". If we keep the same pattern, I guess we could go with "usage_limit_coupon" with a method getCouponUsageLimit()

The only problem is if we now introduce this, the default value for these fields is "0" which actually means unlimited, and the current behavior is 1. But perhaps it is fine to limit to 1 by default?

jsacksick’s picture

jsacksick commented

Perhaps "usage_limit_order" makes more sense, though you can't apply the same promotion multiple times if not using multiple coupons, but it does sound more explicit.

tBKoT made their first commit to this issue’s fork.

onfire84’s picture

onfire84 commented

+1

i just run into the same issue when implementing the promotion feature for a customer. it wolud be great if we could get a configurable solution for that setting.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: Unassigned » sorabh.v6

I am interested in working on this issue. Just to confirm, we are going with "usage_limit_order"? I was thinking this name might be little bit confusing.

IMO if we put a checkbox in the usage limit details container that says "allow applying multiple coupons per promotion". If this is not checked then we can compare promotion id of the applied coupons else we compare the coupons id and promotion id.

Let me know how this sounds to you @jsacksick.

Thanks

jsacksick’s picture

jsacksick commented

Ok, this isn't really a "usage" setting, so I'm thinking we could go with:

  • Field name: allow_multiple_coupons
  • Method: isMultipleCouponsAllowed()

sorabh.v6 opened merge request !406

ivnish’s picture

ivnish
he/him
commented

Looks like MR is broken

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Version: 8.x-2.x-dev » 3.0.x-dev

I think its better that we add this into the 3.0.x-dev.

@jsacksick Let me know you opinion.

jsacksick’s picture

jsacksick commented

Definitely, didn't pay attention to that, 3.0.x is the recommended version.

sorabh.v6 opened merge request !407

sorabh.v6 closed merge request !406

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: sorabh.v6 » Unassigned
Status: Active » Reviewed & tested by the community

Hi, please review the MR https://git.drupalcode.org/project/commerce/-/merge_requests/407

jsacksick’s picture

jsacksick commented
Issue tags: +Needs tests

Code looks ok at first glance, not sure why the MR was closed. Any chance we could get tests coverage for this extra setting?

jsacksick’s picture

jsacksick commented
Status: Reviewed & tested by the community » Needs work

Also we might need a description explaining what the setting does. Setting to needs work for the tests.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: Unassigned » sorabh.v6

Sure, I will add test coverage for this extra setting. I will add description/help text for the field as well.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: sorabh.v6 » Unassigned
Status: Needs work » Needs review

@jsacksick There's some issue with the phpunit pipeline. Its giving me failed tests for the files which I did not changed.

Other than that, MR has the updated code that has -

  • need a description explaining what the setting does
  • PhpUnit tests for the change

Let me know if I understood it wrong and those failing tests are related to my code.

Thanks

jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

The code added looks wrong... The order module shouldn't have promotion related code? The coupons field is not necessarily present and is added by the promotion module for example.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented

Actually, when multiple coupons are not allow and we apply coupons from UI then we get validation error. But when we apply the coupons programmatically then we do not get error and multiple coupons are applied.

I thought to prevent this I need to update the code in order presave.

If you do not want me to update the code in the order module, then I can add that validation in the hook_entity_presave.

Let me know if that works.

Thanks

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Status: Needs work » Needs review

As per the conversation in the slack -> https://drupal.slack.com/archives/C1TLCCF9B/p1740661087476619

Updated the MR and needs review now.

Thanks

jsacksick’s picture

jsacksick commented

I just don't understand why the existing code in CouponTest was touched. Would have been better to add new code rather than modifying the existing logic which makes this more complicated to review.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Status: Needs review » Needs work

Hey @jsacksick, I will move my changes to a new method in the CouponTest. I hope that would make it better to review.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Status: Needs work » Needs review

Hey @jsacksick, I have made the changes. Let me know if you think anymore changes are needed.

jsacksick’s picture

jsacksick commented

Ok almost there, let's change the description to:
"Allow multiple coupons to apply to a single order."

Also, let's add setSetting('display_description', TRUE) to the field definition. Thanks to this no form alteration would be needed.

Also, let's update the promotion order processor test to test the case where multiple coupons are allowed using the new setting (we're missing that).

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: Unassigned » sorabh.v6
sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented

@jsacksick

Also, let's update the promotion order processor test to test the case where multiple coupons are allowed using the new setting (we're missing that).

Do you mean this code -
https://git.drupalcode.org/project/commerce/-/merge_requests/407/diffs#f...

jsacksick’s picture

jsacksick commented

Referring to PromotionOrderProcessorTest:: testMultipleCouponRedemption().

Also, the kernel promotion test doesn't need an extra method, but I can refactor that myself prior to commit, I can also make the final changes myself prior to commit when I get some time to finally wrap this up.

sorabh.v6’s picture

sorabh.v6
He/His
Primary language Hindi
commented
Assigned: sorabh.v6 » Unassigned

This isn't needed with "display_description", correct?

Yes, but we can show the description.

jsacksick’s picture

jsacksick commented

I don't understand? The setting is there so the description is correctly added, so no need for injecting it from the PromotionForm?

jsacksick’s picture

jsacksick commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I tweaked the setting description, moved the setting under "Usage limits", and manual testing works as expected... Let's see if the tests are still green but I believe this is good to go.

  • jsacksick committed 68008c97 on 3.0.x authored by sorabh.v6 
    Issue #3414323 by sorabh.v6, jsacksick, tbkot: Promotion setting: Allow...
jsacksick’s picture

jsacksick commented
Status: Reviewed & tested by the community » Fixed

@sorabh.v6: Thank you for your work on this, committed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.