Discount: Respect addon grouping in line selection (Z#23220058) (#5782)

* Discount: Respect addon grouping in line selection (Z#23220058)

* Update src/pretix/base/models/discount.py

Co-authored-by: Richard Schreiber <schreiber@pretix.eu>

---------

Co-authored-by: Richard Schreiber <schreiber@pretix.eu>

src/pretix/api/serializers/order.py

@@ -1601,7 +1601,7 @@ class OrderCreateSerializer(I18nAwareModelSerializer):
                 order.sales_channel,
                 [
                     (cp.item_id, cp.subevent_id, cp.subevent.date_from if cp.subevent_id else None, cp.price,
-                     bool(cp.addon_to), cp.is_bundled, pos._voucher_discount)
+                     cp.addon_to, cp.is_bundled, pos._voucher_discount)
                     for cp in order_positions
                 ]
             )

src/pretix/base/models/discount.py

@@ -37,7 +37,7 @@ from pretix.base.decimal import round_decimal
 from pretix.base.models.base import LoggedModel
 
 PositionInfo = namedtuple('PositionInfo',
-                          ['item_id', 'subevent_id', 'subevent_date_from', 'line_price_gross', 'is_addon_to',
+                          ['item_id', 'subevent_id', 'subevent_date_from', 'line_price_gross', 'addon_to',
                            'voucher_discount'])
 
 
@@ -279,6 +279,42 @@ class Discount(LoggedModel):
             for idx in condition_idx_group:
                 collect_potential_discounts[idx] = [(self, inf, -1, subevent_id)]
 
+    def _addon_idx(self, positions, idx):
+        """
+        If we have the following cart:
+
+        - Main product
+          - 10x Addon product 5€
+        - Main product
+          - 10x Addon product 5€
+
+        And we have a discount rule that grants "every 10th product is free", people tend to expect
+
+        - Main product
+          - 9x Addon product 5€
+          - 1x Addon product free
+        - Main product
+          - 9x Addon product 5€
+          - 1x Addon product free
+
+        And get confused if they get
+
+        - Main product
+          - 8x Addon product 5€
+          - 2x Addon product free
+        - Main product
+          - 10x Addon product 5€
+
+        Even if the result is the same. Therefore, we sort positions in the cart not only by price, but also by their
+        relative index within their addon group. This is only a heuristic and there are *still* scenarios where the more
+        unexpected version happens, e.g. if prices are different. We need to accept this as long as discounts work on
+        cart level and not on addon-group level, but this simple sorting reduces the number of support issues by making
+        the weird case less likely.
+        """
+        if not positions[idx].addon_to:
+            return 0
+        return len([1 for i, p in positions.items() if i < idx and p.addon_to == positions[idx].addon_to])
+
     def _apply_min_count(self, positions, condition_idx_group, benefit_idx_group, result, collect_potential_discounts, subevent_id):
         if len(condition_idx_group) < self.condition_min_count:
             return
@@ -288,8 +324,8 @@ class Discount(LoggedModel):
 
         if self.benefit_only_apply_to_cheapest_n_matches:
             # sort by line_price
-            condition_idx_group = sorted(condition_idx_group, key=lambda idx: (positions[idx].line_price_gross, -idx))
+            condition_idx_group = sorted(condition_idx_group, key=lambda idx: (positions[idx].line_price_gross, self._addon_idx(positions, idx), -idx))
-            benefit_idx_group = sorted(benefit_idx_group, key=lambda idx: (positions[idx].line_price_gross, -idx))
+            benefit_idx_group = sorted(benefit_idx_group, key=lambda idx: (positions[idx].line_price_gross, self._addon_idx(positions, idx), -idx))
 
             # Prevent over-consuming of items, i.e. if our discount is "buy 2, get 1 free", we only
             # want to match multiples of 3
@@ -434,7 +470,7 @@ class Discount(LoggedModel):
             for idx, p in positions.items():
                 subevent_to_idx[p.subevent_id].append(idx)
             for v in subevent_to_idx.values():
-                v.sort(key=lambda idx: positions[idx].line_price_gross)
+                v.sort(key=lambda idx: (positions[idx].line_price_gross, self._addon_idx(positions, idx)))
             subevent_order = sorted(list(subevent_to_idx.keys()), key=lambda s: len(subevent_to_idx[s]), reverse=True)
 
             # Build groups of exactly condition_min_count distinct subevents
@@ -458,7 +494,7 @@ class Discount(LoggedModel):
 
                 # Sort the list by prices, then pick one. For "buy 2 get 1 free" we apply a "pick 1 from the start
                 # and 2 from the end" scheme to optimize price distribution among groups
-                candidates = sorted(candidates, key=lambda idx: positions[idx].line_price_gross)
+                candidates = sorted(candidates, key=lambda idx: (positions[idx].line_price_gross, self._addon_idx(positions, idx)))
                 if len(current_group) < (self.benefit_only_apply_to_cheapest_n_matches or 0):
                     candidate = candidates[0]
                 else:

src/pretix/base/services/cart.py

@@ -1528,7 +1528,7 @@ class CartManager:
             self._sales_channel.identifier,
             [
                 (cp.item_id, cp.subevent_id, cp.subevent.date_from if cp.subevent_id else None, cp.line_price_gross,
-                 bool(cp.addon_to), cp.is_bundled, cp.listed_price - cp.price_after_voucher)
+                 cp.addon_to, cp.is_bundled, cp.listed_price - cp.price_after_voucher)
                 for cp in positions
             ]
         )

