Promotion weight is not respected if applied via coupon

There has been several issues opened related to this behavior. changing this now would be a BC. Applying coupon based promotions first was done by design afair, I don't completely recall the rationale behing this decision though.

This is a common use case in which the store is on sale and a coupon is provided to be applied on top of that. However, because the coupon is always applied first, you unexpectedly provide a larger discount than expected.

The client is perplexed as to why this occurs. To make matters worse, we can't explain why it "has to be this way."

@zaporylie's proposal appears to be very feasible, don't you think, @jsacksick?

I have a different problem regarding the promotion weights of coupon and non-coupon promotions.

You cannot apply a coupon promotion that is not compatible with others, when a non-coupon order already applies to a certain cart - no matter which weights the promotions have.

A concrete example, we have:

• promotion A: weight -1, with coupon
• promotion B: weight 0, no coupon, automatically added at 50,-
• promotion C: weight 1, no coupon, automatically added on having certain brands with a minimum value in the cart

It's no problem, when A and B get combined, also no problem B+C, but A+C should never be allowed. We do not care, if A is applied and B cannot be applied then anymore, but it should always be possible to enter the coupon code of A.

So, let's assume our cart's total is above 50,- - so B and C get applied automatically.

If we make A compatible with other promotions, we are allowed to enter the coupon code and A gets applied additionally -> but we do not want to allow A+C, so that's a problem.

If we declare A as non-compatible with others, we simply do not have any chance to get the coupon code applied, although it's prioritized first, but unfortunately the automatically added promotions are already applied to the cart, so A loses this battle and won't get applied at all

If coupon promotions are always applied first, why is there the option to set the weight in the first place?

Isn't the point of ordering the weight of the promotions to let the admin decide what they want? or am I missing the point?

I'm not against changing this... We'd probably need to expose this setting in a new UI though and tests coverage...
Also, as far as I remember, whenever I tried merging coupon based promotions with the ones returned by loadAvailable(), tests were failing... I think the promotion compatibility doesn't work as expected in this case.

I don't exactly remember, but there was some logic failing...

@zaporylie: Let me know if you're still interested to work on this.

@jsacksick It can be tested.
I've added a new field for the promotion entities to control when it will be applied, before or after coupons.
I guess it should not be a breaking change and should give more flexibility, just in case.
We can add additional configuration to define the default value for a new field to affect all promotions if needed.

Hm... I was thinking more of a global configuration / flag.
On new installs, promotions / coupons should be evaluated at the same time, and for existing installs, the behavior should remain the same.

Handling this as a setting that can be set from the settings.php isn't exactly ideal as it can be easily missed.
I don't think it makes sense for this flag to be per promotion, but open to discussing that during our next call.

Proposal for the setting:

Label: "Apply all promotions by sort order to enforce compatibility, including those with coupons."
Description: "If disabled, coupon based promotions will apply first regardless of compatibility rules."
Key: "enforce_weight_ordering"?

Config object:

commerce_promotion.settings

The logic should look like the following:

// 1. Collect all promotions.
$promotions = [];

// Collect coupon promotions.
$coupons = $order->get('coupons')->referencedEntities();
foreach ($coupons as $coupon) {
  $promotions[$coupon->getPromotionId()] = $coupon->getPromotion();
}

// Collect non-coupon promotions.
$promotion_storage = $this->entityTypeManager->getStorage('commerce_promotion');
foreach ($promotion_storage->loadAvailable($order) as $promotion) {
  $promotions[$promotion->id()] = $promotion;
}

// 2. Sort promotions by weight.
uasort($promotions, function($a, $b) {
  return $a->getWeight() <=> $b->getWeight();
});

// 3. Apply each promotion in weight order.
foreach ($promotions as $promotion) {
  if ($promotion->applies($order)) {
    $promotion->apply($order);
  }
}

I think the main problem with this change is the error messaging whenever a customer applies a coupon code that can't be applied due the promotion compatbility.

With this new global approach, one could still allow a couponless promotion to apply with a coupon based promotion thanks to the compatibility settings (If it's set to "Compatible with any promotion").

Fixing this should allow us to close #2869209: "Not with any other promotions" does not stop other promotions from applying as a duplicate and should also unblock #3010862: Add missing promotion compatibility strategies which currently doesn't work well due to this bug...

Let's see if with the suggested changes, the logic works properly.
Also, perhaps there is no need for an update hook? If the config object isn't set then the setting won't evaluate to TRUE?

Regarding the settings form location, after discussing this with @rszrama, we think it should be under Config -> Orders -> Promotion settings

@tbkot: Thanks! Could you update the setting label + description to:
Label: "Apply all promotions by sort order to enforce compatibility, including those with coupons."
Description: "If disabled, coupon based promotions will apply first regardless of compatibility rules."

Also, the test should be updated to test that the compatibility logic works as expected as well.

For example, if a coupon less promotion applies before a coupon based promotion, but the coupon based promotion isn't compatible with other promotions, it shouldn't apply.
Can we test this use case in particular? I believe right now this test would be failing without the changes and this is the main thing we're trying to address. Ensuring the weight is respected is needed in order to fix the compatibility (which is the primary underlying issue we're fixing here).

I think the PromotionCompatibilityTest should be expanded instead.

@jsacksick, it should now consider promotions that are not compatible with the other promotions. However, it does not cover that many options as in the #3010862: Add missing promotion compatibility strategies

@tbkot: I'm surprised getCompatiblePromotions() is needed? Promotion::applies() is supposed to check for compatibility already? If not then I guess we have to fix that? we're kind of duplicating the compatibility logic in both places.

I pushed some changes (See https://git.drupalcode.org/project/commerce/-/merge_requests/550/diffs?c...). But I think this is still not sufficient, fixing the compatibility logic from the promotion order processor isn't actually trivial.

The problem is, when $promotion->apply() is called, there is no guarantee that the promotion applied:

For example, if the promotion offer is an instanceof OrderItemPromotionOfferInterface, its possible that the offer conditions don't actually evaluate... In which case the promotion didn't apply.

      foreach ($order->getItems() as $order_item) {
        // Skip order items with a null unit price or with a quantity = 0.
        if (!$order_item->getUnitPrice() || Calculator::compare($order_item->getQuantity(), '0') === 0) {
          continue;
        }
        if ($offer_conditions->evaluate($order_item)) {
          $offer->apply($order_item, $this);
        }
      }

But also, even for the other case:

    else {
      $offer->apply($order, $this);
    }

It's actually possible that no adjustment was added, or in the case of the BuyXGetY offer, it's possible that no extra order item was added.

I think what we need to do (perhaps performance consuming is check that there is either a "promotion" or a "shipping_promotion" adjustment added to the order. THis should confirm 100% that a promotion was applied.

Will work on adding that check.

Also... one of the problems with this change is we're actually re-evaluating promotion conditions for coupon based promotions... Since this is currently already done when the coupons field is validated, via the following code:

    $constraints = $coupons_field_list->validate();

This invokes the CouponValidConstraintValidator which does this:

      if (!$coupon->available($order) || !$coupon->getPromotion()->applies($order)) {
        $this->context->buildViolation($constraint->message, ['%code' => $coupon->getCode()])
          ->atPath((string) $delta)
          ->addViolation();
      }

Which is actually the reason why coupon based promotions were then simply applied by calling $coupon->getPromotion()->apply().

I pushed a commit to address #20. With that said, the tests are failing which leads me to believe our tests are incorrect.