Fix mail_send

src/pretix/base/migrations/0297_outgoingmail.py

@@ -40,6 +40,7 @@ class Migration(migrations.Migration):
                 ("to", models.JSONField(default=list)),
                 ("cc", models.JSONField(default=list)),
                 ("bcc", models.JSONField(default=list)),
+                ("recipient_count", models.IntegerField(default=1)),
                 ("should_attach_tickets", models.BooleanField(default=False)),
                 ("should_attach_ical", models.BooleanField(default=False)),
                 ("should_attach_other_files", models.JSONField(default=list)),

src/pretix/base/models/mail.py

@@ -42,26 +42,48 @@ def CASCADE_IF_QUEUED(collector, field, sub_objs, using):
 
 class OutgoingMail(models.Model):
     STATUS_QUEUED = "queued"
+    STATUS_WITHHELD = "withheld"
     STATUS_INFLIGHT = "inflight"
-    STATUS_AWAWITING_RETRY = "awaiting_retry"
+    STATUS_AWAITING_RETRY = "awaiting_retry"
     STATUS_FAILED = "failed"
     STATUS_SENT = "sent"
     STATUS_BOUNCED = "bounced"
+    STATUS_ABORTED = "aborted"
     STATUS_CHOICES = (
         (STATUS_QUEUED, _("queued")),
         (STATUS_INFLIGHT, _("being sent")),
-        (STATUS_AWAWITING_RETRY, _("awaiting retry")),
+        (STATUS_AWAITING_RETRY, _("awaiting retry")),
+        (STATUS_WITHHELD, _("withheld")),  # for plugin use
         (STATUS_FAILED, _("failed")),
+        (STATUS_ABORTED, _("aborted")),
         (STATUS_SENT, _("sent")),
-        (STATUS_BOUNCED, _("bounced")),
+        (STATUS_BOUNCED, _("bounced")),  # for plugin use
     )
+    STATUS_LIST_ABORTABLE = {
+        STATUS_QUEUED,
+        STATUS_WITHHELD,
+        STATUS_AWAITING_RETRY,
+    }
+    STATUS_LIST_RETRYABLE = {
+        STATUS_FAILED,
+        STATUS_WITHHELD,
+    }
 
+    # The GUID is a globally unique ID for the email added to a header of the email for later tracing
+    # in bug reports etc. We could theoretically also use this as a basis for the Message-ID header, but
+    # we currently don't since we are unsure if some intermediary SMTP servers have opinions on setting
+    # their own Message-ID headers.
     guid = models.UUIDField(db_index=True, default=uuid.uuid4)
+
     status = models.CharField(max_length=200, choices=STATUS_CHOICES, default=STATUS_QUEUED)
     created = models.DateTimeField(auto_now_add=True)
+
+    # sent will be the time the email was sent or the email failed
     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)
 
@@ -122,6 +144,7 @@ class OutgoingMail(models.Model):
     to = models.JSONField(default=list)
     cc = models.JSONField(default=list)
     bcc = models.JSONField(default=list)
+    recipient_count = models.IntegerField()
 
     # 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
@@ -143,6 +166,7 @@ class OutgoingMail(models.Model):
     # if the email is sent. Otherwise, they will be skipped. We accept that risk.
     should_attach_other_files = models.JSONField(default=list)
 
+    # [{name, type size}] of the attachments we actually setn
     actual_attachments = models.JSONField(default=list)
 
     class Meta:
@@ -164,7 +188,11 @@ class OutgoingMail(models.Model):
 
     @property
     def is_failed(self):
-        return self.status in (OutgoingMail.STATUS_FAILED, OutgoingMail.STATUS_AWAWITING_RETRY, OutgoingMail.STATUS_BOUNCED)
+        return self.status in (
+            OutgoingMail.STATUS_FAILED,
+            OutgoingMail.STATUS_AWAITING_RETRY,
+            OutgoingMail.STATUS_BOUNCED,
+        )
 
     def save(self, *args, **kwargs):
         if self.orderposition_id and not self.order_id:
@@ -175,6 +203,7 @@ class OutgoingMail(models.Model):
             self.organizer = self.event.organizer
         if self.customer_id and not self.organizer_id:
             self.organizer = self.customer.organizer
+        self.recipient_count = len(self.to) + len(self.cc) + len(self.bcc)
         super().save(*args, **kwargs)
 
     def log_parameters(self):

src/pretix/base/services/cleanup.py

