Compatibility settings other than "all" causes Discount+Coupon to be removed

Question 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

Thanks 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:

mostly fixes the symptom for now, but not the underlying issue

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

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.

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

Changing the title and referencing a related issue.

Tagging, and including my findings from the previous issue:

The current behavior is to remove all incompatible discounts from an order when a discount is present that proclaims itself to be incompatible with said other discounts. My question is, can we alter (or provide an option for) this behavior to, rather than remove all those discounts, simply not apply them?

For instance, I created a custom offer called "HD Upgrade" which is a product/line-item level discount. Using Entityreference and the magic of the Discounts system, if a user owns a Standard Definition product, but wants to upgrade, they can visit the product page where the HD version's price has been set to be the difference between the two (usually this is about $2).

As it stands now, if a user has a coupon code for say 15%, by default this price gets adjusted to about $1.70. When I select the "incompatible with any offers" option, it no longer receives the 15% off, which is awesome. However this removes the coupon code from the order, even if it's a product discount, which kind of doesn't make

I would love the logic to be more along the lines of, "The HD Upgrade discount is incompatible with other discounts, however it does not prevent you from receiving discounts on other products." I would imagine this would be checking the price components on the product, and removing the ones that are marked as incompatible.

I have confirmed that if you have coupon A with discount on it (in our case a 15% off product discount), and it is active in your cart, then you have a product with Discount B on it which is "not compatible with any other discounts", coupon A is removed, leaving you with just that sole Discount B getting applied. Even in the case where the other products do not fire Discount A (so no crossover between the two discounts).

Not only that, but if you add two products that can apply this product discount, they will cancel each other out and NO discounts will be on the order. Pretty weird behavior!

Definitely seems to be an all or nothing approach, and I feel like the logic should honor the type of discount that it is (order vs. product).

Ah yes good point. Let's move it to Commerce Discount.

Fixing 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!

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

I'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:

One thing I did notice is that the commerce_discount_compatibility_check() function sets a $applied_discounts variable by calling commerce_discount_get_discounts_applied_to_price(), and the discount that is being checked for compatibility is sometimes included IN the list of applied discounts.

This causes the compatibility check to fail when the discount has compatibility settings configured on it, because it ends up conflicting with itself! I.e. it thinks that another discount has already been applied to the order, causing the rule to evaluate to FALSE.

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.

After further testing, commerce_coupon is not even required to reproduce the core issue. My steps to reproduce:

1. Install Commerce Kickstart.
2. Upgrade commerce_discount to 7.x-1.0-alpha7.
3. Create two product discounts (the discount amounts do not matter), both with This discount is not compatible with any other discount. set.
4. Add a product to the cart.
5. Go to the cart.
6. Refresh the cart page multiple times.

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.

Hi 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:

foreach ($applied_discounts as $applied_discount_id => $applied_discount_name) {
    if ($applied_discount_name == $discount_name) {
      break;
    }
  }

Your original solution was:

foreach ($applied_discounts as $applied_discount_id => $applied_discount_name) {
    if ($applied_discount_name == $discount_name) {
      unset($applied_discounts[$applied_discount_id]);
    }
  }

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!

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

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

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

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

It looks like a bunch of the order discount tests failed, which is pretty much what I expected. I'll continue working.

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

Considering hiding this feature due to complexity/bugs it introduced.

#2632072: Move compatibility feature to optional experimental feature

@Scott: Does this issue only occur if you use a Discount with a Coupon?

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

We 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

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

@drewmacphee: No, that feature is broken and should be considered experimental at this point. See #2632072: Move compatibility feature to optional experimental feature

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

@andyg5000: Nice! I'd be interested to see if this solves others' issues as well.

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

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

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

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

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

I created two product discounts:

1. 30% off Everything
2. 50% off Product Two

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.

@rszrama: That sounds exactly like the issue I was seeing in #35. Happy to help test any potential fixes!

I have also been able to confirm the fix with the patch from #32, although also noticing some weirdness ( which I will also look into ).

