If we are on the development server we must filter emails #501

Merged

johnsturgeon merged 3 commits from change-all-outgoing-email-s-on-dev-server-to-tgfp-us-500 into main 2026-09-09 20:28:26 +02:00

Conversation 1 Commits 3 Files changed 4 +21 -3

johnsturgeon commented 2026-09-09 15:49:36 +02:00

Owner

Copy link

I'm just going to route all emails to tgfp.us

I'm just going to route all emails to tgfp.us

johnsturgeon added 1 commit 2026-09-09 15:49:37 +02:00

If we are on the development server we must filter emails ... b7ef2816c6

I'm just going to route all emails to tgfp.us

johnsturgeon commented 2026-09-09 15:50:46 +02:00

Author

Owner

Copy link

This is a small, self-contained change. I've reviewed the mailer logic, the config wiring, and the test setup.

---

Review

This PR adds a development-only safeguard that rewrites outbound email domains to a configurable redirect domain (DEV_REDIRECT_EMAIL_DOMAIN), sparing the admin address, so a dev database can't accidentally spam real players. The logic is correct and the config/env wiring is complete; my only substantive concern is that the new code path has no test coverage.

Worth fixing

• No test exercises the new redirect path (app/mailer.py:38-47). The existing suite runs under ENVIRONMENT="testing" (tests/conftest.py:74), so _send's if config.ENVIRONMENT == 'development' branch is never entered — the redirect, the admin-email exception, and the new log counts all ship untested. This is exactly the kind of "don't email real players" safety net that warrants a pinned test. tests/test_mailer.py already has a built_message fixture that captures the MessageSchema; a test that monkeypatch.setattr(config, "ENVIRONMENT", "development") and asserts message.recipients were rewritten (and that TGFP_ADMIN_EMAIL was left intact) would cover it directly.

• DEV_REDIRECT_EMAIL_DOMAIN is now required in every environment (app/config/config.py:85). get_config() reads it with os.environ[...], which KeyErrors at startup if unset — including in production, where the value is never used. It is added to config/op.env, so a normal prod deploy is fine, but it's a required variable purely for a dev feature. Not blocking given op.env is the manifest, just worth being aware of.

Nits

• Local-part collisions in dev (app/mailer.py:44). Two recipients that share a local part across different domains (e.g. john@gmail.com and john@yahoo.com) both collapse to john@<redirect>, so a dev run would send one of them a duplicate and the other nothing. Harmless in practice, but worth a mental note when eyeballing dev sends.

• Quote-style inconsistency (app/mailer.py:39). 'development' uses single quotes; the same literal is written "development" elsewhere (app/main.py:597). Cosmetic — neither pylint nor flake8 enforces this here.

• The if/else could be a single comprehension, but the explicit loop is arguably clearer for a safety-critical branch, so I'd leave it.

Nothing blocking — this is a sensible guardrail and the implementation matches the intent described in docs/sample.env.

<!-- claude-code-review --> This is a small, self-contained change. I've reviewed the mailer logic, the config wiring, and the test setup. --- ## Review This PR adds a development-only safeguard that rewrites outbound email domains to a configurable redirect domain (`DEV_REDIRECT_EMAIL_DOMAIN`), sparing the admin address, so a dev database can't accidentally spam real players. The logic is correct and the config/env wiring is complete; my only substantive concern is that the new code path has no test coverage. ### Worth fixing - **No test exercises the new redirect path** (`app/mailer.py:38-47`). The existing suite runs under `ENVIRONMENT="testing"` (`tests/conftest.py:74`), so `_send`'s `if config.ENVIRONMENT == 'development'` branch is never entered — the redirect, the admin-email exception, and the new log counts all ship untested. This is exactly the kind of "don't email real players" safety net that warrants a pinned test. `tests/test_mailer.py` already has a `built_message` fixture that captures the `MessageSchema`; a test that `monkeypatch.setattr(config, "ENVIRONMENT", "development")` and asserts `message.recipients` were rewritten (and that `TGFP_ADMIN_EMAIL` was left intact) would cover it directly. - **`DEV_REDIRECT_EMAIL_DOMAIN` is now required in every environment** (`app/config/config.py:85`). `get_config()` reads it with `os.environ[...]`, which `KeyError`s at startup if unset — including in production, where the value is never used. It is added to `config/op.env`, so a normal prod deploy is fine, but it's a required variable purely for a dev feature. Not blocking given op.env is the manifest, just worth being aware of. ### Nits - **Local-part collisions in dev** (`app/mailer.py:44`). Two recipients that share a local part across different domains (e.g. `john@gmail.com` and `john@yahoo.com`) both collapse to `john@<redirect>`, so a dev run would send one of them a duplicate and the other nothing. Harmless in practice, but worth a mental note when eyeballing dev sends. - **Quote-style inconsistency** (`app/mailer.py:39`). `'development'` uses single quotes; the same literal is written `"development"` elsewhere (`app/main.py:597`). Cosmetic — neither pylint nor flake8 enforces this here. - The `if/else` could be a single comprehension, but the explicit loop is arguably clearer for a safety-critical branch, so I'd leave it. Nothing blocking — this is a sensible guardrail and the implementation matches the intent described in `docs/sample.env`.

johnsturgeon added 1 commit 2026-09-09 15:54:18 +02:00

fixed review feedback bug e045ab62e0

johnsturgeon added 1 commit 2026-09-09 16:40:26 +02:00

updated to move out email domains to the config ... b0806a1550

Also guarding the admin email instead of just hard coding the domain

johnsturgeon merged commit fe213b8448 into main 2026-09-09 20:28:26 +02:00

johnsturgeon deleted branch change-all-outgoing-email-s-on-dev-server-to-tgfp-us-500 2026-09-09 20:28:26 +02:00

johnsturgeon referenced this pull request from a commit 2026-09-09 20:28:28 +02:00

Merge pull request 'If we are on the development server we must filter emails' (#501) from change-all-outgoing-email-s-on-dev-server-to-tgfp-us-500 into main

johnsturgeon referenced this pull request from a commit 2026-09-09 21:00:20 +02:00

Fix the development recipient rewrite for display-name addresses

johnsturgeon referenced this pull request 2026-09-09 21:29:52 +02:00

BCC copy of outgoing mail never reaches Sent #504

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Kestra

  

bug

Something isn't working

  

enhancement

New feature or request

  

someday

Someday / Maybe

  

subtask

  

☁️ api

  

🎛️ infrastructure

  

🐞 sentry

  

📆 2025 Season

  

📝 pages

allpicks

Related to the All Picks page

  

📝 pages

picks

  

📝 pages

standings

  

🚀 performance

No labels

Kestra

bug

enhancement

someday

subtask

☁️ api

🎛️ infrastructure

🐞 sentry

📆 2025 Season

📝 pages

allpicks

📝 pages

picks

📝 pages

standings

🚀 performance

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

johnsturgeon/tgfp-web!501

No description provided.