feat: generic email blast with pluggable audience queries #461

Merged

forgejo_admin merged 3 commits from 456-email-blast-system into main 2026-04-12 20:43:20 +00:00

Conversation 2 Commits 3 Files changed 6 +583 -1

forgejo_admin commented 2026-04-12 20:14:31 +00:00

Owner

Copy link

Summary

Extends the generic email blast system with pluggable audience queries, Queens pink brand tokens, a reusable build_action_email builder, and query_params passthrough so new email types ship as config instead of code changes.

Changes

• src/basketball_api/brand.py -- Added COLOR_PINK and COLOR_PINK_HOVER Queens design tokens alongside existing Kings red tokens
• src/basketball_api/models.py -- Extended EmailType enum with tournament_fee and payment_request values
• src/basketball_api/services/email.py -- Added build_action_email() function that renders brand-aware inline-style HTML (Kings red for boys, Queens pink for girls)
• src/basketball_api/services/email_queries.py -- Added query_tournament_committed() query that returns signed players on specified team_ids, with team_gender for brand-aware rendering
• src/basketball_api/routes/admin.py -- Added query_params field to BlastRequest and wired it through to query functions via inspect.signature detection
• alembic/versions/039_add_tournament_fee_email_types.py -- Migration to add new enum values to PostgreSQL
• tests/test_tournament_blast.py -- 14 tests covering query, builder, and endpoint integration

Test Plan

• pytest tests/test_tournament_blast.py -v -- 14 tests pass (query unit tests, build_action_email unit tests, blast endpoint integration)
• pytest tests/test_email_blast.py -v -- 25 existing tests pass (no regressions)
• ruff check and ruff format clean

Review Checklist

• No unrelated changes
• Tests pass locally
• Ruff format and check clean
• Migration slot 039 is next after 038

Related Notes

• None

Related

Closes #456

## Summary Extends the generic email blast system with pluggable audience queries, Queens pink brand tokens, a reusable `build_action_email` builder, and `query_params` passthrough so new email types ship as config instead of code changes. ## Changes - `src/basketball_api/brand.py` -- Added `COLOR_PINK` and `COLOR_PINK_HOVER` Queens design tokens alongside existing Kings red tokens - `src/basketball_api/models.py` -- Extended `EmailType` enum with `tournament_fee` and `payment_request` values - `src/basketball_api/services/email.py` -- Added `build_action_email()` function that renders brand-aware inline-style HTML (Kings red for boys, Queens pink for girls) - `src/basketball_api/services/email_queries.py` -- Added `query_tournament_committed()` query that returns signed players on specified `team_ids`, with `team_gender` for brand-aware rendering - `src/basketball_api/routes/admin.py` -- Added `query_params` field to `BlastRequest` and wired it through to query functions via `inspect.signature` detection - `alembic/versions/039_add_tournament_fee_email_types.py` -- Migration to add new enum values to PostgreSQL - `tests/test_tournament_blast.py` -- 14 tests covering query, builder, and endpoint integration ## Test Plan - `pytest tests/test_tournament_blast.py -v` -- 14 tests pass (query unit tests, build_action_email unit tests, blast endpoint integration) - `pytest tests/test_email_blast.py -v` -- 25 existing tests pass (no regressions) - `ruff check` and `ruff format` clean ## Review Checklist - [x] No unrelated changes - [x] Tests pass locally - [x] Ruff format and check clean - [x] Migration slot 039 is next after 038 ## Related Notes - None ## Related Closes #456

forgejo_admin added 1 commit 2026-04-12 20:14:31 +00:00

feat: add tournament_committed query, build_action_email, Queens pink tokens, and query_params support ... 7ce11ee1fc

Extends the generic email blast system with pluggable audience queries
so new email types ship as config instead of code changes.

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

forgejo_admin commented 2026-04-12 20:15:23 +00:00

Author

Owner

Copy link

QA Review -- PR #461

Summary

Extends the generic email blast system with tournament_committed query, build_action_email builder, Queens pink brand tokens, query_params passthrough, and new EmailType enum values. 14 new tests, 25 existing tests confirmed passing.

Findings

Nit -- import inspect inside function body (admin.py L1190)
The import inspect is inside the admin_email_blast endpoint handler. Should be at module top level. Not a blocker (Python caches imports), but not idiomatic for this codebase.

Nit -- accent_hover assigned but unused (email.py build_action_email)
The # noqa: F841 suppresses the lint warning, but the variable is genuinely unused. Inline CSS in emails cannot use :hover pseudo-selectors, so the hover color serves no purpose here. Consider removing the variable and the import of COLOR_PINK_HOVER/COLOR_RED_HOVER rather than suppressing the warning.

Note -- build_action_email not yet wired into blast endpoint
The function is defined and tested but the blast endpoint uses send_templated_email with MJML templates. This is fine per issue spec (building block for downstream #457), just noting for awareness.

VERDICT: APPROVED (with nits)

All acceptance criteria met. No regressions. Migration slot correct. Brand tokens properly extracted. Tests thorough.

## QA Review -- PR #461 ### Summary Extends the generic email blast system with `tournament_committed` query, `build_action_email` builder, Queens pink brand tokens, `query_params` passthrough, and new `EmailType` enum values. 14 new tests, 25 existing tests confirmed passing. ### Findings **Nit -- `import inspect` inside function body** (admin.py L1190) The `import inspect` is inside the `admin_email_blast` endpoint handler. Should be at module top level. Not a blocker (Python caches imports), but not idiomatic for this codebase. **Nit -- `accent_hover` assigned but unused** (email.py `build_action_email`) The `# noqa: F841` suppresses the lint warning, but the variable is genuinely unused. Inline CSS in emails cannot use `:hover` pseudo-selectors, so the hover color serves no purpose here. Consider removing the variable and the import of `COLOR_PINK_HOVER`/`COLOR_RED_HOVER` rather than suppressing the warning. **Note -- `build_action_email` not yet wired into blast endpoint** The function is defined and tested but the blast endpoint uses `send_templated_email` with MJML templates. This is fine per issue spec (building block for downstream #457), just noting for awareness. ### VERDICT: APPROVED (with nits) All acceptance criteria met. No regressions. Migration slot correct. Brand tokens properly extracted. Tests thorough.

forgejo_admin referenced this pull request from a commit 2026-04-12 20:16:42 +00:00

fix: move inspect import to module level, remove unused accent_hover variable

forgejo_admin added 1 commit 2026-04-12 20:16:42 +00:00

fix: move inspect import to module level, remove unused accent_hover variable ... 44aa88a603

Addresses QA review nits from PR #461.

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

forgejo_admin commented 2026-04-12 20:20:35 +00:00

Author

Owner

Copy link

PR #461 Review

Branch: 456-email-blast-system -- follows {issue-number}-{kebab-case-purpose} convention.
Tech stack: Python / FastAPI / SQLAlchemy / Alembic
Files changed: 7 (585 additions, 1 deletion)

DOMAIN REVIEW

Python / FastAPI / SQLAlchemy analysis

1. email_queries.py -- query_tournament_committed(): Clean query pattern. Joins Player -> player_teams -> Team, filters by tenant_id, team_ids, and ContractStatus.signed. Uses joinedload for parent and teams to avoid N+1. The .distinct() is correct since joins can produce duplicates. Returns well-structured dicts with all required per-recipient fields (to, parent_id, parent_name, player_name, team_name, team_gender, checkout_url).

2. email_queries.py -- Registry pattern: QUERY_REGISTRY dict maps string names to callables. tournament_committed correctly registered. The 422 for unknown query names is handled by the existing blast endpoint code (not in this diff, but referenced).

3. routes/admin.py -- query_params passthrough via inspect.signature: This is a pragmatic backward-compatible approach. Existing query functions (unsigned_contracts, incomplete_profiles) lack the query_params parameter and won't receive it. New queries that need it declare it in their signature. The inspect module is stdlib, no new dependency.

4. services/email.py -- build_action_email(): Good structure. Uses escape() on headline, cta_text, cta_url, and footer_note (XSS protection). Reuses existing _brand_wrapper() for consistent email chrome. Brand-aware accent color selection (COLOR_PINK for girls, COLOR_RED for boys) is clean.

5. brand.py: COLOR_PINK and COLOR_PINK_HOVER added alongside existing Kings red tokens. Existing tokens untouched. Clean.

6. models.py: EmailType enum extended with tournament_fee and payment_request. Additive only. Existing values untouched.

7. Migration 039: Uses ADD VALUE IF NOT EXISTS which is safe for idempotent reruns. Downgrade is a no-op (correct -- PostgreSQL cannot remove enum values). Known issue: Migration slot is 039 but should be 046 per the user's note. Flagged but not blocking per instruction.

8. Test coverage (test_tournament_blast.py): 14 tests across three test classes:

   • TestQueryTournamentCommitted (6 tests): signed players returned, unsigned excluded, empty without team_ids, girls gender detection, test_email filtering, registry presence.
   • TestBuildActionEmail (6 tests): Kings red for boys, Queens pink for girls, headline/CTA presence, footer note included/omitted, XSS escaping of headline.
   • TestBlastWithQueryParams (2 tests): full integration with mock Gmail -- blast with test_email, blast without test_email.

Type hints: All function signatures have proper PEP 484 type hints. query_params: dict | None = None uses modern union syntax (Python 3.10+).

Docstrings: PEP 257 compliant. build_action_email has complete Args/Returns documentation. query_tournament_committed documents result dict fields.

BLOCKERS

None.

All new functionality has test coverage. No unvalidated user input reaches SQL (team_ids are used in .in_() which parameterizes). No secrets in code. No DRY violations in auth paths -- reuses existing require_admin dependency.

NITS

1. build_action_email body parameter is raw HTML (line ~280 in email.py diff): The body param is inserted without escape(), while headline, cta_text, cta_url, and footer_note are all escaped. The docstring documents this ("may contain <p> tags"), and the endpoint is admin-only behind require_admin, so this is not a security blocker. However, consider adding a docstring note explicitly stating that body is trusted HTML and callers must sanitize if sourcing from user input.

2. checkout_url defaults to empty string: In query_tournament_committed, checkout_url is always "" with a comment "populated by body.data override." The placeholder resolution happens in the blast endpoint's template rendering loop. This works but the coupling is implicit -- a caller who forgets to pass checkout_url in data will send emails with empty links. Consider raising a warning if checkout_url is empty after placeholder resolution.

3. test_footer_note_omitted_when_empty (line ~340 in test diff): The assertion assert "font-size: 12px" not in html or "text-align: center" in html is a weak check. Since "text-align: center" appears in the CTA div regardless of footer, this assertion is always true. A stronger check would be: assert html.count('font-size: 12px') == 0 or check that no <p> with font-size: 12px exists after the CTA.

4. Migration slot 039: As noted by the user, remote main has through 044 and PR #460 takes 045. This migration needs to be renumbered to 046 before merge. Acknowledged as a known issue being handled separately.

SOP COMPLIANCE

• Branch named after issue (456-email-blast-system)
• PR body has Summary, Changes, Test Plan, Related sections
• Related section references plan slug -- says "None" (no plan slug referenced, but this may be standalone kanban work)
• No secrets committed
• No unnecessary file changes -- all 7 files are directly related to the feature
• Commit message is descriptive (feat: generic email blast with pluggable audience queries)
• Ruff format and check reported clean

PROCESS OBSERVATIONS

• Deployment frequency: This PR is well-scoped -- one feature, one query, one builder, one migration, tests. Good for throughput.
• Change failure risk: Low. Additive changes only. No existing endpoints modified (the blast endpoint change is backward-compatible via inspect.signature). Migration uses IF NOT EXISTS.
• Test confidence: 14 new tests with unit + integration coverage. The weak assertion in test_footer_note_omitted_when_empty (nit #3) is minor.
• Migration coordination: The 039 vs 046 slot mismatch will cause a merge conflict or Alembic chain break if not renumbered before merge. This is the only merge risk.

VERDICT: APPROVED

Clean, well-tested, additive feature with no blockers. The migration slot renumbering (039 -> 046) must happen before merge but is being tracked separately per the user's note. Nits are non-blocking quality suggestions.

## PR #461 Review **Branch**: `456-email-blast-system` -- follows `{issue-number}-{kebab-case-purpose}` convention. **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic **Files changed**: 7 (585 additions, 1 deletion) ### DOMAIN REVIEW **Python / FastAPI / SQLAlchemy analysis** 1. **`email_queries.py` -- `query_tournament_committed()`**: Clean query pattern. Joins `Player -> player_teams -> Team`, filters by `tenant_id`, `team_ids`, and `ContractStatus.signed`. Uses `joinedload` for `parent` and `teams` to avoid N+1. The `.distinct()` is correct since joins can produce duplicates. Returns well-structured dicts with all required per-recipient fields (`to`, `parent_id`, `parent_name`, `player_name`, `team_name`, `team_gender`, `checkout_url`). 2. **`email_queries.py` -- Registry pattern**: `QUERY_REGISTRY` dict maps string names to callables. `tournament_committed` correctly registered. The 422 for unknown query names is handled by the existing blast endpoint code (not in this diff, but referenced). 3. **`routes/admin.py` -- `query_params` passthrough via `inspect.signature`**: This is a pragmatic backward-compatible approach. Existing query functions (`unsigned_contracts`, `incomplete_profiles`) lack the `query_params` parameter and won't receive it. New queries that need it declare it in their signature. The `inspect` module is stdlib, no new dependency. 4. **`services/email.py` -- `build_action_email()`**: Good structure. Uses `escape()` on `headline`, `cta_text`, `cta_url`, and `footer_note` (XSS protection). Reuses existing `_brand_wrapper()` for consistent email chrome. Brand-aware accent color selection (`COLOR_PINK` for girls, `COLOR_RED` for boys) is clean. 5. **`brand.py`**: `COLOR_PINK` and `COLOR_PINK_HOVER` added alongside existing Kings red tokens. Existing tokens untouched. Clean. 6. **`models.py`**: `EmailType` enum extended with `tournament_fee` and `payment_request`. Additive only. Existing values untouched. 7. **Migration `039`**: Uses `ADD VALUE IF NOT EXISTS` which is safe for idempotent reruns. Downgrade is a no-op (correct -- PostgreSQL cannot remove enum values). **Known issue**: Migration slot is 039 but should be 046 per the user's note. Flagged but not blocking per instruction. 8. **Test coverage (`test_tournament_blast.py`)**: 14 tests across three test classes: - `TestQueryTournamentCommitted` (6 tests): signed players returned, unsigned excluded, empty without team_ids, girls gender detection, test_email filtering, registry presence. - `TestBuildActionEmail` (6 tests): Kings red for boys, Queens pink for girls, headline/CTA presence, footer note included/omitted, XSS escaping of headline. - `TestBlastWithQueryParams` (2 tests): full integration with mock Gmail -- blast with test_email, blast without test_email. **Type hints**: All function signatures have proper PEP 484 type hints. `query_params: dict | None = None` uses modern union syntax (Python 3.10+). **Docstrings**: PEP 257 compliant. `build_action_email` has complete Args/Returns documentation. `query_tournament_committed` documents result dict fields. ### BLOCKERS None. All new functionality has test coverage. No unvalidated user input reaches SQL (team_ids are used in `.in_()` which parameterizes). No secrets in code. No DRY violations in auth paths -- reuses existing `require_admin` dependency. ### NITS 1. **`build_action_email` `body` parameter is raw HTML** (line ~280 in email.py diff): The `body` param is inserted without `escape()`, while `headline`, `cta_text`, `cta_url`, and `footer_note` are all escaped. The docstring documents this ("may contain `<p>` tags"), and the endpoint is admin-only behind `require_admin`, so this is not a security blocker. However, consider adding a docstring note explicitly stating that `body` is trusted HTML and callers must sanitize if sourcing from user input. 2. **`checkout_url` defaults to empty string**: In `query_tournament_committed`, `checkout_url` is always `""` with a comment "populated by body.data override." The placeholder resolution happens in the blast endpoint's template rendering loop. This works but the coupling is implicit -- a caller who forgets to pass `checkout_url` in `data` will send emails with empty links. Consider raising a warning if `checkout_url` is empty after placeholder resolution. 3. **`test_footer_note_omitted_when_empty`** (line ~340 in test diff): The assertion `assert "font-size: 12px" not in html or "text-align: center" in html` is a weak check. Since `"text-align: center"` appears in the CTA div regardless of footer, this assertion is always true. A stronger check would be: `assert html.count('font-size: 12px') == 0` or check that no `<p>` with `font-size: 12px` exists after the CTA. 4. **Migration slot 039**: As noted by the user, remote main has through 044 and PR #460 takes 045. This migration needs to be renumbered to 046 before merge. Acknowledged as a known issue being handled separately. ### SOP COMPLIANCE - [x] Branch named after issue (`456-email-blast-system`) - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug -- says "None" (no plan slug referenced, but this may be standalone kanban work) - [x] No secrets committed - [x] No unnecessary file changes -- all 7 files are directly related to the feature - [x] Commit message is descriptive (`feat: generic email blast with pluggable audience queries`) - [x] Ruff format and check reported clean ### PROCESS OBSERVATIONS - **Deployment frequency**: This PR is well-scoped -- one feature, one query, one builder, one migration, tests. Good for throughput. - **Change failure risk**: Low. Additive changes only. No existing endpoints modified (the blast endpoint change is backward-compatible via `inspect.signature`). Migration uses `IF NOT EXISTS`. - **Test confidence**: 14 new tests with unit + integration coverage. The weak assertion in `test_footer_note_omitted_when_empty` (nit #3) is minor. - **Migration coordination**: The 039 vs 046 slot mismatch will cause a merge conflict or Alembic chain break if not renumbered before merge. This is the only merge risk. ### VERDICT: APPROVED Clean, well-tested, additive feature with no blockers. The migration slot renumbering (039 -> 046) must happen before merge but is being tracked separately per the user's note. Nits are non-blocking quality suggestions.

forgejo_admin referenced this pull request from a commit 2026-04-12 20:25:05 +00:00

fix: move inspect import to module level, remove unused accent_hover variable

forgejo_admin force-pushed 456-email-blast-system from 44aa88a603 to 68eb10f2e3 2026-04-12 20:25:05 +00:00 Compare

forgejo_admin merged commit db3f3dcf54 into main 2026-04-12 20:43:20 +00:00

forgejo_admin deleted branch 456-email-blast-system 2026-04-12 20:43:20 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-12 20:43:22 +00:00

feat: generic email blast with pluggable audience queries (#461)

forgejo_admin referenced this pull request from a commit 2026-04-12 20:49:32 +00:00

fix: update EmailType enum test to include new values

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!461

No description provided.