Unify Reply-To handling across tickets and talks components#2930
Open
MukundC25 wants to merge 1 commit intofossasia:devfrom
Open
Unify Reply-To handling across tickets and talks components#2930MukundC25 wants to merge 1 commit intofossasia:devfrom
MukundC25 wants to merge 1 commit intofossasia:devfrom
Conversation
- Add helper method to build Reply-To address with display name - Update mail sending functions to use unified Reply-To logic - Make Event.email field optional (blank=True, null=True) - Add migration to remove placeholder emails and update field - Update forms to handle optional event email - Ensure consistent Reply-To behavior across components
Contributor
Reviewer's GuideUnifies Reply-To resolution across tickets, talks, and bulk emails via a centralized helper that is SMTP‑context aware, and makes Event.email an optional organizer email with updated help texts and migration away from placeholder/legacy behavior. Class diagram for updated email and bulk send structuresclassDiagram
class Event {
+EmailField email
+JSONField mail_settings
}
%% optional organizer email, blank/null allowed
class MailTemplate {
+EmailField reply_to
+CharField subject
+TextField text
+TextField html
+to_mail(user, event, locale, context, address)
}
class QueuedMail {
+Event event
+MailTemplate template
+CharField to
+CharField reply_to
+CharField bcc
+CharField subject
+TextField text
+TextField html
}
%% reply_to uses unified default behavior
class BulkReplyToMixin {
+_get_reply_to_for_bulk_email()
}
class SenderView {
+form_valid(form)
}
class ComposeTeamsMail {
+form_valid(form)
}
Event "1" --> "*" MailTemplate : owns_templates
Event "1" --> "*" QueuedMail : has_mails
MailTemplate "1" --> "*" QueuedMail : enqueues
BulkReplyToMixin <|.. SenderView : mixin
BulkReplyToMixin <|.. ComposeTeamsMail : mixin
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
As suggested #1781 (review) Organizer email no longer required - it's optional, removed the Event Creation Screen.Recording.2026-01-30.at.9.51.29.PM.movTickets component Screen.Recording.2026-01-30.at.10.06.01.PM.movTalks component Screen.Recording.2026-01-30.at.10.23.07.PM.MP4Updated help texts
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
get_reply_to_addressdocstring describes step 3 as “Custom SMTP reply_to” and mentions SMTP context, but the implementation always usesevent.mail_settings['reply_to']regardless of SMTP flags; consider either conditioning this on custom SMTP usage or updating the docstring to accurately match the behavior. - The sender resolution for bulk emails in
BulkReplyToMixinusesevent.settings.get('mail_from'), while other mail paths rely onevent.mail_settings['smtp_use_custom']/mail_from; aligning these to a single source of truth (or shared helper) would reduce the risk of divergent Reply-To behavior across email types. - The new organizer email / Reply-To help text string is duplicated across several forms and model fields; extracting this into a shared constant or translation key would simplify future updates and avoid inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `get_reply_to_address` docstring describes step 3 as “Custom SMTP reply_to” and mentions SMTP context, but the implementation always uses `event.mail_settings['reply_to']` regardless of SMTP flags; consider either conditioning this on custom SMTP usage or updating the docstring to accurately match the behavior.
- The sender resolution for bulk emails in `BulkReplyToMixin` uses `event.settings.get('mail_from')`, while other mail paths rely on `event.mail_settings['smtp_use_custom']`/`mail_from`; aligning these to a single source of truth (or shared helper) would reduce the risk of divergent Reply-To behavior across email types.
- The new organizer email / Reply-To help text string is duplicated across several forms and model fields; extracting this into a shared constant or translation key would simplify future updates and avoid inconsistencies.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes Reply-To resolution across ticketing, talk (CFP) mails, and bulk email sending by introducing a shared get_reply_to_address() helper and updating the affected send paths to use it. It also makes Event.email optional (including a migration to remove the previous placeholder) and updates UI help texts to describe the new behavior.
Changes:
- Added
get_reply_to_address()incommon/mail.pyand migrated ticket/talk/bulk flows to use unified precedence. - Made
Event.emailoptional (model + migration) and removed reliance on legacy placeholder/default handling. - Updated multiple forms/settings help texts to reflect platform-vs-custom-sender Reply-To behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/common/mail.py | Adds unified Reply-To resolver and wires it into the common mail sending task. |
| app/eventyay/base/services/mail.py | Switches ticket/service mail header construction to use the shared Reply-To resolver. |
| app/eventyay/base/models/mail.py | Uses shared Reply-To resolver when creating queued mails from talk templates; updates help text. |
| app/eventyay/plugins/sendmail/views.py | Uses shared Reply-To resolver for bulk email queue creation and deduplicates logic via a mixin. |
| app/eventyay/base/models/submission.py | Avoids sending internal submission notifications to a missing organizer email. |
| app/eventyay/base/models/event.py | Makes organizer email optional and updates its help text. |
| app/eventyay/base/migrations/0021_make_event_email_optional.py | Alters DB field and removes placeholder organizer emails from existing rows. |
| app/eventyay/base/configurations/default_setting.py | Updates sender help text to explain reply behavior with custom senders. |
| app/eventyay/orga/forms/event.py | Updates Reply-To / sender help texts in organizer mail settings form. |
| app/eventyay/control/forms/event.py | Makes organizer email optional in control-side wizard/forms and updates help text. |
| app/eventyay/event/forms.py | Makes organizer email optional in event creation wizard and removes strict validation. |
| app/eventyay/eventyay_common/forms/event.py | Makes organizer email optional and updates help text. |
| app/eventyay/plugins/sendmail/forms.py | Clarifies queued-mail Reply-To help text behavior. |
Comment on lines
+172
to
+176
| if override: | ||
| return override | ||
|
|
||
| if template and hasattr(template, 'reply_to') and template.reply_to: | ||
| return template.reply_to |
There was a problem hiding this comment.
override is treated as truthy (if override:). This makes it impossible to explicitly clear Reply-To by passing an empty string (e.g. bulk/manual emails setting reply_to to ''). Instead, the resolver will fall back to event defaults and re-add Reply-To. Consider treating “override provided” as override is not None and returning None/'' early so an explicit empty override suppresses Reply-To.
Comment on lines
+181
to
+182
| # Only use Event.email when using default system sender | ||
| if sender_email == settings.MAIL_FROM and event.email: |
There was a problem hiding this comment.
The "use Event.email only with default system sender" check relies solely on sender_email == settings.MAIL_FROM. If an event uses custom SMTP but configures mail_from equal to MAIL_FROM, this will incorrectly use Event.email as Reply-To, contradicting the documented custom-SMTP behavior. Consider also checking event.mail_settings['smtp_use_custom'] (or backend type) before falling back to Event.email.
Suggested change
| # Only use Event.email when using default system sender | |
| if sender_email == settings.MAIL_FROM and event.email: | |
| # Only use Event.email when using default system sender and no custom SMTP | |
| use_default_sender = not event.mail_settings.get('smtp_use_custom') | |
| if use_default_sender and sender_email == settings.MAIL_FROM and event.email: |
Comment on lines
+599
to
601
| if self.event.mail_settings['mail_on_new_submission'] and self.event.email: | ||
| self.event.get_mail_template(MailTemplateRoles.NEW_SUBMISSION_INTERNAL).to_mail( | ||
| user=self.event.email, |
There was a problem hiding this comment.
With Event.email now optional, this internal notification is silently skipped when mail_on_new_submission is enabled but event.email is empty. That likely breaks the expected behavior (“receive an email on new proposal”) for events that rely on mail_settings['reply_to'] or mail_from instead. Consider resolving a recipient fallback (e.g., event.email -> event.mail_settings['reply_to'] -> event.settings.mail_from) and/or raising a validation error when enabling the setting without a recipient address.
Suggested change
| if self.event.mail_settings['mail_on_new_submission'] and self.event.email: | |
| self.event.get_mail_template(MailTemplateRoles.NEW_SUBMISSION_INTERNAL).to_mail( | |
| user=self.event.email, | |
| recipient_email = self.event.email or self.event.mail_settings.get('reply_to') or self.event.settings.mail_from | |
| if self.event.mail_settings['mail_on_new_submission'] and recipient_email: | |
| self.event.get_mail_template(MailTemplateRoles.NEW_SUBMISSION_INTERNAL).to_mail( | |
| user=recipient_email, |
| verbose_name=_('Organizer email address'), | ||
| help_text=_('Will be used as Reply-To in emails.'), | ||
| default='org@mail.com', | ||
| help_text=_("Enter an organizer email address for event-related emails. When the platform sender is used, this address will be used as the Reply-To. If left empty, emails will be sent using the platform's default email address."), |
There was a problem hiding this comment.
This help text suggests that leaving the organizer email empty changes what address emails are sent from ("emails will be sent using the platform's default email address"), but this field only affects Reply-To resolution; the sender is controlled by mail_from/SMTP settings. Consider rewording to clarify that if empty, no automatic Reply-To is set and replies will go to the sender address (platform default if mail_from is not customized).
Suggested change
| help_text=_("Enter an organizer email address for event-related emails. When the platform sender is used, this address will be used as the Reply-To. If left empty, emails will be sent using the platform's default email address."), | |
| help_text=_( | |
| "Enter an organizer email address for event-related emails. " | |
| "If set, this address will be used as the Reply-To when the platform sender address is used. " | |
| "If left empty, no Reply-To will be added automatically and replies will go to the sender address (platform default if not customized)." | |
| ), |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updates - (Old PR) #1781
Fixes #1564
Follow-up PR for #1611 #1781 (review)
The Event Creation Unification PR added
Event.emailas the single "Organizer email address" set during event creation. However, Reply-To logic remained inconsistent as described earlier in #1611 (comment):event.settings.contact_mailcould silently overrideEvent.emailcontact_mailThis caused
Event.email(shown in UI as canonical) to be silently overridden bycontact_mailin certain scenarios.Changes
Unified Reply-To logic via
get_reply_to_address()helper with SMTP context awareness:Key Features:
Removes
event.settings.contact_mailfrom all Reply-To resolution paths.Modified files:
Removed:
event.settings.contact_mailfrom all Reply-To pathsuse_custom_smtpparameter from helper functionComposeTeamsMailclass definitionLEGACY_DEFAULT_EMAIL)org@mail.comfrom Event.email model fieldImpact
test_mail_send_ignored_sender_but_custom_reply_to) continue to passSummary by Sourcery
Unify Reply-To handling across tickets, talks, and bulk emails and make the event organizer email truly optional with updated help texts and migration.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Chores: