Fix invalid orders being created in a complex situation (#4054)

This was a bug that took days to find. The story goes like this: A cart
is created with four positions that each include four bundled positions.
A discount is applied, changing the price of *one* of the four top-level
positions to a reduced value. The list of position IDs gets passed to
`perform_order()`, which later passes it on to `transform_cart_positions()`.
`transform_cart_positions()`, however, receives the positions in an order
that has the first-level product *after* the bundled products that
belong to it. Therefore, it can't properly assign the parent-child
relationship between the positions.

The main reason is that cart positions are processed in "database order"
in a number of places, i.e. we make `SELECT` queries without an explicit
`ORDER BY` statement, leading the database to respond in unspecified
order. This is the case for `get_cart()` and hence for `CartMixin.positions`,
and hence for the list of position IDs that is passed to `perform_order()`
and hence for the order in which discounts are processed.

Therefore, if this "databse order" of the cart positions changes, the
discount compuation in `_check_positions()` might make a different choice
of *which* cart position should receive the discount than the CartManager
originally did. That's not nice, but most customers would not even
notice that a different one of their four (otherwise identical) tickets
is now discounted than the cart originally showed.

This leads to `_check_positions()` changing the price on two of the
cart positions. However, it only changes the price on the copy of
the CartPosition object that is directly part of the positions array,
while the `addon_to` attribute of its bundled positions contain a
*different* representation of the same cart position, that is not
refreshed to have the updated price now in the database.

This causes the `CartPosition.sort_key` of the bundled products to be
significantly different from the one of their parent products, which can
cause `transform_cart_positions()` to try to insert them before their
respective parent product, which is how the bug leads to the nasty end
result.

Now, I'm still not sure why this has happened *now* for the first time,
but I suspect it *might* even have something to do with our operations
team tuning our autovacuum parameters on our production installation,
which might make it *more likely* that newly created cart positions are
arbitrarily  stored on PostgreSQL disk pages in a different order than
they were inserted than before.

This commit now fixes the bug now in two ways, each of which would be
sufficient to fix it for now, but together they make it hopefully more
stable in the future:

- `perform_order` no longer respects the order of the position IDs it
  gets passed in, but instead uses the order last displayed in the cart.
  Additionally, both `CartManager` and `_check_positions()` now sort
  positions by their `pk` value before applying discounts to ensure
  consistent choice of which position is discounted (using  `sort_key`
  here does not make much sense since it includes sorting by price,
  which is about to change).

- `_check_positions()` makes sure that after its completion, only one
  copy of the same `CartPosition` is in use that has the current price.

Additionally, this commit makes sure `sort_key` cache is cleared after
e.g. a price change.

It was hard to write a regression test, since "database order" is, by
definition, unreliable, but I tried my best.

src/pretix/base/models/orders.py

@@ -2546,14 +2546,6 @@ class OrderPosition(AbstractPosition):
                 op.valid_until = valid_until
 
             if op.is_bundled and not op.addon_to_id:
-                logger.info(
-                    "Triggered bug that causes unattached bundle products. Dumping cart state in original order: " +
-                    repr([{k.name: getattr(c, k.name) for k in CartPosition._meta.fields} for c in cp])
-                )
-                logger.info(
-                    "Sorted order with sort key was: " +
-                    repr([(c.pk, c.sort_key) for c in sorted(cp, key=lambda c: c.sort_key)])
-                )
                 raise ValueError("Bundled cart position without parent does not make sense.")
 
             op.positionid = i + 1
@@ -2974,6 +2966,14 @@ class CartPosition(AbstractPosition):
             self.item.id, self.variation.id if self.variation else 0, self.cart_id
         )
 
+    def save(self, *args, **kwargs):
+        super().save(*args, **kwargs)
+        # invalidate cached values of cached properties that likely have changed
+        try:
+            del self.sort_key
+        except AttributeError:
+            pass
+
     @property
     def tax_value(self):
         net = round_decimal(self.price - (self.price * (1 - 100 / (100 + self.tax_rate))),

src/pretix/base/services/cart.py

@@ -1358,7 +1358,7 @@ class CartManager:
         return err
 
     def recompute_final_prices_and_taxes(self):
-        positions = sorted(list(self.positions), key=lambda op: -(op.addon_to_id or 0))
+        positions = sorted(list(self.positions), key=lambda cp: (-(cp.addon_to_id or 0), cp.pk))
         diff = Decimal('0.00')
         for cp in positions:
             if cp.listed_price is None:

src/pretix/base/services/orders.py

@@ -665,7 +665,7 @@ def _check_positions(event: Event, now_dt: datetime, positions: List[CartPositio
             deleted_positions.add(cp.pk)
             cp.delete()
 
-    sorted_positions = sorted(positions, key=lambda s: -int(s.is_bundled))
+    sorted_positions = sorted(positions, key=lambda c: (-int(c.is_bundled), c.pk))
 
     for cp in sorted_positions:
         cp._cached_quotas = list(cp.quotas)
@@ -876,6 +876,13 @@ def _check_positions(event: Event, now_dt: datetime, positions: List[CartPositio
             cp.discount = discount
             cp.save(update_fields=['price', 'discount'])
 
+    # After applying discounts, add-on positions might still have a reference to the *old* version of the
+    # parent position, which can screw up ordering later since the system sees inconsistent data.
+    by_id = {cp.pk: cp for cp in sorted_positions}
+    for cp in sorted_positions:
+        if cp.addon_to_id:
+            cp.addon_to = by_id[cp.addon_to_id]
+
     new_total = sum(cp.price for cp in sorted_positions)
     if old_total != new_total:
         err = err or error_messages['price_changed']
@@ -884,7 +891,7 @@ def _check_positions(event: Event, now_dt: datetime, positions: List[CartPositio
     for cp in sorted_positions:
         cp.expires = now_dt + timedelta(
             minutes=event.settings.get('reservation_time', as_type=int))
-        cp.save()
+        cp.save(update_fields=['expires'])
 
     if err:
         raise OrderError(err)
@@ -1144,7 +1151,7 @@ def _perform_order(event: Event, payment_requests: List[dict], position_ids: Lis
         positions = list(
             positions.select_related('item', 'variation', 'subevent', 'seat', 'addon_to').prefetch_related('addons')
         )
-        positions.sort(key=lambda k: position_ids.index(k.pk))
+        positions.sort(key=lambda c: c.sort_key)
         if len(positions) == 0:
             raise OrderError(error_messages['empty'])
         if len(position_ids) != len(positions):