From c167ed02a6fd716187b94d1e50c98f76347d5b2e Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Thu, 22 Jan 2026 16:40:41 +0100 Subject: [PATCH] tests, comments --- .../base/migrations/0297_outgoingmail.py | 16 +-- src/pretix/base/models/mail.py | 47 ++++++-- src/pretix/base/services/mail.py | 47 +++++--- src/pretix/testutils/mail.py | 5 + src/tests/base/test_mail.py | 100 +++++++++++++++++- 5 files changed, 186 insertions(+), 29 deletions(-) diff --git a/src/pretix/base/migrations/0297_outgoingmail.py b/src/pretix/base/migrations/0297_outgoingmail.py index 2cf669047f..edc5f36565 100644 --- a/src/pretix/base/migrations/0297_outgoingmail.py +++ b/src/pretix/base/migrations/0297_outgoingmail.py @@ -1,9 +1,11 @@ -# Generated by Django 4.2.17 on 2025-09-04 12:35 +# Generated by Django 4.2.26 on 2026-01-22 13:44 import django.db.models.deletion from django.conf import settings from django.db import migrations, models +import pretix.base.models.mail + class Migration(migrations.Migration): @@ -24,6 +26,8 @@ class Migration(migrations.Migration): ("status", models.CharField(default="queued", max_length=200)), ("created", models.DateTimeField(auto_now_add=True)), ("sent", models.DateTimeField(blank=True, null=True)), + ("inflight_since", models.DateTimeField(blank=True, null=True)), + ("retry_after", models.DateTimeField(blank=True, null=True)), ("error", models.TextField(null=True)), ("error_detail", models.TextField(null=True)), ("subject", models.TextField()), @@ -42,7 +46,7 @@ class Migration(migrations.Migration): "customer", models.ForeignKey( null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=pretix.base.models.mail.CASCADE_IF_QUEUED, related_name="outgoing_mails", to="pretixbase.customer", ), @@ -51,7 +55,7 @@ class Migration(migrations.Migration): "event", models.ForeignKey( null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=pretix.base.models.mail.CASCADE_IF_QUEUED, related_name="outgoing_mails", to="pretixbase.event", ), @@ -60,7 +64,7 @@ class Migration(migrations.Migration): "order", models.ForeignKey( null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=pretix.base.models.mail.CASCADE_IF_QUEUED, related_name="outgoing_mails", to="pretixbase.order", ), @@ -69,7 +73,7 @@ class Migration(migrations.Migration): "orderposition", models.ForeignKey( null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=pretix.base.models.mail.CASCADE_IF_QUEUED, related_name="outgoing_mails", to="pretixbase.orderposition", ), @@ -99,7 +103,7 @@ class Migration(migrations.Migration): "user", models.ForeignKey( null=True, - on_delete=django.db.models.deletion.SET_NULL, + on_delete=django.db.models.deletion.CASCADE, related_name="outgoing_mails", to=settings.AUTH_USER_MODEL, ), diff --git a/src/pretix/base/models/mail.py b/src/pretix/base/models/mail.py index 1a25055be6..4a75f9fbcd 100644 --- a/src/pretix/base/models/mail.py +++ b/src/pretix/base/models/mail.py @@ -25,6 +25,19 @@ from django.utils.translation import gettext_lazy as _ from django_scopes import scope, scopes_disabled +def CASCADE_IF_QUEUED(collector, field, sub_objs, using): + # If the email is still queued and the thing it is related to vanishes, the email can vanish as well + print(sub_objs) + cascade_objs = [ + o for o in sub_objs if o.status == OutgoingMail.STATUS_QUEUED + ] + if cascade_objs: + models.CASCADE(collector, field, cascade_objs, using) + + # In all other cases, set to NULL to keep the email on record + models.SET_NULL(collector, field, [o for o in sub_objs if o not in cascade_objs], using) + + class OutgoingMail(models.Model): STATUS_QUEUED = "queued" STATUS_INFLIGHT = "inflight" @@ -42,9 +55,23 @@ class OutgoingMail(models.Model): status = models.CharField(max_length=200, choices=STATUS_CHOICES, default=STATUS_QUEUED) created = models.DateTimeField(auto_now_add=True) sent = models.DateTimeField(null=True, blank=True) + inflight_since = models.DateTimeField(null=True, blank=True) + retry_after = models.DateTimeField(null=True, blank=True) error = models.TextField(null=True, blank=True) error_detail = models.TextField(null=True, blank=True) + # There is a conflict here between the different purposes of the model. As a system administrator, + # one wants *all* emails to be persisted as long as possible to debug issues. This means that if + # e.g. the event or order is deleted, we want SET_NULL behavior. However, in that case, the email + # would be an "orphan" forever and there's no way to remove the personal information. + # We try to find a middle-ground with the following behaviour: + # - The email is always deleted if the entire organize or user is deleted + # - The email is always deleted if it has not yet been sent + # - The email is kept in all other cases + # This is only an acceptable trade-off since emails are stored for a short period only, and because + # orders and customers are never deleted during normal operation. If we ever make this a long-term + # storage / email archive, we'd need to find another way to make sure personal information is removed + # if personal information of orders etc is removed. organizer = models.ForeignKey( 'pretixbase.Organizer', on_delete=models.CASCADE, @@ -53,31 +80,31 @@ class OutgoingMail(models.Model): ) event = models.ForeignKey( 'pretixbase.Event', - on_delete=models.SET_NULL, # todo think, only for non-queued! + on_delete=CASCADE_IF_QUEUED, related_name='outgoing_mails', null=True, blank=True, ) order = models.ForeignKey( 'pretixbase.Order', - on_delete=models.SET_NULL, + on_delete=CASCADE_IF_QUEUED, related_name='outgoing_mails', null=True, blank=True, ) orderposition = models.ForeignKey( 'pretixbase.OrderPosition', - on_delete=models.SET_NULL, + on_delete=CASCADE_IF_QUEUED, related_name='outgoing_mails', null=True, blank=True, ) customer = models.ForeignKey( 'pretixbase.Customer', - on_delete=models.SET_NULL, + on_delete=CASCADE_IF_QUEUED, related_name='outgoing_mails', null=True, blank=True, ) user = models.ForeignKey( 'pretixbase.User', - on_delete=models.SET_NULL, + on_delete=models.CASCADE, related_name='outgoing_mails', null=True, blank=True, ) @@ -91,17 +118,25 @@ class OutgoingMail(models.Model): cc = models.JSONField(default=list) bcc = models.JSONField(default=list) + # We don't store the actual invoices, tickets or calendar invites, so if the email is re-sent at a later time, a + # newer version of the files might be used. We accept that risk to save on storage and also because the new + # version might actually be more useful. should_attach_invoices = models.ManyToManyField( 'pretixbase.Invoice', related_name='outgoing_mails' ) should_attach_tickets = models.BooleanField(default=False) should_attach_ical = models.BooleanField(default=False) + + # We need to make sure cached files are kept as l should_attach_cached_files = models.ManyToManyField( 'pretixbase.CachedFile', related_name='outgoing_mails', ) # todo: prevent deletion? - should_attach_other_files = models.JSONField(default=list) # todo_ prevent deletion? + + # This is used to send files stored in settings. In most cases, these aren't short-lived and should still be there + # if the email is sent. Otherwise, they will be skipped. We accept that risk. + should_attach_other_files = models.JSONField(default=list) actual_attachments = models.JSONField(default=list) diff --git a/src/pretix/base/services/mail.py b/src/pretix/base/services/mail.py index 336a7c5a33..df51bfbb9a 100644 --- a/src/pretix/base/services/mail.py +++ b/src/pretix/base/services/mail.py @@ -40,6 +40,7 @@ import os import re import smtplib import warnings +from datetime import timedelta from email.mime.image import MIMEImage from email.utils import formataddr from typing import Any, Dict, Optional, Sequence, Union @@ -354,15 +355,20 @@ class CustomEmail(EmailMultiAlternatives): @app.task(base=TransactionAwareTask, bind=True, acks_late=True) def mail_send_task(self, *args, outgoing_mail: int) -> bool: with transaction.atomic(): - outgoing_mail = OutgoingMail.objects.select_for_update(of=OF_SELF).get(pk=outgoing_mail) + try: + outgoing_mail = OutgoingMail.objects.select_for_update(of=OF_SELF).get(pk=outgoing_mail) + except OutgoingMail.DoesNotExist: + logger.info(f"Ignoring job for non existing email {outgoing_mail}") + return False if outgoing_mail.status == OutgoingMail.STATUS_INFLIGHT: - logger.info("Ignoring job for inflight email") + logger.info(f"Ignoring job for inflight email {outgoing_mail.pk}") return False elif outgoing_mail.status in (OutgoingMail.STATUS_SENT, OutgoingMail.STATUS_FAILED): - logger.info(f"Ignoring job for email in final state {outgoing_mail.status}") + logger.info(f"Ignoring job for email {outgoing_mail.pk} in final state {outgoing_mail.status}") return False outgoing_mail.status = OutgoingMail.STATUS_INFLIGHT - outgoing_mail.save(update_fields=["status"]) + outgoing_mail.inflight_since = now() + outgoing_mail.save(update_fields=["status", "inflight_since"]) email = CustomEmail( subject=outgoing_mail.subject, @@ -405,7 +411,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: except MaxRetriesExceededError: # Well then, something is really wrong, let's send it without attachment before we # don't sent at all - logger.exception('Could not attach tickets to email') + logger.exception(f'Could not attach tickets to email {outgoing_mail.pk}') pass if attach_size * 1.37 < settings.FILE_UPLOAD_MAX_SIZE_EMAIL_ATTACHMENT - 1024 * 1024: @@ -464,7 +470,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: ) invoices_attached.append(inv) except Exception: - logger.exception('Could not attach invoice to email') + logger.exception(f'Could not attach invoice to email {outgoing_mail.pk}') pass else: if inv.transmission_type == "email": @@ -494,7 +500,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: ftype ) except: - logger.exception('Could not attach file to email') + logger.exception(f'Could not attach file to email {outgoing_mail.pk}') pass for cf in outgoing_mail.should_attach_cached_files.all(): @@ -506,7 +512,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: cf.type, ) except: - logger.exception('Could not attach file to email') + logger.exception(f'Could not attach file to email {outgoing_mail.pk}') pass if outgoing_mail.event: @@ -532,7 +538,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: try: backend.send_messages([email]) except Exception as e: - logger.exception('Error sending email') + logger.exception(f'Error sending email {outgoing_mail.pk}') retry_strategy = _retry_strategy(e) err, err_detail = _format_error(e) @@ -554,19 +560,23 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: retry_after = min(30 + cnt * 10, 1800) outgoing_mail.status = OutgoingMail.STATUS_AWAWITING_RETRY - outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent"]) + outgoing_mail.retry_after = now() + timedelta(seconds=retry_after) + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after"]) self.retry(max_retries=max_retries, countdown=retry_after) # throws RetryException, ends function flow elif retry_strategy in ("microsoft_concurrency", "quick"): max_retries = 5 retry_after = [10, 30, 60, 300, 900, 900][self.request.retries] outgoing_mail.status = OutgoingMail.STATUS_AWAWITING_RETRY - outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent"]) + outgoing_mail.retry_after = now() + timedelta(seconds=retry_after) + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after"]) self.retry(max_retries=max_retries, countdown=retry_after) # throws RetryException, ends function flow elif retry_strategy == "slow": + retry_after = [60, 300, 600, 1200, 1800, 1800][self.request.retries] outgoing_mail.status = OutgoingMail.STATUS_AWAWITING_RETRY - outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent"]) - self.retry(max_retries=5, countdown=[60, 300, 600, 1200, 1800, 1800][self.request.retries]) # throws RetryException, ends function flow + outgoing_mail.retry_after = now() + timedelta(seconds=retry_after) + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after"]) + self.retry(max_retries=5, countdown=retry_after) # throws RetryException, ends function flow except MaxRetriesExceededError: for i in invoices_to_mark_transmitted: @@ -586,12 +596,18 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: 'invoices': [], } ) + + outgoing_mail.status = OutgoingMail.STATUS_FAILED + outgoing_mail.sent = now() + outgoing_mail.retry_after = None + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after"]) return False # If we reach this, it's a non-retryable error outgoing_mail.status = OutgoingMail.STATUS_FAILED outgoing_mail.sent = now() - outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent"]) + outgoing_mail.retry_after = None + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after"]) for i in invoices_to_mark_transmitted: i.set_transmission_failed(provider="email_pdf", data={ "reason": "exception", @@ -615,7 +631,8 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool: outgoing_mail.error_detail = None outgoing_mail.actual_attachments = actual_attachments outgoing_mail.sent = now() - outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "actual_attachments"]) + outgoing_mail.retry_after = None + outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "actual_attachments", "retry_after"]) for i in invoices_to_mark_transmitted: if i.transmission_status != Invoice.TRANSMISSION_STATUS_COMPLETED: i.transmission_date = now() diff --git a/src/pretix/testutils/mail.py b/src/pretix/testutils/mail.py index 29a569894d..5313689a81 100644 --- a/src/pretix/testutils/mail.py +++ b/src/pretix/testutils/mail.py @@ -29,3 +29,8 @@ class FailingEmailBackend(EmailBackend): raise smtplib.SMTPRecipientsRefused({ 'recipient@example.org': (450, b'Recipient unknown') }) + + +class PermanentlyFailingEmailBackend(EmailBackend): + def send_messages(self, email_messages): + raise smtplib.SMTPNotSupportedError() diff --git a/src/tests/base/test_mail.py b/src/tests/base/test_mail.py index edd994815f..d68a02ac04 100644 --- a/src/tests/base/test_mail.py +++ b/src/tests/base/test_mail.py @@ -39,14 +39,15 @@ from email.mime.text import MIMEText import pytest from django.conf import settings from django.core import mail as djmail +from django.test import override_settings from django.utils.timezone import now from django.utils.translation import gettext_lazy as _ from django_scopes import scope from i18nfield.strings import LazyI18nString from pretix.base.email import get_email_context -from pretix.base.models import Event, Organizer, User -from pretix.base.services.mail import mail +from pretix.base.models import Event, Organizer, OutgoingMail, User +from pretix.base.services.mail import mail, mail_send_task @pytest.fixture @@ -162,6 +163,101 @@ def test_send_mail_with_user_locale(env): assert 'The language code used for rendering this email is de.' in djmail.outbox[0].body +@pytest.mark.django_db +def test_queue_state_sent(env): + m = OutgoingMail.objects.create( + to=['recipient@example.com'], + subject='Test', + body_plain='Test', + sender='sender@example.com', + headers={}, + ) + assert m.status == OutgoingMail.STATUS_QUEUED + mail_send_task.apply(kwargs={ + 'outgoing_mail': m.pk, + }, max_retries=0) + m.refresh_from_db() + assert m.status == OutgoingMail.STATUS_SENT + + +@pytest.mark.django_db +@override_settings(EMAIL_BACKEND='pretix.testutils.mail.PermanentlyFailingEmailBackend') +def test_queue_state_permanent_failure(env): + m = OutgoingMail.objects.create( + to=['recipient@example.com'], + subject='Test', + body_plain='Test', + sender='sender@example.com', + headers={}, + ) + assert m.status == OutgoingMail.STATUS_QUEUED + mail_send_task.apply(kwargs={ + 'outgoing_mail': m.pk, + }, max_retries=0) + m.refresh_from_db() + assert m.status == OutgoingMail.STATUS_FAILED + + +@pytest.mark.django_db +@override_settings(EMAIL_BACKEND='pretix.testutils.mail.FailingEmailBackend') +def test_queue_state_retry_failure(env, monkeypatch): + def retry(*args, **kwargs): + raise Exception() + + monkeypatch.setattr('celery.app.task.Task.retry', retry, raising=True) + m = OutgoingMail.objects.create( + to=['recipient@example.com'], + subject='Test', + body_plain='Test', + sender='sender@example.com', + headers={}, + ) + assert m.status == OutgoingMail.STATUS_QUEUED + mail_send_task.apply(kwargs={ + 'outgoing_mail': m.pk, + }, max_retries=0) + m.refresh_from_db() + assert m.status == OutgoingMail.STATUS_AWAWITING_RETRY + assert m.retry_after > now() + + +@pytest.mark.django_db +def test_queue_state_foreign_key_handling(): + o = Organizer.objects.create(name='Dummy', slug='dummy') + event = Event.objects.create( + organizer=o, name='Dummy', slug='dummy', + date_from=now() + ) + + mail_queued = OutgoingMail.objects.create( + organizer=o, + event=event, + to=['recipient@example.com'], + subject='Test', + body_plain='Test', + sender='sender@example.com', + headers={}, + ) + mail_sent = OutgoingMail.objects.create( + organizer=o, + event=event, + to=['recipient@example.com'], + subject='Test', + body_plain='Test', + sender='sender@example.com', + headers={}, + status=OutgoingMail.STATUS_SENT, + ) + + event.delete() + + assert not OutgoingMail.objects.filter(pk=mail_queued.pk).exists() + assert OutgoingMail.objects.get(pk=mail_sent.pk).event is None + + o.delete() + assert not OutgoingMail.objects.filter(pk=mail_sent.pk).exists() + + @pytest.mark.django_db def test_sendmail_placeholder(env): djmail.outbox = []