From 9d1cfd1eb69c8b6897afd0f18f6dd875ffdc4387 Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Mon, 10 Oct 2022 12:59:49 +0200 Subject: [PATCH] Clarify cart order (#2844) --- src/pretix/base/models/orders.py | 21 ++++++-- src/pretix/base/views/mixins.py | 10 ---- src/pretix/presale/checkoutflow.py | 4 +- src/pretix/presale/views/__init__.py | 73 ++++++++++----------------- src/pretix/presale/views/questions.py | 2 +- 5 files changed, 47 insertions(+), 63 deletions(-) diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index d464f29053..89198448cc 100644 --- a/src/pretix/base/models/orders.py +++ b/src/pretix/base/models/orders.py @@ -2237,7 +2237,7 @@ class OrderPosition(AbstractPosition): @cached_property def sort_key(self): - return self.addon_to.positionid if self.addon_to else self.positionid, self.addon_to_id or 0 + return self.addon_to.positionid if self.addon_to else self.positionid, self.addon_to_id or 0, self.positionid @property def checkins(self): @@ -2263,7 +2263,7 @@ class OrderPosition(AbstractPosition): ops = [] cp_mapping = {} # The sorting key ensures that all addons come directly after the position they refer to - for i, cartpos in enumerate(sorted(cp, key=lambda c: (c.addon_to_id or c.pk, c.addon_to_id or 0))): + for i, cartpos in enumerate(sorted(cp, key=lambda c: c.sort_key)): op = OrderPosition(order=order) for f in AbstractPosition._meta.fields: if f.name == 'addon_to': @@ -2659,6 +2659,20 @@ class CartPosition(AbstractPosition): self.event.currency) return self.price - net + @cached_property + def sort_key(self): + subevent_key = (self.subevent.date_from, str(self.subevent.name), self.subevent_id) if self.subevent_id else (0, "", 0) + category_key = (self.item.category.position, self.item.category.id) if self.item.category_id is not None else (0, 0) + item_key = self.item.position, self.item_id + variation_key = (self.variation.position, self.variation.id) if self.variation_id is not None else (0, 0) + line_key = (self.price, (self.voucher_id or 0), (self.seat.sorting_rank if self.seat_id else None), self.pk) + sort_key = subevent_key + category_key + item_key + variation_key + line_key + + if self.addon_to_id: + return self.addon_to.sort_key + (1 if self.is_bundled else 2,) + sort_key + else: + return sort_key + def update_listed_price_and_voucher(self, voucher_only=False, max_discount=None): from pretix.base.services.pricing import ( get_listed_price, is_included_for_free, @@ -2716,7 +2730,8 @@ class CartPosition(AbstractPosition): @property def addons_without_bundled(self): - return [op for op in self.addons.all() if not op.is_bundled] + addons = [op for op in self.addons.all() if not op.is_bundled] + return sorted(addons, key=lambda cp: cp.sort_key) class InvoiceAddress(models.Model): diff --git a/src/pretix/base/views/mixins.py b/src/pretix/base/views/mixins.py index 65e11b547e..16440b7b82 100644 --- a/src/pretix/base/views/mixins.py +++ b/src/pretix/base/views/mixins.py @@ -44,16 +44,6 @@ class BaseQuestionsViewMixin: form_class = BaseQuestionsForm all_optional = False - @staticmethod - def _keyfunc(pos): - # Sort addons after the item they are an addon to - if isinstance(pos, OrderPosition): - i = pos.addon_to.positionid if pos.addon_to else pos.positionid - else: - i = pos.addon_to.pk if pos.addon_to else pos.pk - addon_penalty = 1 if pos.addon_to else 0 - return i, addon_penalty, pos.pk - @cached_property def _positions_for_questions(self): raise NotImplementedError() diff --git a/src/pretix/presale/checkoutflow.py b/src/pretix/presale/checkoutflow.py index 8d6de2ac99..1a5fc79b29 100644 --- a/src/pretix/presale/checkoutflow.py +++ b/src/pretix/presale/checkoutflow.py @@ -497,9 +497,9 @@ class AddOnsStep(CartMixin, AsyncAction, TemplateFlowStep): formset = [] quota_cache = {} item_cache = {} - for cartpos in get_cart(self.request).filter(addon_to__isnull=True).prefetch_related( + for cartpos in sorted(get_cart(self.request).filter(addon_to__isnull=True).prefetch_related( 'item__addons', 'item__addons__addon_category', 'addons', 'addons__variation', - ).order_by('pk'): + ), key=lambda c: c.sort_key): formsetentry = { 'pos': cartpos, 'item': cartpos.item, diff --git a/src/pretix/presale/views/__init__.py b/src/pretix/presale/views/__init__.py index d1ed53162f..208b7bb1d8 100644 --- a/src/pretix/presale/views/__init__.py +++ b/src/pretix/presale/views/__init__.py @@ -35,7 +35,7 @@ from collections import defaultdict from datetime import datetime, timedelta from decimal import Decimal -from functools import partial, wraps +from functools import wraps from itertools import groupby from django.conf import settings @@ -46,7 +46,7 @@ from django_scopes import scopes_disabled from pretix.base.i18n import language from pretix.base.models import ( - CartPosition, Customer, InvoiceAddress, ItemAddOn, OrderPosition, Question, + CartPosition, Customer, InvoiceAddress, ItemAddOn, Question, QuestionAnswer, QuestionOption, ) from pretix.base.services.cart import get_fees @@ -145,57 +145,32 @@ class CartMixin: # Group items of the same variation # We do this by list manipulations instead of a GROUP BY query, as # Django is unable to join related models in a .values() query - def keyfunc(pos, for_sorting=False): - if isinstance(pos, OrderPosition): - if pos.addon_to_id: - i = pos.addon_to.positionid - else: - i = pos.positionid - else: - if pos.addon_to_id: - i = pos.addon_to_id - else: - i = pos.pk - + def group_key(pos): # only used for grouping, sorting is done before already has_attendee_data = pos.item.admission and ( self.request.event.settings.attendee_names_asked or self.request.event.settings.attendee_emails_asked + or self.request.event.settings.attendee_company_asked + or self.request.event.settings.attendee_addresses_asked or pos_additional_fields.get(pos.pk) ) + grouping_allowed = ( + # Never group when we have per-ticket download buttons + not downloads and + # Never group if the position has add-ons + pos.pk not in has_addons and + # Never group if we have answers to show + (not answers or (not has_attendee_data and not bool(pos.item.questions.all()))) and # do not use .exists() to re-use prefetch cache + # Never group when we have a final order and a gift card code + (isinstance(pos, CartPosition) or not pos.item.issue_giftcard) + ) - addon_penalty = 1 if pos.addon_to_id else 0 - - if downloads \ - or pos.pk in has_addons \ - or pos.item.issue_giftcard \ - or (answers and (has_attendee_data or bool(pos.item.questions.all()))): # do not use .exists() to re-use prefetch cache - return ( - # standalone positions are grouped by main product position id, addons below them also sorted by position id - i, addon_penalty, pos.positionid if isinstance(pos, OrderPosition) else pos.pk, - # all other places are only used for positions that can be grouped. We just put zeros. - ) + (0, ) * 12 - - # positions are sorted and grouped by various attributes - category_key = (pos.item.category.position, pos.item.category.id) if pos.item.category is not None else (0, 0) - item_key = pos.item.position, pos.item_id - variation_key = (pos.variation.position, pos.variation.id) if pos.variation is not None else (0, 0) - subevent_key = (pos.subevent.date_from, str(pos.subevent.name), pos.subevent_id) if pos.subevent_id else (0, "", 0) - grp = subevent_key + category_key + item_key + variation_key + (pos.price, (pos.voucher_id or 0), (pos.seat_id or 0)) - if pos.addon_to_id: - if for_sorting: - ii = pos.positionid if isinstance(pos, OrderPosition) else pos.pk - else: - ii = 0 - return ( - i, addon_penalty, ii, - ) + grp - return ( - # These are grouped by attributes so we don't put any position ids - 0, 0, 0, - ) + grp + if not grouping_allowed: + return (pos.pk,) + (0, ) * 6 + else: + return (pos.addon_to_id or 0), pos.subevent_id, pos.item_id, pos.variation_id, pos.price, (pos.voucher_id or 0), (pos.seat_id or 0) positions = [] - for k, g in groupby(sorted(lcp, key=partial(keyfunc, for_sorting=True)), key=keyfunc): + for k, g in groupby(sorted(lcp, key=lambda c: c.sort_key), key=group_key): g = list(g) group = g[0] group.count = len(g) @@ -291,7 +266,7 @@ def get_cart(request): 'item__category__position', 'item__category_id', 'item__position', 'item__name', 'variation__value' ).select_related( 'item', 'variation', 'subevent', 'subevent__event', 'subevent__event__organizer', - 'item__tax_rule', 'addon_to', 'used_membership', 'used_membership__membership_type' + 'item__tax_rule', 'item__category', 'used_membership', 'used_membership__membership_type' ).select_related( 'addon_to' ).prefetch_related( @@ -312,8 +287,12 @@ def get_cart(request): ).select_related('dependency_question'), to_attr='questions_to_ask') ) + by_id = {cp.pk: cp for cp in request._cart_cache} for cp in request._cart_cache: - cp.event = request.event # Populate field with known value to save queries + # Populate fields with known values to save queries + cp.event = request.event + if cp.addon_to_id: + cp.addon_to = by_id[cp.addon_to_id] return request._cart_cache diff --git a/src/pretix/presale/views/questions.py b/src/pretix/presale/views/questions.py index 2ad8cb270f..76265434a5 100644 --- a/src/pretix/presale/views/questions.py +++ b/src/pretix/presale/views/questions.py @@ -46,7 +46,7 @@ class QuestionsViewMixin(BaseQuestionsViewMixin): @cached_property def _positions_for_questions(self): cart = get_cart(self.request) - return sorted(list(cart), key=self._keyfunc) + return sorted(list(cart), key=lambda cp: cp.sort_key) def question_form_kwargs(self, cr): d = {