@@ -56,7 +56,7 @@ def clean_cached_files(sender, **kwargs):
             status__in=(
                 OutgoingMail.STATUS_QUEUED,
                 OutgoingMail.STATUS_INFLIGHT,
-                OutgoingMail.STATUS_AWAWITING_RETRY,
+                OutgoingMail.STATUS_AWAITING_RETRY,
                 OutgoingMail.STATUS_FAILED,
             ),
         )

src/pretix/base/services/mail.py

@@ -103,6 +103,12 @@ class SendMailException(Exception):
     pass
 
 
+class WithholdMailException(Exception):
+    def __init__(self, error, error_detail):
+        self.error = error
+        self.error_detail = error_detail
+
+
 def clean_sender_name(sender_name: str) -> str:
     # Even though we try to properly escape sender names, some characters seem to cause problems when the escaping
     # fails due to some forwardings, etc.
@@ -366,13 +372,13 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
         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}")
+            logger.info(f"Ignoring job for non existing email {outgoing_mail.guid}")
             return False
         if outgoing_mail.status == OutgoingMail.STATUS_INFLIGHT:
-            logger.info(f"Ignoring job for inflight email {outgoing_mail.pk}")
+            logger.info(f"Ignoring job for inflight email {outgoing_mail.guid}")
             return False
-        elif outgoing_mail.status in (OutgoingMail.STATUS_SENT, OutgoingMail.STATUS_FAILED):
+        elif outgoing_mail.status not in (OutgoingMail.STATUS_AWAITING_RETRY, OutgoingMail.STATUS_QUEUED):
-            logger.info(f"Ignoring job for email {outgoing_mail.pk} in final state {outgoing_mail.status}")
+            logger.info(f"Ignoring job for email {outgoing_mail.guid} in final state {outgoing_mail.status}")
             return False
         outgoing_mail.status = OutgoingMail.STATUS_INFLIGHT
         outgoing_mail.inflight_since = now()
@@ -387,7 +393,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
         to=outgoing_mail.to,
         cc=outgoing_mail.cc,
         bcc=outgoing_mail.bcc,
-        headers=outgoing_mail.headers,
+        headers=headers,
     )
 
     # Rewrite all <img> tags from real URLs or data URLs to inline attachments referred to by content ID
@@ -420,7 +426,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(f'Could not attach tickets to email {outgoing_mail.pk}')
+                            logger.exception(f'Could not attach tickets to email {outgoing_mail.guid}')
                             pass
 
                 if attach_size * 1.37 < settings.FILE_UPLOAD_MAX_SIZE_EMAIL_ATTACHMENT - 1024 * 1024:
@@ -479,7 +485,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
                         )
                     invoices_attached.append(inv)
                 except Exception:
-                    logger.exception(f'Could not attach invoice to email {outgoing_mail.pk}')
+                    logger.exception(f'Could not attach invoice to email {outgoing_mail.guid}')
                     pass
                 else:
                     if inv.transmission_type == "email":
@@ -509,7 +515,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
                     ftype
                 )
             except:
-                logger.exception(f'Could not attach file to email {outgoing_mail.pk}')
+                logger.exception(f'Could not attach file to email {outgoing_mail.guid}')
                 pass
 
         for cf in outgoing_mail.should_attach_cached_files.all():
@@ -521,20 +527,9 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
                         cf.type,
                     )
                 except:
-                    logger.exception(f'Could not attach file to email {outgoing_mail.pk}')
+                    logger.exception(f'Could not attach file to email {outgoing_mail.guid}')
                     pass
 
-        if outgoing_mail.event:
-            with outgoing_mail.scope_manager():
-                email = email_filter.send_chained(
-                    outgoing_mail.event, 'message', message=email, order=outgoing_mail.order, user=outgoing_mail.user
-                )
-
-        email = global_email_filter.send_chained(
-            outgoing_mail.event, 'message', message=email, user=outgoing_mail.user, order=outgoing_mail.order,
-            organizer=outgoing_mail.organizer, customer=outgoing_mail.customer
-        )
-
         outgoing_mail.actual_attachments = [
             {
                 "name": a[0],
@@ -543,11 +538,58 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
             } for a in email.attachments
         ]
 
