Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'm using commerce_discount 7.x-1.0-alpha7 and commerce_coupon 7.x-2.0-rc2+6-dev, and coupons with product disscounts disappear when I hit the review page of checkout.
For now I've applied the following patches, and set the coupons to apply even when the discount doesn't apply, which mostly fixes the symptom for now, but not the underlying issue.
> commerce_coupon-auto_unserialize_data-2280505-6.patch
> ommerce_coupon-apply_without_discount-2263315-25.patch
Comment | File | Size | Author |
---|---|---|---|
#116 | commerce_discount-2621526-compatibility-116.patch | 12.67 KB | rszrama |
| |||
#110 | 2621526-110-tests-only.patch | 4.69 KB | joelpittet |
#108 | commerce_discount-2621526-compatibility-108.patch | 11.58 KB | czigor |
| |||
#106 | interdiff-100-106.txt | 2.26 KB | czigor |
#106 | commerce_discount-2621526-compatibility-106.patch | 11.54 KB | czigor |
|
Comments
Comment #2
mishac CreditAttribution: mishac commentedComment #3
torgosPizzaQuestion for you: do you have this patch applied to your Commerce install? #1268472: Recursion in price calculation causes cart to skip calculation rules
If so, I'd revert it - that patch was causing some discounts to be removed for me.
Also related issue: #2619448: Cart Refresh check for discount price components causes Coupon removal
Comment #4
mishac CreditAttribution: mishac commentedno I don't have that patch installed.
Comment #5
torgosPizzaThanks for the info. You might also want to try applying the patch from #1804592-64: During AJAX form submission in checkout, the $order argument passed by the Form API is incorrect. as that has solved a lot of issues, especially regarding Ajax checkout panes (which the Coupon textfield is!). That patch has solved many issues and that might be worth trying.
Second question, do you have any discount Rules that have been overridden? If so, try reverting them and then re-adding any customizations. I had found that some of my rules were failing after some of the recent updates and that also seemed to help when an update to the underlying modules had occurred.
But I wanted to ask more about what you said:
Just trying to understand what you believe the underlying issue to be, and if there is at all a way you can give us a more thorough example or walkthrough? Please be as specific as you can. You mentioned the coupon gets removed at Review - if you click "Go back" to get back to the first Checkout page, does it reappear, or is it gone for good?
It would also be super helpful if you could investigate your logs for any errors (perhaps even set your Rules settings to log the debug output to the system logs, and maybe add some additional calls to watchdog() within the coupon removal code), so you can see more details about what is happening and what function might be causing it. But I understand if this is asking too much! (I'm just trying to be thorough.)
Comment #6
mishac CreditAttribution: mishac commentedThe underlying issue is that coupon codes (and their related discounts) get removed when going to the review page of checkout. I installed the patch that allows coupons to be applied even when their discount conditions are not met, which fixes the problem, because the coupons now remain during checkout, and the discount eventually gets re-added, but the underlying issue is that the discounts conditions are evaluating as not met in the first place.
I'm assuming the discount is removed, which removes the couopn, which makes the discount's coupon condition unmet, but I'm not familiar enough with the internals of the thing to judge that for sure.
I do not have any overridden discount rules.
Comment #7
mishac CreditAttribution: mishac commentedThe coupon is gone for good when you hit "go back"..it doesn't come back.
I'll see what I can do about getting logs.
Comment #8
mishac CreditAttribution: mishac commentedMore weirdness:
If I apply the coupon on the first page of checkout, and then refresh, the discount appears. Refresh again, the discount is gone. Refresh again, and the discount is back.
I added a watchdog and it looks like the discount condition "commerce_discount_compatibility_check" is evaluating to false every other time, so it looks like my issue is actually being caused by the discount compatibility issue described here https://www.drupal.org/node/2557119#comment-10344893
If I set the discount to be compatible with all other discounts, it works fine.
Comment #9
torgosPizzaInteresting! Well I'm glad we've figured out something more akin to a root cause.
Since THAT issue is considered "Fixed", perhaps we should rename this issue to be more specific, since I was also seeing this problem when I had a mixed-discount-compatibility scenario as well. I'll admit, I actually haven't used the compatibility feature since experiencing those issues, so this would be a good way for you and I to try and tackle this thing, since it's something we have both encountered.
Comment #10
torgosPizzaChanging the title and referencing a related issue.
Comment #11
mishac CreditAttribution: mishac commentedshould the issue be moved to commerce_discount or is it still appropriate to have it here?
Comment #12
torgosPizzaTagging, and including my findings from the previous issue:
Comment #13
torgosPizzaAh yes good point. Let's move it to Commerce Discount.
Comment #14
torgosPizzaFixing title.
@mishac: Can you see if this problem still exists without a coupon? I'm curious to see if removing the coupon from your order allows the discount to stick around. Because if so then this may in fact be something related to #2619448: Cart Refresh check for discount price components causes Coupon removal. If you can test that situation I'd love to know what the result is!
Comment #15
torgosPizzaYep so as I discovered in my previous comments, the Rules condition commerce_discount_compatibility_check only checks the entire order for discounts, rather than - in the case of a lot of Coupons - the product level price components. But since it's checking at the order level I imagine this problem would also exist for order-level discounts.
Not sure how much work it would take but since the product discounts (which we may remove in a future update) use the "Calculating the sell price" event on an individual product, it doesn't make much sense to have this Rules Condition commerce_discount_compatibility_check always be looking at the order level all the time.
EDIT: Now I can't get this issue to reproduce, at least so far. I'll continue to dig.
Comment #16
Scott Robertson CreditAttribution: Scott Robertson commentedI've experienced similar problems with the compatibility settings and have had to just use the default option for all of my discounts to prevent them from being removed. Still working on steps to reproduce with a fresh Kickstart install.
To quote myself from issue #2619448, in my testing I encountered some issues with the compatibility check function:
I'm hoping that I might be able to dedicate some serious time to this today or tomorrow, so I'll post any more findings in this issue.
Comment #17
Scott Robertson CreditAttribution: Scott Robertson commentedAfter further testing, commerce_coupon is not even required to reproduce the core issue. My steps to reproduce:
Each time you refresh the cart (and the other checkout pages), the discounts will toggle on and off. I.e on one page load, you will see BOTH discounts applied to the order total (which also isn't correct as they should be exclusive). On the next page load, both discounts will disappear from the total.
This in turn affects Commerce Coupon, because as soon as you apply a coupon to your order during checkout, the discount will no longer be present on the next page load, causing the rules in Commerce Coupon to remove the coupon code entirely from the order.
Now that I have this occurring on a fresh Kickstart install, I'll start debugging to hopefully come up with a solution/patch.
Comment #18
torgosPizzaHi Scott,
That makes sense. As you had noted in the other issue, I believe that the issue is in fact that the discount is checking for compatibility with itself (since it's part of the $applied_discounts array).
I think a good solution might be the following:
Your original solution was:
Which could also work but I don't know if you'd want to necessarily unset that discount from the applied discounts (otherwise it could be possible that the discount gets removed?) but if we break the loop that should keep the discount intact while bailing on continuing to check for other conditions. I assume you've been testing without this solution in place. Or have you? In which case your proposed solution is not enough?
Would also love to know if @mglaman or @rszrama have an idea. This is a regression and hopefully people aren't using this feature with these settings, since it essentially makes the feature broken.
Thanks for your continued work on this!
Comment #19
Scott Robertson CreditAttribution: Scott Robertson commented@torgosPizza With my previous solution in place, the issue of the discounts being removed is resolved, however the other issue of multiple discounts being applied when they shouldn't still exists.
I believe this is due to the fact that the $order_wrapper variable passed to commerce_discount_compatibility_check() is always a "fresh" version of the order, without any discount price components added to its total, so it causes each discount rule to evaluate to TRUE when commerce calculates the sell price of the first line item.
I'm currently testing a different solution that I've come up with where commerce_discount_compatibility_check() ALSO looks at the line item's total field to look for any applied discounts. However, I'm not sure how well this is going to work with order-level discounts, so I'm currently doing some more testing.
I'll generate a patch for what I have so far and attach it shortly, in case anyone else wants to work off of it.
Comment #20
Scott Robertson CreditAttribution: Scott Robertson commentedAttaching what I've been working with thus far. I haven't had a chance to test order discounts yet, but this seems to help with conflicting product discounts.
EDIT: should be applied against latest DEV release.
Comment #21
torgosPizzaIntriguing. I like that you changed this from order- to line-item-level. Also note there is discussion about moving all product level discounts to the order level:
#2621680: Remove Product Display discounts and only use order discounts for 1.2
#2107451: Apply percentage order discounts to each line item
#2276227: [META] Use order discount with VAT
Since there's a patch here, setting to Needs Review. Thanks for working on this!
Comment #23
joelpittetTBH I don't understand this compatibility settings much at the moment, it's a fairly new feature. Assigning to Ryan to review this bug.
@Scott Robertson feel free to keep working at your patch in the meantime.
Comment #24
Scott Robertson CreditAttribution: Scott Robertson commentedIt looks like a bunch of the order discount tests failed, which is pretty much what I expected. I'll continue working.
Comment #25
torgosPizzaYeah, I have this feeling that the culprit of the tests failing is moving the compatibility from order to line item.
I think we're going to need @rszrama to chime in here to figure out an approach. There are benefits to both methods (and as I had mentioned there is talk of moving everything to a line item, which would also benefit product discounts due to the chicken-and-egg scenario I have encountered with price components) but I think that we may, at least for the time being, want to keep things in the order level just for consistency sake.
Unless and until we can come up with a standardized way of dealing with discounts at a line item level. It may require refactoring a lot of code, e.g. always ensuring a "commerce_discounts" array is added to the order whenever a product or order-level discount is attached. In my experience this array not being set with product level discounts was a cause of much of the issues we faced - since a lot of the cart refresh code would look for it. Since EMWs are chainable and a line item can easily reference its parent order wrapper I think this would make the most sense. Taking this a step further then I wonder if we want to move the compatibility of a given order to such an array in the same way - rather than checking it at runtime every single time, we attach this as a new array (or even within the $order_wrapper->commerce_discounts array) and deal with it there. Not sure if that'd be better or worse honestly but it's an idea I have at the moment.
Comment #26
joelpittetConsidering hiding this feature due to complexity/bugs it introduced.
#2632072: Move compatibility feature to optional experimental feature
Comment #27
torgosPizza@Scott: Does this issue only occur if you use a Discount with a Coupon?
Comment #28
Scott Robertson CreditAttribution: Scott Robertson commented@torgosPizza In my testing, the issue occurred regardless of whether or not Commerce Coupon was even installed. When I tested using a fresh Kickstart install for my steps in #17, I didn't use any coupons.
Comment #29
jncrucesWe have this issue too.
The checkout process is correct and at the end of the process using a commerce_order_load to get the order and save the transaction of the payment this order haven't any discount_coupon.
Edit:
The coupon is removed because the related discount not exists on the order.
I'm sorry my continous editing, i must resolve this today. Bad idea attempt to collaborate while thinking :D
Comment #30
drewmacphee CreditAttribution: drewmacphee commentedSo I'm seeing the same thing.
Product discount with a coupon. As soon as I set it to "Not with any other discounts" it'll get removed when I hit go back from the review page.
Is there a work-a-round? We'd like to ramp up promotions but I don't want people using multiples.
Comment #31
torgosPizza@drewmacphee: No, that feature is broken and should be considered experimental at this point. See #2632072: Move compatibility feature to optional experimental feature
Comment #32
andyg5000Here's a simple patch that fixes the issue. In #20, I don't think we need to check for components at the line item level since they should also be in the order total price object. Also, #20 rewrites the rules parameter which would likely cause problems with sites that are already expecting order as the parameter.
Comment #33
torgosPizza@andyg5000: Nice! I'd be interested to see if this solves others' issues as well.
Comment #34
rszrama CreditAttribution: rszrama at Centarro commentedYeah, fwiw I tested this locally w/ Andy and it did the trick. Just wanted to see how the tests turn out. : )
(And I don't even think there's a way to change the condition from using an order to the line item... that would require a change to the entire way the discount rules are built, which wasn't included in the patch in #20.)
Comment #35
torgosPizzaHow are you guys testing this? For me it still fails in this scenario:
- Add two discounts ("A" and "B") that could both be applicable for one product. (This is a product-level discount.)
- Add the product to your cart.
With "any" discount compatibility, it works, and both discounts or added.
BUT if you edit discount A, so to allow "Any discounts except 'B'" to be compatible, neither discount is applied. This is because in the 'except' strategy, and pretty much every other strategy, an incompatible discount is stripped from the order when it should probably be stripped from the price components.
In its current form this compatibility logic makes it impossible to have multiple product discounts on an order, so to me, this is still Needs Work.
Comment #36
torgosPizzaUpon further testing... not sure if this had to do with saving a Rule again or adjusting the Weight orders of the active Discounts (something possibly triggering a Rules cache clear) but... I'm happy to report that my scenario above actually DOES work now (in fact, an even more complicated scenario works:
1. I have a 50% off discount good on ALL products.
2. I have a "Deal of the Week" for one specific product.
3. I have an "HD Upgrade" custom discount written in Code.
In my setup the only compatibility setting is that Discount #2 "Deal of the Week" is compatible with ANY discount EXCEPT "HD Upgrade"
Previously this scenario would result in both of those discounts being removed.
Now, though, (magically?) I get the correct discounts applying:
- Two items in cart, one regular product, and another product that a) I have an HD Upgrade available for me, and b) is also marked "Deal of the Week"
- One of them is 50% off.
- The HD Upgrade discount is applied.
- Deal of the Week is not applied (which is the way it should be).
OF course in this given scenario the HD Upgrade product which is normally $2 is now $1 because the 50% off discount is working for it as well. So I will probably adjust that to exclude HD Upgrade and with any luck nothing will break.
This makes me very happy, and I'm happy to set this back to RTBC. Seems no matter what I change in the Discounts config area now, the discounts remain applied, which is awesome.
Comment #37
andyg5000Thanks Erik.
I noticed some weird things when testing your scenario as well, but it turned out to be caused by an existing discount on the order. For others testing this, be sure to test a brand new order with each scenario.
Comment #38
torgosPizza@andyg5000: That could have been it. It seemed to go away and then come back whenever I changed the compatibility settings, but then when I added a new discount it seemed to work fine. Not really sure.
I'm okay with calling this RTBC for now, since overall it does seem to help things. If I discover other weirdness I'll try to replicate it more studiously.
Comment #39
rszrama CreditAttribution: rszrama at Centarro commentedI created two product discounts:
With no restrictions on compatibility, they work fine.
With discount 1 set to not be compatible with discount 2, I see the old behavior of discounts reappearing then not appearing then reappearing.
I believe the issue here is with using an order total price components array that hasn't been cleared of all price components at the start of the price calculation process. So, what happens is the first time it tries to apply, it has a price components array with nothing in it... so it adds both discounts. The second time around, it detects the incompatible discount's presence and doesn't apply either.
I'll dig into it a little more to see if we have a safe way to reset the order total at the beginning of sell price calculation. That really may need to be a core patch to Drupal Commerce. Alternately, we can stop using the price component array and use a pseudo-property on the order object to identify discounts added to the order. This would never be saved to the database, so we'd know it was initialized empty in subsequent compatibility checks.
Comment #40
torgosPizza@rszrama: That sounds exactly like the issue I was seeing in #35. Happy to help test any potential fixes!
Comment #41
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commentedI have also been able to confirm the fix with the patch from #32, although also noticing some weirdness ( which I will also look into ).
Comment #42
torgosPizza@mdupree: Can you elaborate on the weirdness you're seeing?
Comment #43
mdupree CreditAttribution: mdupree as a volunteer and at Acro Commerce commented@torgosPizza Sorry disregard that last message, I believe I was the cause. Going to retest on a fresh install as soon as I get some time.
Comment #44
torgosPizzaIt looks like product-level discounts are still an issue. My scenario:
1. An "HD Video" product that has an "HD upgrade" Product Discount applied to it.
2. A Product Discount available to ALL products that applies 50% off (as a test) that is compatible with all discounts "except" the HD Upgrade discount.
I have two items in my cart:
1. Regular product (test should apply, hd upgrade should not)
2. HD Video product (upgrade should apply, but not test)
I'll try to dig in a bit more, again, and see if my earlier comments around price components being the culprit still holds.
Comment #45
mdupree CreditAttribution: mdupree commented- upgrade to commerce_discount 7.x-1.0-alpha7
- Created 3 discounts
1. 50% off everything
2. Deal of the week (product discount on specific Bag product) (compatible with anything, except mug sale)
3. Mug Sale (product discount on specific Mug product)(compatible with any discount)
- I add any product ( and get the 50% off everything )
- I add the Mug ( adds the mug sale )
- I add the Bag ( No deal of the week )
--Works good
Comment #46
Scott Robertson CreditAttribution: Scott Robertson commentedI've been doing more digging into this and came to the same conclusion as Ryan. The main issue with the compatibility check seems to be that when the function is called, the order's total always contains every discount applied to the order at the time of the request.
I've been able to reproduce the disappearing and re-appearing issue via the following steps. Please note that all of this is WITH the pitch from #32 applied, as I believe we still need @andyg5000's approach, along with some additional tweaks.
Assuming that both discounts are applied to the order on the initial page request, this is what happens:
commerce_discount_compatibility_check
determines that discount #1 is not compatible with any other discounts and evaluates to FALSE because discount #2 is applied to the order.commerce_discount_compatibility_check
determines that, while discount #2 is compatible with ALL discounts, discount #1 is still applied to the order, so the logic yet again evaluates to FALSE.The result is now an order with BOTH discounts removed, when only 1 should have been.
The same situation occurs when you refresh the page again. Both compatibility rules fire and both see that no discounts are applied to the order yet, so BOTH discount #1 and #2 are applied, even though only 1 of them should be.
The best solution I can come up with is to use a static variable to keep track of which discounts are still applied to the order as each compatibility rule fires. Whenever a compatibility rule evaluates to FALSE, when remove that discount from the static variable, which we then use in our subsequent compatibility checks.
So far, this method seems to be working for me, but I need to get a cleaner patch going and run some tests.
Comment #47
Scott Robertson CreditAttribution: Scott Robertson commentedRunning patch through tests.
Comment #49
Scott Robertson CreditAttribution: Scott Robertson commentedHmmmm will look into why the tests are failing, I seem to be getting the expected results when I run it through simplytest.me, so I'll have to see what the difference is.
Comment #50
Scott Robertson CreditAttribution: Scott Robertson commentedAttaching a new patch that should pass tests. I'm not super sold on the static variable solution, but I can't think of a better way at the moment without making alterations to the order's actual total.
As a side note, combining product and order discounts doesn't play well with compatibility. I think this is because the compatibility of product discounts will ALWAYS be calculated before the compatibility of order discounts, regardless of what sort order you use. This is because the "Calculating the sell price of a line item" event is always invoked before the cart order refresh event, but that comes from core Commerce and would require a lot of reworking on the discount end of things and isn't really related to this issue.
Comment #51
Scott Robertson CreditAttribution: Scott Robertson commentedAll tests appear to be good, would love to get some more sets of eyes on this, though, especially in combination with coupons.
Comment #52
Scott Robertson CreditAttribution: Scott Robertson commentedComment #53
Scott Robertson CreditAttribution: Scott Robertson commentedI opened up #2690697: Product discounts are always evaluated before order discounts, regardless of sort order to keep track of my note in #50. Solving that issue may in fact solve this one as well.
Comment #54
Scott Robertson CreditAttribution: Scott Robertson commentedI attempted to rework my patch to use an arbitrary array attached to the $order object that would keep track of the applied discounts (instead of a static), but the problem seems to be that rules acting on
commerce_product_calculate_sell_price
are given a different instance of the order than what is used incommerce_cart_order_refresh
. I'm assuming this is because the order isn't explicitly passed into the rule event.The result of this is that product discounts can't determine their compatibility with order discounts, only with one another. Likewise, order discounts can only determine their compatibility with one another as well. To solve this, it seems like the order itself has to be passed when
commerce_product_calculate_sell_price
is invoked.Comment #55
torgosPizzaNice detective work, Scott. I'd love to know more from Ryan et al about why an order isn't passed to commerce_product_calculate_sell_price() but it's possible there is a technical reason for that.
I agree with your direction of a static variable, though, and I actually like that solution better than attaching an array of all discounts to the order, which was proposed as another potential solution to this issue.
Comment #56
Scott Robertson CreditAttribution: Scott Robertson commented@torgosPizza The product discounts get to the order by using the line item (i.e. the selector in rules is just commerce-line-item:order). Normally it seems like it would be fine because that order is just loaded out of the entity static cache, but it appears that the $order variable passed to
commerce_cart_order_refresh
is NOT the same object as what is in the entity static cache.I did a quick test of this via the following code:
I'm just not sure whether this is intentional or not, as I haven't really found out WHY the object's aren't the same yet.
Comment #57
Scott Robertson CreditAttribution: Scott Robertson commentedSetting this back to needs work for now, as I see that I'm getting bit by the same issue of getting two different instances of the order object :(
Comment #58
Scott Robertson CreditAttribution: Scott Robertson commentedComment #59
rszrama CreditAttribution: rszrama at Centarro commentedHey dude, non sequitur, but are you sure you aren't supposed to be named Sean Robertson?
Comment #60
Scott Robertson CreditAttribution: Scott Robertson commentedI've got a first attempt at a patch for the issue I noted in #56 going over at #2690739: commerce_cart_order_refresh receives a different order object than its subsequent rule events. Going to see if I can rework my patch from #50 a bit as well...
@rszrama Just what we need, a third Sean in the office :P
Comment #61
maxplus CreditAttribution: maxplus commented@Scott Robertson
Just to let you know: I'm testing your patch from #50 and it seems to work for me even when I use the product compatibility discount option "Not with any other discounts"
Before you patch I was getting the same strange behaviour when refreshing my cart form or checkout review pane (discounts disappear and re-appear)
I'm not using Commerce Coupon and running the current Commerce Discount 7.x-1.x-dev from 2016-Mar-22.
Until now, I'm only using product discounts, no order discounts.
Thanks for your great work already!
Comment #62
mishac CreditAttribution: mishac commentedUnfortunately I'm in a situation where I have both order and product discounts, and @Scott Robertson's patch in #50 isn't doing the trick for me. I no longer have the behavior with the discounts applying and then disappearing in the cart or review page, but now the discount just won't apply at all.
Is it too late vote to move to move the compatiblity to an experimental feature we can disable?
Comment #63
rszrama CreditAttribution: rszrama at Centarro commentedYes, it's too late. Let's fix this. It's hardly a regression at this point given "all" was the only behavior before. : P
Comment #64
torgosPizza@ryan: Can we completely gut and rewrite how discounts are stored / evaluated? :)
I feel like #2690697: Product discounts are always evaluated before order discounts, regardless of sort order and #2690739: commerce_cart_order_refresh receives a different order object than its subsequent rule events probably need to get Fixed first as they feel like they could potentially be the root cause of much of this.
@Scott: Have you had any luck in this realm with the patch you're working on in #2690739: commerce_cart_order_refresh receives a different order object than its subsequent rule events? Since the tests came back green I'm wondering if that one should get some more eyeballs and possibly committed to Commerce dev.
Comment #65
Scott Robertson CreditAttribution: Scott Robertson commented@torgosPizza
That patch does seem to work OK. It's a bit hacky for my tastes, but I can't come up with a better way, aside from having to submit a patch to core so that the load hook is called after static caching.
We're in a bit of a unique situation where we're attempting to load an entity from within that exact same entities load hook, which normally would result in an infinite loop. But unless we want to rewrite even MORE of commerce_core so that the sell price calculation rule somehow always accepts an order, the patch may just have to suffice.
Assuming that the patch in #2690739 (or a similar fix) is committed, I'm thinking my next steps are as follows:
1) Continue my work on the patch in #50 and at least get compatibility settings working for the most part.
2) Unfortunately my patch won't fix #2690697: Product discounts are always evaluated before order discounts, regardless of sort order. That is going to require a far larger rework of how discounts are evaluated and applied. Essentially I think we're going to need to split up logic so that discount evaluation occurs first and THEN we actually apply it.
Comment #66
czigor CreditAttribution: czigor at Liip for FREITAG lab. AG commentedThanks for working on this! Patch works for me.
Rerolling and removing an unused function.
Comment #67
torgosPizzaComment #68
jmary CreditAttribution: jmary as a volunteer and commentedThe patch #66 works
Comment #69
joelpittet@rszrama What do you think?
Comment #70
torgosPizzaPatch in #66 also seems to resolve this particular issue. (Other child issues will need their own work but I think this is a good first step.)
Thanks for everyone's work on it so far. I'll mark it as RTBC pending any further review from Ryan.
Comment #71
torgosPizzaDoing more testing, and it seems like anything other than "Any" now prevents coupons from being applied.
However, I'm not sure if this is something that the Coupon module needs to fix itself in order to work correctly with the compatibility strategies. I'll keep digging.
EDIT: Confirmed. Here are my steps:
1. Created two product discounts. A is applied automatically, B via coupon.
2. Coupon discount B (coupon) is set to "Only" be compatible with the Discount A.
3. At checkout, I have two products in my cart, one of which is compatible with A, the other with B. (A is applied successfully already.)
4. Attempt to apply the coupon code
5. Receive the error "Coupon could not be applied."
Setting Discount B back to "Any" allows the coupon to be applied.
I stepped through this with XDebug and it looks like when these other compatibility settings are set, the discount price component is for some reason NOT added to the price array. Setting Discount B back to Any results in the component being added successfully.
The price component not existing in the first scenario is why the check in commerce_coupon_order_coupon_code_discounts() fails, because the components array is inspected and their order IDs pulled from there, as opposed to Order discounts who keep a discounts array on the order itself.
Comment #72
torgosPizzaI disabled some of my other discounts and now it is working fine, so that's pretty interesting. I will keep digging but I am starting to think there was a conflicting discount of some kind.
Comment #73
torgosPizzaI think I've figured it out. This snippet of logic from commerce_discount.module in commerce_discount_evaluate_compatibility():
The issue is that most of our other discounts are compatible with "Any" other discounts. During this phase all possible discounts are added to the $applied_discounts array and evaluated in turn. However when using the "except" strategy, the $applied_discounts array are evaluated while ignoring their own conditions, resulting in $discount_is_compatible evaluating to FALSE.
If I change:
if (in_array($applied_discount_id, $selection)) {
to
if (in_array($applied_discount_id, $selection) && $existing_strategy != 'any') {
the other discount is allowed to apply, and the logic from the compatibility strategies is correctly maintained. (I also needed to set a default value for $existing_strategy to 'any' outside of the rest of the loop, or else it might possibly be undefined.)
I have tested this with various mixed scenarios with only/any/except as well as other discounts and so far it seems to be working correctly now.
Can someone else please try to reproduce my issue and then test with various discount scenarios? The issue I saw was that, when setting a discount to "except" or "any" it seemed to affect other discounts as well, not just the ones with the strategy in place. This rendered pretty much all of our other discounts (such as this weekend's Fourth of July sale) unusable. With this small change, everything seems to work properly, including restrictions on certain discounts.
Comment #74
potop CreditAttribution: potop commentedTwo testcases added. One for scenario mentioned in #17 and another one for discount's compatibility "with itself".
Comment #75
joelpittetThanks for adding test cases! Setting the status to see what the testbot says before reviewing them.
Comment #77
hevmills CreditAttribution: hevmills as a volunteer and commentedHi,
I have a live site suffering from not being able to apply a discount when it is set to not be compatible with any other discount.
I have tried #66 + the suggestion in #73, but have found that this stops commerce_price_savings_formatter from working. After applying the patch if you visit a page with a product which is on sale, the sale price doesn't display anymore. However, the moment you add a sale-price item to your basket then the sale price magically appears.
I have reverted the above patch, and now having done a bit of debugging I found that in my case, when just a single (non couponed) product-specific discount was being applied, every page fresh or so I would loose the discount line. Checking further, the discount line disappeared when commerce_discount_compatibility_check() was being called in commerce_discount.rules.inc. When the discount line vanished it was because the function was being called to check the compatability of the discount when this discount was already in $applied_discounts. This caused the function to return FALSE as it thought it already had a discount code applied, even though it was one in the same discount.
I have reverted back to the version of commerce_discount in commerce_kickstart-7.x-2.39 (which is 7.x-1.0-alpha8)
I have added the following line to commerce_discount.rules.inc, which seems to solve the problem for me:
Comment #78
lathanWhen commenting out the lines in commerce_discount-2621526-compat-settings-remove-discounts-66.patch
You can then apply a discount that is not compatible, what seems to be happening is that is some how caching discounts that are no longer applicable or removed.
Comment #79
lathanHere is a reroll with
- torgosPizza changes in #73
- combined potop tests in #74
- removed invalid caching check as per #78
Comment #81
scotwith1tmy symptoms are similar (coupon is removed when a discount's conditions are no longer met), but my discount in this case has compatibility set to all, which seems to contradict the OP here. i tried applying the patch in #79 (minus the extra lines at top and bottom, which may be why it's failing the bot) and no change for me. coupon and discount were both still removed once i no longer met a condition of the discount (order total > £50, in our exact use case; once i added the item to put me over 50, discount and coupon go bye-bye).
in our case, we'd even be ok with just a message saying what happened and why (which condition is no longer met).
Comment #82
jonaskills CreditAttribution: jonaskills commentedWhat version of Commerce Discount am I suppose to apply patch from #80 to? I am currently on 7.x-1.0-alpha8. Any idea when these changes will be added to the module? Thanks!
Comment #83
joelpittet@jonaskills The version they are to be applied to is the dev branch of 7.x-1.x-dev. It's usually on the issue and next to the patch. This will be added when it passes tests and someone has reviewed and tested it manually as well(Set the status to "reviewed and tested by the community").
Comment #84
torgosPizzaWhat's weird about the 2 remaining test failures is that they do not fail when I test them in an order on my local working copy. So I'm not sure why they are actually failing.
Comment #85
attiks CreditAttribution: attiks at Attiks commentedI ran into the issue of appearing and disappearing discounts on the cart page (multiple product discounts active) and the patch from #79 solved this after
drush updb
anddrush cc all
NR for bot, otherwise RTBC
Comment #87
leendertdb CreditAttribution: leendertdb as a volunteer commentedI could not apply the patch from #79 anymore, probably due to the 7.x-1.x-dev branch which has been updated.
I manually rerolled the patch of #79, which should apply cleanly to the latest dev branch as of 22 december 2016. Note: this patch has no functional changes, so it is essentially the same as #79.
Comment #88
torgosPizzaComment #90
goose2000 CreditAttribution: goose2000 commentedThank you all for working on this one, I discovered it was happening on our site last night. I called it the the 'blinking discount' cart bug.
Comment #91
hugronaphor CreditAttribution: hugronaphor at Acrosto for Dropsolid commentedI hope I'm not going to introduce even more confusion in this problem but I'll share my approach.
Yes, the compatibility strategy rules is a big mess up in general which I fixed by applying the patch on https://www.drupal.org/node/2618072#comment-11592333 + a lot of other patches for other issues.
To avoid Coupon discount being ignored, I did the following.
Given I have:
1. A discount with the compatibility option "Not with any other discounts";
2. A coupon --> Discount with the compatibility option "Any discount";
I would expect the coupon to be applicable on top of the discount 1.
While we need somehow to distinguish a discount entity which actually is a part of a coupon I added a new option as a compatibility strategy "Only with Coupons" and implemented related logic in "_commerce_order_get_discount_compatibility" so a coupon discount is always applicable if the "Only with Coupons" is chosen.
Attaching a patch created for my local version of the project.
Comment #92
dmsmidtCurrently testing #87, at least things work better now.
One nit (Typ0):
Comment #93
dmsmidtI found a simple setup without coupons, only product discounts, that didn't work with the patch from #87:
One % discount compatible with no other discounts and one other % discount compatible with all.
Result: both discounts where applied. Shuffling the order didn't help.
Here is a patch that simplifies
commerce_discount_compatibility_check()
. There was a lot of logic in there that wasn't clearly documented why it was setup like that, so maybe other stuff breaks now. But at least the scenario I mention works.I also tested adding a product coupon with a lower weight incompatible with all, and it was neatly applied. With a higher weight, and it was skipped.
Note: the weight of the discounts is important. The first discount that applies, is the one that sticks, later incompatible discounts don't apply. The incompatibility can be set from the 1st or 2nd discount.
Comment #94
dmsmidt@91, I wouldn't make the same assumption. Depending on the weight of the discounts I would think only the lowest weight discount would apply, not both.
Comment #96
dmsmidtIt is needed to evaluate compatibility after all other rules conditions.
Otherwise it is for example never possible to prevent a discount to apply when any other discount is present.
There is a bug in Rules that prevents this #2864402: Children (conditions) weight not stored when saving programatically.
Here is a patch that fixes the problem in this module until Rules is fixed.
Comment #98
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commentedThis is the current workflow of applying order and product discounts:
1.
commerce_cart_order_refresh()
gets called. The order total and line item price fields contain all discounts that have been applied the last time.2. Pricing rules are invoked (
commerce_product_pricing_invoke()
). This is where we are trying to attach product discounts.2A. Discount compatiblity checks in
commerce_discount_compatibility_check()
still rely on the previously applied discounts.3. In the end of
commerce_cart_order_refresh()
,hook_commerce_cart_order_refresh
is invoked. This adds order discounts incommerce_discount_commerce_cart_order_refresh()
. This goes like this:3A First all order discount price components are removed from the order total and the line items' unit and total prices.
3B. Then all order discount line items are removed
3C. Then all order discount line items are re-added by invoking the
commerce_discount_order
rules event.4. Finally the order gets saved, updating the order total with discount price components.
As @rszrama said above, the problem with this is that product discount compatibility checks don't see a fresh order total price but one with discount components in it. We have to make sure that only those discounts are found as 'applied' in
commerce_discount_compatibility_check()
that have been applied in the current cart refresh.The attached patch implements hook_commerce_cart_order_pre_refresh() to purge all price data of discount components at the very beginning of the cart refresh process. Then it adds quite a few entity saves (line item and order) to make sure entity metadata wrapper sees the price components it should see.
Comment #99
czigor CreditAttribution: czigor at Centarro for FREITAG lab. AG commentedStealing and adding the tests from #96. They are green locally.
Comment #100
czigor CreditAttribution: czigor at Centarro commentedRemoving an accidental info file change.
Comment #101
GuyPaddock CreditAttribution: GuyPaddock at RedBottle Design, LLC for Inveniem commentedAfter applying the patch, we're able to use the "Not with any other discounts" option, but 1 out of every 5 customers is somehow getting a corrupt order / cart. We're not exactly sure what the repro steps are for these 1 out of 5, but basically during checkout the user starts getting EntityMetadataWrapper exceptions about a line item that is missing its commerce_unit_price.
The stack involves code that the patch from #100 is modifying, so either it's not updating the order properly, or it needs to catch this exception. The problem is that once an order gets into this state, the user can no longer access the site until we delete the order because even trying to display whether the user has any items in their cart will trigger the exception.
Here's the exception, with stack:
Comment #102
czigor CreditAttribution: czigor at Centarro commented@GuyPaddock Can it be that you have orders with broken line item references? commerce_unit_price is supposed to be set on all line item types (see
commerce_line_item_configure_line_item_type()
);.We could catch the exception but this might be an issue in your site's handling of line items.
Comment #104
jsacksick CreditAttribution: jsacksick at Centarro commentedOne comment regarding the order pre refresh hook (hook_commerce_cart_order_pre_refresh()), with the current code, all the referenced line items as well as the order will be saved everytime the cart order is refreshed, no matter what if discount price components were removed.
Maybe we should find a way to save it only if price components were updated? For that we can probably make commerce_discount_remove_discount_components() return a boolean indicating whether or not price components were updated?
Alternatively, we could check if the discount reference field is not empty at the beginning of commerce_discount_commerce_cart_order_pre_refresh()?
Comment #105
GuyPaddock CreditAttribution: GuyPaddock at RedBottle Design, LLC for Inveniem commented@czigor: Not disagreeing with you that it could be some other module on the site. We did have orphan line items, but the orphans are on new orders and corresponded to coupons that were recently added/removed from the order, so that's why I was wondering whether it was something in this patch that was causing it. As far as I know, I have not written any custom code to deal with our checkout. We're using Commerce Discount and Commerce Coupon, with this patch applied.
Unfortunately, until #2179303: Ensure Commerce core uses try / catch when using the entity metadata wrapper gets addressed, it's nearly impossible to track down exactly what sequence of steps causes the orphans to appear. I do know that in a beta group of 60 users, 10 users tried checkout already, and 2 ran into the line item orphans.
I don't think catching the exception is the appropriate way to go, since effectively the order is in a bad data state at that point. But I am wondering if there's anything wrong with order of operations or updates that could cause the order to be saved with coupon line items that no longer exist.
Comment #106
czigor CreditAttribution: czigor at Centarro commentedAdded the modifications recommended by @jsacksick.
Comment #107
torgosPizza@czigor and @GuyPaddock:
I'm able to reproduce the
EntityMetadataWrapperException
error after applying this patch as well. I receive the error when adding a coupon to the order, and then immediately clicking the coupon "Remove" link. It seems that attempting to retrieve$price_wrapper->value()
on a deleted line item throws the exception and stopping checkout flow.I think it's probably safe to put the call to that inside a try/catch, since an exception from retrieving a line item's price component simply means the line item is gone. Whether we want to log that happened is another story.
Also:
It looks like commerce_discount_commerce_cart_order_pre_refresh() expects an $order_wrapper. I think we'd need to call
$order_wrapper = entity_metadata_wrapper('commerce_order', $order->value());
or remove that line altogether and reuse the order_wrapper that is passed along... unless it is done this way here for a reason I'm not aware of.Comment #108
czigor CreditAttribution: czigor at Centarro commented@torgosPizza: I still can't reproduce it. Are you on a clean commerce install? I tried it with product and order discount coupons. My customer has the "View any coupon of any type" permission to make the order_coupon_list view show something.
As per the order_wrapper vs order thing: commerce_cart.api.php is wrong here, unfortunately the unwrapped order is passed to the hook in commerce_cart_order_refresh():
Just a reroll of the patch.
Comment #109
torgosPizzaAh, I see. That is some confusion then. (It doesn't help that every implementation of that hook refers to the incorrect
$order_wrapper
parameter ;))Not a clean install, no - working on our local copy of the site. I double-checked and it looks like the error is actually a Commerce Giftcard one, or at least, has something to do with how the entities themselves are structured. I might need to take a closer look at how those line items are (or are not) removed properly from an order. Perhaps that's the culprit throwing this red herring into the mix.
So I guess for me then it's RTBC because it works as advertised. I'm very excited to get a compatibility strategy enabled for our discounts. Thanks!
Comment #110
joelpittetPutting up the test only patch to see.
A bit concerned about the extra calls to
but that may the only viable solution.
See what the testbot says first.
Comment #112
czigor CreditAttribution: czigor at Centarro commented@torgosPizza: Commerce Giftcard is not using Commerce Discount. So if you can see the error only with the patch, it's probably not giftcard-related.
Comment #113
torgosPizza@czigor: You are right, I think there is something else happening with that. Please disregard.
The only other issues I was having, were my code-based discounts somehow bypassing the compatibility strategies I had configured for them. Turns out, the fix required adding the same lines from these patches to my own code:
Adding those lines, after the call to add a new price component to the line item, now correctly triggers pricing components elsewhere, and allows the discount compatibility structure to work exactly as expected. @joelpitter, this might be an issue further upstream, but at this point I agree that it seems like we need to recalculate totals in all cases. Hopefully that doesn't result in too much additional overhead.
Thanks for all the hard work on this; I think it is RTBC in my opinion.
Comment #114
jim.shreds CreditAttribution: jim.shreds commentedFor what its worth. Was chasing this issue for the past few days. Applied all patches with no forward movement. Turned off Commerce Checkout Paths module and everything worked again. This was only happening in my Prod environment on Acquia.
Comment #115
czigor CreditAttribution: czigor at Centarro commented@jim.shreds Can you describe when exactly the issue happened for you and what the issue was?
Comment #116
rszrama CreditAttribution: rszrama at Centarro commentedReroll for #2910160: Fix the "free shipping" offer type.
Comment #117
rszrama CreditAttribution: rszrama at Centarro commentedCommitting the rerolled patch and spawning child issues for resolving scoping problems between Product vs. Order level discounts.
Comment #118
torgosPizzaSweet! Thanks, Ryan.
Comment #119
rszrama CreditAttribution: rszrama at Centarro commentedCreated #2916183: Reduce the scope for Product discount compatibility checks to the line item level and #2916184: Group Active vs. Disabled discounts on the admin listing.