feat: admin email -- profile reminders + roster export

forgejo_admin commented

2026-03-18 19:53:21 +00:00

Author

Owner

PR #102 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Pydantic -- applying Python/FastAPI domain checklist.

What this PR does: Adds three new admin endpoints (GET /admin/players/incomplete, POST /admin/email/profile-reminder, POST /admin/email/roster-export), extends the EmailType enum with roster_export, makes EmailLog.parent_id and EmailLog.player_id nullable, adds a shared _brand_wrapper() HTML helper, and includes 21 new tests. Alembic migration 011 handles the schema changes.

Positive observations:

Clean separation: query helpers (_query_incomplete_players, _get_missing_fields) are extracted and reused between the incomplete-players endpoint and the reminder endpoint. Good DRY.
_brand_wrapper() is a good refactoring step -- shared HTML shell for new email types. The existing _build_confirmation_html() still has its own inline brand colors, but consolidating that is discovered scope, not this PR's job.
XSS protection: All user-supplied values are html.escape()-d before insertion into HTML email bodies. Both tenant_name, parent.name, player.name, and all roster fields go through escape().
Auth enforcement: All three endpoints use require_admin dependency. Auth denial tests exist for all three.
Migration uses IF NOT EXISTS for the enum addition -- idempotent. Downgrade has appropriate comments about PostgreSQL enum limitations.
Pydantic response models are well-defined for all endpoints.
Test coverage is thorough: 21 tests covering happy path, edge cases (all complete, all missing, multiple incomplete), error handling (send failure), email content verification, and email logging verification.
EmailLog for roster export correctly leaves parent_id and player_id as None -- tested explicitly.

Findings:

