Regression: Limit promotion (via coupon) to only apply once per order

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...

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.

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.

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.

+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.

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?

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.

+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.

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

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

• Field name: allow_multiple_coupons
• Method: isMultipleCouponsAllowed()

Looks like MR is broken

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

@jsacksick Let me know you opinion.

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

sorabh.v6 → closed merge request !406

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

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

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

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

@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

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.

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

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

Updated the MR and needs review now.

Thanks

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.

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

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

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).

@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#d...

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.

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

Yes, but we can show the description.

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

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.

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

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