Issue #2929103 by istavros, bojanz: Promotions have incorrect condition logic when the operator is "OR"

Author: istavros

modules/promotion/src/Entity/Promotion.php

@@ -464,20 +464,35 @@ public function applies(OrderInterface $order) {
       /** @var \Drupal\commerce\Plugin\Commerce\Condition\ConditionInterface $condition */
       return $condition->getEntityTypeId() == 'commerce_order_item';
     });
-    $order_conditions = new ConditionGroup($order_conditions, $this->getConditionOperator());
-    $order_item_conditions = new ConditionGroup($order_item_conditions, $this->getConditionOperator());
+    $condition_operator = $this->getConditionOperator();
+    $order_conditions = new ConditionGroup($order_conditions, $condition_operator);
+    $order_item_conditions = new ConditionGroup($order_item_conditions, $condition_operator);
 
-    if (!$order_conditions->evaluate($order)) {
+    $order_conditions_apply = $order_conditions->evaluate($order);
+    if ($condition_operator == 'AND' && !$order_conditions_apply) {
       return FALSE;
     }
+
+    $order_item_conditions_apply = FALSE;
     foreach ($order->getItems() as $order_item) {
       // Order item conditions must match at least one order item.
       if ($order_item_conditions->evaluate($order_item)) {
-        return TRUE;
+        $order_item_conditions_apply = TRUE;
+        break;
       }
     }
 
-    return FALSE;
+    if ($condition_operator == 'AND') {
+      return $order_conditions_apply && $order_item_conditions_apply;
+    }
+    elseif ($condition_operator == 'OR') {
+      // Empty condition groups are TRUE by default, which leads to incorrect
+      // logic with ORed groups due to false positives.
+      $order_conditions_apply = $order_conditions->getConditions() && $order_conditions_apply;
+      $order_item_conditions_apply = $order_item_conditions->getConditions() && $order_item_conditions_apply;
+
+      return $order_conditions_apply || $order_item_conditions_apply;
+    }
   }
 
   /**
@@ -493,7 +508,7 @@ public function apply(OrderInterface $order) {
         /** @var \Drupal\commerce\Plugin\Commerce\Condition\ConditionInterface $condition */
         return $condition->getEntityTypeId() == 'commerce_order_item';
       });
-      $order_item_conditions = new ConditionGroup($order_item_conditions, 'AND');
+      $order_item_conditions = new ConditionGroup($order_item_conditions, $this->getConditionOperator());
       // Apply the offer to order items that pass the conditions.
       foreach ($order->getItems() as $order_item) {
         if ($order_item_conditions->evaluate($order_item)) {

modules/promotion/tests/src/Kernel/PromotionConditionTest.php

@@ -5,6 +5,7 @@
 use Drupal\commerce_order\Entity\OrderItem;
 use Drupal\commerce_order\Entity\OrderItemType;
 use Drupal\commerce_order\Entity\Order;
+use Drupal\commerce_price\Price;
 use Drupal\commerce_promotion\Entity\Promotion;
 use Drupal\Tests\commerce\Kernel\CommerceKernelTestBase;
 
@@ -141,12 +142,17 @@ public function testOrderCondition() {
     $this->order->save();
     $result = $promotion->applies($this->order);
     $this->assertTrue($result);
+
+    // No order total can satisfy both conditions.
+    $promotion->setConditionOperator('AND');
+    $result = $promotion->applies($this->order);
+    $this->assertFalse($result);
   }
 
   /**
-   * Tests promotions with an order item condition.
+   * Tests promotions with both order and order item conditions.
    */
-  public function testOrderItemCondition() {
+  public function testMixedCondition() {
     // Starts now, enabled. No end time.
     $promotion = Promotion::create([
       'name' => 'Promotion 1',
@@ -165,7 +171,7 @@ public function testOrderItemCondition() {
           'target_plugin_configuration' => [
             'operator' => '>',
             'amount' => [
-              'number' => '10.00',
+              'number' => '30.00',
               'currency_code' => 'USD',
             ],
           ],
@@ -174,7 +180,7 @@ public function testOrderItemCondition() {
           'target_plugin_id' => 'order_item_quantity',
           'target_plugin_configuration' => [
             'operator' => '>',
-            'quantity' => 2,
+            'quantity' => 1,
           ],
         ],
       ],
@@ -184,31 +190,62 @@ public function testOrderItemCondition() {
 
     $order_item = OrderItem::create([
       'type' => 'test',
-      'quantity' => 2,
+      'quantity' => 4,
       'unit_price' => [
-        'number' => '20.00',
+        'number' => '10.00',
         'currency_code' => 'USD',
       ],
     ]);
     $order_item->save();
+
+    // AND: Both conditions apply.
     $this->order->addItem($order_item);
     $this->order->save();
+    $result = $promotion->applies($this->order);
+    $this->assertTrue($result);
+
+    // OR: Both conditions apply.
+    $promotion->setConditionOperator('OR');
+    $result = $promotion->applies($this->order);
+    $this->assertTrue($result);
 
+    // AND: Neither condition applies.
+    $order_item->setQuantity(1);
+    $order_item->save();
+    $this->order->save();
+    $promotion->setConditionOperator('AND');
     $result = $promotion->applies($this->order);
     $this->assertFalse($result);
 
-    $order_item = OrderItem::create([
-      'type' => 'test',
-      'quantity' => 4,
-      'unit_price' => [
-        'number' => '20.00',
-        'currency_code' => 'USD',
-      ],
-    ]);
+    // OR: Neither condition applies.
+    $promotion->setConditionOperator('OR');
+    $result = $promotion->applies($this->order);
+    $this->assertFalse($result);
+
+    // AND: Order condition fails, order item condition passes.
+    $order_item->setQuantity(2);
+    $order_item->save();
+    $this->order->save();
+    $promotion->setConditionOperator('AND');
+    $result = $promotion->applies($this->order);
+    $this->assertFalse($result);
+
+    // OR: Order condition fails, order item condition passes.
+    $promotion->setConditionOperator('OR');
+    $result = $promotion->applies($this->order);
+    $this->assertTrue($result);
+
+    // AND: Order condition passes, order item condition fails.
+    $order_item->setUnitPrice(new Price('40', 'USD'));
+    $order_item->setQuantity(1);
     $order_item->save();
-    $this->order->addItem($order_item);
     $this->order->save();
+    $promotion->setConditionOperator('AND');
+    $result = $promotion->applies($this->order);
+    $this->assertFalse($result);
 
+    // OR: Order condition passes, order item condition fails.
+    $promotion->setConditionOperator('OR');
     $result = $promotion->applies($this->order);
     $this->assertTrue($result);
   }

modules/promotion/tests/src/Kernel/PromotionOrderProcessorTest.php

@@ -120,7 +120,7 @@ public function testOrderTotal() {
     ]);
     $promotion->save();
 
-    $this->assertNotEmpty($promotion->applies($this->order));
+    $this->assertTrue($promotion->applies($this->order));
     $this->container->get('commerce_order.order_refresh')->refresh($this->order);
 
     $this->assertEquals(1, count($this->order->getAdjustments()));