pal-e-docs

Review: Fix confirmation email skipped for existing Keycloak accounts + token=None in profile URL

review-891-2026-04-08 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Bug
  • [x] Lineage — Discovered during registration flow validation (2026-04-07)
  • [x] Repo — forgejo_admin/basketball-api
  • [x] What Broke — Two related issues described clearly (email skip + token=None)
  • [x] Repro Steps — 4 steps, reproducible
  • [x] Expected Behavior — Clear before/after
  • [x] Environment — Production, basketball-api, promo registration path
  • [x] File Targets — 2 files with line references
  • [x] Acceptance Criteria — 3 criteria
  • [x] Test Expectations — 2 unit tests + run command
  • [x] Constraints — credentials block handling documented
  • [x] Checklist — PR, tests, no unrelated changes
  • [x] Related — westside-basketball

Traceability

  • [x] story:WS-S22 label — "As a parent, I want to receive clear email communications with action links so that I never miss a deadline"
  • [x] story note verified — found in project-westside-basketball user-stories section (Parent stories list)
  • [x] arch:email label — email architecture component
  • [ ] arch note MISSING — [SCOPE] Create architecture note arch-email for the email component
  • [x] Forgejo issue — forgejo_admin/basketball-api#390, open

File Targets

  • [x] src/basketball_api/routes/register.py — verified: send_confirmation_email() is inside if keycloak_credentials: at line 1402. When create_account_for_parent() returns None for existing Keycloak accounts (keycloak.py:303), email is skipped entirely.
  • [x] src/basketball_api/services/registration.py — verified: registration_token generation at lines 77-78 only fires in the webhook/card path. The promo path in register.py never calls this function, so promo parents may have registration_token=None.
  • [x] src/basketball_api/services/email.py:92 — verified: f"...?token={parent.registration_token}" interpolates None as literal string "None".
  • [ ] src/basketball_api/routes/webhooks.py — MISSING from issue. Same if keycloak_credentials: guard at line 347. See Blast Radius.

Repo Placement

OK. Issue filed on basketball-api, all file targets are in basketball-api. Single repo fix.

Dependencies

No blocking dependencies found on the board. This is a standalone bug fix. Related done items: #623 (email Marcus on registration), #229 (Gmail OAuth persistence), #206 (base_url fix in email links). None block this work.

Acceptance Criteria

3 AC are testable by an agent. Test commands are real (pytest tests/ -k test_register). The 2 test expectations (unit tests) are concrete and verifiable. AC #3 ("When I register with card payment (webhook path), same behavior") implies the webhook path should also be fixed, but the File Targets section only lists the promo path. This inconsistency needs resolution — either add webhooks.py as a file target or remove AC #3.

Blast Radius

IMPORTANT: The webhook path in src/basketball_api/routes/webhooks.py:347 has the EXACT same bug pattern: if keycloak_credentials: guards send_confirmation_email(). If a card-paying parent already has a Keycloak account, the webhook path also skips the confirmation email. The issue body mentions this in AC #3 but does NOT include webhooks.py as a file target.
The registration_token=None issue does NOT affect the webhook path because that path goes through registration.py which generates the token at lines 77-78 before the email is sent.
Rollback is straightforward — single repo, no migrations, no schema changes.

Decomposition Assessment

3 file targets (after adding webhooks.py), 1 repo, 3 AC, 2 test expectations. All changes are tightly coupled (same bug pattern in two code paths + token generation). Estimated agent time: under 5 minutes. No decomposition needed. No independent subtasks worth parallelizing.

Recommendation

  • [BODY] Add file target: src/basketball_api/routes/webhooks.py — move send_confirmation_email() call outside if keycloak_credentials: block (line ~347). Same pattern as register.py fix.
  • [BODY] Clarify that when Keycloak account already exists, credentials param should be passed as None so email sends without credentials block.
  • [SCOPE] Create architecture note arch-email for the email component in pal-e-docs.