Skip to content

feat(np): Warn, don't fail on missing test email configuration#114506

Merged
hobzcalvin merged 1 commit intomasterfrom
notification-test-email
May 4, 2026
Merged

feat(np): Warn, don't fail on missing test email configuration#114506
hobzcalvin merged 1 commit intomasterfrom
notification-test-email

Conversation

@hobzcalvin
Copy link
Copy Markdown
Contributor

While testing #114505 it was helpful to use e.g. sentry notifications send email --source 'data-export-success' without having an actual email backend set up. This turns the exception into a warning.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 30, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ad340c0. Configure here.

"\n - remove `mail.backend` (if set to 'dummy' or 'console')"
"\n - set `mail.host`, `mail.port`, `mail.username` and `mail.password`"
)
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contradictory output: warns "won't send" then says "sent"

Low Severity

After removing the early return, the code now warns "Email will not actually send with current configuration!" but then unconditionally prints "Example '{source}' email sent to {email}." on line 65. These messages directly contradict each other. The success message doesn't account for the case where the email config is known to be non-functional. Adjusting the final message to reflect the warning (e.g., distinguishing "processed" from "sent") would avoid confusing developers using this debugging tool.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ad340c0. Configure here.

Comment on lines 56 to 57
)
return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Removing the return causes the send-test-notification command to crash for users without email configured, as it attempts to connect to an SMTP server with None credentials.
Severity: MEDIUM

Suggested Fix

Restore the early return statement after logging the warning for missing email configuration. This will prevent the code from attempting to send an email via the SMTP backend when no valid settings are available, thus avoiding the unhandled exception.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/sentry/runner/commands/notifications.py#L56-L57

Potential issue: The removal of a `return` statement after a warning for missing email
configuration will cause the `send-test-notification` command to crash for users without
email settings. The code will proceed to call `get_connection()` with `None` for
`mail.host`, `mail.port`, and other credentials. This leads to an unhandled connection
error when Django's SMTP mail backend attempts to connect to a `None` host. The
exception propagates up the call stack (`notify_sync` -> `notify_target` ->
`provider.send`) and terminates the command.

Did we get this right? 👍 / 👎 to inform future reviews.

@hobzcalvin hobzcalvin merged commit 1c519ce into master May 4, 2026
60 checks passed
@hobzcalvin hobzcalvin deleted the notification-test-email branch May 4, 2026 16:36
cleptric pushed a commit that referenced this pull request May 5, 2026
While testing #114505 it was helpful to use e.g. `sentry notifications
send email --source 'data-export-success'` without having an actual
email backend set up. This turns the exception into a warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants