"Not with any other promotions" does not stop other promotions from applying

+1
The compatibility 'Not with any other promotions' uses $order->collectAdjustments() to find other promotions, but other promotions might not have been applied yet.
We need some sort of weight to first apply the 'Any other promotion' compatibility promotions, and then after that the other type.

This could be done by collecting all promotions together in PromotionOrderProcessor::process() and custom sorting them on compatibility.

I'm trying to figure this one out. My thought is that we use a process of elimination starting with the least restrictive (any) and move to most restrictive (none), i.e a coupon is restricted (none) against a less restrictive (or equal) coupon. It's still hairy trying to figure out which ones would win, but.

I'm also trying to consider this in the context of having additional restrictions, i.e. "any_except" and "only", so it would work something like this:

- "any_except" is reduced against "any_except" first, and then "any". This gets complicated when you have several any_except:
--- A is any_except B
--- B is any_except C
--- C is any_except D
What wins? Is A and B eliminated, or just B? Chronological DESC?
- Now we have a list of "any" and "any_except" so we filter "only" against that
- If there is nothing left, the "none" can move on

So, we end up with promotions that have A) passed the conditions individually and B) are compatible with each other. I made an attempt at moving this to code and I'll attach it here but I'd like to know if I'm in the right ballpark.

Only return 1 'only'

Here is a proper patch that I hope will be a good starting point for at least solving this issue and make way for extending compatibility options.

