feat: QA nit fixes + email log table

forgejo_admin commented

2026-03-13 06:23:44 +00:00

Owner

Summary

Addresses QA nits from PR #47 review: deduplicate Stripe link, add un-check-in endpoint, fix admin dashboard to exclude orphan players
Adds email_log table for CRM email tracking with EmailType enum and EmailLog model

Changes

src/basketball_api/routes/register.py: Removed duplicated STRIPE_TRYOUT_LINK constant; all references now use settings.stripe_tryout_link from config
src/basketball_api/routes/tryouts.py: Added POST /tryouts/admin/{slug}/uncheckin/{player_id} endpoint; fixed admin dashboard query to JOIN on Registration (excludes orphan Players); added Undo button with JS handler
src/basketball_api/models.py: Added EmailType enum (registration, reminder) and EmailLog model
src/basketball_api/services/email.py: Updated send_confirmation_email to accept optional db and email_type params; writes to email_log when db provided
alembic/versions/007_add_email_log_table.py: New migration creating the email_log table
tests/test_tryouts.py: Added tests for un-check-in, admin filtering, Stripe config, EmailLog CRUD, and email logging integration

Test Plan

Tests pass locally (pytest tests/ -v -- 115 passed)
ruff check and ruff format clean
Manual verification: check in a player, click Undo, confirm status reverts
Manual verification: admin dashboard excludes players without Registration records

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #48
plan-2026-03-08-tryout-prep
Depends on PR #47 (merge first -- this branch is based on PR #47's HEAD)

## Summary - Addresses QA nits from PR #47 review: deduplicate Stripe link, add un-check-in endpoint, fix admin dashboard to exclude orphan players - Adds `email_log` table for CRM email tracking with `EmailType` enum and `EmailLog` model ## Changes - `src/basketball_api/routes/register.py`: Removed duplicated `STRIPE_TRYOUT_LINK` constant; all references now use `settings.stripe_tryout_link` from config - `src/basketball_api/routes/tryouts.py`: Added `POST /tryouts/admin/{slug}/uncheckin/{player_id}` endpoint; fixed admin dashboard query to JOIN on Registration (excludes orphan Players); added Undo button with JS handler - `src/basketball_api/models.py`: Added `EmailType` enum (`registration`, `reminder`) and `EmailLog` model - `src/basketball_api/services/email.py`: Updated `send_confirmation_email` to accept optional `db` and `email_type` params; writes to `email_log` when db provided - `alembic/versions/007_add_email_log_table.py`: New migration creating the `email_log` table - `tests/test_tryouts.py`: Added tests for un-check-in, admin filtering, Stripe config, EmailLog CRUD, and email logging integration ## Test Plan - [x] Tests pass locally (`pytest tests/ -v` -- 115 passed) - [x] `ruff check` and `ruff format` clean - [ ] Manual verification: check in a player, click Undo, confirm status reverts - [ ] Manual verification: admin dashboard excludes players without Registration records ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #48 - `plan-2026-03-08-tryout-prep` - Depends on PR #47 (merge first -- this branch is based on PR #47's HEAD)

forgejo_admin added 3 commits

2026-03-13 06:23:44 +00:00

