feat(email): implement an outbox for ticketing mails with multiple receivers #1190
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR implements an outbox-based system for ticketing emails by replacing direct mail sending with a persistent queue (EmailQueue) that stores filter criteria and recipients, and introduces new models, tasks, views, forms, templates, and mixins to compose, manage, preview, and dispatch queued emails—including support for team-based mailings and UI enhancements for drafts, search, and ordering. Sequence diagram for composing and sending an email to outboxsequenceDiagram
actor Organizer
participant WebUI
participant SenderView
participant EmailQueue
participant EmailQueueFilter
participant EmailQueueToUser
participant CeleryTask
participant MailService
Organizer->>WebUI: Fill compose email form and submit
WebUI->>SenderView: POST form data
SenderView->>EmailQueue: Create EmailQueue instance
SenderView->>EmailQueueFilter: Create EmailQueueFilter with filter criteria
SenderView->>EmailQueue: Call populate_to_users()
EmailQueue->>EmailQueueToUser: Create recipient entries
SenderView->>WebUI: Redirect to outbox list
Organizer->>WebUI: Click 'Send' on queued mail
WebUI->>CeleryTask: POST to send_queued_mail
CeleryTask->>EmailQueue: Load queued mail
CeleryTask->>EmailQueueToUser: Iterate recipients
CeleryTask->>MailService: Send email for each recipient
MailService-->>CeleryTask: Success/failure per recipient
CeleryTask->>EmailQueue: Update sent_at if all sent
Class diagram for new and updated email queue modelsclassDiagram
class EmailQueue {
+Event event
+User user
+ComposingFor composing_for
+I18nTextField subject
+I18nTextField message
+String reply_to
+Text bcc
+String locale
+UUID[] attachments
+DateTime created_at
+DateTime updated_at
+DateTime sent_at
+get_recipient_emails()
+populate_to_users()
+send()
}
class EmailQueueFilter {
+EmailQueue mail
+String recipients
+String[] sendto
+Integer[] products
+Integer[] checkin_lists
+Boolean filter_checkins
+Boolean not_checked_in
+Integer subevent
+DateTime subevents_from
+DateTime subevents_to
+DateTime created_from
+DateTime created_to
+Integer[] orders
+Integer[] teams
}
class EmailQueueToUser {
+EmailQueue mail
+Email email
+String[] orders
+String[] positions
+String[] products
+Integer team
+Boolean sent
+Text error
}
EmailQueue "1" --o "1" EmailQueueFilter : has
EmailQueue "1" --o "*" EmailQueueToUser : recipients
EmailQueueFilter "1" --o "1" EmailQueue : mail
EmailQueueToUser "*" --o "1" EmailQueue : mail
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability. (link)
- Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability. (link)
- Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability. (link)
- Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability. (link)
General comments:
- Consider wrapping the EmailQueue creation, EmailQueueFilter creation, and populate_to_users call in a single atomic transaction to avoid orphaned records if any step fails.
- The form_valid methods in SenderView and ComposeTeamsMail are quite long and mix filtering, queueing, and rendering logic; extracting the queue‐building and recipient resolution into service or model methods would improve readability and testability.
- Since outbox listing and search filter by subject and recipient email, adding database indexes on EmailQueue.subject and EmailQueueToUser.email (or a materialized first_recipient_email field) could help maintain performance at scale.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider wrapping the EmailQueue creation, EmailQueueFilter creation, and populate_to_users call in a single atomic transaction to avoid orphaned records if any step fails.
- The form_valid methods in SenderView and ComposeTeamsMail are quite long and mix filtering, queueing, and rendering logic; extracting the queue‐building and recipient resolution into service or model methods would improve readability and testability.
- Since outbox listing and search filter by subject and recipient email, adding database indexes on EmailQueue.subject and EmailQueueToUser.email (or a materialized first_recipient_email field) could help maintain performance at scale.
## Individual Comments
### Comment 1
<location> `app/eventyay/plugins/sendmail/forms.py:515-519` </location>
<code_context>
+ if email.strip()
+ ]
+
+ if len(updated_emails) > len(self.recipient_objects):
+ raise ValidationError(
+ _("You cannot add new recipients. Only editing existing email addresses is allowed.")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** EmailQueueEditForm prevents adding new recipients but not removing all.
Please add validation to ensure at least one recipient remains after editing to prevent emails from being queued without recipients.
```suggestion
if len(updated_emails) > len(self.recipient_objects):
raise ValidationError(
_("You cannot add new recipients. Only editing existing email addresses is allowed.")
)
if len(updated_emails) == 0:
raise ValidationError(
_("At least one recipient must remain. You cannot remove all recipients.")
)
return updated_emails
```
</issue_to_address>
### Comment 2
<location> `app/eventyay/plugins/sendmail/forms.py:526` </location>
<code_context>
+
+ updated_emails = self.cleaned_data['emails']
+
+ for i, email in enumerate(updated_emails):
+ self.recipient_objects[i].email = email
+ if commit:
</code_context>
<issue_to_address>
**issue:** Potential index error if fewer emails are provided than recipients.
Currently, extra recipient objects remain unchanged if fewer emails are provided. Please handle this case by either removing the extra recipients or raising a validation error.
</issue_to_address>
### Comment 3
<location> `app/eventyay/base/services/mail.py:174-177` </location>
<code_context>
if event:
timezone = event.timezone
renderer = event.get_html_mail_renderer()
- if event.settings.mail_bcc:
+ if not auto_email:
+ if event_bcc is not None and event_bcc != '': # The organizer may edit the BCC field in the EmailQueue editor to either specify a different BCC address or remove it entirely.
+ for bcc_mail in event_bcc.split(','):
</code_context>
<issue_to_address>
**suggestion:** BCC and Reply-To logic is duplicated for auto_email and not auto_email.
Refactor BCC and Reply-To handling to avoid duplication and ensure headers are set consistently regardless of auto_email state.
</issue_to_address>
### Comment 4
<location> `app/eventyay/base/email.py:424-425` </location>
<code_context>
for v in val:
- if all(rp in kwargs for rp in v.required_context):
- ctx[v.identifier] = v.render(kwargs)
+ try:
+ if all(rp in kwargs for rp in v.required_context):
+ ctx[v.identifier] = v.render(kwargs)
+ except (KeyError, AttributeError, TypeError, ValueError) as e:
+ logger.warning("Skipping placeholder %s due to error: %s", v.identifier, e)
logger.info('Email context: %s', ctx)
return ctx
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Logging skipped placeholders may leak sensitive context.
Sanitize log messages or reduce verbosity to avoid exposing sensitive user data from errors.
```suggestion
except (KeyError, AttributeError, TypeError, ValueError) as e:
logger.warning(
"Skipping placeholder %s due to error type: %s",
v.identifier,
type(e).__name__
)
```
</issue_to_address>
### Comment 5
<location> `app/eventyay/plugins/sendmail/templates/pretixplugins/sendmail/outbox_form.html:133` </location>
<code_context>
<strong>{{ out.subject|safe }}</strong><br><br>
</code_context>
<issue_to_address>
**security (python.flask.security.xss.audit.template-unescaped-with-safe):** Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `app/eventyay/plugins/sendmail/templates/pretixplugins/sendmail/outbox_form.html:134` </location>
<code_context>
{{ out.html|safe }}
</code_context>
<issue_to_address>
**security (python.flask.security.xss.audit.template-unescaped-with-safe):** Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `app/eventyay/plugins/sendmail/templates/pretixplugins/sendmail/send_team_form.html:45` </location>
<code_context>
<strong>{{ out.subject|safe }}</strong><br><br>
</code_context>
<issue_to_address>
**security (python.flask.security.xss.audit.template-unescaped-with-safe):** Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `app/eventyay/plugins/sendmail/templates/pretixplugins/sendmail/send_team_form.html:46` </location>
<code_context>
{{ out.html|safe }}
</code_context>
<issue_to_address>
**security (python.flask.security.xss.audit.template-unescaped-with-safe):** Detected a segment of a Flask template where autoescaping is explicitly disabled with '| safe' filter. This allows rendering of raw HTML in this segment. Ensure no user data is rendered here, otherwise this is a cross-site scripting (XSS) vulnerability.
*Source: opengrep*
</issue_to_address>
### Comment 9
<location> `app/eventyay/plugins/sendmail/forms.py:503` </location>
<code_context>
def _set_field_placeholders(self, fn, base_parameters):
phs = ['{%s}' % p for p in sorted(get_available_placeholders(self.event, base_parameters).keys())]
ht = _('Available placeholders: {list}').format(list=', '.join(phs))
if self.fields[fn].help_text:
self.fields[fn].help_text += ' ' + str(ht)
else:
self.fields[fn].help_text = ht
self.fields[fn].validators.append(PlaceholderValidator(phs))
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use f-string instead of string concatenation ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
```suggestion
self.fields[fn].help_text += f' {str(ht)}'
```
</issue_to_address>
### Comment 10
<location> `app/eventyay/plugins/sendmail/mixins.py:20` </location>
<code_context>
def load_copy_draft(self, request, form_kwargs, team_mode=False):
if copy_id := request.GET.get('copyToDraft'):
try:
mail_id = int(copy_id)
qm = EmailQueue.objects.get(id=mail_id, event=request.event)
form_kwargs['initial'] = form_kwargs.get('initial', {})
subject = qm.subject
message = qm.message
attachment = CachedFile.objects.filter(id__in=qm.attachments).first()
try:
qmf = EmailQueueFilter.objects.get(mail=qm)
except EmailQueueFilter.DoesNotExist:
qmf = None
form_kwargs['initial'].update({
'subject': subject,
'message': message,
'reply_to': qm.reply_to,
'bcc': qm.bcc,
})
if attachment:
form_kwargs['initial']['attachment'] = attachment
if qmf:
if team_mode:
form_kwargs['initial']['teams'] = qmf.teams or []
else:
form_kwargs['initial'].update({
'recipients': qmf.recipients or [],
'sendto': qmf.sendto or ['p', 'na'],
'filter_checkins': qmf.filter_checkins,
'not_checked_in': qmf.not_checked_in,
})
if qmf.products:
form_kwargs['initial']['products'] = request.event.products.filter(id__in=qmf.products)
if qmf.checkin_lists:
form_kwargs['initial']['checkin_lists'] = request.event.checkin_lists.filter(
id__in=qmf.checkin_lists
)
if qmf.subevent:
try:
form_kwargs['initial']['subevent'] = request.event.subevents.get(id=qmf.subevent)
except SubEvent.DoesNotExist:
pass
for field in ['subevents_from', 'subevents_to', 'created_from', 'created_to']:
value = getattr(qmf, field, None)
if value:
form_kwargs['initial'][field] = dateutil.parser.isoparse(value) if isinstance(value, str) else value
except (EmailQueue.DoesNotExist, ValueError, TypeError) as e:
logger.warning('Failed to load EmailQueue for copyToDraft: %s' % e)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Add guard clause ([`last-if-guard`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/last-if-guard/))
- Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Replace interpolated string formatting with f-string ([`replace-interpolation-with-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/replace-interpolation-with-fstring/))
</issue_to_address>
### Comment 11
<location> `app/eventyay/plugins/sendmail/models.py:142-145` </location>
<code_context>
def _finalize_send_status(self):
if all(r.sent for r in self.recipients.all()):
self.sent_at = now()
else:
self.sent_at = None
self.save(update_fields=["sent_at"])
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
self.sent_at = now() if all(r.sent for r in self.recipients.all()) else None
```
</issue_to_address>
### Comment 12
<location> `app/eventyay/plugins/sendmail/models.py:208` </location>
<code_context>
def get_recipient_emails(self):
"""
Resolve and return the full list of unique email addresses
this queued mail will send to.
"""
return sorted(set(r.email.strip().lower() for r in self.recipients.all() if r.email))
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace list(), dict() or set() with comprehension ([`collection-builtin-to-comprehension`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/collection-builtin-to-comprehension/))
```suggestion
return sorted(
{r.email.strip().lower() for r in self.recipients.all() if r.email}
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Thanks for the heads-up. Two clarifications and actions from our side:
Net result: The “Flask” label is a mis-ID, but the underlying XSS concern is addressed. We’ll keep How to scope your rule to Django (or paths) to avoid Flask-specific wording going forward? |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sendmail plugin to introduce a queue-based email system, replacing the previous immediate send approach. Instead of sending emails directly, messages are now queued in an outbox for review and manual sending, with comprehensive recipient tracking and error handling.
Key Changes
- Introduces new models (
EmailQueue,EmailQueueFilter,EmailQueueToUser) for email queue management - Replaces
EmailHistoryViewwith new views:OutboxListView,SentMailView,EditEmailQueueView - Adds support for composing emails to team members in addition to attendees
- Implements draft copying functionality and improved email preview system
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| views.py | Refactored email composition to use queue system; added outbox management views |
| models.py | New models for email queue, recipient tracking, and filter data |
| tasks.py | Added send_queued_mail task for asynchronous email sending |
| forms.py | New forms for team emails and email queue editing |
| urls.py | Updated URL patterns to support outbox and team email composition |
| signals.py | Updated navigation structure and log entry action types |
| templates/ | New templates for outbox, sent mail lists, and composition forms |
| mixins.py | New mixins for draft copying and query filtering |
| mail.py | Enhanced with event_bcc and event_reply_to parameters |
Comments suppressed due to low confidence (3)
app/eventyay/plugins/sendmail/tasks.py:127
- This assignment to 'event' is unnecessary as it is redefined before this value is used.
event = event_id
app/eventyay/plugins/sendmail/tasks.py:130
- This assignment to 'event' is unnecessary as it is redefined before this value is used.
event = Event.objects.get(id=original_event_id)
app/eventyay/plugins/sendmail/views.py:255
- This assignment to 'get_queryset' is unnecessary as it is redefined before this value is used.
def get_queryset(self):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Gagan-Ram
force-pushed
the
email_outbox
branch
from
e998319 to
909d1ea
Compare
mariobehling
requested changes
Nov 4, 2025
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks, please add screenshots or a video.
hongquan
requested changes
Nov 4, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
I have tested this PR against multiple Localization too. This works as expected. |
Member
Author
Added in description. |
hongquan
requested changes
Nov 7, 2025
app/eventyay/plugins/sendmail/templates/pretixplugins/sendmail/outbox_list.html
Outdated
Show resolved
Hide resolved
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
hongquan
requested changes
Nov 11, 2025
| </a> | ||
| <a href="?ordering={{ desc }}{% if query %}&q={{ query }}{% endif %}" | ||
| class="{% if current_ordering == desc %}fw-bold{% else %}text-muted{% endif %}"> | ||
| <a href="?{% if encoded_query %}{{ encoded_query }}&{% endif %}ordering={{ desc }}" |
Member
There was a problem hiding this comment.
Any reason why the ordering parameter is not inside encoded_query?
Member
Author
There was a problem hiding this comment.
When generating the sorting arrows (the ▲/▼ links), you are creating new ordering options.
If encoded_query already included the current ordering parameter, clicking one of those arrows would produce a URL like this:
?q=foo&ordering=-subject&ordering=subject
That would lead to duplicate ordering parameters, and Django would only use the first one, effectively ignoring the new sort you just clicked.
Member
There was a problem hiding this comment.
It means that the "the ▲/▼ links" are dumb, they should overwrite the ordering parameter, instead of appending. Those links should be fixed.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+310
to
+317
| thumbnail_filename = 'pretixbase/email/thumb.png' | ||
| template_name = 'pretixbase/email/plainwrapper.html' | ||
|
|
||
|
|
||
| class UnembellishedMailRenderer(TemplateBasedMailRenderer): | ||
| verbose_name = _('Simple with logo') | ||
| identifier = 'simple_logo' | ||
| thumbnail_filename = 'eventyay/email/thumb_simple_logo.png' | ||
| template_name = 'eventyay/email/simple_logo.jinja' | ||
| thumbnail_filename = 'pretixbase/email/thumb_simple_logo.png' |
There was a problem hiding this comment.
The thumbnail filenames are also changed to "pretixbase" paths, which is inconsistent with the rest of the codebase that uses "eventyay" paths. These should be "eventyay/email/thumb.png" and "eventyay/email/thumb_simple_logo.png" to maintain consistency.
app/eventyay/base/services/mail.py
Outdated
Comment on lines
178
to
180
| if event_bcc is not None and event_bcc != '': # Use custom BCC if specified | ||
| for bcc_mail in event_bcc.split(','): | ||
| bcc.append(bcc_mail.strip()) |
There was a problem hiding this comment.
The condition event_bcc != '' is redundant. In Python, an empty string is falsy, so event_bcc is not None and event_bcc would be sufficient and more idiomatic. The same applies to the event_reply_to check on line 187.
| :type user: User | ||
| :param composing_for: To whom the organizer is composing email for. Either "attendees" or "teams" | ||
| :type raw_composing_for: str |
There was a problem hiding this comment.
The docstring type annotation :type raw_composing_for: str doesn't match the actual field name composing_for. This should be :type composing_for: str.
Suggested change
| :type raw_composing_for: str | |
| :type composing_for: str |
Comment on lines
120
to
126
| changed = False | ||
|
|
||
| for recipient in recipients: | ||
| if recipient.sent: | ||
| continue | ||
| result = self._send_to_recipient(recipient, subject, message) | ||
| changed = changed or result |
There was a problem hiding this comment.
The variable changed is set but never used meaningfully. It's set to changed or result on line 126, but the final value is never checked or returned. This variable should either be used to track changes or removed.
Suggested change
| changed = False | |
| for recipient in recipients: | |
| if recipient.sent: | |
| continue | |
| result = self._send_to_recipient(recipient, subject, message) | |
| changed = changed or result | |
| for recipient in recipients: | |
| if recipient.sent: | |
| continue | |
| self._send_to_recipient(recipient, subject, message) |
Comment on lines
+16
to
22
| if isinstance(event_id, Event): | ||
| original_event_id = event_id.pk | ||
| event = event_id | ||
| else: | ||
| original_event_id = event_id | ||
| event = Event.objects.get(id=original_event_id) | ||
|
|
There was a problem hiding this comment.
The task checks if event_id is an Event instance (line 16), but Celery tasks serialize their arguments, so event_id will always be an integer when the task is executed asynchronously. This check will never be True in actual async execution. If this is for testing purposes, it should be documented; otherwise, this code path is dead code and should be removed.
Suggested change
| if isinstance(event_id, Event): | |
| original_event_id = event_id.pk | |
| event = event_id | |
| else: | |
| original_event_id = event_id | |
| event = Event.objects.get(id=original_event_id) | |
| original_event_id = event_id | |
| event = Event.objects.get(id=original_event_id) |
app/eventyay/base/services/mail.py
Outdated
| if not auto_email: | ||
| if ( | ||
| event_reply_to is not None | ||
| and event_reply_to != '' |
There was a problem hiding this comment.
The condition event_reply_to != '' is redundant. An empty string is falsy in Python, so event_reply_to is not None and event_reply_to would be more idiomatic.
Suggested change
| and event_reply_to != '' | |
| and event_reply_to |
| Supports both team and attendee email composition modes. | ||
| """ | ||
| def load_copy_draft(self, request, form_kwargs, team_mode=False): | ||
| if copy_id := request.GET.get('copyToDraft'): |
There was a problem hiding this comment.
[nitpick] Using a walrus operator (:=) in an if condition is valid in Python 3.8+, but for consistency with the rest of the codebase and better readability, consider extracting this to a separate line: copy_id = request.GET.get('copyToDraft') followed by if copy_id:.
Suggested change
| if copy_id := request.GET.get('copyToDraft'): | |
| copy_id = request.GET.get('copyToDraft') | |
| if copy_id: |
| send_queued_mail.apply_async(args=[request.event.pk, mail.pk]) | ||
| messages.success( | ||
| request, | ||
| _('The mail has been sent.') |
There was a problem hiding this comment.
The success message says "The mail has been sent" but the mail is actually being queued for asynchronous sending via Celery. The message should be "The mail has been queued for sending" to accurately reflect what's happening.
Suggested change
| _('The mail has been sent.') | |
| _('The mail has been queued for sending.') |
Comment on lines
+442
to
+448
| def question(self): | ||
| count = EmailQueue.objects.filter(event=self.request.event, sent_at__isnull=True).count() | ||
| return ngettext_lazy( | ||
| "Do you really want to purge the mail?", | ||
| "Do you really want to purge {count} mails?", | ||
| count | ||
| ).format(count=count) |
There was a problem hiding this comment.
The question() method is defined but never called. Similar to the issue in DeleteEmailQueueView, this should either be decorated with @property or the template should call it with parentheses.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 19 comments.
Comments suppressed due to low confidence (2)
app/eventyay/plugins/sendmail/views.py:3
- Import of 'uuid' is not used.
import uuid
app/eventyay/plugins/sendmail/views.py:14
- Import of 'urlencode' is not used.
from urllib.parse import urlencode
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import bleach | ||
| import dateutil | ||
| import logging | ||
| import uuid |
There was a problem hiding this comment.
Unused import. The uuid import is not used anywhere in this file.
Suggested change
| import uuid |
| return None | ||
|
|
||
| def _finalize_send_status(self): | ||
| self.sent_at = now() if all(r.sent for r in self.recipients.all()) else None |
There was a problem hiding this comment.
Inefficient query pattern. The method calls self.recipients.all() multiple times (lines 114, 147, 269). Consider fetching recipients once and storing them in a variable to avoid multiple database queries.
|
|
||
| urlpatterns = [ | ||
| path( | ||
| 'control/event/<str:organizer>/<str:event>/mails/compose/', |
There was a problem hiding this comment.
Trailing whitespace on line 7. Remove the trailing space after the comma.
Suggested change
| 'control/event/<str:organizer>/<str:event>/mails/compose/', | |
| 'control/event/<str:organizer>/<str:event>/mails/compose/', |
| name='send_single' | ||
| ), | ||
| path( | ||
| 'control/event/<str:organizer>/<str:event>/mails/<int:pk>/', |
There was a problem hiding this comment.
Trailing whitespace on line 22. Remove the trailing space after the comma.
Suggested change
| 'control/event/<str:organizer>/<str:event>/mails/<int:pk>/', | |
| 'control/event/<str:organizer>/<str:event>/mails/<int:pk>/', |
| 'control/event/<str:organizer>/<str:event>/sendmail/history/', | ||
| views.EmailHistoryView.as_view(), | ||
| name='history', | ||
| 'control/event/<str:organizer>/<str:event>/mails/compose/teams/', |
There was a problem hiding this comment.
Trailing whitespace on line 42. Remove the trailing space after the comma.
Suggested change
| 'control/event/<str:organizer>/<str:event>/mails/compose/teams/', | |
| 'control/event/<str:organizer>/<str:event>/mails/compose/teams/', |
| from django.views.generic import FormView, ListView | ||
| from django.utils.translation import gettext_lazy as _, ngettext_lazy | ||
| from django.views.generic import FormView, ListView, TemplateView, UpdateView, View | ||
| from urllib.parse import urlencode |
There was a problem hiding this comment.
Unused import. The urlencode import is not used anywhere in this file.
Suggested change
| from urllib.parse import urlencode |
Comment on lines
+389
to
+390
| def question(self): | ||
| return _("Do you really want to delete this mail?") |
There was a problem hiding this comment.
The question() method doesn't have a return statement. It should return the translated string instead of just calling _() without returning it.
| if p.addon_to_id is not None: | ||
| continue | ||
| @app.task(base=ProfiledEventTask, bind=True, max_retries=3, default_retry_delay=60, acks_late=True) | ||
| def send_queued_mail(self, event_id: int, queued_mail_id: int): |
There was a problem hiding this comment.
Missing docstring. The send_queued_mail function should have a docstring explaining its parameters, return value, and error handling behavior, especially since it's a Celery task with retry logic.
Suggested change
| def send_queued_mail(self, event_id: int, queued_mail_id: int): | |
| def send_queued_mail(self, event_id: int, queued_mail_id: int): | |
| """ | |
| Celery task to send a queued email for a specific event. | |
| Parameters: | |
| self: The Celery task instance. | |
| event_id (int or Event): The ID of the event or an Event instance. | |
| queued_mail_id (int): The ID of the queued email to send. | |
| Returns: | |
| None | |
| Error Handling: | |
| - Logs an error if the event or email queue does not exist. | |
| - Logs warnings for partial or unsuccessful sends. | |
| - Retries the task up to 3 times on unexpected exceptions. | |
| - Logs an error if maximum retries are exceeded. | |
| """ |
| name='outbox', | ||
| ), | ||
| path( | ||
| 'control/event/<str:organizer>/<str:event>/outbox/send/<int:pk>/', |
There was a problem hiding this comment.
Trailing whitespace on line 17. Remove the trailing space after the comma.
Suggested change
| 'control/event/<str:organizer>/<str:event>/outbox/send/<int:pk>/', | |
| 'control/event/<str:organizer>/<str:event>/outbox/send/<int:pk>/', |
| name='edit_mail' | ||
| ), | ||
| path( | ||
| 'control/event/<str:organizer>/<str:event>/outbox/delete/<int:pk>/', |
There was a problem hiding this comment.
Trailing whitespace on line 27. Remove the trailing space after the comma.
Suggested change
| 'control/event/<str:organizer>/<str:event>/outbox/delete/<int:pk>/', | |
| 'control/event/<str:organizer>/<str:event>/outbox/delete/<int:pk>/', |
hongquan
approved these changes
Nov 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #658
Video:
email_outbox.webm
Summary by Sourcery
Implement an outbox system for ticketing emails to allow queuing, managing, and sending batch mails to attendees, orders, or team members.
New Features:
Enhancements: