ldraney/basketball-api

Fork 0

feat: add single-player contract email endpoint #464

Merged

forgejo_admin merged 2 commits from 462-admin-contract-email-endpoint into main

2026-04-12 23:51:49 +00:00

forgejo_admin commented

2026-04-12 23:38:30 +00:00

Contributor

Summary

Adds POST /admin/email/contract/{player_id} to send a branded contract email to a single player's parent, reusing the existing send_contract_reminder_email service function with a 1-element player list.

Changes

src/basketball_api/routes/admin.py — New endpoint with SingleContractEmailResponse model. Supports ?test_email= query param to redirect sends. Returns 404 for missing player, 422 for missing contract_token.
tests/test_single_contract_email.py — 4 tests: success (200), player not found (404), no token (422), test_email redirect.

Test Plan

pytest tests/test_single_contract_email.py — 4/4 passing
pytest tests/test_contract_reminder.py — 12/12 passing (no regressions)
ruff check and ruff format clean

Review Checklist

Endpoint follows existing admin email route pattern
Reuses send_contract_reminder_email — no new email logic
404/422 error handling matches issue spec
test_email param temporarily swaps parent email, restores in finally block
All tests pass, ruff clean

Closes #462

None — no pal-e-docs notes affected.

## Summary Adds `POST /admin/email/contract/{player_id}` to send a branded contract email to a single player's parent, reusing the existing `send_contract_reminder_email` service function with a 1-element player list. ## Changes - `src/basketball_api/routes/admin.py` — New endpoint with `SingleContractEmailResponse` model. Supports `?test_email=` query param to redirect sends. Returns 404 for missing player, 422 for missing `contract_token`. - `tests/test_single_contract_email.py` — 4 tests: success (200), player not found (404), no token (422), test_email redirect. ## Test Plan - `pytest tests/test_single_contract_email.py` — 4/4 passing - `pytest tests/test_contract_reminder.py` — 12/12 passing (no regressions) - `ruff check` and `ruff format` clean ## Review Checklist - [x] Endpoint follows existing admin email route pattern - [x] Reuses `send_contract_reminder_email` — no new email logic - [x] 404/422 error handling matches issue spec - [x] `test_email` param temporarily swaps parent email, restores in `finally` block - [x] All tests pass, ruff clean ## Related - Closes #462 ## Related Notes None — no pal-e-docs notes affected.

forgejo_admin added 1 commit

2026-04-12 23:38:30 +00:00

feat: add POST /admin/email/contract/{player_id} endpoint

ci/woodpecker/pr/woodpecker Pipeline failed

Details

e6558abdf7

Sends a branded contract email to a single player's parent, reusing
the existing send_contract_reminder_email service with a 1-element
player list. Supports ?test_email= query param to redirect sends.

Returns 404 for missing player, 422 for missing contract_token.
Includes 4 tests covering success, 404, 422, and test_email redirect.

Closes #462

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

forgejo_admin added 1 commit

2026-04-12 23:41:00 +00:00

fix: expunge parent before test_email swap to prevent DB corruption

ci/woodpecker/pr/woodpecker Pipeline failed

Details

75c7cf4c68

The send_contract_reminder_email service commits an EmailLog entry,
which would flush the swapped test_email to the parent row. Expunging
the parent from the session before mutation prevents this.

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

forgejo_admin commented

2026-04-12 23:41:15 +00:00

Author

Contributor

QA Review -- PR #464

Self-Review Findings

Bug found and fixed (commit 75c7cf4): The test_email swap mutated the SQLAlchemy-tracked parent object. When send_contract_reminder_email calls db.commit() to persist the EmailLog entry (email.py line 1757), the swapped test email would be flushed to the parents table, corrupting the real parent email. The finally block restored the Python attribute but never committed, so the DB row would retain the test address.

Fix: Added db.expunge(parent) before the email swap so the parent object is detached from the session and the commit inside the service function does not flush the mutation.

Review Summary

Area	Status
Route pattern matches existing admin email endpoints	OK
Reuses `send_contract_reminder_email` with 1-element list	OK
404 for missing player, 422 for missing token/parent	OK
`test_email` param safely detaches parent before swap	OK (fixed)
Response model has all required fields	OK
`_CONTRACT_BASE_URL` duplicated from email.py	Acceptable (private constant)
4 tests cover 200, 404, 422, test_email redirect	OK
ruff format + check clean	OK

VERDICT: APPROVED

