forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 10 Pull requests 3 Projects Releases Packages Wiki Activity Actions

Phase 4e+4f: QA nit fixes + email log table #48

New issue
Closed
opened 2026-03-13 06:16:16 +00:00 by forgejo_admin · 0 comments
forgejo_admin commented 2026-03-13 06:16:16 +00:00
Owner
Copy link

Lineage

plan-2026-03-08-tryout-prep → Phase 4 → Phase 4e + 4f

Repo

forgejo_admin/basketball-api

User Story

As an admin, I want every email we send tracked in the database so I can see who received what and when — a basic CRM log.

As a developer, I want the QA nits from PR #47 addressed so the codebase stays clean.

Context

PR #47 implements Phase 4a-4d (roster, check-in, walk-ups). QA approved with 5 nits. This issue addresses the actionable nits and adds an email_log table for CRM tracking.

QA nits from PR #47 review:

  1. Duplicated Stripe link — register.py has a hardcoded STRIPE_TRYOUT_LINK constant, while config.py now has stripe_tryout_link setting. Should use the config setting everywhere. Remove the constant from register.py and import from config.
  2. Public coach roster — Intentional per plan decision. No fix needed. (ACKNOWLEDGED)
  3. No un-check-in endpoint — Add a POST endpoint to undo a check-in (set checked_in=False). Mistakes happen at the door.
  4. Admin dashboard queries all players — Should filter to only players who have a Registration record, not orphaned Player rows.
  5. Stripe payment link not a secret — Confirmed, it's a public checkout URL. No fix needed. (ACKNOWLEDGED)

Email log table: Currently we only have a confirmation_email_sent boolean on Registration. No history, no timestamps, no email type tracking. Need an append-only log.

File Targets

Files to create:

  • alembic/versions/XXX_add_email_log_table.py — new migration

Files to modify:

  • src/basketball_api/models.py — add EmailLog model and EmailType enum
  • src/basketball_api/services/email.py — update send_confirmation_email to write a row to email_log after successful send. Accept optional db: Session parameter. Log: parent_id, player_id, email_type, recipient_email, gmail_message_id, sent_at, tenant_id.
  • src/basketball_api/routes/register.py — remove STRIPE_TRYOUT_LINK constant, import settings.stripe_tryout_link from config instead
  • src/basketball_api/routes/tryouts.py — add un-check-in POST endpoint, fix admin query to filter by Registration join
  • tests/test_tryouts.py — add test for un-check-in endpoint, test for admin query filtering

Files NOT to touch:

  • src/basketball_api/routes/coach.py — no changes needed

Acceptance Criteria

Nit fixes:

  • STRIPE_TRYOUT_LINK constant removed from register.py. All references use settings.stripe_tryout_link from config.
  • POST /tryouts/admin/{tenant_slug}/uncheckin/{player_id} sets checked_in=False. Auth-gated admin. Returns redirect to admin page.
  • Admin dashboard (/tryouts/admin/{slug}) only shows players who have a Registration record (JOIN, not just all Players).
  • Existing tests still pass after Stripe link refactor.

Email log:

  • EmailType enum with values: registration, reminder (extensible later)
  • EmailLog model: id, tenant_id (FK), parent_id (FK), player_id (FK), email_type (EmailType enum), recipient_email (String), gmail_message_id (String, nullable), sent_at (DateTime, server_default now)
  • Alembic migration creates email_log table
  • send_confirmation_email accepts optional db: Session and email_type: EmailType parameters. When db is provided, writes a row to email_log after successful Gmail send. Default email_type is registration.
  • If db is not provided (backwards compat), email sends still work — just no logging.

Test Expectations

  • Test: un-check-in endpoint sets checked_in=False
  • Test: un-check-in requires admin auth
  • Test: admin dashboard only shows players with registrations
  • Test: Stripe link comes from config settings
  • Test: EmailLog model can be created and queried
  • Test: send_confirmation_email writes to email_log when db is provided
  • Run: pytest tests/ -v

Constraints

  • This should be done on the same branch as PR #47 (worktree-agent-a00d22f6) so it's one merge. If that's not practical, a new branch targeting main is fine — PR #47 can merge first.
  • Keep the confirmation_email_sent boolean on Registration for now — don't remove it. The email_log table supplements it; migration to remove can happen post-tryout.
  • The migration should be numbered after the existing ones (currently through 006).
  • Follow existing patterns in models.py for the new model.