+++ b/src/PromotionOrderProcessor.php
@@ -58,11 +59,76 @@ public function process(OrderInterface $order) {
+    // @todo better way of doing this without error.

Argh - that's old

+++ b/src/PromotionOrderProcessor.php
@@ -58,11 +59,76 @@ public function process(OrderInterface $order) {
+ protected function getCompatiblePromotions(array $is_compatible) {

I already don't like the naming of this, $is_compatible is what this method is returning.

You are only processing non-coupon promotions (by starting your change to src/PromotionOrderProcessor.php on line 62. We need to apply compatibility checks to all promotions, I think.

Yep, I see that now.

I will draft a simple patch/PR to at least get the suggested functionality in the current GUI (compatible with all or none) working. I would suggest expanding this after the basics are working, as an expanded compatibility would require additional GUI elements and tests.
Here https://github.com/drupalcommerce/commerce/pull/790

I'm wondering if the initial goal isn't sufficient and only causes confusion. In one scenario brought to my attention:

A promotion with weight of 1 is not compatible with any other promotion
A promotion with weight of 10 is compatible with any promotion

The feedback I'm getting is that marketing expects the highest-in-list promotion to be applied if applicable, but it will never be applied. I don't have a solution right now, but this is some real life feedback.

I updated the patch on github to apply to the latest 8.x-2.x-dev. For people using the 8.x-2.1 release of Commerce there is a patch attached here.

@chrisrockwell - I just ran into this exact scenario with the promotions for my project. I expected that once a weight 1 promotion with "no compatibility" was applied, no other promotions with higher weight would be applied. But a weight 10 promotion with "any compatibility" was also applied. I tried to solve the problem for myself before I came upon this Issue. I'm not using coupons yet, so I didn't think about them at first, but after reading through everything, I think that reasonable expectations for processing promotions with the current compatibility options are:

1. Start by checking/applying any coupon promotions, in order from lowest to highest weight. (I assume coupons are prioritized because they are applied explicitly by the customer, and this seems to be the assumption made in the process() method of the PromotionOrderProcessor class.) Then proceed with non-coupon promotions in weighted order.

2. Once an any-compatibility promotion is applied, any subsequent no-compatibility promotions will be skipped (i.e., not applied.)

3. If a no-compatibility promotion is applied, processing should stop since no other promotions can be applied.

I've altered the process() method in the PromotionOrderProcessor class to prevent no-compatibility promotions from being combined with other promotions.

I think the decision needs to be made - does weight matter? If not, we should remove it from the table. If so, it should be the first point of decision making.

If I create a coupon that isn't compatible with any other promotions, but I move it to the top of the list (i.e. the lowest weight) I expect that to be applied.

+++ b/modules/promotion/src/PromotionOrderProcessor.php
@@ -36,22 +37,34 @@ class PromotionOrderProcessor implements OrderProcessorInterface {
+            $exclusive_coupon = TRUE;

Did you mean for this to be $exclusive_coupon_applied?

Did you mean for this to be $exclusive_coupon_applied?

Yes! Sorry about that. I missed that one when I changed the variable name.

1. +++ b/modules/promotion/src/PromotionOrderProcessor.php
   @@ -36,22 +37,34 @@ class PromotionOrderProcessor implements OrderProcessorInterface {
   +    if ($exclusive_coupon_applied) {
   +      return;
   +    }
   

   Hmm, I think you might be returning too late - should this happen within the loop?

2. +++ b/modules/promotion/src/PromotionOrderProcessor.php
   @@ -36,22 +37,34 @@ class PromotionOrderProcessor implements OrderProcessorInterface {
   +        if ($promotion->getCompatibility() == Promotion::COMPATIBLE_NONE) {
   +          break;
   +        }
   

   I think a return would be appropriate here also, as opposed to break

For #1, I don't return so that remaining coupons will be processed, just in case any should be removed:

$order->get('coupons')->removeItem($index);

For #2, I completely agree. That should be return, not break.

Obviously, I didn't test this very well! My list of promotions happens to have all COMPATIBLE:NONE promotions first (based on customer), then three COMPATIBLE:ANY promotions at the end (based on quantity). So the PromotionOrderProcessor wasn't working for me, and my changes fixed it for me. Thank you for going over everything carefully.

Tacking on another scenario: Auto-apply promotion set with combine none but a user has a coupon (also combine none) - the coupon won't apply even if it's on a lower-weight promotion.

Tagging with needs tests because I feel like we need to document these scenarios as tests.

I'm in a rush so I'm going to drop this here. Within apply, I'm checking the weight of the promotion being applied. It's ugly but I'll be able to come back and spend more time on it this weekend.

 use Drupal\commerce\ConditionGroup;
+use Drupal\commerce_order\Adjustment;
 use Drupal\commerce_order\Entity\OrderInterface;
 use Drupal\commerce_promotion\Plugin\Commerce\PromotionOffer\PromotionOfferInterface;
 use Drupal\Core\Datetime\DrupalDateTime;
@@ -439,9 +440,20 @@ public function applies(OrderInterface $order) {
     switch ($this->getCompatibility()) {
       case self::COMPATIBLE_NONE:
         // If there are any existing promotions, then this cannot apply.
+        /** @var Adjustment $adjustment */
         foreach ($order->collectAdjustments() as $adjustment) {
           if ($adjustment->getType() == 'promotion') {
-            return FALSE;
+            // Load the promotion
+            /** @var Promotion $promotion */
+            $promotion = $this->entityTypeManager()
+              ->getStorage('commerce_promotion')
+              ->load($adjustment->getSourceId());
+            if ($promotion->getWeight() > $this->getWeight()) {
+              $order->removeAdjustment($adjustment);
+            }
+            else {
+              return FALSE;
+            }
           }

Setting to needs work - if anyone thinks we should have separate issues for this stuff please change it back.

Taking a step back, I've created a test that shows an order that already has a "Not with any other" promotion applied will still have another promotion applied.

Adding another test. To summarize the additional tests:
- If a compatible none is already on an order, other promotion shouldn't apply
- If a coupon with a lower weight is applied, it should be applied. If a promotion with COMPATIBLE_NONE has already been applied, it should be removed.

[EDIT - I see now that using the coupon isn't necessary, the logic tests it without the coupon being on the promotion]

An alternative approach that

• Keeps the apply logic in Promotion.php (I'm not certain of that even, I feel like a service would be good for this maybe)
• Separates the condition check from the compatibility check
• Compares the one being applied against everything else already applied (ensuring lowest weight comes first)
• Provides an attempt at expanded test coverage (not my strong suit :D).

In my experience from a custom Drupal 7 module for promotions, many sites need if a "non compatible" applies then no other should apply, and many other sites need the "non compatible" to apply only if no other applies.

I suggest we have a general setting in commerce_promotion to indicate this policy and use Drupal\commerce_promotion\Entity\Promotion::sort() method to get the "non compatible" first or last depending on the policy, and also the Drupal\commerce_promotion\PromotionOrderProcessor::process() to prevent applying other promotions.

I will give a patch.

@istavros - there is a "Not compatible with any" setting it just currently doesn't work.

I suggest we have a general setting in commerce_promotion to indicate this policy and use Drupal\commerce_promotion\Entity\Promotion::sort() method to get the "non compatible" first or last depending on the policy

I had asked a question about using the weight, it sounds like you're suggesting weight should not matter - is that accurate? I'm not sure either way; however, if weight does matter I think it should be considered first. If it doesn't matter, we should remove it or use it within groups of promotions (hurts my head to even think about going down that path).

Well I was suggesting that the weight should matter between promotions of the same compatibility, and the "non compatible" should be checked first or last depending on the web-master preferences.

However, after re-thinking about it, this suggestion is not very good.
We could just make the existing compatibility setting work, and the web-master should place the promotions in the desired order.

So @lisastreeter's approach seems the most sensible.

I wonder if the 'Compatibility with other promotions' widget should also be extended to accept an entity reference of other promotions it is not compatible with.

I ran into this issue whist researching a solution for my use case, where 'price breaks' are required to work alongside coupons. Obviously the different tiers of price break are not compatible with each other, but still need to be compatible with any coupon discounts.

May be for a separate discussion, but I thought it worth mentioning here.

@orphans: I have the same issue. I need a coupon with 5% discount on a certain product type and use it next to a 'price break' on certain products. Offcourse the discounted product can't receive the 5% discount on top off their discount.

When using Compatibility with other promotions, you can only have 1 promotion in your order when you use "Not with any other promotions".

Orphans explaination is correct.

@fotograafinge Mine was a bit of an edge case because the client wanted discounts based on the line item quantity. I solved this in the end by adding a custom promotion condition which allowed a 'Between' quantity operator and assessed each line item.

This then let me set some rules like:

If Qty between 5 and 10 - DIscount 5%
If Qty between 10 and 15 - Discount 10%
if Qty between 15 and 20 - Discount 15%

etc.

None of these overlap with each other so I was able to leave the 'Not compatible with other promotions' setting unchecked.

<?php

namespace Drupal\etl_catalogue\Plugin\Commerce\Condition;

use Drupal\commerce\Plugin\Commerce\Condition\ConditionBase;
use Drupal\commerce\Plugin\Commerce\Condition\ParentEntityAwareInterface;
use Drupal\commerce\Plugin\Commerce\Condition\ParentEntityAwareTrait;
use Drupal\Core\Entity\EntityInterface;
use Drupal\commerce_promotion\Plugin\Commerce\PromotionOffer\OrderItemPromotionOfferInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\commerce_price\Calculator;

/**
 * Provides the product condition for order items.
 *
 * @CommerceCondition(
 *   id = "order_item_product_quantity",
 *   label = @Translation("Line item Quantity"),
 *   display_label = @Translation("Line item quantity"),
 *   category = @Translation("Products"),
 *   entity_type = "commerce_order_item",
 *   weight = -1,
 * )
 */
class OrderItemProductQuantity extends ConditionBase implements ParentEntityAwareInterface {

  use ParentEntityAwareTrait;


  /**
   * {@inheritdoc}
   */
  public function defaultConfiguration() {
    return [
        'operator' => '>',
        'quantity_min' => 1,
        'quantity_max' => NULL,
      ] + parent::defaultConfiguration();
  }

  /**
   * {@inheritdoc}
   */
  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildConfigurationForm($form, $form_state);

    $form['operator'] = [
      '#type' => 'select',
      '#title' => t('Operator'),
      '#options' => $this->getComparisonOperators(),
      '#default_value' => $this->configuration['operator'],
      '#required' => TRUE,
    ];
    $form['quantity_min'] = [
      '#type' => 'number',
      '#title' => t('Min Quantity'),
      '#default_value' => $this->configuration['quantity_min'],
      '#min' => 1,
      '#required' => TRUE,
    ];
    $form['quantity_max'] = [
      '#type' => 'number',
      '#title' => t('Max Quantity'),
      '#default_value' => $this->configuration['quantity_max'],
      '#min' => 1,
      '#required' => FALSE,
      '#description' => $this->t('Only used with Between operator')
    ];

    return $form;
  }

  /**
   * {@inheritdoc}
   */
  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    parent::submitConfigurationForm($form, $form_state);

    $values = $form_state->getValue($form['#parents']);
    $this->configuration['operator'] = $values['operator'];
    $this->configuration['quantity_min'] = $values['quantity_min'];
    $this->configuration['quantity_max'] = $values['quantity_max'];
  }

  /**
   * {@inheritdoc}
   */
  public function evaluate(EntityInterface $entity) {
    $this->assertEntity($entity);
    /** @var \Drupal\commerce_order\Entity\OrderItemInterface $orderItem */
    $orderItem = $entity;
    $quantity = $orderItem->getQuantity();

    switch ($this->configuration['operator']) {
      case '>= && <':
        return $quantity >= $this->configuration['quantity_min'] && $quantity < $this->configuration['quantity_max'];

      case '>=':
        return $quantity >= $this->configuration['quantity_min'];

      case '>':
        return $quantity > $this->configuration['quantity_min'];

      case '<=':
        return $quantity <= $this->configuration['quantity_min'];

      case '<':
        return $quantity < $this->configuration['quantity_min'];

      case '==':
        return $quantity == $this->configuration['quantity_min'];

      default:
        throw new \InvalidArgumentException("Invalid operator {$this->configuration['operator']}");
    }
  }

  /**
   * Gets the comparison operators.
   *
   * @return array
   *   The comparison operators.
   */
  protected function getComparisonOperators() {
    return [
      '>' => $this->t('Greater than'),
      '>=' => $this->t('Greater than or equal to'),
      '<=' => $this->t('Less than or equal to'),
      '<' => $this->t('Less than'),
      '==' => $this->t('Equals'),
      '>= && <' => $this->t('Between (>= && <)'),
    ];
  }


}

I don't know, whether I should open a new issue for that, but it is so closely related that I'll comment here:

given that scenario:

1. You have a promotion without coupon code, weighted first, that is valid for product A (giving a 50,- discount) but not for product B, allowing other promotions.
2. You have -10% order sub total promotion with a coupon code, being not compatible with others.

Scenario 1:

add product A to cart and try to enter the coupon -> it is rejected, which is the expected behaviour.

Scenario 2:

(empty cart) and add product B + enter the coupon code. it is accepted adn you get -10% on order subtotal --> correct

But now, add product A to cart --> you get -10% on both products, which is absolutely wrong. correct would be to remove the coupon code completely.

You can now go even furthe rand remove product B from cart, so that only A is left. Even now you get both discounts!

and further I can confirm the problem mentioned in #18

I'm sharing how I fixed it in my project without using any patches. I created an event subscriber that removed the coupon if the condition "Not with any other promotions" was met.

I hope you find it useful.

  public static function getSubscribedEvents()
  {
    return [
      'commerce_order.commerce_order.presave' => ['onOrderPresave']
    ];
  }


 public function onOrderPresave(OrderEvent $event)
  {
    $order = $event->getOrder();
    /** @var \Drupal\commerce_promotion\Entity\CouponInterface[] $coupons */
    $coupons = $order->coupons->getValue();
    if (count($coupons) < 2) {
      return;
    }
    foreach ($coupons as $index => $item) {
      $coupon = Coupon::load($item["target_id"]);
      $promotion = $coupon->getPromotion();
      $compatibility = $promotion->getCompatibility();
      if ($compatibility == "none") {
        \Drupal::messenger()->addWarning(t('%label was removed because it was incompatible.', ['%label' => $coupon->getCode()]));
        unset($coupons[$index]);
      }
    }
    $order->coupons = array_values($coupons);
    $order->setRefreshState(OrderInterface::REFRESH_ON_LOAD);
  }
}

This is still a problem. Also, the documentation page says the following:

- Once a promotion that is not compatible with other promotions is added to an order, no other promotions will be added.
- If a promotion that is compatible with any promotion is added to an order, then any subsequent promotions with limited compatibility will not be added to the order.

Which is not true. The current behavior would be:

- Once a promotion that is not compatible with other promotions is added to an order, no other promotions will be added which are set to not be compatible with any other promotions. Promotions that are compatible with any other promotions will still be applied.
- If a promotion that is compatible with any promotion is added to an order, then any subsequent promotions with limited compatibility will not be added to the order.

Closing this as a duplicate of #3358727: Promotion weight is not respected if applied via coupon as I believe fixing the weight issue should address what we're trying to fix here.