feat: tryout day -- roster, check-in, walk-ups (#46 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

b91cbf658f

Add tryout-day endpoints for coach roster (mobile cards with search/filter),
print-optimized roster table, admin check-in dashboard with live status,
idempotent tryout number assignment, and Stripe payment redirect.

Endpoints:
- GET /pay -> 302 to Stripe checkout
- POST /tryouts/admin/{slug}/assign-numbers (auth, idempotent)
- GET /tryouts/roster/{slug} (public, noindex, mobile-first cards)
- GET /tryouts/roster/{slug}/print (public, print-optimized table)
- GET /tryouts/admin/{slug} (auth, admin check-in dashboard)
- POST /tryouts/admin/{slug}/checkin/{id} (auth, mark checked in)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor: centralize STRIPE_TRYOUT_LINK in config, remove dead code

ci/woodpecker/pr/woodpecker Pipeline failed

Details

592680e9a6

Move Stripe tryout payment link to Settings.stripe_tryout_link so it
can be overridden via BASKETBALL_STRIPE_TRYOUT_LINK env var. Remove
unused require_coach_or_admin dependency from tryouts module.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

feat: QA nit fixes + email log table (#48 )

ci/woodpecker/pr/woodpecker Pipeline failed

Details

d78eabf159

- Remove duplicated STRIPE_TRYOUT_LINK from register.py, use config setting
- Add POST /tryouts/admin/{slug}/uncheckin/{id} to undo check-ins
- Fix admin dashboard to only show players with Registration records
- Add EmailType enum and EmailLog model for CRM email tracking
- Update send_confirmation_email to log to email_log when db provided
- Add Alembic migration 007 for email_log table
- Add tests for all new functionality

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented

2026-03-13 06:24:40 +00:00

Author

Owner

Self-Review: Passed

Review scope: All changes in this PR that go beyond PR #47's base (commit 592680e).

Findings: No issues. All acceptance criteria from issue #48 are met:

STRIPE_TRYOUT_LINK constant removed from register.py, replaced with settings.stripe_tryout_link
POST /tryouts/admin/{slug}/uncheckin/{player_id} endpoint added, auth-gated
Admin dashboard filters to players with Registration records (JOIN, not all Players)
EmailType enum with registration and reminder values
EmailLog model with all required fields
Alembic migration 007 creates email_log table
send_confirmation_email accepts optional db and email_type, logs when db provided
Backwards compatible -- no db = no logging, email still sends
All 115 tests pass, ruff clean
7 new tests cover un-check-in, admin filtering, Stripe config, EmailLog CRUD, and email logging integration

## Self-Review: Passed **Review scope:** All changes in this PR that go beyond PR #47's base (commit `592680e`). **Findings:** No issues. All acceptance criteria from issue #48 are met: - [x] `STRIPE_TRYOUT_LINK` constant removed from `register.py`, replaced with `settings.stripe_tryout_link` - [x] `POST /tryouts/admin/{slug}/uncheckin/{player_id}` endpoint added, auth-gated - [x] Admin dashboard filters to players with Registration records (JOIN, not all Players) - [x] `EmailType` enum with `registration` and `reminder` values - [x] `EmailLog` model with all required fields - [x] Alembic migration 007 creates `email_log` table - [x] `send_confirmation_email` accepts optional `db` and `email_type`, logs when db provided - [x] Backwards compatible -- no db = no logging, email still sends - [x] All 115 tests pass, ruff clean - [x] 7 new tests cover un-check-in, admin filtering, Stripe config, EmailLog CRUD, and email logging integration

forgejo_admin commented

2026-03-13 06:27:44 +00:00

Author

Owner

PR #49 Review

BLOCKERS

None.

NITS

_clean_all in tests/test_tryouts.py does not delete EmailLog rows. The function deletes Registration, Player, and Parent, but EmailLog has FK constraints to all three of those tables. This is not a problem today because the setup_db fixture drops and recreates all tables between tests, so _clean_all always runs against empty tables. But if the test infrastructure ever switches to transaction-based isolation (rollback instead of drop/create), _clean_all would fail with FK constraint violations. Consider adding db.query(EmailLog).delete() as the first line of _clean_all for defensive completeness.
Migration numbering: 007 follows e09c9e678004 (auto-generated hash). The chain is 005 -> e09c9e678004 -> 007. The mixing of sequential numbers (005, 007) with a hex hash (e09c9e678004) is a cosmetic inconsistency. Not blocking -- Alembic resolves the chain by down_revision, not by filename convention -- but worth noting for future migrations.
EmailLog relationships are unidirectional. The model defines relationship() to Tenant, Parent, and Player without back_populates, meaning you cannot navigate from those models to their email logs (e.g., parent.email_logs). This is fine for a log/audit table where you typically query EmailLog directly, but if reverse navigation is ever needed, back_populates and a corresponding Mapped[list["EmailLog"]] on the parent models would be required.

SOP COMPLIANCE

Branch named after issue: 48-qa-nits-email-log references issue #48
PR body has: ## Summary, ## Changes, ## Test Plan, ## Related
Related section references the plan slug: plan-2026-03-08-tryout-prep
Related section references issue: Closes #48
No secrets committed (no .env, sk_live, sk_test, whsec_ patterns found)
No unnecessary file changes -- all changes are scoped to the PR description
Tests exist: TestUncheckin, TestAdminDashboardFiltering, TestStripeLinkConfig, TestEmailLog with integration test
Stripe link deduplication confirmed: STRIPE_TRYOUT_LINK constant fully removed, all references use settings.stripe_tryout_link
Commit messages are descriptive

Code Quality Notes

Un-check-in endpoint (POST /tryouts/admin/{slug}/uncheckin/{player_id}): Properly auth-gated with require_admin_role, validates tenant ownership, returns consistent CheckinResponse model. Good.
Admin dashboard orphan filtering: Inner JOIN on Registration correctly excludes players without registration records. Test explicitly verifies this with an orphan player fixture.
Email logging: send_confirmation_email accepts optional db parameter -- backwards compatible. When provided, writes an EmailLog row with the Gmail message ID. Clean separation of concerns.
Migration 007: Creates emailtype enum and email_log table with proper FK constraints and indexes on tenant_id, parent_id, player_id. Downgrade correctly drops the table then the enum.
JavaScript Undo button: Client-side stat counter correctly decrements on uncheckin and re-renders the Check In button. No XSS risk -- player IDs are integers, tenant slug is escape()-d.

VERDICT: APPROVED

## PR #49 Review ### BLOCKERS None. ### NITS 1. **`_clean_all` in `tests/test_tryouts.py` does not delete `EmailLog` rows.** The function deletes `Registration`, `Player`, and `Parent`, but `EmailLog` has FK constraints to all three of those tables. This is not a problem today because the `setup_db` fixture drops and recreates all tables between tests, so `_clean_all` always runs against empty tables. But if the test infrastructure ever switches to transaction-based isolation (rollback instead of drop/create), `_clean_all` would fail with FK constraint violations. Consider adding `db.query(EmailLog).delete()` as the first line of `_clean_all` for defensive completeness. 2. **Migration numbering: 007 follows `e09c9e678004` (auto-generated hash).** The chain is `005 -> e09c9e678004 -> 007`. The mixing of sequential numbers (`005`, `007`) with a hex hash (`e09c9e678004`) is a cosmetic inconsistency. Not blocking -- Alembic resolves the chain by `down_revision`, not by filename convention -- but worth noting for future migrations. 3. **`EmailLog` relationships are unidirectional.** The model defines `relationship()` to `Tenant`, `Parent`, and `Player` without `back_populates`, meaning you cannot navigate from those models to their email logs (e.g., `parent.email_logs`). This is fine for a log/audit table where you typically query EmailLog directly, but if reverse navigation is ever needed, `back_populates` and a corresponding `Mapped[list["EmailLog"]]` on the parent models would be required. ### SOP COMPLIANCE - [x] Branch named after issue: `48-qa-nits-email-log` references issue #48 - [x] PR body has: ## Summary, ## Changes, ## Test Plan, ## Related - [x] Related section references the plan slug: `plan-2026-03-08-tryout-prep` - [x] Related section references issue: `Closes #48` - [x] No secrets committed (no `.env`, `sk_live`, `sk_test`, `whsec_` patterns found) - [x] No unnecessary file changes -- all changes are scoped to the PR description - [x] Tests exist: `TestUncheckin`, `TestAdminDashboardFiltering`, `TestStripeLinkConfig`, `TestEmailLog` with integration test - [x] Stripe link deduplication confirmed: `STRIPE_TRYOUT_LINK` constant fully removed, all references use `settings.stripe_tryout_link` - [x] Commit messages are descriptive ### Code Quality Notes - **Un-check-in endpoint** (`POST /tryouts/admin/{slug}/uncheckin/{player_id}`): Properly auth-gated with `require_admin_role`, validates tenant ownership, returns consistent `CheckinResponse` model. Good. - **Admin dashboard orphan filtering**: Inner `JOIN` on `Registration` correctly excludes players without registration records. Test explicitly verifies this with an orphan player fixture. - **Email logging**: `send_confirmation_email` accepts optional `db` parameter -- backwards compatible. When provided, writes an `EmailLog` row with the Gmail message ID. Clean separation of concerns. - **Migration 007**: Creates `emailtype` enum and `email_log` table with proper FK constraints and indexes on `tenant_id`, `parent_id`, `player_id`. Downgrade correctly drops the table then the enum. - **JavaScript Undo button**: Client-side stat counter correctly decrements on uncheckin and re-renders the Check In button. No XSS risk -- player IDs are integers, tenant slug is `escape()`-d. ### VERDICT: APPROVED

forgejo_admin force-pushed 48-qa-nits-email-log from d78eabf159

ci/woodpecker/pr/woodpecker Pipeline failed

Details

to 2031b3e206

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2026-03-13 06:33:33 +00:00

Compare