_query_incomplete_players asymmetry with photo_url -- The SQL filter checks Player.photo_url.is_(None) but does NOT check Player.photo_url == "", while the other three fields (current_school, height, position) check for both None and empty string. Meanwhile, _get_missing_fields uses not player.photo_url which catches both None and "". If a player has photo_url="" in the database, they would NOT be returned by the query but _get_missing_fields would flag photo_url as missing. This is a minor inconsistency -- in practice photo_url is likely always either a URL or None, never an empty string. But the pattern should be consistent. (NIT -- not a blocker since the query is conservative, not permissive.)
PlayerData = dict type alias is untyped (email.py line 11) -- This is dict with no key/value type hints. A TypedDict would provide actual type safety and document the expected shape of the roster data dictionaries. Currently the roster export route builds raw dicts and the email service accesses them with .get() -- correct but fragile. (NIT)
Roster export endpoint has no error handling -- admin_send_roster_export calls send_roster_export_email() without a try/except. If the Gmail API fails, the endpoint returns a raw 500. Compare with the profile-reminder endpoint which gracefully catches exceptions and returns them in an errors list. The asymmetry is notable -- one admin action fails gracefully, the other crashes. There is also no test for this failure path. (NIT -- admin-only endpoint, low blast radius, but worth noting.)
Profile reminder uses EmailType.reminder instead of a distinct type -- The reminder email is logged as EmailType.reminder, which is the same type used for other reminder emails. This makes it impossible to distinguish profile-completion reminders from other reminder types in the email_log table. A dedicated enum value like profile_reminder would improve auditability. (NIT)
DRY: tenant lookup pattern duplicated -- db.query(Tenant).filter(Tenant.slug == DEFAULT_TENANT_SLUG).first() with the "Tenant not configured" HTTPException appears twice in this PR (lines 388 and 428) and also exists in registration.py. This is a pre-existing pattern, but this PR adds two more copies. (NIT -- tracked in epilogue from PR #96 as pre-existing.)
Hardcoded "Westside Kings & Queens" in email subject (email.py line 346: f"Complete {player.name}'s Profile -- Westside Kings & Queens") -- The tenant name is available via tenant.name but the subject hardcodes the program name. The roster export subject similarly hardcodes "Westside Roster". Not a bug for a single-tenant deployment, but inconsistent with the _brand_wrapper which properly uses tenant.name. (NIT)
Migration downgrade sets FK values to 0 -- The downgrade sets parent_id = 0 and player_id = 0 for NULL rows. There is almost certainly no parent or player with id = 0, so this would violate FK constraints in practice. The comment acknowledges this ("will fail if FK constraint is enforced, but this is best-effort rollback") which is honest. Best-effort is acceptable for a downgrade path that is unlikely to be exercised in production. (NIT)

BLOCKERS

None. All new functionality has test coverage (21 tests). No unvalidated user input -- admin endpoints are auth-gated and user-supplied data in emails is HTML-escaped. No secrets or credentials in code. No DRY violations in auth/security paths.

NITS

Add Player.photo_url == "" to _query_incomplete_players for consistency with the other three field checks.
Replace PlayerData = dict with a TypedDict for type safety.
Add try/except to admin_send_roster_export for parity with the profile-reminder error handling pattern (and add a test for it).
Consider a profile_reminder enum value distinct from reminder for better email log auditability.
Use tenant.name in email subjects instead of hardcoding "Westside Kings & Queens" / "Westside Roster".
Extract the tenant-lookup-or-404 pattern into a shared helper.

SOP COMPLIANCE

Branch named after issue (101-admin-email-profile-completion-reminders matches issue #101)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related section references plan slug -- says "Closes #101" but does not reference plan-wkq
Tests exist (21 new tests, 368 total passing per PR body)
No secrets committed
No unnecessary file changes (5 files, all directly related)
Commit messages are descriptive

PROCESS OBSERVATIONS

Deployment frequency: Clean feature addition with proper migration. Low merge risk -- no conflicts, additive changes only.
Change failure risk: Low. Admin-only endpoints, well-tested, no changes to existing behavior. The nullable column migration is safe (additive, no data loss).
Documentation: PR body is thorough. Test plan includes both automated and manual verification steps (email rendering in Gmail, migration against live DB).
Epilogue items: Nits 1-6 above should be tracked in the plan-wkq epilogue if this PR is approved.

VERDICT: APPROVED

## PR #102 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Alembic / Pydantic -- applying Python/FastAPI domain checklist. **What this PR does:** Adds three new admin endpoints (`GET /admin/players/incomplete`, `POST /admin/email/profile-reminder`, `POST /admin/email/roster-export`), extends the `EmailType` enum with `roster_export`, makes `EmailLog.parent_id` and `EmailLog.player_id` nullable, adds a shared `_brand_wrapper()` HTML helper, and includes 21 new tests. Alembic migration 011 handles the schema changes. **Positive observations:** - Clean separation: query helpers (`_query_incomplete_players`, `_get_missing_fields`) are extracted and reused between the incomplete-players endpoint and the reminder endpoint. Good DRY. - `_brand_wrapper()` is a good refactoring step -- shared HTML shell for new email types. The existing `_build_confirmation_html()` still has its own inline brand colors, but consolidating that is discovered scope, not this PR's job. - XSS protection: All user-supplied values are `html.escape()`-d before insertion into HTML email bodies. Both `tenant_name`, `parent.name`, `player.name`, and all roster fields go through `escape()`. - Auth enforcement: All three endpoints use `require_admin` dependency. Auth denial tests exist for all three. - Migration uses `IF NOT EXISTS` for the enum addition -- idempotent. Downgrade has appropriate comments about PostgreSQL enum limitations. - Pydantic response models are well-defined for all endpoints. - Test coverage is thorough: 21 tests covering happy path, edge cases (all complete, all missing, multiple incomplete), error handling (send failure), email content verification, and email logging verification. - `EmailLog` for roster export correctly leaves `parent_id` and `player_id` as `None` -- tested explicitly. **Findings:** 1. **`_query_incomplete_players` asymmetry with `photo_url`** -- The SQL filter checks `Player.photo_url.is_(None)` but does NOT check `Player.photo_url == ""`, while the other three fields (`current_school`, `height`, `position`) check for both `None` and empty string. Meanwhile, `_get_missing_fields` uses `not player.photo_url` which catches both `None` and `""`. If a player has `photo_url=""` in the database, they would NOT be returned by the query but `_get_missing_fields` would flag `photo_url` as missing. This is a minor inconsistency -- in practice `photo_url` is likely always either a URL or `None`, never an empty string. But the pattern should be consistent. (NIT -- not a blocker since the query is conservative, not permissive.) 2. **`PlayerData = dict` type alias is untyped** (`email.py` line 11) -- This is `dict` with no key/value type hints. A `TypedDict` would provide actual type safety and document the expected shape of the roster data dictionaries. Currently the roster export route builds raw dicts and the email service accesses them with `.get()` -- correct but fragile. (NIT) 3. **Roster export endpoint has no error handling** -- `admin_send_roster_export` calls `send_roster_export_email()` without a try/except. If the Gmail API fails, the endpoint returns a raw 500. Compare with the profile-reminder endpoint which gracefully catches exceptions and returns them in an `errors` list. The asymmetry is notable -- one admin action fails gracefully, the other crashes. There is also no test for this failure path. (NIT -- admin-only endpoint, low blast radius, but worth noting.) 4. **Profile reminder uses `EmailType.reminder` instead of a distinct type** -- The reminder email is logged as `EmailType.reminder`, which is the same type used for other reminder emails. This makes it impossible to distinguish profile-completion reminders from other reminder types in the `email_log` table. A dedicated enum value like `profile_reminder` would improve auditability. (NIT) 5. **DRY: tenant lookup pattern duplicated** -- `db.query(Tenant).filter(Tenant.slug == DEFAULT_TENANT_SLUG).first()` with the "Tenant not configured" HTTPException appears twice in this PR (lines 388 and 428) and also exists in `registration.py`. This is a pre-existing pattern, but this PR adds two more copies. (NIT -- tracked in epilogue from PR #96 as pre-existing.) 6. **Hardcoded "Westside Kings & Queens" in email subject** (`email.py` line 346: `f"Complete {player.name}'s Profile -- Westside Kings & Queens"`) -- The tenant name is available via `tenant.name` but the subject hardcodes the program name. The roster export subject similarly hardcodes "Westside Roster". Not a bug for a single-tenant deployment, but inconsistent with the `_brand_wrapper` which properly uses `tenant.name`. (NIT) 7. **Migration downgrade sets FK values to 0** -- The downgrade sets `parent_id = 0` and `player_id = 0` for NULL rows. There is almost certainly no parent or player with `id = 0`, so this would violate FK constraints in practice. The comment acknowledges this ("will fail if FK constraint is enforced, but this is best-effort rollback") which is honest. Best-effort is acceptable for a downgrade path that is unlikely to be exercised in production. (NIT) ### BLOCKERS None. All new functionality has test coverage (21 tests). No unvalidated user input -- admin endpoints are auth-gated and user-supplied data in emails is HTML-escaped. No secrets or credentials in code. No DRY violations in auth/security paths. ### NITS 1. Add `Player.photo_url == ""` to `_query_incomplete_players` for consistency with the other three field checks. 2. Replace `PlayerData = dict` with a `TypedDict` for type safety. 3. Add try/except to `admin_send_roster_export` for parity with the profile-reminder error handling pattern (and add a test for it). 4. Consider a `profile_reminder` enum value distinct from `reminder` for better email log auditability. 5. Use `tenant.name` in email subjects instead of hardcoding "Westside Kings & Queens" / "Westside Roster". 6. Extract the tenant-lookup-or-404 pattern into a shared helper. ### SOP COMPLIANCE - [x] Branch named after issue (`101-admin-email-profile-completion-reminders` matches issue #101) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [ ] Related section references plan slug -- says "Closes #101" but does not reference `plan-wkq` - [x] Tests exist (21 new tests, 368 total passing per PR body) - [x] No secrets committed - [x] No unnecessary file changes (5 files, all directly related) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency:** Clean feature addition with proper migration. Low merge risk -- no conflicts, additive changes only. - **Change failure risk:** Low. Admin-only endpoints, well-tested, no changes to existing behavior. The nullable column migration is safe (additive, no data loss). - **Documentation:** PR body is thorough. Test plan includes both automated and manual verification steps (email rendering in Gmail, migration against live DB). - **Epilogue items:** Nits 1-6 above should be tracked in the plan-wkq epilogue if this PR is approved. ### VERDICT: APPROVED

2026-03-18 19:50:45 +00:00

Summary

Add three new admin endpoints: incomplete players query, branded profile reminder emails to parents, and full roster export email to the admin
Extend EmailType enum with roster_export and make EmailLog FK columns nullable for admin-targeted emails
21 new tests covering filtering, email content, error handling, and auth enforcement

Changes

src/basketball_api/models.py: Add roster_export to EmailType enum; make EmailLog.parent_id and EmailLog.player_id nullable
src/basketball_api/routes/admin.py: Add GET /admin/players/incomplete, POST /admin/email/profile-reminder, POST /admin/email/roster-export with shared query helpers
src/basketball_api/services/email.py: Add send_profile_reminder_email(), send_roster_export_email(), and _brand_wrapper() shared HTML helper
alembic/versions/011_add_roster_export_email_type.py: Migration for enum value and nullable column changes
tests/test_admin_email.py: 21 tests for all new endpoints and service functions

Test Plan

Tests pass locally -- 368/368 pass, 21 new tests added
pytest tests/ -k "incomplete or reminder or roster_export" -- 16 selected tests pass
ruff check and ruff format clean
Verify Alembic migration 011 applies against live database
Manual verification of email HTML rendering in Gmail

Review Checklist

No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #101

## Summary - Add three new admin endpoints: incomplete players query, branded profile reminder emails to parents, and full roster export email to the admin - Extend EmailType enum with `roster_export` and make EmailLog FK columns nullable for admin-targeted emails - 21 new tests covering filtering, email content, error handling, and auth enforcement ## Changes - `src/basketball_api/models.py`: Add `roster_export` to `EmailType` enum; make `EmailLog.parent_id` and `EmailLog.player_id` nullable - `src/basketball_api/routes/admin.py`: Add `GET /admin/players/incomplete`, `POST /admin/email/profile-reminder`, `POST /admin/email/roster-export` with shared query helpers - `src/basketball_api/services/email.py`: Add `send_profile_reminder_email()`, `send_roster_export_email()`, and `_brand_wrapper()` shared HTML helper - `alembic/versions/011_add_roster_export_email_type.py`: Migration for enum value and nullable column changes - `tests/test_admin_email.py`: 21 tests for all new endpoints and service functions ## Test Plan - [x] Tests pass locally -- 368/368 pass, 21 new tests added - [x] `pytest tests/ -k "incomplete or reminder or roster_export"` -- 16 selected tests pass - [x] `ruff check` and `ruff format` clean - [ ] Verify Alembic migration 011 applies against live database - [ ] Manual verification of email HTML rendering in Gmail ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #101

forgejo_admin added 1 commit

feat: admin email endpoints -- incomplete players, profile reminders, roster export

ci/woodpecker/pr/woodpecker Pipeline failed

Details

777d13e2de

Add three new admin endpoints:
- GET /admin/players/incomplete: query players with missing profile fields
- POST /admin/email/profile-reminder: send branded reminder emails to parents
- POST /admin/email/roster-export: send full roster table to admin email

Also adds roster_export to EmailType enum, makes EmailLog parent_id/player_id
nullable for admin-targeted emails, and includes Alembic migration 011.

Closes #101

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