@mdupree: Can you elaborate on the weirdness you're seeing?

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

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

- 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

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

1. Create 2 new product discounts that apply to all products. Make both discounts compatible with any other discount.
2. Add a product to your cart.
3. Go to the cart page, both discounts should be applied.
4. Now go and change discount #1's compatibility settings to "Not compatible with any other discounts".
5. Go back to your cart and refresh it multiple times. On one page load, both discounts will be applied. On the next page load, neither will be applied, and so on and so forth...

Assuming that both discounts are applied to the order on the initial page request, this is what happens:

1. Compatibility rule fires for discount #1. The list of applied discounts is grabbed using the order's total, which results in an array containing both discounts.
2. The logic in 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.
3. Discount #1 is removed from the line item's price components.
4. Compatibility rule fires fir discount #2. The same thing happens where the applied discount is grabbed using the order's total. HOWEVER, the order's total still contains BOTH discounts in its price components, so the resulting array is one that still contains both discounts in it.
5. The logic in 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.
6. Discount #2 is removed from the line item's price components.

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.

Running patch through tests.

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

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

All tests appear to be good, would love to get some more sets of eyes on this, though, especially in combination with coupons.

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

I 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 in commerce_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.

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

@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:

function commerce_cart_order_refresh($order) {
  $cached_order = commerce_order_load($order->order_id);
  
  // $same_object is FALSE because $order and $cached_order are not referencing the same object in memory.
  $same_object = ($cached_order === $order);

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.

Setting 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 :(

Hey dude, non sequitur, but are you sure you aren't supposed to be named Sean Robertson?

I'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

@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!

Yes, it's too late. Let's fix this. It's hardly a regression at this point given "all" was the only behavior before. : P

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

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

Thanks for working on this! Patch works for me.

Rerolling and removing an unused function.

The patch #66 works

@rszrama What do you think?

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

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

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

I think I've figured it out. This snippet of logic from commerce_discount.module in commerce_discount_evaluate_compatibility():

  // If the discount is not compatible with selected discounts, ensure they
  // are not on the order.
  if ($strategy == 'except') {
    foreach ($applied_discounts as $applied_discount_id => $applied_discount_name) {
      if (in_array($applied_discount_id, $selection)) {
        return FALSE;
      }
    }
  }

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.

Two testcases added. One for scenario mentioned in #17 and another one for discount's compatibility "with itself".

Thanks for adding test cases! Setting the status to see what the testbot says before reviewing them.

Hi,

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:

function commerce_discount_compatibility_check(EntityDrupalWrapper $order_wrapper, $discount_name) {
  // Ensure the discount we're loading still exists.
  if (!$discount = entity_load_single('commerce_discount', $discount_name)) {
    return FALSE;
  }

  // Get the strategy from the compatibility strategy field.
  $strategy = commerce_discount_get_compatibility_strategy($discount);

  // Determine which discounts have been applied to the order thus far.
  $order_total = $order_wrapper->commerce_order_total->value();
  $applied_discounts = commerce_discount_get_discounts_applied_to_price($order_total);

+  // Remove the current discount from the array so we don't compare it.
+ unset($applied_discounts[$discount->discount_id]);

When commenting out the lines in commerce_discount-2621526-compat-settings-remove-discounts-66.patch

  // foreach ($applied_order_discounts as $key => $info) {
  //   // Try and find an existing array containing the discount information. If
  //   // we've already processed this discount before, we should start again from
  //   // scratch.
  //   if ($info['order'] === $order_wrapper->value() && !in_array($discount_name, $info['processed_discounts'])) {
  //     $discount_information = &$applied_order_discounts[$key];
  //     break;
  //   }
  // }

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.

Here is a reroll with

- torgosPizza changes in #73
- combined potop tests in #74
- removed invalid caching check as per #78

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

What 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!

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

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

I 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 and drush cc all

NR for bot, otherwise RTBC

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

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

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

Currently testing #87, at least things work better now.

One nit (Typ0):

+++ b/commerce_discount.module
@@ -1204,3 +1206,95 @@ function commerce_discount_delete_discount_line_item_by_name($order_wrapper, $di
+  // Get the selected discount value from the disocunt being evaluated.

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

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

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

This 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 in commerce_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.

Stealing and adding the tests from #96. They are green locally.

Removing an accidental info file change.

After 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:

EntityMetadataWrapperException: Uncaught exception 'EntityMetadataWrapperException' with message 'Unable to get the data property commerce_unit_price as the parent data structure is not set.' in modules/contrib/api/entity/includes/entity.wrapper.inc:457

in EntityStructureWrapper::getPropertyValue called at modules/contrib/api/entity/includes/entity.wrapper.inc (86)
in EntityMetadataWrapper::value called at modules/contrib/ecommerce/commerce_discount/commerce_discount.module (114)
in commerce_discount_remove_discount_components called at modules/contrib/ecommerce/commerce_discount/commerce_discount.module (21)
in commerce_discount_commerce_cart_order_pre_refresh called at ? (?)
in call_user_func_array called at /srv/bindings/41edec478d0348ee8f6a4bdfbb6258c5/code/includes/module.inc (957)
in module_invoke_all called at modules/contrib/ecommerce/commerce/modules/cart/commerce_cart.module (1069)
in commerce_cart_order_refresh called at modules/contrib/ecommerce/commerce/modules/cart/commerce_cart.module (763)
in commerce_cart_commerce_order_load called at ? (?)
in call_user_func_array called at /srv/bindings/41edec478d0348ee8f6a4bdfbb6258c5/code/includes/entity.inc (368)
in DrupalDefaultEntityController::attachLoad called at modules/contrib/ecommerce/commerce/modules/order/includes/commerce_order.controller.inc (140)
in CommerceOrderEntityController::attachLoad called at /srv/bindings/41edec478d0348ee8f6a4bdfbb6258c5/code/includes/entity.inc (206)
in DrupalDefaultEntityController::load called at /srv/bindings/41edec478d0348ee8f6a4bdfbb6258c5/code/includes/common.inc (8048)
in entity_load called at modules/contrib/ecommerce/commerce/modules/order/commerce_order.module (782)
in commerce_order_load_multiple called at modules/contrib/ecommerce/commerce/modules/order/commerce_order.module (751)
in commerce_order_load called at includes/menu.inc (603)
in _menu_load_objects called at includes/menu.inc (771)
in _menu_translate called at includes/menu.inc (482)
in menu_get_item called at includes/menu.inc (1766)
in menu_get_custom_theme called at includes/menu.inc (1781)
in menu_set_custom_theme called at includes/common.inc (5334)
in _drupal_bootstrap_full called at includes/bootstrap.inc (2558)

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

One 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()?

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

Added the modifications recommended by @jsacksick.

@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:

+function commerce_discount_commerce_cart_order_pre_refresh($order) {
+  $order_wrapper = entity_metadata_wrapper('commerce_order', $order);

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.

@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():

  $order_wrapper = entity_metadata_wrapper('commerce_order', $order);

  // Allow other modules to act on the order prior to the refresh logic.
  module_invoke_all('commerce_cart_order_pre_refresh', $order);

Just a reroll of the patch.

Ah, 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!

Putting up the test only patch to see.

A bit concerned about the extra calls to

+      commerce_order_calculate_total($order);
+      commerce_order_save($order);

but that may the only viable solution.

See what the testbot says first.

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

@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:

      // Save the line item so that it correctly appears in the subsequent
      // order total calculation.
      $wrapper->save();
      // Include the discount price components in the order total so that
      // commerce_discount_compatibility_check() can find them.
      $order = $wrapper->order->value();
      commerce_order_calculate_total($order);
      commerce_order_save($order);

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.

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

@jim.shreds Can you describe when exactly the issue happened for you and what the issue was?

Reroll for #2910160: Fix the "free shipping" offer type.

Committing the rerolled patch and spawning child issues for resolving scoping problems between Product vs. Order level discounts.

Sweet! Thanks, Ryan.

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