+        try:
+            if outgoing_mail.event:
+                with outgoing_mail.scope_manager():
+                    email = email_filter.send_chained(
+                        sender=outgoing_mail.event,
+                        chain_kwarg_name='message',
+                        message=email,
+                        order=outgoing_mail.order,
+                        user=outgoing_mail.user,
+                        outgoing_mail=outgoing_mail,
+                    )
+
+            email = global_email_filter.send_chained(
+                sender=outgoing_mail.event,
+                chain_kwarg_name='message',
+                message=email,
+                user=outgoing_mail.user,
+                order=outgoing_mail.order,
+                organizer=outgoing_mail.organizer,
+                customer=outgoing_mail.customer,
+                outgoing_mail=outgoing_mail,
+            )
+        except WithholdMailException as e:
+            outgoing_mail.status = OutgoingMail.STATUS_WITHHELD
+            outgoing_mail.error = e.error
+            outgoing_mail.error_detail = e.error_detail
+            outgoing_mail.sent = now()
+            outgoing_mail.retry_after = None
+            outgoing_mail.actual_attachments = [
+                {
+                    "name": a[0],
+                    "size": len(a[1]),
+                    "type": a[2],
+                } for a in email.attachments
+            ]
+            outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after", "actual_attachments"])
+            logger.info(f"Email {outgoing_mail.guid} withheld")
+            return False
+
+        # Seems duplicate, but needs to be in this order since plugins might change this
+        outgoing_mail.actual_attachments = [
+            {
+                "name": a[0],
+                "size": len(a[1]),
+                "type": a[2],
+            } for a in email.attachments
+        ]
         backend = outgoing_mail.get_mail_backend()
         try:
             backend.send_messages([email])
         except Exception as e:
-            logger.exception(f'Error sending email {outgoing_mail.pk}')
+            logger.exception(f'Error sending email {outgoing_mail.guid}')
             retry_strategy = _retry_strategy(e)
             err, err_detail = _format_error(e)
 
@@ -568,21 +610,21 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
                     max_retries = 10
                     retry_after = min(30 + cnt * 10, 1800)
 
-                    outgoing_mail.status = OutgoingMail.STATUS_AWAWITING_RETRY
+                    outgoing_mail.status = OutgoingMail.STATUS_AWAITING_RETRY
                     outgoing_mail.retry_after = now() + timedelta(seconds=retry_after)
                     outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after", "actual_attachments"])
                     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.status = OutgoingMail.STATUS_AWAITING_RETRY
                     outgoing_mail.retry_after = now() + timedelta(seconds=retry_after)
                     outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after", "actual_attachments"])
                     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.status = OutgoingMail.STATUS_AWAITING_RETRY
                     outgoing_mail.retry_after = now() + timedelta(seconds=retry_after)
                     outgoing_mail.save(update_fields=["status", "error", "error_detail", "sent", "retry_after", "actual_attachments"])
                     self.retry(max_retries=5, countdown=retry_after)  # throws RetryException, ends function flow
@@ -681,7 +723,7 @@ def mail_send_task(self, *args, outgoing_mail: int) -> bool:
                 )
 
 
-def mail_send(to: List[str], subject: str, body: str, html: str, sender: str,
+def mail_send(to: List[str], subject: str, body: str, html: Optional[str], sender: str,
               event: int = None, position: int = None, headers: dict = None, cc: List[str] = None, bcc: List[str] = None,
               invoices: List[int] = None, order: int = None, attach_tickets=False, user=None,
               organizer=None, customer=None, attach_ical=False, attach_cached_files: List[int] = None,
@@ -708,6 +750,15 @@ def mail_send(to: List[str], subject: str, body: str, html: str, sender: str,
         should_attach_ical=attach_ical,
         should_attach_other_files=attach_other_files or [],
     )
+    if invoices and not position:
+        m.should_attach_invoices.add(*invoices)
+    if attach_cached_files:
+        for cf in attach_cached_files:
+            if not isinstance(cf, CachedFile):
+                m.should_attach_cached_files.add(CachedFile.objects.get(pk=cf))
+            else:
+                m.should_attach_cached_files.add(cf)
+
     mail_send_task.apply_async(kwargs={"outgoing_mail": m.pk})
 
 
@@ -1007,7 +1058,7 @@ def retry_stuck_queued_mails(sender, **kwargs):
             status=OutgoingMail.STATUS_QUEUED,
             created__lt=now() - timedelta(hours=1),
         ) | Q(
-            status=OutgoingMail.STATUS_AWAWITING_RETRY,
+            status=OutgoingMail.STATUS_AWAITING_RETRY,
             retry_after__lt=now() - timedelta(hours=1),
         )
     ):

src/pretix/base/signals.py

