Fix and restructure email configuration and improve email settings UI in event settings#3384
Open
Rachit7168 wants to merge 29 commits intofossasia:devfrom
Open
Fix and restructure email configuration and improve email settings UI in event settings#3384Rachit7168 wants to merge 29 commits intofossasia:devfrom
Rachit7168 wants to merge 29 commits intofossasia:devfrom
Conversation
Contributor
Reviewer's GuideRefactors how email backends are tested and restructures the event email settings UI and forms so that custom email, provider-specific fields (SendGrid vs SMTP), test email behavior, and reply-to handling are clearer, more consistent with global settings, and validated correctly. Sequence diagram for testing custom email configuration from event settingssequenceDiagram
actor Organizer
participant Browser
participant EventEmailSettingsView as EventEmailSettingsView
participant MailSettingsForm as MailSettingsForm
participant Event
participant MailBackend as MailBackend
Organizer->>Browser: Submit email settings form (test=1, optional test_email)
Browser->>EventEmailSettingsView: POST /control/event/.../mail
EventEmailSettingsView->>MailSettingsForm: Instantiate with POST data
MailSettingsForm-->>EventEmailSettingsView: is_valid(), cleaned_data
alt Form valid and test requested
EventEmailSettingsView->>Event: get_mail_backend(force_custom=True, timeout=10)
Event-->>EventEmailSettingsView: MailBackend instance
alt test_email provided
EventEmailSettingsView->>EventEmailSettingsView: Parse test_email into to_addrs list
else no test_email provided
EventEmailSettingsView->>EventEmailSettingsView: Set to_addrs = None
end
alt Backend supports test
EventEmailSettingsView->>MailBackend: test(from_addr=event.settings.mail_from, to_addrs=to_addrs)
MailBackend-->>EventEmailSettingsView: Success or exception
else Backend missing test
EventEmailSettingsView->>EventEmailSettingsView: Raise AttributeError
end
alt Exception raised
EventEmailSettingsView->>Browser: Redirect with warning message
else No exception
EventEmailSettingsView->>Browser: Redirect with success message
end
else Form invalid or no test requested
EventEmailSettingsView-->>Browser: Re-render form with errors or normal save
end
note over MailBackend: For SendGridEmail, CustomSMTPBackend, FileSavedEmailBackend
note over MailBackend: test(from_addr, to_addrs) sends real test message
Updated class diagram for email backends, mail service, and event mail settings formclassDiagram
class SendGridEmail {
- api_key
+ __init__(api_key)
+ test(from_addr, to_addrs)
}
class CustomSMTPBackend {
+ test(from_addr, to_addrs)
+ send_messages(email_messages)
}
class FileSavedEmailBackend {
- _fname
+ send_messages(email_messages)
+ test(from_addr, to_addrs)
+ _get_filename()
}
class MailSettingsForm {
+ mail_prefix
+ mail_from
+ mail_reply_to
+ mail_from_name
+ smtp_use_custom
+ email_vendor
+ send_grid_api_key
+ smtp_host
+ smtp_port
+ smtp_username
+ smtp_password
+ smtp_use_tls
+ smtp_use_ssl
+ test_email
+ clean()
}
class EventSettings {
+ mail_from
+ mail_reply_to
+ mail_bcc
+ get_mail_backend(force_custom, timeout)
}
class MailService {
+ mail(event, auto_email, headers, event_reply_to)
}
EventSettings --> SendGridEmail : configures
EventSettings --> CustomSMTPBackend : configures
EventSettings --> FileSavedEmailBackend : configures
MailSettingsForm --> EventSettings : saves_to
MailService --> EventSettings : reads_settings
MailService ..> SendGridEmail : uses_backend
MailService ..> CustomSMTPBackend : uses_backend
MailService ..> FileSavedEmailBackend : uses_backend
note for MailService "If EventSettings.mail_reply_to is set and no Reply-To header exists, it is applied for non auto_email messages before using event_reply_to"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
@mariobehling , |
![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 use of hard-coded vendor index selectors like
#id_email_vendor_0/#id_email_vendor_1is brittle; consider deriving these selectors from the field’schoicesor using data attributes that don’t depend on positional indices. - The
data-display-dependencypattern is now duplicated between the global settings form and the event mail template; consider centralizing this behavior (e.g., via a helper or shared JS/HTML snippet) to keep the logic consistent and reduce future maintenance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of hard-coded vendor index selectors like `#id_email_vendor_0` / `#id_email_vendor_1` is brittle; consider deriving these selectors from the field’s `choices` or using data attributes that don’t depend on positional indices.
- The `data-display-dependency` pattern is now duplicated between the global settings form and the event mail template; consider centralizing this behavior (e.g., via a helper or shared JS/HTML snippet) to keep the logic consistent and reduce future maintenance.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
Updates the organiser and global email settings UIs to be more context-driven by conditionally showing email vendor configuration only when relevant (custom email enabled and chosen vendor).
Changes:
- Hide email vendor configuration until “Use custom email” is enabled, and show only the selected vendor’s fields (SendGrid vs SMTP) in event email settings.
- Add
data-display-dependencyattributes to global email settings fields to toggle visibility based on selected vendor. - Normalize label casing (“Use custom email”, “Sendgrid token”).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/eventyay/control/templates/pretixcontrol/event/mail.html | Wraps vendor fields in data-display-dependency containers to conditionally display relevant email config fields. |
| app/eventyay/control/forms/global_settings.py | Adds vendor-based data-display-dependency widget attrs and updates SendGrid label casing. |
| app/eventyay/control/forms/event.py | Updates email settings field labels for consistency. |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
testbutton label and surrounding copy inmail.htmlstill refer specifically to "custom SMTP connection" even though SendGrid is now also supported; consider updating the text to reflect that it tests the currently selected custom email provider. - The
MailSettingsForm.clean()logic hard-codessettings.MAIL_FROMas the only allowed non-custom sender; if there are other global fallbacks (e.g., instance-level overrides), you may want to reuse the same resolution logic as the actual mail backend instead of comparing against a single setting. - The
testmethods forSendGridEmail,CustomSMTPBackend, andFileSavedEmailBackendeach reimplement nearly identicalto_addrsnormalization and test-message construction; consider extracting a small helper to avoid duplication and keep behavior consistent across backends.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test` button label and surrounding copy in `mail.html` still refer specifically to "custom SMTP connection" even though SendGrid is now also supported; consider updating the text to reflect that it tests the currently selected custom email provider.
- The `MailSettingsForm.clean()` logic hard-codes `settings.MAIL_FROM` as the only allowed non-custom sender; if there are other global fallbacks (e.g., instance-level overrides), you may want to reuse the same resolution logic as the actual mail backend instead of comparing against a single setting.
- The `test` methods for `SendGridEmail`, `CustomSMTPBackend`, and `FileSavedEmailBackend` each reimplement nearly identical `to_addrs` normalization and test-message construction; consider extracting a small helper to avoid duplication and keep behavior consistent across backends.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 restructures event email configuration to better match provider requirements and improves the event email settings UI by conditionally showing provider-specific fields, adding Reply-To support, and enabling test emails to custom recipients.
Changes:
- Move “Sender address” (
mail_from) out of the General section and group custom email/provider fields under the “Use custom email” toggle. - Add
mail_reply_tosetting and apply it as the outgoing emailReply-Toheader. - Extend backend “test” functionality to optionally send test emails to user-specified addresses.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/control/views/event.py | Extends email backend test invocation to accept optional recipient list and adds backend capability check. |
| app/eventyay/control/templates/pretixcontrol/event/mail.html | Restructures email settings UI to conditionally display vendor-specific sections and adds test-email input. |
| app/eventyay/control/forms/global_settings.py | Updates SendGrid label casing and adds display-dependency attributes to vendor-specific global email fields. |
| app/eventyay/control/forms/event.py | Adds mail_reply_to to auto settings fields, introduces test_email, and attempts to add validation around custom sender usage. |
| app/eventyay/base/services/mail.py | Introduces mail_reply_to into Reply-To header handling. |
| app/eventyay/base/email.py | Updates backend .test() methods to accept optional recipients and sends an actual test message. |
| app/eventyay/base/configurations/default_setting.py | Adds the new mail_reply_to default/field definition. |
Comments suppressed due to low confidence (2)
app/eventyay/control/views/event.py:825
- The new
AttributeErrorpath and multi-vendor testing mean errors/success messages can be unrelated to SMTP (e.g. SendGrid selected, or backend doesn’t supporttest). Right now everything is reported as “contacting the SMTP server”, andAttributeErroris caught and shown as an SMTP error. Consider handlingAttributeErrorseparately with a vendor-agnostic message, and avoidhasattrby callingbackend.test(...)and catchingAttributeErrorexplicitly.
if hasattr(backend, 'test'):
backend.test(self.request.event.settings.mail_from, to_addrs=to_addrs)
else:
raise AttributeError(_('The active email backend does not support connection testing.'))
except Exception as e:
messages.warning(
self.request,
_('An error occurred while contacting the SMTP server: %s') % str(e),
)
app/eventyay/base/services/mail.py:197
- This change makes
event.settings.mail_reply_tooverride anyevent_reply_topassed intomail(). That breaks per-email Reply-To overrides (e.g. the sendmail plugin passesevent_reply_to=self.reply_to). The precedence should usually be: explicitevent_reply_to(for manual emails) > event defaultmail_reply_to> other fallbacks, while still applying the event default to auto-emails when no explicit reply-to is provided.
if event.settings.mail_reply_to and not headers.get('Reply-To'):
headers['Reply-To'] = event.settings.mail_reply_to
elif not auto_email:
if (
event_reply_to
and not headers.get('Reply-To')
):
headers['Reply-To'] = event_reply_to
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures how event-level email configuration is presented and enforced, adding a dedicated Reply-To setting and improving the “custom email” UI so organizers only see provider-relevant fields.
Changes:
- Moves “Sender address” out of the General section and into the custom email configuration area; adds
mail_reply_to. - Adds “send test email to …” support and extends email backends’
test()to accept recipients. - Adds conditional UI display for SendGrid vs SMTP fields and standardizes labels (“Use custom email”, “Sendgrid token”).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/control/views/event.py | Adds parsing for test-email recipients and guards backend test support. |
| app/eventyay/control/templates/pretixcontrol/event/mail.html | Reorganizes form layout; conditionally shows vendor-specific fields and moves test button. |
| app/eventyay/control/forms/global_settings.py | Standardizes labels and adds display dependencies for vendor-specific global email fields. |
| app/eventyay/control/forms/event.py | Adds mail_reply_to, test_email, validation around custom sender, and prevents persisting test_email. |
| app/eventyay/base/services/mail.py | Sets Reply-To header from new mail_reply_to setting with precedence handling. |
| app/eventyay/base/email.py | Extends SendGrid/SMTP/file backends’ test() to send a test email (optionally to custom recipients). |
| app/eventyay/base/configurations/default_setting.py | Adds new mail_reply_to setting to defaults/config schema. |
Comments suppressed due to low confidence (1)
app/eventyay/control/views/event.py:825
- The exception handling here is now used for SendGrid/file backends and for the explicit
AttributeError, but the warning message still says “contacting the SMTP server” and formats the exception withstr(e). Please make the message backend-agnostic (e.g. “testing email settings”) and pass the exception object directly to interpolation instead ofstr().
raise AttributeError(_('The active email backend does not support connection testing.'))
except Exception as e:
messages.warning(
self.request,
_('An error occurred while contacting the SMTP server: %s') % str(e),
)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures event email configuration to better reflect provider requirements and improves the event email settings UI by conditionally showing provider-specific fields, adding a Reply-To setting, and enabling test emails to custom recipients.
Changes:
- Add
mail_reply_toevent setting and apply it with correct precedence when sending event emails. - Restructure the event “E-mail settings” UI to hide provider-specific fields unless “Use custom email” and the selected vendor are active, and add a “Send test email to” field.
- Update SendGrid/SMTP test methods to send a test email (optionally to user-specified recipients) and add coverage for Reply-To precedence.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/tests/tickets/base/test_mail.py | Adds tests for Reply-To precedence; adjusts existing placeholder test. |
| app/eventyay/control/views/event.py | Extends “test” flow to accept comma-separated recipients for test emails. |
| app/eventyay/control/templates/pretixcontrol/event/mail.html | Moves sender address into Email settings and adds conditional UI + test email controls. |
| app/eventyay/control/forms/global_settings.py | Updates labels and adds vendor-dependent UI attributes for global email settings. |
| app/eventyay/control/forms/event.py | Adds mail_reply_to and test_email, adjusts validation and settings persistence behavior. |
| app/eventyay/base/services/mail.py | Ensures mail_reply_to takes precedence unless an explicit Reply-To header is provided. |
| app/eventyay/base/email.py | Updates backend test() methods to send test emails and supports custom recipients. |
| app/eventyay/base/configurations/default_setting.py | Registers new mail_reply_to setting. |
Sak1012
requested changes
May 5, 2026
Member
Sak1012
left a comment
Sak1012
left a comment
There was a problem hiding this comment.
Please move "Reply-To" to the general tab above the BCC field and rename the "Email Settings" tab to "Advanced Settings". The rest of the changes looks good.
Contributor
Author
|
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20603f1a7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nostics - Preserve SMTP/SendGrid settings when switching vendors or disabling custom email in MailSettingsForm - Improve exception handling and logging in test-email flow to match global settings - Remove redundant hasattr check for email backend test capability
Contributor
Author
Thanks for the feedback. Resolved |
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.
Fixes: #3366
Overview
This PR fixes and restructures email configuration and improves UI clarity in event settings.
Case C: Improve UI behavior and clarity
C.Improve.UI.behavior.and.clarity.mp4
Case B: Improve Email settings functionality
B.Improve.Email.settings.functionality.mp4
Case A: Move and fix sender email configuration
A.Move.and.fix.sender.email.configuration.mp4
Changes Made
A. Sender Email Configuration
B. Email Functionality Improvements
C. UI Improvements
Use Custom Email→Use custom emailSendgrid Token→Sendgrid tokenExpected Behavior
Acceptance Criteria