From df2d8925ed74c7195f48f1d4bca9b0748e041e5c Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Mon, 29 Oct 2018 17:27:12 +0100 Subject: [PATCH] Prevent some race conditions --- src/pretix/api/views/order.py | 6 +- .../migrations/0100_auto_20181023_2300.py | 2 +- src/pretix/base/models/base.py | 47 ++++++ src/pretix/base/models/orders.py | 6 +- src/pretix/base/services/orders.py | 152 ++++++++++-------- src/pretix/control/views/orders.py | 12 +- src/pretix/plugins/banktransfer/tasks.py | 6 +- src/setup.cfg | 2 +- src/tests/base/test_orders.py | 50 +++++- src/tests/plugins/banktransfer/test_import.py | 14 ++ 10 files changed, 209 insertions(+), 88 deletions(-) diff --git a/src/pretix/api/views/order.py b/src/pretix/api/views/order.py index b7ac7105de..b1be284a1f 100644 --- a/src/pretix/api/views/order.py +++ b/src/pretix/api/views/order.py @@ -251,7 +251,7 @@ class OrderViewSet(CreateModelMixin, viewsets.ReadOnlyModelViewSet): ) order.status = Order.STATUS_PENDING - order.save() + order.save(update_fields=['status']) order.log_action( 'pretix.event.order.unpaid', user=request.user if request.user.is_authenticated else None, @@ -556,7 +556,7 @@ class PaymentViewSet(viewsets.ReadOnlyModelViewSet): payment.order.event.subevents.filter( id__in=payment.order.positions.values_list('subevent_id', flat=True)) ) - payment.order.save() + payment.order.save(update_fields=['status', 'expires']) return Response(OrderRefundSerializer(r).data, status=status.HTTP_200_OK) @detail_route(methods=['POST']) @@ -622,7 +622,7 @@ class RefundViewSet(CreateModelMixin, viewsets.ReadOnlyModelViewSet): refund.order.event.subevents.filter( id__in=refund.order.positions.values_list('subevent_id', flat=True)) ) - refund.order.save() + refund.order.save(update_fields=['status', 'expires']) return self.retrieve(request, [], **kwargs) @detail_route(methods=['POST']) diff --git a/src/pretix/base/migrations/0100_auto_20181023_2300.py b/src/pretix/base/migrations/0100_auto_20181023_2300.py index c7adf0471b..0929ccba5b 100644 --- a/src/pretix/base/migrations/0100_auto_20181023_2300.py +++ b/src/pretix/base/migrations/0100_auto_20181023_2300.py @@ -1,7 +1,7 @@ # Generated by Django 2.1 on 2018-10-23 23:00 -from django.db import migrations, models import django_countries.fields +from django.db import migrations, models class Migration(migrations.Migration): diff --git a/src/pretix/base/models/base.py b/src/pretix/base/models/base.py index c4b783b08c..c052c42b20 100644 --- a/src/pretix/base/models/base.py +++ b/src/pretix/base/models/base.py @@ -3,6 +3,7 @@ import uuid from django.contrib.contenttypes.models import ContentType from django.db import models +from django.db.models.constants import LOOKUP_SEP from django.db.models.signals import post_delete from django.dispatch import receiver from django.utils.crypto import get_random_string @@ -100,3 +101,49 @@ class LoggedModel(models.Model, LoggingMixin): return LogEntry.objects.filter( content_type=ContentType.objects.get_for_model(type(self)), object_id=self.pk ).select_related('user', 'event', 'oauth_application', 'api_token', 'device') + + +class LockModel: + def refresh_for_update(self, fields=None, using=None, **kwargs): + """ + Like refresh_from_db(), but with select_for_update(). + See also https://code.djangoproject.com/ticket/28344 + """ + if fields is not None: + if not fields: + return + if any(LOOKUP_SEP in f for f in fields): + raise ValueError( + 'Found "%s" in fields argument. Relations and transforms ' + 'are not allowed in fields.' % LOOKUP_SEP) + + hints = {'instance': self} + db_instance_qs = self.__class__._base_manager.db_manager(using, hints=hints).filter(pk=self.pk).select_for_update(**kwargs) + + # Use provided fields, if not set then reload all non-deferred fields. + deferred_fields = self.get_deferred_fields() + if fields is not None: + fields = list(fields) + db_instance_qs = db_instance_qs.only(*fields) + elif deferred_fields: + fields = [f.attname for f in self._meta.concrete_fields + if f.attname not in deferred_fields] + db_instance_qs = db_instance_qs.only(*fields) + + db_instance = db_instance_qs.get() + non_loaded_fields = db_instance.get_deferred_fields() + for field in self._meta.concrete_fields: + if field.attname in non_loaded_fields: + # This field wasn't refreshed - skip ahead. + continue + setattr(self, field.attname, getattr(db_instance, field.attname)) + # Clear cached foreign keys. + if field.is_relation and field.is_cached(self): + field.delete_cached_value(self) + + # Clear cached relations. + for field in self._meta.related_objects: + if field.is_cached(self): + field.delete_cached_value(self) + + self._state.db = db_instance._state.db diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index 0839f1c87c..01df920da5 100644 --- a/src/pretix/base/models/orders.py +++ b/src/pretix/base/models/orders.py @@ -31,7 +31,7 @@ from pretix.base.i18n import language from pretix.base.models import User from pretix.base.reldate import RelativeDateWrapper -from .base import LoggedModel +from .base import LockModel, LoggedModel from .event import Event, SubEvent from .items import Item, ItemVariation, Question, QuestionOption, Quota @@ -47,7 +47,7 @@ def generate_position_secret(): return get_random_string(length=settings.ENTROPY['ticket_secret'], allowed_chars='abcdefghjkmnpqrstuvwxyz23456789') -class Order(LoggedModel): +class Order(LockModel, LoggedModel): """ An order is created when a user clicks 'buy' on his cart. It holds several OrderPositions and is connected to a user. It has an @@ -903,7 +903,7 @@ class OrderPayment(models.Model): if not force and can_be_paid is not True: raise Quota.QuotaExceededException(can_be_paid) self.order.status = Order.STATUS_PAID - self.order.save() + self.order.save(update_fields=['status']) self.order.log_action('pretix.event.order.paid', { 'provider': self.provider, diff --git a/src/pretix/base/services/orders.py b/src/pretix/base/services/orders.py index 8bb52c1e01..641d094bf5 100644 --- a/src/pretix/base/services/orders.py +++ b/src/pretix/base/services/orders.py @@ -93,7 +93,7 @@ def extend_order(order: Order, new_date: datetime, force: bool=False, user: User raise OrderError(_('The new expiry date needs to be in the future.')) if order.status == Order.STATUS_PENDING: order.expires = new_date - order.save() + order.save(update_fields=['expires']) order.log_action( 'pretix.event.order.expirychanged', user=user, @@ -109,7 +109,7 @@ def extend_order(order: Order, new_date: datetime, force: bool=False, user: User if is_available is True or force is True: order.expires = new_date order.status = Order.STATUS_PENDING - order.save() + order.save(update_fields=['expires', 'status']) order.log_action( 'pretix.event.order.expirychanged', user=user, @@ -136,7 +136,7 @@ def mark_order_refunded(order, user=None, auth=None, api_token=None): user = User.objects.get(pk=user) with order.event.lock(): order.status = Order.STATUS_REFUNDED - order.save() + order.save(update_fields=['status']) order.log_action('pretix.event.order.refunded', user=user, auth=auth or api_token) i = order.invoices.filter(is_cancellation=False).last() @@ -159,7 +159,7 @@ def mark_order_expired(order, user=None, auth=None): user = User.objects.get(pk=user) with order.event.lock(): order.status = Order.STATUS_EXPIRED - order.save() + order.save(update_fields=['status']) order.log_action('pretix.event.order.expired', user=user, auth=auth) i = order.invoices.filter(is_cancellation=False).last() @@ -181,7 +181,7 @@ def approve_order(order, user=None, send_mail: bool=True, auth=None): order.require_approval = False order.set_expires(now(), order.event.subevents.filter(id__in=[p.subevent_id for p in order.positions.all()])) - order.save() + order.save(update_fields=['require_approval', 'expires']) order.log_action('pretix.event.order.approved', user=user, auth=auth) if order.total == Decimal('0.00'): @@ -258,7 +258,7 @@ def deny_order(order, comment='', user=None, send_mail: bool=True, auth=None): with order.event.lock(): order.status = Order.STATUS_CANCELED - order.save() + order.save(update_fields=['status']) order.log_action('pretix.event.order.denied', user=user, auth=auth, data={ 'comment': comment @@ -327,7 +327,7 @@ def _cancel_order(order, user=None, send_mail: bool=True, api_token=None, device if not order.cancel_allowed(): raise OrderError(_('You cannot cancel this order.')) order.status = Order.STATUS_CANCELED - order.save() + order.save(update_fields=['status']) order.log_action('pretix.event.order.canceled', user=user, auth=api_token or oauth_application or device) i = order.invoices.filter(is_cancellation=False).last() @@ -661,47 +661,52 @@ def send_expiry_warnings(sender, **kwargs): eventcache = {} today = now().replace(hour=0, minute=0, second=0) - for o in Order.objects.filter(expires__gte=today, expiry_reminder_sent=False, status=Order.STATUS_PENDING).select_related('event'): - eventsettings = eventcache.get(o.event.pk, None) - if eventsettings is None: - eventsettings = o.event.settings - eventcache[o.event.pk] = eventsettings + for o in Order.objects.filter(expires__gte=today, expiry_reminder_sent=False, status=Order.STATUS_PENDING).only('pk'): + with transaction.atomic(): + o = Order.objects.select_related('event').select_for_update().get(pk=o.pk) + if o.status != Order.STATUS_PENDING or o.expiry_reminder_sent: + # Race condition + continue + eventsettings = eventcache.get(o.event.pk, None) + if eventsettings is None: + eventsettings = o.event.settings + eventcache[o.event.pk] = eventsettings - days = eventsettings.get('mail_days_order_expire_warning', as_type=int) - tz = pytz.timezone(eventsettings.get('timezone', settings.TIME_ZONE)) - if days and (o.expires - today).days <= days: - with language(o.locale): - o.expiry_reminder_sent = True - o.save() - try: - invoice_name = o.invoice_address.name - invoice_company = o.invoice_address.company - except InvoiceAddress.DoesNotExist: - invoice_name = "" - invoice_company = "" - email_template = eventsettings.mail_text_order_expire_warning - email_context = { - 'event': o.event.name, - 'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={ - 'order': o.code, - 'secret': o.secret - }), - 'expire_date': date_format(o.expires.astimezone(tz), 'SHORT_DATE_FORMAT'), - 'invoice_name': invoice_name, - 'invoice_company': invoice_company, - } - if eventsettings.payment_term_expire_automatically: - email_subject = _('Your order is about to expire: %(code)s') % {'code': o.code} - else: - email_subject = _('Your order is pending payment: %(code)s') % {'code': o.code} + days = eventsettings.get('mail_days_order_expire_warning', as_type=int) + tz = pytz.timezone(eventsettings.get('timezone', settings.TIME_ZONE)) + if days and (o.expires - today).days <= days: + with language(o.locale): + o.expiry_reminder_sent = True + o.save(update_fields=['expiry_reminder_sent']) + try: + invoice_name = o.invoice_address.name + invoice_company = o.invoice_address.company + except InvoiceAddress.DoesNotExist: + invoice_name = "" + invoice_company = "" + email_template = eventsettings.mail_text_order_expire_warning + email_context = { + 'event': o.event.name, + 'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={ + 'order': o.code, + 'secret': o.secret + }), + 'expire_date': date_format(o.expires.astimezone(tz), 'SHORT_DATE_FORMAT'), + 'invoice_name': invoice_name, + 'invoice_company': invoice_company, + } + if eventsettings.payment_term_expire_automatically: + email_subject = _('Your order is about to expire: %(code)s') % {'code': o.code} + else: + email_subject = _('Your order is pending payment: %(code)s') % {'code': o.code} - try: - o.send_mail( - email_subject, email_template, email_context, - 'pretix.event.order.email.expire_warning_sent' - ) - except SendMailException: - logger.exception('Reminder email could not be sent') + try: + o.send_mail( + email_subject, email_template, email_context, + 'pretix.event.order.email.expire_warning_sent' + ) + except SendMailException: + logger.exception('Reminder email could not be sent') @receiver(signal=periodic_task) @@ -709,6 +714,7 @@ def send_download_reminders(sender, **kwargs): today = now().replace(hour=0, minute=0, second=0, microsecond=0) for e in Event.objects.filter(date_from__gte=today): + days = e.settings.get('mail_days_download_reminder', as_type=int) if days is None: continue @@ -717,30 +723,35 @@ def send_download_reminders(sender, **kwargs): if now() < reminder_date: continue - for o in e.orders.filter(status=Order.STATUS_PAID, download_reminder_sent=False): - if not all([r for rr, r in allow_ticket_download.send(e, order=o)]): - continue + for o in e.orders.filter(status=Order.STATUS_PAID, download_reminder_sent=False).only('pk'): + with transaction.atomic(): + o = Order.objects.select_related('event').select_for_update().get(pk=o.pk) + if o.download_reminder_sent: + # Race condition + continue + if not all([r for rr, r in allow_ticket_download.send(e, order=o)]): + continue - with language(o.locale): - o.download_reminder_sent = True - o.save() - email_template = e.settings.mail_text_download_reminder - email_context = { - 'event': o.event.name, - 'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={ - 'order': o.code, - 'secret': o.secret - }), - } - email_subject = _('Your ticket is ready for download: %(code)s') % {'code': o.code} - try: - o.send_mail( - email_subject, email_template, email_context, - 'pretix.event.order.email.download_reminder_sent', - attach_tickets=True - ) - except SendMailException: - logger.exception('Reminder email could not be sent') + with language(o.locale): + o.download_reminder_sent = True + o.save(update_fields=['download_reminder_sent']) + email_template = e.settings.mail_text_download_reminder + email_context = { + 'event': o.event.name, + 'url': build_absolute_uri(o.event, 'presale:event.order', kwargs={ + 'order': o.code, + 'secret': o.secret + }), + } + email_subject = _('Your ticket is ready for download: %(code)s') % {'code': o.code} + try: + o.send_mail( + email_subject, email_template, email_context, + 'pretix.event.order.email.download_reminder_sent', + attach_tickets=True + ) + except SendMailException: + logger.exception('Reminder email could not be sent') class OrderChangeManager: @@ -767,6 +778,7 @@ class OrderChangeManager: def __init__(self, order: Order, user=None, auth=None, notify=True): self.order = order + self.order.refresh_for_update() self.user = user self.auth = auth self.split_order = None @@ -1170,7 +1182,7 @@ class OrderChangeManager: fee.save() if not self.open_payment.fee: self.open_payment.fee = fee - self.open_payment.save() + self.open_payment.save(update_fields=['fee']) elif fee: fee.delete() diff --git a/src/pretix/control/views/orders.py b/src/pretix/control/views/orders.py index a81ef10ebd..8e53abdb26 100644 --- a/src/pretix/control/views/orders.py +++ b/src/pretix/control/views/orders.py @@ -218,7 +218,7 @@ class OrderComment(OrderView): self.order.log_action('pretix.event.order.checkin_attention', user=self.request.user, data={ 'new_value': form.cleaned_data.get('checkin_attention') }) - self.order.save() + self.order.save(update_fields=['checkin_attention', 'comment']) messages.success(self.request, _('The comment has been updated.')) else: messages.error(self.request, _('Could not update the comment.')) @@ -345,7 +345,7 @@ class OrderRefundProcess(OrderView): self.order.event.subevents.filter( id__in=self.order.positions.values_list('subevent_id', flat=True)) ) - self.order.save() + self.order.save(update_fields=['status', 'expires']) messages.success(self.request, _('The refund has been processed.')) else: @@ -507,7 +507,7 @@ class OrderRefundView(OrderView): manual_value = formats.sanitize_separators(manual_value) try: manual_value = Decimal(manual_value) - except (DecimalException, TypeError) as e: + except (DecimalException, TypeError): messages.error(self.request, _('You entered an invalid number.')) is_valid = False else: @@ -530,7 +530,7 @@ class OrderRefundView(OrderView): offsetting_value = formats.sanitize_separators(offsetting_value) try: offsetting_value = Decimal(offsetting_value) - except (DecimalException, TypeError) as e: + except (DecimalException, TypeError): messages.error(self.request, _('You entered an invalid number.')) is_valid = False else: @@ -561,7 +561,7 @@ class OrderRefundView(OrderView): value = formats.sanitize_separators(value) try: value = Decimal(value) - except (DecimalException, TypeError) as e: + except (DecimalException, TypeError): messages.error(self.request, _('You entered an invalid number.')) is_valid = False else: @@ -630,7 +630,7 @@ class OrderRefundView(OrderView): self.order.event.subevents.filter( id__in=self.order.positions.values_list('subevent_id', flat=True)) ) - self.order.save() + self.order.save(update_fields=['status', 'expires']) return redirect(self.get_order_url()) else: diff --git a/src/pretix/plugins/banktransfer/tasks.py b/src/pretix/plugins/banktransfer/tasks.py index d6ac8f97b2..7e29ebb050 100644 --- a/src/pretix/plugins/banktransfer/tasks.py +++ b/src/pretix/plugins/banktransfer/tasks.py @@ -142,12 +142,12 @@ def process_banktransfers(self, job: int, data: list) -> None: code_len = settings.ENTROPY['order_code'] if job.event: - pattern = re.compile(job.event.slug.upper() + "[ \-_]*([A-Z0-9]{%s})" % code_len) + pattern = re.compile(job.event.slug.upper() + r"[ \-_]*([A-Z0-9]{%s})" % code_len) else: if not prefixes: - prefixes = [e.slug.upper().replace(".", r"\.").replace("-", r"\-") + prefixes = [e.slug.upper().replace(".", r"\.").replace("-", r"[\- ]*") for e in job.organizer.events.all()] - pattern = re.compile("(%s)[ \-_]*([A-Z0-9]{%s})" % ("|".join(prefixes), code_len)) + pattern = re.compile("(%s)[ \\-_]*([A-Z0-9]{%s})" % ("|".join(prefixes), code_len)) for trans in transactions: match = pattern.search(trans.reference.replace(" ", "").replace("\n", "").upper()) diff --git a/src/setup.cfg b/src/setup.cfg index 69f088e42c..065a65eb48 100644 --- a/src/setup.cfg +++ b/src/setup.cfg @@ -1,5 +1,5 @@ [flake8] -ignore = N802,W503,E402,C901,E722 +ignore = N802,W503,E402,C901,E722,W504,E252 max-line-length = 160 exclude = migrations,.ropeproject,static,mt940.py,_static,build,make_testdata.py,*/testutils/settings.py,tests/settings.py,pretix/base/models/__init__.py max-complexity = 11 diff --git a/src/tests/base/test_orders.py b/src/tests/base/test_orders.py index 13038dc96c..8ec06dc11a 100644 --- a/src/tests/base/test_orders.py +++ b/src/tests/base/test_orders.py @@ -19,7 +19,7 @@ from pretix.base.reldate import RelativeDate, RelativeDateWrapper from pretix.base.services.invoices import generate_invoice from pretix.base.services.orders import ( OrderChangeManager, OrderError, _create_order, approve_order, deny_order, - expire_orders, send_download_reminders, + expire_orders, send_download_reminders, send_expiry_warnings, ) @@ -289,6 +289,54 @@ def test_deny(event): assert 'denied' in djmail.outbox[0].subject +class PaymentReminderTests(TestCase): + def setUp(self): + super().setUp() + o = Organizer.objects.create(name='Dummy', slug='dummy') + self.event = Event.objects.create( + organizer=o, name='Dummy', slug='dummy', + date_from=now() + timedelta(days=2), + plugins='pretix.plugins.banktransfer' + ) + self.order = Order.objects.create( + code='FOO', event=self.event, email='dummy@dummy.test', + status=Order.STATUS_PENDING, locale='en', + datetime=now(), + expires=now() + timedelta(days=10), + total=Decimal('46.00'), + ) + self.ticket = Item.objects.create(event=self.event, name='Early-bird ticket', + default_price=Decimal('23.00'), admission=True) + self.op1 = OrderPosition.objects.create( + order=self.order, item=self.ticket, variation=None, + price=Decimal("23.00"), attendee_name="Peter", positionid=1 + ) + djmail.outbox = [] + + def test_disabled(self): + send_expiry_warnings(sender=self.event) + assert len(djmail.outbox) == 0 + + def test_sent_once(self): + self.event.settings.mail_days_order_expire_warning = 12 + send_expiry_warnings(sender=self.event) + assert len(djmail.outbox) == 1 + + def test_paid(self): + self.order.status = Order.STATUS_PAID + self.order.save() + send_expiry_warnings(sender=self.event) + assert len(djmail.outbox) == 0 + + def test_sent_days(self): + self.event.settings.mail_days_order_expire_warning = 9 + send_expiry_warnings(sender=self.event) + assert len(djmail.outbox) == 0 + self.event.settings.mail_days_order_expire_warning = 10 + send_expiry_warnings(sender=self.event) + assert len(djmail.outbox) == 1 + + class DownloadReminderTests(TestCase): def setUp(self): super().setUp() diff --git a/src/tests/plugins/banktransfer/test_import.py b/src/tests/plugins/banktransfer/test_import.py index fc2efa8caf..b945371bee 100644 --- a/src/tests/plugins/banktransfer/test_import.py +++ b/src/tests/plugins/banktransfer/test_import.py @@ -248,6 +248,20 @@ def test_mark_paid_organizer(env, orga_job): assert env[2].status == Order.STATUS_PAID +@pytest.mark.django_db +def test_mark_paid_organizer_dash_in_slug(env, orga_job): + env[0].slug = "foo-bar" + env[0].save() + process_banktransfers(orga_job, [{ + 'payer': 'Karla Kundin', + 'reference': 'Bestellung FOO-BAR-1234S', + 'date': '2016-01-26', + 'amount': '23.00' + }]) + env[2].refresh_from_db() + assert env[2].status == Order.STATUS_PAID + + @pytest.mark.django_db def test_mark_paid_organizer_weird_slug(env, orga_job): env[0].slug = 'du.m-y'