feat: add generic /email/blast endpoint with query registry #299

Merged

forgejo_admin merged 2 commits from 295-admin-blast-endpoint into main 2026-04-03 19:44:52 +00:00

Conversation 2 Commits 2 Files changed 3 +759

forgejo_admin commented 2026-04-03 19:39:00 +00:00

Owner

Copy link

Summary

Adds a generic POST /admin/email/blast endpoint that accepts layout, email_type, query, subject, and data params to send templated email campaigns. New campaigns only need a compiled MJML template and a registered query function -- no new endpoint code required. Ships with the unsigned_contracts query for contract-offer reminders.

Changes

• src/basketball_api/services/email_queries.py (new): Query registry module with QUERY_REGISTRY dict and query_unsigned_contracts function. Queries players with contract_status='offered', contract_signed_at IS NULL, and a valid contract_token, grouped by parent.
• src/basketball_api/routes/admin.py: Added POST /email/blast endpoint with BlastRequest/BlastResponse models. Validates query name, email_type enum, and template existence. Merges per-recipient query data with request-level data overrides. Supports {{placeholder}} replacement in both subject and template body. Uses send_templated_email() for sending.
• tests/test_email_blast.py (new): 15 tests covering query unit tests (unsigned offered, excludes signed, excludes no-token, test_email filter, registry registration) and endpoint integration tests (single send, multiple recipients, empty results, unknown query 422, invalid email_type 422, missing template 422, subject placeholders, data overrides, send failure errors, no-match test_email).

Test Plan

• pytest tests/test_email_blast.py -v -- all 15 pass
• Full suite: 771 passed, 9 pre-existing failures (checkout/jersey opt-out tests on main)
• ruff format and ruff check clean

Review Checklist

• ruff format and ruff check pass
• All new tests pass (15/15)
• No regressions in existing tests
• Follows existing blast endpoint patterns (profile-reminder, jersey-reminder, contract-reminder)
• Uses send_templated_email() from #298
• Query registry is extensible -- new queries only need a function and a dict entry

Related

• Closes #295
• Depends on: #296 (MJML system), #297 (mkdir fix), #298 (send_templated_email + EmailType)

Related Notes

• None

## Summary Adds a generic `POST /admin/email/blast` endpoint that accepts layout, email_type, query, subject, and data params to send templated email campaigns. New campaigns only need a compiled MJML template and a registered query function -- no new endpoint code required. Ships with the `unsigned_contracts` query for contract-offer reminders. ## Changes - **`src/basketball_api/services/email_queries.py`** (new): Query registry module with `QUERY_REGISTRY` dict and `query_unsigned_contracts` function. Queries players with `contract_status='offered'`, `contract_signed_at IS NULL`, and a valid `contract_token`, grouped by parent. - **`src/basketball_api/routes/admin.py`**: Added `POST /email/blast` endpoint with `BlastRequest`/`BlastResponse` models. Validates query name, email_type enum, and template existence. Merges per-recipient query data with request-level `data` overrides. Supports `{{placeholder}}` replacement in both subject and template body. Uses `send_templated_email()` for sending. - **`tests/test_email_blast.py`** (new): 15 tests covering query unit tests (unsigned offered, excludes signed, excludes no-token, test_email filter, registry registration) and endpoint integration tests (single send, multiple recipients, empty results, unknown query 422, invalid email_type 422, missing template 422, subject placeholders, data overrides, send failure errors, no-match test_email). ## Test Plan - `pytest tests/test_email_blast.py -v` -- all 15 pass - Full suite: 771 passed, 9 pre-existing failures (checkout/jersey opt-out tests on main) - `ruff format` and `ruff check` clean ## Review Checklist - [x] ruff format and ruff check pass - [x] All new tests pass (15/15) - [x] No regressions in existing tests - [x] Follows existing blast endpoint patterns (profile-reminder, jersey-reminder, contract-reminder) - [x] Uses send_templated_email() from #298 - [x] Query registry is extensible -- new queries only need a function and a dict entry ## Related - Closes #295 - Depends on: #296 (MJML system), #297 (mkdir fix), #298 (send_templated_email + EmailType) ## Related Notes - None