## QA Review -- PR #464 ### Self-Review Findings **Bug found and fixed (commit 75c7cf4):** The `test_email` swap mutated the SQLAlchemy-tracked `parent` object. When `send_contract_reminder_email` calls `db.commit()` to persist the `EmailLog` entry (email.py line 1757), the swapped test email would be flushed to the `parents` table, corrupting the real parent email. The `finally` block restored the Python attribute but never committed, so the DB row would retain the test address. **Fix:** Added `db.expunge(parent)` before the email swap so the parent object is detached from the session and the commit inside the service function does not flush the mutation. ### Review Summary | Area | Status | |------|--------| | Route pattern matches existing admin email endpoints | OK | | Reuses `send_contract_reminder_email` with 1-element list | OK | | 404 for missing player, 422 for missing token/parent | OK | | `test_email` param safely detaches parent before swap | OK (fixed) | | Response model has all required fields | OK | | `_CONTRACT_BASE_URL` duplicated from email.py | Acceptable (private constant) | | 4 tests cover 200, 404, 422, test_email redirect | OK | | ruff format + check clean | OK | **VERDICT: APPROVED**

forgejo_admin commented

2026-04-12 23:42:43 +00:00

Author

Contributor

PR #464 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Gmail SDK

Endpoint design -- Clean. Single-purpose POST /admin/email/contract/{player_id} with proper admin guard (require_admin), correct HTTP status codes (404/422/500), and a well-typed Pydantic response model. Follows existing admin endpoint patterns in this file.

db.expunge for test_email swap -- This is the most interesting design decision. The bulk endpoint (admin_send_contract_reminder) handles test_email differently: it filters to only parents matching that email. The new endpoint instead expunges the parent from the session and mutates parent.email in-memory, then restores it in a finally block. This approach works because expunge detaches the object from the session so db.commit() inside send_contract_reminder_email cannot flush the fake email to the database. The finally block restoring original_email is defensive but correct -- the object is detached, so the restore is a no-op from the DB perspective but keeps the in-memory object clean. Acceptable pattern.

Eager loading -- joinedload(Player.parent) and joinedload(Player.teams) matches the existing bulk endpoint pattern. Good -- avoids N+1 queries.

Error handling -- The endpoint does NOT wrap send_contract_reminder_email in a try/except. If Gmail fails, the exception will propagate as a 500. The bulk endpoint catches exceptions and collects them in an errors list. For a single-player endpoint, letting it propagate as 500 is acceptable -- the caller gets a clear failure signal. However, the error message will be a raw Python traceback via FastAPI's default handler rather than a structured JSON error. This is a nit, not a blocker.

BLOCKERS

None.

Test coverage: 4 tests covering success (200), player not found (404), no token (422), and test_email redirect. All four code paths are covered. The mocks target the correct layer (get_gmail_client and load_email_template).

No secrets in code: The hardcoded URL https://westside-contracts.tail5b443a.ts.net/contract is an internal Tailscale hostname, not a secret. It is already hardcoded in 3 other places in this codebase (see nit below).

No unvalidated input: player_id is typed as int (FastAPI validates), test_email is a query param used only to override the send target. No SQL injection or path traversal risk.

NITS

DRY: _CONTRACT_BASE_URL is now duplicated in 4 files. This PR adds a 4th copy at routes/admin.py line ~1135. The same constant already exists in:
- services/email.py:1612
- services/contract_offers.py:69
- services/email_queries.py:30
This should be a single constant imported from one canonical location. Not a blocker because it is not in an auth/security path, but it is real DRY debt. File a follow-up issue.
Missing parent email validation on test_email param. The endpoint accepts any string as test_email without validating it looks like an email address. If someone passes test_email=garbage, send_contract_reminder_email will attempt to send to "garbage" and fail at the Gmail layer. A lightweight EmailStr validation (or even just an @ check) would give a better 422 response. Minor.
No explicit exception handling around send_contract_reminder_email. As noted above, Gmail failures will surface as raw 500s. For consistency with the bulk endpoint's structured error reporting, consider wrapping in try/except and returning a structured error. Minor.
Test coverage gap: no test for missing parent. The endpoint has an explicit check if not parent: raise HTTPException(422, "Player has no parent") but no test exercises this path. Consider adding a fixture with an orphan player (no parent_id). Minor.

SOP COMPLIANCE

Branch named after issue: 462-admin-contract-email-endpoint
PR body follows template (Summary, Changes, Test Plan, Related)
Related references issue: Closes #462
Related references plan slug -- N/A (no plan slug; standalone ticket)
No secrets committed
No unnecessary file changes (2 files, both on-topic)
Commit messages descriptive

PROCESS OBSERVATIONS