@@ -944,32 +944,40 @@ As with all event-plugin signals, the ``sender`` keyword argument will contain t
 
 email_filter = EventPluginSignal()
 """
-Arguments: ``message``, ``order``, ``user``
+Arguments: ``message``, ``order``, ``user``, ``outgoing_mail``
 
 This signal allows you to implement a middleware-style filter on all outgoing emails. You are expected to
 return a (possibly modified) copy of the message object passed to you.
 
 As with all event-plugin signals, the ``sender`` keyword argument will contain the event.
 The ``message`` argument will contain an ``EmailMultiAlternatives`` object.
+The ``outgoing_mail`` argument will contain the ``OutgoingMail`` model instance. Note that the ``message`` object
+might have newer information if a previous plugin already modified the email.
 If the email is associated with a specific order, the ``order`` argument will be passed as well, otherwise
 it will be ``None``.
 If the email is associated with a specific user, e.g. a notification email, the ``user`` argument will be passed as
 well, otherwise it will be ``None``.
+
+You can raise ``WithholdMailException`` to prevent the email from being sent, e.g. when implementing rate limiting.
 """
 
 global_email_filter = GlobalSignal()
 """
-Arguments: ``message``, ``order``, ``user``, ``customer``, ``organizer``
+Arguments: ``message``, ``order``, ``user``, ``customer``, ``organizer``, ``outgoing_mail``
 
 This signal allows you to implement a middleware-style filter on all outgoing emails. You are expected to
 return a (possibly modified) copy of the message object passed to you.
 
 This signal is called on all events and even if there is no known event. ``sender`` is an event or None.
 The ``message`` argument will contain an ``EmailMultiAlternatives`` object.
+The ``outgoing_mail`` argument will contain the ``OutgoingMail`` model instance. Note that the ``message`` object
+might have newer information if a previous plugin already modified the email.
 If the email is associated with a specific order, the ``order`` argument will be passed as well, otherwise
 it will be ``None``.
 If the email is associated with a specific user, e.g. a notification email, the ``user`` argument will be passed as
 well, otherwise it will be ``None``.
+
+You can raise ``WithholdMailException`` to prevent the email from being sent, e.g. when implementing rate limiting.
 """
 
 

src/pretix/control/logdisplay.py

@@ -700,6 +700,7 @@ class CoreUserImpersonatedLogEntryType(UserImpersonatedLogEntryType):
     'pretix.organizer.export.schedule.executed': _('A scheduled export has been executed.'),
     'pretix.organizer.export.schedule.failed': _('A scheduled export has failed: {reason}.'),
     'pretix.organizer.outgoingmails.retried': _('Failed emails have been scheduled to be retried.'),
+    'pretix.organizer.outgoingmails.aborted': _('Queued emails have been aborted.'),
     'pretix.giftcards.acceptance.added': _('Gift card acceptance for another organizer has been added.'),
     'pretix.giftcards.acceptance.removed': _('Gift card acceptance for another organizer has been removed.'),
     'pretix.giftcards.acceptance.acceptor.invited': _('A new gift card acceptor has been invited.'),

src/pretix/control/templates/pretixcontrol/organizers/outgoing_mail.html

@@ -35,7 +35,7 @@
                         <dt>{% trans "Status" %}</dt>
                         <dd>
                             {% if mail.status == "queued" %}
-                                <span class="label label-info">{% icon "clock" %} {% trans "queued" %}</span>
+                                <span class="label label-info">{% icon "clock-o" %} {% trans "queued" %}</span>
                             {% elif mail.status == "inflight" %}
                                 <span class="label label-info">{% icon "send" %} {% trans "being sent" %}</span>
                             {% elif mail.status == "awaiting_retry" %}
@@ -43,7 +43,11 @@
                             {% elif mail.status == "failed" %}
                                 <span class="label label-danger">{% icon "warning" %} {% trans "failed" %}</span>
                             {% elif mail.status == "bounced" %}
-                                <span class="label label-danger">{% icon "ban" %} {% trans "bounced" %}</span>
+                                <span class="label label-danger">{% icon "exclamation-circle" %} {% trans "bounced" %}</span>
+                            {% elif mail.status == "withheld" %}
+                                <span class="label label-warning">{% icon "ban" %} {% trans "withheld" %}</span>
+                            {% elif mail.status == "aborted" %}
+                                <span class="label label-danger">{% icon "ban" %} {% trans "aborted" %}</span>
                             {% elif mail.status == "sent" %}
                                 <span class="label label-success">{% icon "check" %} {% trans "sent" %}</span>
                             {% endif %}

src/pretix/control/templates/pretixcontrol/organizers/outgoing_mails.html

@@ -133,7 +133,7 @@
                             </td>
                             <td>
                                 {% if m.status == "queued" %}