forgejo_admin added 1 commit 2026-04-03 19:39:00 +00:00

feat: add generic /email/blast endpoint with query registry ... 2ea1f22bd2

Adds POST /admin/email/blast that accepts layout, email_type, query,
subject, and data params to send templated email campaigns without
new code per blast type. Includes unsigned_contracts query and 15 tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin added 1 commit 2026-04-03 19:40:12 +00:00

fix: use Callable type annotation instead of builtin callable ... 47bb0ca137

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-04-03 19:40:26 +00:00

Author

Owner

Copy link

QA Review

Findings

Fixed (pushed):

1. QUERY_REGISTRY type annotation used lowercase callable (builtin function) instead of Callable from collections.abc. Fixed in follow-up commit.

Verified clean:

• Endpoint follows the same pattern as /email/profile-reminder, /email/jersey-reminder, /email/contract-reminder -- admin auth via require_admin, tenant lookup via DEFAULT_TENANT_SLUG, test_email filter, sent_count/errors response shape
• Query function uses the same join+filter pattern as the contract-reminder endpoint (join player_teams, filter contract_status, contract_token, contract_signed_at)
• send_templated_email() used correctly with all required kwargs
• FileNotFoundError from send_templated_email re-raised as 422 (not 500)
• send_templated_email returns None on gmail failure (does not raise), correctly captured in errors list
• EmailType validated before use -- mirrors CoachRole validation pattern at line 133
• Subject placeholder replacement only applies to str values (skips int player_count)
• Internal keys (to, parent_id, players) stripped from template data before send
• ruff format + ruff check clean
• 15 new tests, all pass. Full suite: 771 passed, 9 pre-existing failures (checkout/jersey -- confirmed on main)

No issues found.

VERDICT: APPROVED

## QA Review ### Findings **Fixed (pushed):** 1. `QUERY_REGISTRY` type annotation used lowercase `callable` (builtin function) instead of `Callable` from `collections.abc`. Fixed in follow-up commit. **Verified clean:** - Endpoint follows the same pattern as `/email/profile-reminder`, `/email/jersey-reminder`, `/email/contract-reminder` -- admin auth via `require_admin`, tenant lookup via `DEFAULT_TENANT_SLUG`, `test_email` filter, `sent_count`/`errors` response shape - Query function uses the same join+filter pattern as the contract-reminder endpoint (join `player_teams`, filter `contract_status`, `contract_token`, `contract_signed_at`) - `send_templated_email()` used correctly with all required kwargs - `FileNotFoundError` from `send_templated_email` re-raised as 422 (not 500) - `send_templated_email` returns `None` on gmail failure (does not raise), correctly captured in errors list - `EmailType` validated before use -- mirrors `CoachRole` validation pattern at line 133 - Subject placeholder replacement only applies to `str` values (skips int `player_count`) - Internal keys (`to`, `parent_id`, `players`) stripped from template data before send - ruff format + ruff check clean - 15 new tests, all pass. Full suite: 771 passed, 9 pre-existing failures (checkout/jersey -- confirmed on main) **No issues found.** ### VERDICT: APPROVED

forgejo_admin commented 2026-04-03 19:44:18 +00:00

Author

Owner

Copy link

PR #299 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic

Architecture: The query registry pattern (QUERY_REGISTRY dict mapping names to functions) is a clean extension point. New campaigns only need a query function and a dict entry -- no new endpoint code. This is the right abstraction after the project accumulated three bespoke blast endpoints (profile-reminder, jersey-reminder, contract-reminder).

Query correctness (query_unsigned_contracts):

