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()