Checklist

  • PR opened (or commits added to PR #47)
  • Tests pass
  • No unrelated changes

Related

  • project-westside-basketball
  • PR #47: #47
  • QA review comment on PR #47
### Lineage `plan-2026-03-08-tryout-prep` → Phase 4 → Phase 4e + 4f ### Repo `forgejo_admin/basketball-api` ### User Story As an **admin**, I want every email we send tracked in the database so I can see who received what and when — a basic CRM log. As a **developer**, I want the QA nits from PR #47 addressed so the codebase stays clean. ### Context PR #47 implements Phase 4a-4d (roster, check-in, walk-ups). QA approved with 5 nits. This issue addresses the actionable nits and adds an `email_log` table for CRM tracking. **QA nits from PR #47 review:** 1. **Duplicated Stripe link** — `register.py` has a hardcoded `STRIPE_TRYOUT_LINK` constant, while `config.py` now has `stripe_tryout_link` setting. Should use the config setting everywhere. Remove the constant from `register.py` and import from config. 2. **Public coach roster** — Intentional per plan decision. No fix needed. (ACKNOWLEDGED) 3. **No un-check-in endpoint** — Add a POST endpoint to undo a check-in (set `checked_in=False`). Mistakes happen at the door. 4. **Admin dashboard queries all players** — Should filter to only players who have a Registration record, not orphaned Player rows. 5. **Stripe payment link not a secret** — Confirmed, it's a public checkout URL. No fix needed. (ACKNOWLEDGED) **Email log table:** Currently we only have a `confirmation_email_sent` boolean on Registration. No history, no timestamps, no email type tracking. Need an append-only log. ### File Targets Files to create: - `alembic/versions/XXX_add_email_log_table.py` — new migration Files to modify: - `src/basketball_api/models.py` — add `EmailLog` model and `EmailType` enum - `src/basketball_api/services/email.py` — update `send_confirmation_email` to write a row to `email_log` after successful send. Accept optional `db: Session` parameter. Log: parent_id, player_id, email_type, recipient_email, gmail_message_id, sent_at, tenant_id. - `src/basketball_api/routes/register.py` — remove `STRIPE_TRYOUT_LINK` constant, import `settings.stripe_tryout_link` from config instead - `src/basketball_api/routes/tryouts.py` — add un-check-in POST endpoint, fix admin query to filter by Registration join - `tests/test_tryouts.py` — add test for un-check-in endpoint, test for admin query filtering Files NOT to touch: - `src/basketball_api/routes/coach.py` — no changes needed ### Acceptance Criteria **Nit fixes:** - [ ] `STRIPE_TRYOUT_LINK` constant removed from `register.py`. All references use `settings.stripe_tryout_link` from config. - [ ] `POST /tryouts/admin/{tenant_slug}/uncheckin/{player_id}` sets `checked_in=False`. Auth-gated admin. Returns redirect to admin page. - [ ] Admin dashboard (`/tryouts/admin/{slug}`) only shows players who have a Registration record (JOIN, not just all Players). - [ ] Existing tests still pass after Stripe link refactor. **Email log:** - [ ] `EmailType` enum with values: `registration`, `reminder` (extensible later) - [ ] `EmailLog` model: id, tenant_id (FK), parent_id (FK), player_id (FK), email_type (EmailType enum), recipient_email (String), gmail_message_id (String, nullable), sent_at (DateTime, server_default now) - [ ] Alembic migration creates `email_log` table - [ ] `send_confirmation_email` accepts optional `db: Session` and `email_type: EmailType` parameters. When db is provided, writes a row to email_log after successful Gmail send. Default email_type is `registration`. - [ ] If db is not provided (backwards compat), email sends still work — just no logging. ### Test Expectations - [ ] Test: un-check-in endpoint sets checked_in=False - [ ] Test: un-check-in requires admin auth - [ ] Test: admin dashboard only shows players with registrations - [ ] Test: Stripe link comes from config settings - [ ] Test: EmailLog model can be created and queried - [ ] Test: send_confirmation_email writes to email_log when db is provided - Run: `pytest tests/ -v` ### Constraints - This should be done on the **same branch as PR #47** (`worktree-agent-a00d22f6`) so it's one merge. If that's not practical, a new branch targeting main is fine — PR #47 can merge first. - Keep the `confirmation_email_sent` boolean on Registration for now — don't remove it. The email_log table supplements it; migration to remove can happen post-tryout. - The migration should be numbered after the existing ones (currently through 006). - Follow existing patterns in `models.py` for the new model. ### Checklist - [ ] PR opened (or commits added to PR #47) - [ ] Tests pass - [ ] No unrelated changes ### Related - `project-westside-basketball` - PR #47: https://forgejo.tail5b443a.ts.net/forgejo_admin/basketball-api/pulls/47 - QA review comment on PR #47
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
hotfix-029-division-enum
205-clean-test-data-from-production-db-playe
114-email-verification
147-feat-add-get-jersey-player-info-endpoint
124-feat-many-to-many-player-team-assignment
125-feat-add-new-players-manually-seydou-gou
126-feat-team-placement-congratulations-emai
121-admin-teams-spa-endpoints
70-add-graduating-class-to-roster-api-respo
59-fix-auth-test
62-phase-4l-sveltekit-dashboard-wkq-tailsca
11-phase-1a-fix-ci-pipeline-venv-in-test-st
No results found.
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
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#48
No description provided.