• Filters contract_status == ContractStatus.offered -- correct enum usage, matches the model at models.py:226.
• Filters contract_signed_at.is_(None) -- correct SQLAlchemy NULL check.
• Filters contract_token.isnot(None) -- ensures only players with active offer tokens are included.
• Joins player_teams to ensure players are on a roster -- correct, since the query needs team name for template data.
• Uses joinedload(Player.parent) and joinedload(Player.teams) -- proper eager loading to avoid N+1 queries.
• Groups by parent with parent_map dict -- correct, each parent gets one email listing all unsigned players.
• test_email filtering happens post-query on parent.email -- correct and safe.
• .distinct() on the query prevents duplicate rows from the join -- good.

Endpoint correctness (admin_email_blast):

• Auth: Uses Depends(require_admin) -- confirmed require_admin = require_role("admin") in auth.py:166. Consistent with all other admin email endpoints.
• Tenant lookup: Uses DEFAULT_TENANT_SLUG pattern identical to existing endpoints at lines 449, 493, 569, 859 of admin.py.
• Query validation: Checks QUERY_REGISTRY.get(body.query) and returns 422 with available query names on miss.
• EmailType validation: EmailType(body.email_type) with try/except ValueError returning 422. Depends on PR #298 adding contract_offer/contract_reminder values to the enum (currently models.py:63-71 has 7 values, no contract-specific ones).
• Template data merge: {**recipient, **body.data} -- request-level data overrides per-recipient data. Correct precedence.
• Internal key cleanup: Removes to, parent_id, players from template data. Correct -- these are routing metadata, not template placeholders.
• Subject placeholder replacement: Iterates template_data items, replaces {{key}} with string values. Only replaces string-typed values, skipping ints/lists safely.
• Error handling: FileNotFoundError re-raised as 422 (template missing). General exceptions caught, logged, appended to errors list. send_templated_email returning None recorded as error.
• Response: BlastResponse with sent_count and errors list -- clean.

Pydantic model: BlastRequest.data: dict = {} -- the mutable default is safe in Pydantic (Pydantic creates a new dict per instance via its __init__). This would be a bug in a regular Python class but is fine here.

BLOCKERS

None.

• Auth: require_admin dependency present. Not duplicated.
• SQL injection: No raw SQL. All queries use SQLAlchemy ORM with parameterized filters. The body.query field is used as a dict lookup key against QUERY_REGISTRY, not interpolated into SQL.
• Secrets: No credentials in code. _CONTRACT_BASE_URL is a public-facing URL, not a secret.
• Test coverage: 15 tests cover: happy path (single/multi recipient), empty results, all 3 validation error paths (unknown query, invalid email_type, missing template), test_email filtering, subject placeholder replacement, data overrides, send failure capture, no-match test_email. Both unit (query function) and integration (endpoint) levels. Comprehensive.
• Input validation: All user-supplied fields validated -- query against registry, email_type against enum, layout against filesystem (via load_email_template).

NITS

1. Hardcoded _CONTRACT_BASE_URL (email_queries.py:28): "https://westside-contracts.tail5b443a.ts.net/contract" -- this follows the existing pattern in the codebase (see config.py:15, config.py:29, auth.py:134) where Tailscale URLs are hardcoded. Consistent with current convention, but worth noting as a future DRY candidate when the codebase extracts these into a central config. Not blocking since the whole codebase does this.

2. Bare Callable type (email_queries.py:119): QUERY_REGISTRY: dict[str, Callable] -- could be Callable[[Session, Tenant], list[dict]] or a Protocol for better IDE support and self-documentation. Minor -- no existing code in the repo uses typed Callables either.

3. FileNotFoundError short-circuits the loop (admin.py, blast endpoint): If the first recipient triggers FileNotFoundError, the endpoint raises 422 immediately, skipping remaining recipients. This is correct behavior (a missing template is a configuration error, not a per-recipient issue), but the re-raise inside the loop is slightly misleading. Consider moving template existence validation before the loop. Non-blocking since the behavior is correct.

4. No player_id in send_templated_email call: The EmailLog model has a player_id field (nullable), but the blast endpoint only passes parent_id. For grouped-by-parent emails this is reasonable (one email per parent, multiple players), but means email logs won't link to specific players. Matches the "per-parent" design intent.