The _CONTRACT_BASE_URL duplication (now 4 copies) is accumulating technical debt. A single follow-up ticket to centralize this constant would prevent future divergence if the contracts hostname ever changes.
Good reuse of the existing send_contract_reminder_email service function rather than writing new email logic. This keeps the email path consistent.
The different test_email semantics between the bulk endpoint (filter by matching parent) and the single endpoint (swap parent email) could confuse future maintainers. A brief docstring note explaining why the approach differs would help.

VERDICT: APPROVED

## PR #464 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Gmail SDK **Endpoint design** -- Clean. Single-purpose `POST /admin/email/contract/{player_id}` with proper admin guard (`require_admin`), correct HTTP status codes (404/422/500), and a well-typed Pydantic response model. Follows existing admin endpoint patterns in this file. **`db.expunge` for test_email swap** -- This is the most interesting design decision. The bulk endpoint (`admin_send_contract_reminder`) handles `test_email` differently: it filters to only parents matching that email. The new endpoint instead expunges the parent from the session and mutates `parent.email` in-memory, then restores it in a `finally` block. This approach works because `expunge` detaches the object from the session so `db.commit()` inside `send_contract_reminder_email` cannot flush the fake email to the database. The `finally` block restoring `original_email` is defensive but correct -- the object is detached, so the restore is a no-op from the DB perspective but keeps the in-memory object clean. Acceptable pattern. **Eager loading** -- `joinedload(Player.parent)` and `joinedload(Player.teams)` matches the existing bulk endpoint pattern. Good -- avoids N+1 queries. **Error handling** -- The endpoint does NOT wrap `send_contract_reminder_email` in a try/except. If Gmail fails, the exception will propagate as a 500. The bulk endpoint catches exceptions and collects them in an `errors` list. For a single-player endpoint, letting it propagate as 500 is acceptable -- the caller gets a clear failure signal. However, the error message will be a raw Python traceback via FastAPI's default handler rather than a structured JSON error. This is a nit, not a blocker. ### BLOCKERS None. **Test coverage**: 4 tests covering success (200), player not found (404), no token (422), and test_email redirect. All four code paths are covered. The mocks target the correct layer (`get_gmail_client` and `load_email_template`). **No secrets in code**: The hardcoded URL `https://westside-contracts.tail5b443a.ts.net/contract` is an internal Tailscale hostname, not a secret. It is already hardcoded in 3 other places in this codebase (see nit below). **No unvalidated input**: `player_id` is typed as `int` (FastAPI validates), `test_email` is a query param used only to override the send target. No SQL injection or path traversal risk. ### NITS 1. **DRY: `_CONTRACT_BASE_URL` is now duplicated in 4 files.** This PR adds a 4th copy at `routes/admin.py` line ~1135. The same constant already exists in: - `services/email.py:1612` - `services/contract_offers.py:69` - `services/email_queries.py:30` This should be a single constant imported from one canonical location. Not a blocker because it is not in an auth/security path, but it is real DRY debt. File a follow-up issue. 2. **Missing parent email validation on `test_email` param.** The endpoint accepts any string as `test_email` without validating it looks like an email address. If someone passes `test_email=garbage`, `send_contract_reminder_email` will attempt to send to "garbage" and fail at the Gmail layer. A lightweight `EmailStr` validation (or even just an `@` check) would give a better 422 response. Minor. 3. **No explicit exception handling around `send_contract_reminder_email`.** As noted above, Gmail failures will surface as raw 500s. For consistency with the bulk endpoint's structured error reporting, consider wrapping in try/except and returning a structured error. Minor. 4. **Test coverage gap: no test for missing parent.** The endpoint has an explicit check `if not parent: raise HTTPException(422, "Player has no parent")` but no test exercises this path. Consider adding a fixture with an orphan player (no `parent_id`). Minor. ### SOP COMPLIANCE - [x] Branch named after issue: `462-admin-contract-email-endpoint` - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [x] Related references issue: `Closes #462` - [ ] Related references plan slug -- N/A (no plan slug; standalone ticket) - [x] No secrets committed - [x] No unnecessary file changes (2 files, both on-topic) - [x] Commit messages descriptive ### PROCESS OBSERVATIONS - The `_CONTRACT_BASE_URL` duplication (now 4 copies) is accumulating technical debt. A single follow-up ticket to centralize this constant would prevent future divergence if the contracts hostname ever changes. - Good reuse of the existing `send_contract_reminder_email` service function rather than writing new email logic. This keeps the email path consistent. - The different `test_email` semantics between the bulk endpoint (filter by matching parent) and the single endpoint (swap parent email) could confuse future maintainers. A brief docstring note explaining *why* the approach differs would help. ### VERDICT: APPROVED