feat: add environment prefix to email subjects#3903
Conversation
Add [dev] or [play] prefix to email subjects when running in dev or play environments to help distinguish them from production emails. Changes: - Modified send_email_with_template() to add prefix based on METACULUS_ENV - Created get_email_subject_with_env_prefix() helper for direct EmailMessage usage - Applied helper to all EmailMessage instances in utils/tasks.py and misc/views.py Fixes #3899 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Sylvain <SylvainChevalier@users.noreply.github.com>
SylvainChevalier
temporarily deployed
to
testing_env
GitHub Actions
Inactive
SylvainChevalier
had a problem deploying
to
testing_env
GitHub Actions
Failure
|
Feel free to reject if this adds too much complexity or is too hard to maintain or etc. |
|
@claude I don't like the solution at all. Implement it globally in one place only -- send_email_async |
|
Claude finished @hlbmtc's task in 5m 3s —— View job Refactoring Complete ✓I've successfully refactored the implementation to centralize all email prefix logic in Changes MadeSingle source of truth in
Cleanup across codebase:
Result: All email prefix logic now lives in one place ( Tests: Verified existing tests only mock Branch: claude/issue-3899-20251210-1048 |
Move all environment prefix logic to a single function in misc/tasks.py: - Created add_env_prefix_to_subject() as the single source of truth - Updated send_email_async() to use this function - Removed duplicate logic from utils/email.py and utils/tasks.py - Updated all EmailMessage instances to use the centralized function This simplifies maintenance by keeping all email prefix logic in one place. Co-authored-by: Hlib <hlbmtc@users.noreply.github.com>
|
@claude the integration tests failed, fix the issues. |
|
Claude finished @SylvainChevalier's task in 1m 32s —— View job Test Failures Fixed ✓Todo List:
Summary: Fixed the circular import error that was causing both unit tests and integration tests to fail. Problem:
Solution:
Changes:
The fix has been committed and pushed. The CI tests should now pass. Branch: claude/issue-3899-20251210-1048 | |
Move the User model import from module level to inside the filter_staff_emails function to avoid circular import during Django initialization. This fixes the ImportError that was causing both unit and integration tests to fail. Co-authored-by: Sylvain <SylvainChevalier@users.noreply.github.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a Django AppConfig and a new Anymail/Django pre_send signal handler that prefixes subjects for dev/play, can redirect all mail, and filters recipients to staff/internal addresses before send; removes duplicate recipient-override/filter logic from task and utility layers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor DjangoApp as "Django App"
participant AppConfig as "misc.apps.MiscConfig"
participant Anymail as "Anymail pre_send"
participant SignalHandler as "misc.signals.handle_pre_send"
participant Settings as "settings"
participant DB as "User DB"
participant EmailBackend as "Email backend"
DjangoApp->>AppConfig: start
AppConfig->>SignalHandler: import misc.signals (register handlers)
Anymail->>SignalHandler: pre_send(message, ...)
SignalHandler->>Settings: read METACULUS_ENV, SEND_ALL_MAIL_TO, EMAIL_ALLOW_SEND_TO_ALL_USERS
SignalHandler->>DB: query staff emails (if filtering enabled)
SignalHandler->>SignalHandler: prefix subject if env dev/play
SignalHandler->>SignalHandler: replace recipients with SEND_ALL_MAIL_TO (if set)
SignalHandler->>SignalHandler: filter recipients to staff/internal (if disallowed)
SignalHandler->>Anymail: proceed or raise AnymailCancelSend
Anymail->>EmailBackend: deliver message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude Let's do another way around. We're currently using Anymail library for django. I see it supports pre-send signals (https://anymail.dev/en/stable/sending/signals/) which allows to modify and cancel sending emails. Let's move prefixes logic there along with SEND_ALL_MAIL_TO and filter_staff_emails logic |
|
Claude finished @hlbmtc's task in 5m 55s —— View job Refactored email logic to Anymail pre-send signal
SummaryAll email processing logic is now centralized in a single Anymail New file
New file Cleanup across codebase:
|
Centralize all email processing logic (subject prefix, SEND_ALL_MAIL_TO, staff-only filtering) into a single Anymail pre-send signal handler. This ensures all emails sent through Django's email system are automatically processed regardless of where they originate. - Create misc/signals.py with handle_pre_send signal handler - Create misc/apps.py to register the signal in AppConfig.ready() - Remove add_env_prefix_to_subject and filter_staff_emails from misc/tasks.py - Remove SEND_ALL_MAIL_TO logic from utils/email.py - Remove scattered add_env_prefix_to_subject calls from utils/tasks.py and misc/views.py Co-authored-by: Hlib <hlbmtc@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
misc/signals.py (1)
30-47: DB query on every email send — consider caching staff emails.
User.objects.filter(is_staff=True)executes a database query for every outgoing email whenEMAIL_ALLOW_SEND_TO_ALL_USERSisFalse. If email volume is non-trivial, this adds latency and DB load per send.Consider caching the staff email set with a short TTL (e.g., via
django.core.cacheorfunctools.lru_cachewith a time-based wrapper) so you're not hitting the DB on every single email.♻️ Example using Django's cache framework
+from django.core.cache import cache + +STAFF_EMAILS_CACHE_KEY = "staff_emails_set" +STAFF_EMAILS_CACHE_TTL = 300 # 5 minutes + +def _get_staff_emails() -> set[str]: + staff_emails = cache.get(STAFF_EMAILS_CACHE_KEY) + if staff_emails is None: + from users.models import User + staff_emails = set( + email.lower() + for email in User.objects.filter(is_staff=True).values_list( + "email", flat=True + ) + ) + cache.set(STAFF_EMAILS_CACHE_KEY, staff_emails, STAFF_EMAILS_CACHE_TTL) + return staff_emailsThen in the handler:
- from users.models import User - - staff_emails = set( - email.lower() - for email in User.objects.filter(is_staff=True).values_list( - "email", flat=True - ) - ) + staff_emails = _get_staff_emails()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@misc/signals.py` around lines 30 - 47, The code in signals.py currently queries User.objects.filter(is_staff=True) on every send when settings.EMAIL_ALLOW_SEND_TO_ALL_USERS is False; change this to read a cached staff email set with a short TTL (e.g., use django.core.cache.cache.get/set with a key like "staff_emails:set" and a TTL of e.g. 60–300s) and fall back to the DB only on cache miss; keep using the resulting set when calling _filter_staff for message.to/cc/bcc and raise AnymailCancelSend if none remain. Also ensure cache is invalidated/updated when staff status changes (connect a post_save/post_delete handler on users.models.User that clears or refreshes the same cache key).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@misc/signals.py`:
- Around line 30-47: The code in signals.py currently queries
User.objects.filter(is_staff=True) on every send when
settings.EMAIL_ALLOW_SEND_TO_ALL_USERS is False; change this to read a cached
staff email set with a short TTL (e.g., use django.core.cache.cache.get/set with
a key like "staff_emails:set" and a TTL of e.g. 60–300s) and fall back to the DB
only on cache miss; keep using the resulting set when calling _filter_staff for
message.to/cc/bcc and raise AnymailCancelSend if none remain. Also ensure cache
is invalidated/updated when staff status changes (connect a
post_save/post_delete handler on users.models.User that clears or refreshes the
same cache key).
# Conflicts: # Claude.md # misc/tasks.py
Cleanup: Preview Environment RemovedThe preview environment for this PR has been destroyed.
Cleanup triggered by PR close at 2026-02-18T13:48:11Z |
hlbmtc
left a comment
hlbmtc
left a comment
There was a problem hiding this comment.
Added extra changes and tested - it's good to go!
2 participants
Add [dev] or [play] prefix to email subjects when running in dev or play environments to help distinguish them from production emails.
Changes
Fixes #3899
Generated with Claude Code
Summary by CodeRabbit