From 0067c3537dbb05e7a8815aec17728fd75a35eced Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Mon, 8 Apr 2024 16:55:54 +0200 Subject: [PATCH] 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 | 16 +++++----- src/pretix/base/services/cart.py | 2 +- src/pretix/base/services/orders.py | 13 ++++++-- src/tests/presale/test_checkout.py | 50 ++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 12 deletions(-) diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index c6fa10e1ff..d92de3c8de 100644 --- a/src/pretix/base/models/orders.py +++ b/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))), diff --git a/src/pretix/base/services/cart.py b/src/pretix/base/services/cart.py index ca1864690e..2387f988e3 100644 --- a/src/pretix/base/services/cart.py +++ b/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: diff --git a/src/pretix/base/services/orders.py b/src/pretix/base/services/orders.py index 3990471256..e56b4e5b92 100644 --- a/src/pretix/base/services/orders.py +++ b/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): diff --git a/src/tests/presale/test_checkout.py b/src/tests/presale/test_checkout.py index 19fcd96d39..e05ef40790 100644 --- a/src/tests/presale/test_checkout.py +++ b/src/tests/presale/test_checkout.py @@ -50,6 +50,7 @@ from pretix.base.models.customers import CustomerSSOProvider from pretix.base.models.items import ( ItemAddOn, ItemBundle, ItemVariation, SubEventItem, SubEventItemVariation, ) +from pretix.base.services.cart import CartManager from pretix.base.services.orders import OrderError, _perform_order from pretix.base.services.tax import VATIDFinalError, VATIDTemporaryError from pretix.testutils.scope import classscope @@ -4316,6 +4317,55 @@ class CheckoutBundleTest(BaseCheckoutTestCase, TestCase): doc = BeautifulSoup(response.content.decode(), "lxml") self.assertEqual(len(doc.select(".thank-you")), 1) + @classscope(attr='orga') + def test_bundle_and_discount_with_inverted_cart_order(self): + """ + This test tells the story of a beautiful bug. + + Cart positions have no natural order. They have a display order (CartPosition.sort_key), which however is + dependant on mutable values like the price. Therefore, e.g. applying a discount can change the cart order. + However, all order logic must be independent of the order in which the cart positions are in the database. + Therefore this test artificially creates a card where the `pk` values of the positions are *inverse* of what + they should be and checks that the order still goes through fine. + + More details about the bug in the commit message that introduces this test. + """ + self.event.discounts.create( + condition_min_count=4, + benefit_discount_matching_percent=50, + benefit_only_apply_to_cheapest_n_matches=1, + ) + CartPosition.objects.filter(addon_to__isnull=False).delete() + CartPosition.objects.all().delete() + cm = CartManager(event=self.event, cart_id="temp") + cm.add_new_items([{ + 'item': self.ticket.pk, + 'variation': None, + 'count': 4 + }]) + cm.commit() + + map = {} + for cp in reversed(CartPosition.objects.filter(addon_to__isnull=True, cart_id="temp")): + map[cp.pk] = cp + cp.pk = None + cp.cart_id = self.session_key + cp.save() + for cp in reversed(CartPosition.objects.filter(addon_to__isnull=False, cart_id="temp")): + cp.pk = None + cp.cart_id = self.session_key + cp.addon_to = map[cp.addon_to_id] + cp.save() + cm = CartManager(event=self.event, cart_id=self.session_key) + cm.commit() # execute discounts on resorted cart + + self._set_payment() + response = self.client.post('/%s/%s/checkout/confirm/' % (self.orga.slug, self.event.slug), follow=True) + doc = BeautifulSoup(response.content.decode(), "lxml") + self.assertEqual(len(doc.select(".thank-you")), 1) + o = Order.objects.get() + assert o.positions.count() == 8 + class CheckoutSeatingTest(BaseCheckoutTestCase, TestCase): @scopes_disabled()