src/pretix/base/services/cross_selling.py

@@ -121,7 +121,7 @@ class CrossSellingService:
             self.sales_channel,
             [
                 (cp.item_id, cp.subevent_id, cp.subevent.date_from if cp.subevent_id else None, cp.line_price_gross,
-                 bool(cp.addon_to), cp.is_bundled,
+                 cp.addon_to, cp.is_bundled,
                  cp.listed_price - cp.price_after_voucher)
                 for cp in self.cartpositions
             ],

src/pretix/base/services/orders.py

@@ -914,7 +914,7 @@ def _check_positions(event: Event, now_dt: datetime, time_machine_now_dt: dateti
         sales_channel.identifier,
         [
             (cp.item_id, cp.subevent_id, cp.subevent.date_from if cp.subevent_id else None, cp.line_price_gross,
-             bool(cp.addon_to), cp.is_bundled, cp.listed_price - cp.price_after_voucher)
+             cp.addon_to, cp.is_bundled, cp.listed_price - cp.price_after_voucher)
             for cp in sorted_positions
         ]
     )

src/pretix/base/services/pricing.py

@@ -174,7 +174,9 @@ def apply_discounts(event: Event, sales_channel: Union[str, SalesChannel],
 
     :param event: Event the cart belongs to
     :param sales_channel: Sales channel the cart was created with
-    :param positions: Tuple of the form ``(item_id, subevent_id, subevent_date_from, line_price_gross, is_addon_to, is_bundled, voucher_discount)``
+    :param positions: Tuple of the form ``(item_id, subevent_id, subevent_date_from, line_price_gross, addon_to_id, is_bundled, voucher_discount)``
+                      ``addon_to_id`` does not have to be the proper ID, any identifier is okay, even ``True``/``False`` are accepted, but
+                      a better result may be given if addons to the same main product have the same distinct value.
     :param collect_potential_discounts: If a `defaultdict(list)` is supplied, all discounts that could be applied to the cart
     based on the "consumed" items, but lack matching "benefitting" items will be collected therein.
     The dict will contain a mapping from index in the `positions` list of the item that could be consumed, to a list
@@ -196,9 +198,9 @@ def apply_discounts(event: Event, sales_channel: Union[str, SalesChannel],
     ).prefetch_related('condition_limit_products', 'benefit_limit_products').order_by('position', 'pk')
     for discount in discount_qs:
         result = discount.apply({
-            idx: PositionInfo(item_id, subevent_id, subevent_date_from, line_price_gross, is_addon_to, voucher_discount)
+            idx: PositionInfo(item_id, subevent_id, subevent_date_from, line_price_gross, addon_to, voucher_discount)
             for
-            idx, (item_id, subevent_id, subevent_date_from, line_price_gross, is_addon_to, is_bundled, voucher_discount)
+            idx, (item_id, subevent_id, subevent_date_from, line_price_gross, addon_to, is_bundled, voucher_discount)
             in enumerate(positions)
             if not is_bundled and idx not in new_prices
         }, collect_potential_discounts)

src/tests/base/test_pricing_discount.py

@@ -781,6 +781,39 @@ testcases_single_rule = [
         )
     ),
 
+    # Distribute discounts somewhat equally over addon groups
+    (
+        (
+            Discount(
+                condition_min_count=3,
+                benefit_discount_matching_percent=20,
+                condition_apply_to_addons=True,
+                benefit_only_apply_to_cheapest_n_matches=1,
+                condition_ignore_voucher_discounted=True,
+            ),
+        ),
+        (
+            (2, 1, now(), Decimal('0.00'), None, False, Decimal('10.00')),  # Main product
+            (1, 1, now(), Decimal('100.00'), 1, False, Decimal('0.00')),
+            (1, 1, now(), Decimal('100.00'), 1, False, Decimal('0.00')),
+            (1, 1, now(), Decimal('100.00'), 1, False, Decimal('0.00')),
+            (2, 1, now(), Decimal('0.00'), None, False, Decimal('10.00')),  # Main product
+            (1, 1, now(), Decimal('100.00'), 2, False, Decimal('0.00')),
+            (1, 1, now(), Decimal('100.00'), 2, False, Decimal('0.00')),
+            (1, 1, now(), Decimal('100.00'), 2, False, Decimal('0.00')),
+        ),
+        (
+            Decimal('0.00'),
+            Decimal('100.00'),
+            Decimal('100.00'),
+            Decimal('80.00'),
+            Decimal('0.00'),
+            Decimal('100.00'),
+            Decimal('100.00'),
+            Decimal('80.00'),
+        )
+    ),
+
     # Ignore bundled
     (
         (