feat: contract reminder email endpoint (#270) #271

Merged

forgejo_admin merged 2 commits from 270-contract-reminder-email into main 2026-03-31 01:07:57 +00:00

Conversation 2 Commits 2 Files changed 4 +707

forgejo_admin commented 2026-03-31 00:59:25 +00:00

Owner

Copy link

Summary

Add POST /admin/email/contract-reminder endpoint that sends branded reminder emails to parents of unsigned rostered players, grouping multiple unsigned players under one email per parent with individual contract sign links.

Changes

• src/basketball_api/routes/admin.py — New POST /email/contract-reminder endpoint with ?test_email param for test mode. Queries unsigned rostered players (contract_status != signed, has contract_token, on at least one team), groups by parent, sends one email per parent.
• src/basketball_api/services/email.py — New send_contract_reminder_email() function and _build_player_section() helper. Single-player emails get one CTA button; multi-player emails get a bullet list with individual CTA buttons. Uses load_email_template("contract-reminder") with plain-text fallback. Logs to email_log table.
• data/email-templates/contract-reminder.html — Standalone HTML email template with Westside dark branding (dark bg #0a0a0a, pink accents #e91e8c, white text). Placeholders: {{parent_name}}, {{player_section}}, {{deadline}}.
• tests/test_contract_reminder.py — 11 tests covering: single/multi player HTML builder, test email mode, send-to-all, no-match filtering, signed player exclusion, unrostered player exclusion, parent grouping, error handling, and contract URL format verification.

Test Plan

19 passed in 2.00s

tests/test_contract_reminder.py::TestBuildPlayerSection::test_single_player_has_cta_button PASSED
tests/test_contract_reminder.py::TestBuildPlayerSection::test_multiple_players_has_list PASSED
tests/test_contract_reminder.py::TestBuildPlayerSection::test_escapes_html_in_names PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_sends_to_test_email PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_sends_to_all_parents PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_no_match_for_test_email PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_skips_signed_players PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_skips_players_without_team PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_groups_multiple_players_per_parent PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_handles_send_failure_gracefully PASSED
tests/test_contract_reminder.py::TestContractReminderEndpoint::test_contract_url_uses_contract_token PASSED
tests/test_jersey_reminder.py (8 tests) — all passing, no regression

Review Checklist

• Follows existing jersey-reminder pattern in admin.py and email.py
• Groups unsigned players by parent (one email per parent)
• Contract URL uses contract_token not registration_token
• Filters: unsigned only, rostered only (has team), has contract_token
• ?test_email param for test mode
• Logs to email_log table with EmailType.reminder
• HTML template uses Westside dark branding with pink accents
• Plain-text fallback if template not found
• HTML escaping on player/team names
• ruff format + ruff check pass
• 11 new tests, 8 existing tests — all passing

Related Notes

None.

Related

Closes #270

## Summary Add `POST /admin/email/contract-reminder` endpoint that sends branded reminder emails to parents of unsigned rostered players, grouping multiple unsigned players under one email per parent with individual contract sign links. ## Changes - `src/basketball_api/routes/admin.py` — New `POST /email/contract-reminder` endpoint with `?test_email` param for test mode. Queries unsigned rostered players (contract_status != signed, has contract_token, on at least one team), groups by parent, sends one email per parent. - `src/basketball_api/services/email.py` — New `send_contract_reminder_email()` function and `_build_player_section()` helper. Single-player emails get one CTA button; multi-player emails get a bullet list with individual CTA buttons. Uses `load_email_template("contract-reminder")` with plain-text fallback. Logs to `email_log` table. - `data/email-templates/contract-reminder.html` — Standalone HTML email template with Westside dark branding (dark bg #0a0a0a, pink accents #e91e8c, white text). Placeholders: `{{parent_name}}`, `{{player_section}}`, `{{deadline}}`. - `tests/test_contract_reminder.py` — 11 tests covering: single/multi player HTML builder, test email mode, send-to-all, no-match filtering, signed player exclusion, unrostered player exclusion, parent grouping, error handling, and contract URL format verification. ## Test Plan ``` 19 passed in 2.00s tests/test_contract_reminder.py::TestBuildPlayerSection::test_single_player_has_cta_button PASSED tests/test_contract_reminder.py::TestBuildPlayerSection::test_multiple_players_has_list PASSED tests/test_contract_reminder.py::TestBuildPlayerSection::test_escapes_html_in_names PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_sends_to_test_email PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_sends_to_all_parents PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_no_match_for_test_email PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_skips_signed_players PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_skips_players_without_team PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_groups_multiple_players_per_parent PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_handles_send_failure_gracefully PASSED tests/test_contract_reminder.py::TestContractReminderEndpoint::test_contract_url_uses_contract_token PASSED tests/test_jersey_reminder.py (8 tests) — all passing, no regression ``` ## Review Checklist - [x] Follows existing jersey-reminder pattern in admin.py and email.py - [x] Groups unsigned players by parent (one email per parent) - [x] Contract URL uses `contract_token` not `registration_token` - [x] Filters: unsigned only, rostered only (has team), has contract_token - [x] `?test_email` param for test mode - [x] Logs to `email_log` table with `EmailType.reminder` - [x] HTML template uses Westside dark branding with pink accents - [x] Plain-text fallback if template not found - [x] HTML escaping on player/team names - [x] ruff format + ruff check pass - [x] 11 new tests, 8 existing tests — all passing ## Related Notes None. ## Related Closes #270

forgejo_admin added 1 commit 2026-03-31 00:59:25 +00:00

feat: add contract reminder email endpoint (#270) ... f8138d2a2d

Add POST /admin/email/contract-reminder that sends branded reminder
emails to parents of unsigned rostered players. Groups multiple
unsigned players under one email per parent with individual sign links.
Includes HTML template, test mode via ?test_email param, and 11 tests.

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

forgejo_admin added 1 commit 2026-03-31 01:00:31 +00:00

fix: add .distinct() to prevent duplicate players in multi-team scenario ... c8f0f1a2a5

Players on multiple teams would appear multiple times in the query
results due to the join on player_teams. Adding .distinct() prevents
duplicate emails for the same player.

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

forgejo_admin commented 2026-03-31 01:00:46 +00:00

Author

Owner

Copy link

Self-Review Findings

Bug Found and Fixed

Duplicate players in multi-team scenario -- The query joins on player_teams to filter rostered players, but if a player belongs to multiple teams, the join produces multiple rows for that player. Combined with joinedload(Player.teams), this would cause the same player to appear multiple times in the grouping dict, resulting in duplicate entries in the reminder email. Fixed by adding .distinct() to the query. Pushed in commit c8f0f1a.

Verified Correct

• Endpoint follows the jersey-reminder pattern exactly (route shape, response model, error handling)
• Parent grouping logic: one email per parent regardless of number of unsigned players
• Contract URL uses player.contract_token (not registration_token)
• Filtering: unsigned only (contract_status != signed), rostered only (join on player_teams), has token (contract_token.isnot(None))
• ?test_email param correctly filters after grouping
• HTML escaping on player/team names via escape()
• Email logging uses existing EmailType.reminder (no migration needed)
• Template uses Westside dark branding with pink accents (#e91e8c) as specified
• Plain-text fallback when template file not found
• 11 new tests, 8 existing jersey-reminder tests -- all 19 passing

No Issues Remaining

VERDICT: APPROVED

## Self-Review Findings ### Bug Found and Fixed **Duplicate players in multi-team scenario** -- The query joins on `player_teams` to filter rostered players, but if a player belongs to multiple teams, the join produces multiple rows for that player. Combined with `joinedload(Player.teams)`, this would cause the same player to appear multiple times in the grouping dict, resulting in duplicate entries in the reminder email. Fixed by adding `.distinct()` to the query. Pushed in commit c8f0f1a. ### Verified Correct - Endpoint follows the jersey-reminder pattern exactly (route shape, response model, error handling) - Parent grouping logic: one email per parent regardless of number of unsigned players - Contract URL uses `player.contract_token` (not `registration_token`) - Filtering: unsigned only (`contract_status != signed`), rostered only (join on `player_teams`), has token (`contract_token.isnot(None)`) - `?test_email` param correctly filters after grouping - HTML escaping on player/team names via `escape()` - Email logging uses existing `EmailType.reminder` (no migration needed) - Template uses Westside dark branding with pink accents (#e91e8c) as specified - Plain-text fallback when template file not found - 11 new tests, 8 existing jersey-reminder tests -- all 19 passing ### No Issues Remaining VERDICT: APPROVED

forgejo_admin commented 2026-03-31 01:04:10 +00:00

Author

Owner

Copy link

PR #271 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy 2.0+ / Gmail SDK

Correctness

1. Query logic is correct. The inner join on player_teams properly filters to rostered-only players. The contract_status != ContractStatus.signed filter correctly catches both none and offered statuses. The contract_token.isnot(None) filter is a sound belt-and-suspenders check since unregistered players likely lack tokens.

2. Parent grouping works. The parent_players dict keyed on parent_id correctly collapses multiple unsigned players per parent into one email. The test test_groups_multiple_players_per_parent validates this with 3 players across 2 teams.

3. Contract URL uses contract_token, not registration_token. Verified in code (f"{_CONTRACT_BASE_URL}/{player.contract_token}") and explicitly tested in test_contract_url_uses_contract_token.

4. test_email filtering works. Applied post-query on the parent email, matching the jersey-reminder pattern of filtering parents by email.

5. Email logging uses EmailType.reminder. Note: the jersey-reminder uses EmailType.announcement, and the profile-reminder uses EmailType.reminder. There is no dedicated contract_reminder enum value. Using reminder is acceptable but makes it harder to distinguish contract reminders from profile reminders in the email_log table. Consider adding a contract_reminder enum value in a follow-up.

Pattern compliance (vs. jersey-reminder)

The endpoint follows the jersey-reminder pattern closely:

• Same tenant lookup via DEFAULT_TENANT_SLUG
• Same test_email query param approach
• Same try/except error collection per parent
• Same send_*_email() function signature (tenant, parent, players, db)
• Same template loading with load_email_template() + plaintext fallback
• Same EmailLog entry + db.commit() pattern

One structural difference: the jersey-reminder queries Parents and iterates, while the contract-reminder queries Players then groups by parent. This is the correct approach given the additional filtering requirements (contract_status, contract_token, rostered).

SQLAlchemy

The joinedload(Player.teams) in the query overrides the model's default lazy="selectin" on the teams relationship. Combined with distinct(), this creates a potential ORM-level deduplication concern. In practice, SQLAlchemy 2.0's legacy Query.all() auto-uniquifies joinedload results, so this will function correctly. However, dropping joinedload(Player.teams) and relying on the default selectin strategy would be cleaner and avoid the interaction with DISTINCT entirely. The joinedload(Player.parent) is fine (many-to-one, no row multiplication). See nit below.

Security

• _build_player_section() correctly uses html.escape() on player_name, team_name, and contract_url. Good.
• parent_name is passed unescaped to load_email_template() via the {{parent_name}} placeholder (raw string replacement, no HTML escaping). This matches the existing jersey-reminder pattern where {{name}} is also unescaped. The risk is minimal (admin-only endpoint, parent names from managed DB, email HTML context), but it is worth noting as a pre-existing pattern gap.
• No SQL injection risk: all queries use SQLAlchemy ORM parameterization.
• No secrets or credentials in code.

Email template

• Well-formed HTML with inline styles (correct for email clients).
• Uses table-safe CSS properties (no flexbox/grid -- good).
• Dark branding matches spec: #0a0a0a background, #e91e8c pink accents, white text.
• Responsive: max-width: 600px container, viewport meta tag.
• & and — HTML entities used correctly.
• Fallback font stack is solid.

Test coverage

11 tests covering: single/multi player HTML builder, HTML escaping, test email mode, send-to-all, no-match, signed exclusion, unrostered exclusion, parent grouping, error handling, and contract URL format. This is thorough. Tests properly mock the Gmail client and template loader, use real DB fixtures via conftest, and verify both the response payload and the arguments passed to send_message.

BLOCKERS

None.

NITS

1. joinedload(Player.teams) is redundant and potentially risky with distinct(). The teams relationship already has lazy="selectin" in the model. Remove joinedload(Player.teams) from the query .options() and let the default selectin strategy handle it. Keep joinedload(Player.parent) since Parent has no eager-load default. This avoids any interaction between JOIN-based loading and DISTINCT.

2. _CONTRACT_DEADLINE = "April 2" is hardcoded. This will need updating for future seasons. Consider making it a query param or config value. Not blocking for this PR since the jersey-reminder has similar hardcoded values, but worth tracking.

3. _CONTRACT_BASE_URL is a hardcoded Tailscale URL. Same pattern as other URLs in the codebase, but ideally this would come from config/settings. Consistent with existing pattern, so not blocking.

4. EmailType.reminder is shared with profile reminders. If you need to query "all contract reminders sent" from the email_log later, you cannot distinguish them from profile reminders. A dedicated contract_reminder enum value would be cleaner. Low priority -- file as discovered scope if desired.

5. Unescaped parent_name in HTML template. The {{parent_name}} placeholder is injected via load_email_template() without HTML escaping, matching the existing {{name}} pattern in jersey-reminder. Pre-existing pattern gap, not a new regression. A future improvement would be to escape all template values by default.

SOP COMPLIANCE

• Branch named after issue: 270-contract-reminder-email follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Related sections present
• Related references plan slug: No plan slug referenced -- "Related Notes: None." This is acceptable if no plan exists for this work.
• No secrets committed
• Tests exist and pass (11 new, 8 existing -- all green per test plan)
• No scope creep -- all 4 files are directly related to the contract reminder feature
• Commit messages are descriptive

PROCESS OBSERVATIONS

• Clean, focused PR. 707 additions with zero deletions indicates pure additive work with no refactoring -- good for a new endpoint.
• Test coverage is above average for this codebase pattern. The HTML escaping test (test_escapes_html_in_names) and the contract token verification test show security awareness.
• The ContractReminderResponse model adds skipped_no_token field beyond what JerseyReminderResponse has -- good operational visibility.
• mergeable: false in the PR metadata may indicate a merge conflict that needs resolution before merge.

VERDICT: APPROVED

## PR #271 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy 2.0+ / Gmail SDK **Correctness** 1. **Query logic is correct.** The inner join on `player_teams` properly filters to rostered-only players. The `contract_status != ContractStatus.signed` filter correctly catches both `none` and `offered` statuses. The `contract_token.isnot(None)` filter is a sound belt-and-suspenders check since unregistered players likely lack tokens. 2. **Parent grouping works.** The `parent_players` dict keyed on `parent_id` correctly collapses multiple unsigned players per parent into one email. The test `test_groups_multiple_players_per_parent` validates this with 3 players across 2 teams. 3. **Contract URL uses `contract_token`, not `registration_token`.** Verified in code (`f"{_CONTRACT_BASE_URL}/{player.contract_token}"`) and explicitly tested in `test_contract_url_uses_contract_token`. 4. **`test_email` filtering works.** Applied post-query on the parent email, matching the jersey-reminder pattern of filtering parents by email. 5. **Email logging uses `EmailType.reminder`.** Note: the jersey-reminder uses `EmailType.announcement`, and the profile-reminder uses `EmailType.reminder`. There is no dedicated `contract_reminder` enum value. Using `reminder` is acceptable but makes it harder to distinguish contract reminders from profile reminders in the email_log table. Consider adding a `contract_reminder` enum value in a follow-up. **Pattern compliance (vs. jersey-reminder)** The endpoint follows the jersey-reminder pattern closely: - Same tenant lookup via `DEFAULT_TENANT_SLUG` - Same `test_email` query param approach - Same `try/except` error collection per parent - Same `send_*_email()` function signature (tenant, parent, players, db) - Same template loading with `load_email_template()` + plaintext fallback - Same `EmailLog` entry + `db.commit()` pattern One structural difference: the jersey-reminder queries Parents and iterates, while the contract-reminder queries Players then groups by parent. This is the correct approach given the additional filtering requirements (contract_status, contract_token, rostered). **SQLAlchemy** The `joinedload(Player.teams)` in the query overrides the model's default `lazy="selectin"` on the `teams` relationship. Combined with `distinct()`, this creates a potential ORM-level deduplication concern. In practice, SQLAlchemy 2.0's legacy `Query.all()` auto-uniquifies `joinedload` results, so this will function correctly. However, dropping `joinedload(Player.teams)` and relying on the default `selectin` strategy would be cleaner and avoid the interaction with `DISTINCT` entirely. The `joinedload(Player.parent)` is fine (many-to-one, no row multiplication). See nit below. **Security** - `_build_player_section()` correctly uses `html.escape()` on `player_name`, `team_name`, and `contract_url`. Good. - `parent_name` is passed unescaped to `load_email_template()` via the `{{parent_name}}` placeholder (raw string replacement, no HTML escaping). This matches the existing jersey-reminder pattern where `{{name}}` is also unescaped. The risk is minimal (admin-only endpoint, parent names from managed DB, email HTML context), but it is worth noting as a pre-existing pattern gap. - No SQL injection risk: all queries use SQLAlchemy ORM parameterization. - No secrets or credentials in code. **Email template** - Well-formed HTML with inline styles (correct for email clients). - Uses table-safe CSS properties (no flexbox/grid -- good). - Dark branding matches spec: `#0a0a0a` background, `#e91e8c` pink accents, white text. - Responsive: `max-width: 600px` container, viewport meta tag. - `&` and `—` HTML entities used correctly. - Fallback font stack is solid. **Test coverage** 11 tests covering: single/multi player HTML builder, HTML escaping, test email mode, send-to-all, no-match, signed exclusion, unrostered exclusion, parent grouping, error handling, and contract URL format. This is thorough. Tests properly mock the Gmail client and template loader, use real DB fixtures via conftest, and verify both the response payload and the arguments passed to `send_message`. ### BLOCKERS None. ### NITS 1. **`joinedload(Player.teams)` is redundant and potentially risky with `distinct()`.** The `teams` relationship already has `lazy="selectin"` in the model. Remove `joinedload(Player.teams)` from the query `.options()` and let the default selectin strategy handle it. Keep `joinedload(Player.parent)` since Parent has no eager-load default. This avoids any interaction between JOIN-based loading and `DISTINCT`. 2. **`_CONTRACT_DEADLINE = "April 2"` is hardcoded.** This will need updating for future seasons. Consider making it a query param or config value. Not blocking for this PR since the jersey-reminder has similar hardcoded values, but worth tracking. 3. **`_CONTRACT_BASE_URL` is a hardcoded Tailscale URL.** Same pattern as other URLs in the codebase, but ideally this would come from config/settings. Consistent with existing pattern, so not blocking. 4. **`EmailType.reminder` is shared with profile reminders.** If you need to query "all contract reminders sent" from the email_log later, you cannot distinguish them from profile reminders. A dedicated `contract_reminder` enum value would be cleaner. Low priority -- file as discovered scope if desired. 5. **Unescaped `parent_name` in HTML template.** The `{{parent_name}}` placeholder is injected via `load_email_template()` without HTML escaping, matching the existing `{{name}}` pattern in jersey-reminder. Pre-existing pattern gap, not a new regression. A future improvement would be to escape all template values by default. ### SOP COMPLIANCE - [x] Branch named after issue: `270-contract-reminder-email` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections present - [ ] Related references plan slug: No plan slug referenced -- "Related Notes: None." This is acceptable if no plan exists for this work. - [x] No secrets committed - [x] Tests exist and pass (11 new, 8 existing -- all green per test plan) - [x] No scope creep -- all 4 files are directly related to the contract reminder feature - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - Clean, focused PR. 707 additions with zero deletions indicates pure additive work with no refactoring -- good for a new endpoint. - Test coverage is above average for this codebase pattern. The HTML escaping test (`test_escapes_html_in_names`) and the contract token verification test show security awareness. - The `ContractReminderResponse` model adds `skipped_no_token` field beyond what `JerseyReminderResponse` has -- good operational visibility. - `mergeable: false` in the PR metadata may indicate a merge conflict that needs resolution before merge. ### VERDICT: APPROVED

forgejo_admin force-pushed 270-contract-reminder-email from c8f0f1a2a5 to b91aebb3dd 2026-03-31 01:05:39 +00:00 Compare

forgejo_admin merged commit 23902fe5fa into main 2026-03-31 01:07:57 +00:00

forgejo_admin deleted branch 270-contract-reminder-email 2026-03-31 01:07:57 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-31 01:07:58 +00:00

feat: contract reminder email endpoint (#270) (#271)

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

No description provided.