SOP COMPLIANCE

• Branch named after issue: 295-admin-blast-endpoint follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related references parent issue: "Closes #295" and dependency chain (#296, #297, #298) documented
• Related references plan slug: No plan slug referenced. If this work is tracked under a plan, it should be noted.
• No secrets committed: No credentials, .env files, or tokens in the diff
• No scope creep: All changes directly serve the blast endpoint feature
• Commit messages: PR title is descriptive ("feat: add generic /email/blast endpoint with query registry")
• Tests exist: 15 tests, claimed passing (771 total, 9 pre-existing failures on main)

PROCESS OBSERVATIONS

• Deployment frequency: This PR completes the email blast pipeline (#296 MJML, #297 mkdir fix, #298 send_templated_email, #299 blast endpoint). Four PRs for one feature -- good decomposition, each PR is independently reviewable and mergeable.
• Change failure risk: Low. The endpoint is additive (new route, new module). No existing code modified beyond import additions. The query registry pattern means future campaigns won't need endpoint changes.
• Documentation: The docstrings on both the query function and the endpoint are thorough. The PR body documents the dependency chain clearly.
• Pre-existing test failures: 9 failing jersey/checkout tests noted in Test Plan. These are tracked in #274 and should not block this PR.

VERDICT: APPROVED

## PR #299 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic **Architecture**: The query registry pattern (`QUERY_REGISTRY` dict mapping names to functions) is a clean extension point. New campaigns only need a query function and a dict entry -- no new endpoint code. This is the right abstraction after the project accumulated three bespoke blast endpoints (`profile-reminder`, `jersey-reminder`, `contract-reminder`). **Query correctness** (`query_unsigned_contracts`): - Filters `contract_status == ContractStatus.offered` -- correct enum usage, matches the model at `models.py:226`. - Filters `contract_signed_at.is_(None)` -- correct SQLAlchemy NULL check. - Filters `contract_token.isnot(None)` -- ensures only players with active offer tokens are included. - Joins `player_teams` to ensure players are on a roster -- correct, since the query needs team name for template data. - Uses `joinedload(Player.parent)` and `joinedload(Player.teams)` -- proper eager loading to avoid N+1 queries. - Groups by parent with `parent_map` dict -- correct, each parent gets one email listing all unsigned players. - `test_email` filtering happens post-query on `parent.email` -- correct and safe. - `.distinct()` on the query prevents duplicate rows from the join -- good. **Endpoint correctness** (`admin_email_blast`): - Auth: Uses `Depends(require_admin)` -- confirmed `require_admin = require_role("admin")` in `auth.py:166`. Consistent with all other admin email endpoints. - Tenant lookup: Uses `DEFAULT_TENANT_SLUG` pattern identical to existing endpoints at lines 449, 493, 569, 859 of `admin.py`. - Query validation: Checks `QUERY_REGISTRY.get(body.query)` and returns 422 with available query names on miss. - EmailType validation: `EmailType(body.email_type)` with try/except ValueError returning 422. Depends on PR #298 adding `contract_offer`/`contract_reminder` values to the enum (currently `models.py:63-71` has 7 values, no contract-specific ones). - Template data merge: `{**recipient, **body.data}` -- request-level data overrides per-recipient data. Correct precedence. - Internal key cleanup: Removes `to`, `parent_id`, `players` from template data. Correct -- these are routing metadata, not template placeholders. - Subject placeholder replacement: Iterates `template_data` items, replaces `{{key}}` with string values. Only replaces string-typed values, skipping ints/lists safely. - Error handling: `FileNotFoundError` re-raised as 422 (template missing). General exceptions caught, logged, appended to errors list. `send_templated_email` returning `None` recorded as error. - Response: `BlastResponse` with `sent_count` and `errors` list -- clean. **Pydantic model**: `BlastRequest.data: dict = {}` -- the mutable default is safe in Pydantic (Pydantic creates a new dict per instance via its `__init__`). This would be a bug in a regular Python class but is fine here. ### BLOCKERS None. - **Auth**: `require_admin` dependency present. Not duplicated. - **SQL injection**: No raw SQL. All queries use SQLAlchemy ORM with parameterized filters. The `body.query` field is used as a dict lookup key against `QUERY_REGISTRY`, not interpolated into SQL. - **Secrets**: No credentials in code. `_CONTRACT_BASE_URL` is a public-facing URL, not a secret. - **Test coverage**: 15 tests cover: happy path (single/multi recipient), empty results, all 3 validation error paths (unknown query, invalid email_type, missing template), test_email filtering, subject placeholder replacement, data overrides, send failure capture, no-match test_email. Both unit (query function) and integration (endpoint) levels. Comprehensive. - **Input validation**: All user-supplied fields validated -- `query` against registry, `email_type` against enum, `layout` against filesystem (via `load_email_template`). ### NITS 1. **Hardcoded `_CONTRACT_BASE_URL`** (`email_queries.py:28`): `"https://westside-contracts.tail5b443a.ts.net/contract"` -- this follows the existing pattern in the codebase (see `config.py:15`, `config.py:29`, `auth.py:134`) where Tailscale URLs are hardcoded. Consistent with current convention, but worth noting as a future DRY candidate when the codebase extracts these into a central config. Not blocking since the whole codebase does this. 2. **Bare `Callable` type** (`email_queries.py:119`): `QUERY_REGISTRY: dict[str, Callable]` -- could be `Callable[[Session, Tenant], list[dict]]` or a `Protocol` for better IDE support and self-documentation. Minor -- no existing code in the repo uses typed Callables either. 3. **`FileNotFoundError` short-circuits the loop** (`admin.py`, blast endpoint): If the first recipient triggers `FileNotFoundError`, the endpoint raises 422 immediately, skipping remaining recipients. This is correct behavior (a missing template is a configuration error, not a per-recipient issue), but the re-raise inside the loop is slightly misleading. Consider moving template existence validation before the loop. Non-blocking since the behavior is correct. 4. **No `player_id` in `send_templated_email` call**: The `EmailLog` model has a `player_id` field (nullable), but the blast endpoint only passes `parent_id`. For grouped-by-parent emails this is reasonable (one email per parent, multiple players), but means email logs won't link to specific players. Matches the "per-parent" design intent. ### SOP COMPLIANCE - [x] Branch named after issue: `295-admin-blast-endpoint` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references parent issue: "Closes #295" and dependency chain (#296, #297, #298) documented - [ ] Related references plan slug: No plan slug referenced. If this work is tracked under a plan, it should be noted. - [x] No secrets committed: No credentials, .env files, or tokens in the diff - [x] No scope creep: All changes directly serve the blast endpoint feature - [x] Commit messages: PR title is descriptive ("feat: add generic /email/blast endpoint with query registry") - [x] Tests exist: 15 tests, claimed passing (771 total, 9 pre-existing failures on main) ### PROCESS OBSERVATIONS - **Deployment frequency**: This PR completes the email blast pipeline (#296 MJML, #297 mkdir fix, #298 send_templated_email, #299 blast endpoint). Four PRs for one feature -- good decomposition, each PR is independently reviewable and mergeable. - **Change failure risk**: Low. The endpoint is additive (new route, new module). No existing code modified beyond import additions. The query registry pattern means future campaigns won't need endpoint changes. - **Documentation**: The docstrings on both the query function and the endpoint are thorough. The PR body documents the dependency chain clearly. - **Pre-existing test failures**: 9 failing jersey/checkout tests noted in Test Plan. These are tracked in #274 and should not block this PR. ### VERDICT: APPROVED

forgejo_admin merged commit fb24245827 into main 2026-04-03 19:44:52 +00:00

forgejo_admin deleted branch 295-admin-blast-endpoint 2026-04-03 19:44:52 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-03 19:44:54 +00:00

feat: add generic /email/blast endpoint with query registry (#299)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

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

forgejo_admin/basketball-api!299

No description provided.