-                                    <span class="label label-info">{% icon "clock" %} {% trans "queued" %}</span>
+                                    <span class="label label-info">{% icon "clock-o" %} {% trans "queued" %}</span>
                                 {% elif m.status == "inflight" %}
                                     <span class="label label-info">{% icon "send" %} {% trans "being sent" %}</span>
                                 {% elif m.status == "awaiting_retry" %}
@@ -141,7 +141,11 @@
                                 {% elif m.status == "failed" %}
                                     <span class="label label-danger">{% icon "warning" %} {% trans "failed" %}</span>
                                 {% elif m.status == "bounced" %}
-                                    <span class="label label-danger">{% icon "ban" %} {% trans "bounced" %}</span>
+                                    <span class="label label-danger">{% icon "exclamation-circle" %} {% trans "bounced" %}</span>
+                                {% elif m.status == "withheld" %}
+                                    <span class="label label-warning">{% icon "ban" %} {% trans "withheld" %}</span>
+                                {% elif m.status == "aborted" %}
+                                    <span class="label label-danger">{% icon "ban" %} {% trans "aborted" %}</span>
                                 {% elif m.status == "sent" %}
                                     <span class="label label-success">{% icon "check" %} {% trans "sent" %}</span>
                                 {% endif %}
@@ -165,7 +169,11 @@
             <div class="batch-select-actions">
                 <button type="submit" class="btn btn-primary btn-save" name="action" value="retry">
                     {% icon "repeat" %}
-                    {% trans "Retry failed" %}
+                    {% trans "Retry (if failed or withheld)" %}
+                </button>
+                <button type="submit" class="btn btn-danger btn-save" name="action" value="abort">
+                    {% icon "ban" %}
+                    {% trans "Abort (if queued, awaiting retry or withheld)" %}
                 </button>
             </div>
         </form>

src/pretix/control/views/mail.py

@@ -131,7 +131,7 @@ class OutgoingMailBulkAction(OutgoingMailQueryMixin, OrganizerPermissionRequired
     def post(self, request, *args, **kwargs):
         if request.POST.get('action') == 'retry':
             ids = set(
-                self.get_queryset().filter(status=OutgoingMail.STATUS_FAILED).values_list("pk", flat=True)
+                self.get_queryset().filter(status__in=OutgoingMail.STATUS_LIST_RETRYABLE).values_list("pk", flat=True)
             )
             with transaction.atomic():
                 OutgoingMail.objects.filter(pk__in=ids).update(
@@ -151,6 +151,30 @@ class OutgoingMailBulkAction(OutgoingMailQueryMixin, OrganizerPermissionRequired
                 "A retry of {num} emails was scheduled.",
                 len(ids),
             ).format(num=len(ids)))
+        elif request.POST.get('action') == 'abort':
+            ids = set(
+                self.get_queryset().filter(
+                    status__in=(OutgoingMail.STATUS_QUEUED, OutgoingMail.STATUS_AWAITING_RETRY)
+                ).values_list("pk", flat=True)
+            )
+            with transaction.atomic():
+                OutgoingMail.objects.filter(pk__in=ids).update(
+                    status=OutgoingMail.STATUS_ABORTED,
+                    sent=None,
+                )
+                self.request.organizer.log_action(
+                    'pretix.organizer.outgoingmails.aborted', user=self.request.user, data={
+                        'mails': list(ids)
+                    }, save=False
+                )
+            for i in ids:
+                mail_send_task.apply_async(kwargs={"outgoing_mail": i})
+
+            messages.success(request, ngettext(
+                "One email was aborted and will not be sent.",
+                "{num} emails were aborted and will not be sent.",
+                len(ids),
+            ).format(num=len(ids)))
         return redirect(self.get_success_url())
 
     def get_success_url(self) -> str:

src/pretix/static/pretixcontrol/js/ui/outgoingmail.js

@@ -4,6 +4,7 @@ function is_sandbox_supported() {
 }
 
 function safe_render(url, parent) {
+    // Estimate the height that prevents the user from having to scroll on two levels to see the full email
     const height = (
         window.innerHeight - parent.parent().get(0).getBoundingClientRect().top - document.querySelector("footer").getBoundingClientRect().height - 20
     ) + "px";

src/tests/base/test_mail.py

@@ -217,7 +217,7 @@ def test_queue_state_retry_failure(env, monkeypatch):
         'outgoing_mail': m.pk,
     }, max_retries=0)
     m.refresh_from_db()
-    assert m.status == OutgoingMail.STATUS_AWAWITING_RETRY
+    assert m.status == OutgoingMail.STATUS_AWAITING_RETRY
     assert m.retry_after > now()