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

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

New issue
Closed
opened 2026-04-07 20:01:09 +00:00 by forgejo_admin · 4 comments
forgejo_admin commented 2026-04-07 20:01:09 +00:00
Owner
Copy link

Type

Bug

Lineage

Discovered during registration flow validation (2026-04-07) while testing #383 fix.

Repo

forgejo_admin/basketball-api

What Broke

Two related issues in the promo registration path:

  1. Confirmation email not sent for existing Keycloak accounts. send_confirmation_email() is nested inside if keycloak_credentials: in register.py:1402. If the Keycloak account already exists, create_account_for_parent() returns None, and no confirmation email is sent. Re-registering parents get nothing.

  2. token=None in profile URL. The plaintext email body includes register?token=None because the parent's registration_token was not generated before the email was built. The token generation happens in registration.py:77-78 but may not fire for existing parents in the promo path.

Repro Steps

  1. Register a player with a promo code using an email that already has a Keycloak account
  2. Registration succeeds, on-screen confirmation shows
  3. No confirmation email is sent to the parent
  4. Admin notification is sent (correctly)

Expected Behavior

  • Confirmation email should send for ALL successful registrations, not just when a new Keycloak account is created
  • Profile URL should always have a valid registration token

Environment

Production, basketball-api, promo registration path.

File Targets

  • src/basketball_api/routes/register.py — move send_confirmation_email() call outside the if keycloak_credentials: block (line ~1402). Call it unconditionally after successful registration. Pass credentials=keycloak_credentials (which will be None for existing accounts, causing the email to send without the login credentials block).
  • src/basketball_api/routes/webhooks.py — apply the same fix at line ~347. The webhook path has an identical if keycloak_credentials: guard around send_confirmation_email(). Move it outside the guard so webhook-completed registrations for existing Keycloak accounts also receive confirmation emails.
  • src/basketball_api/services/registration.py — ensure registration_token is generated for existing parents (line 77-78)

Acceptance Criteria

  • When I register with a promo code and Keycloak account already exists, confirmation email is still sent (without credentials block)
  • When I register with a promo code, profile URL in email has a valid token (not None)
  • When I register with card payment (webhook path), same behavior — email sent even if Keycloak account already exists
  • When Keycloak account is new, credentials are still included in the email (no regression)

Test Expectations

  • Unit test: promo registration with existing Keycloak account sends confirmation email
  • Unit test: confirmation email profile_url contains valid token
  • Unit test: webhook path with existing Keycloak account sends confirmation email
  • Run command: pytest tests/ -k test_register

Constraints

  • Don't break the credentials block — if Keycloak account is new, credentials should still be included in the email
  • If Keycloak account already exists, pass credentials=None to send_confirmation_email() so it sends without the login credentials block
  • send_confirmation_email() already handles credentials=None gracefully (the credentials parameter is optional)

Checklist

  • PR opened
  • Tests pass
  • No unrelated changes

Related

  • westside-basketball
### Type Bug ### Lineage Discovered during registration flow validation (2026-04-07) while testing #383 fix. ### Repo `forgejo_admin/basketball-api` ### What Broke Two related issues in the promo registration path: 1. **Confirmation email not sent for existing Keycloak accounts.** `send_confirmation_email()` is nested inside `if keycloak_credentials:` in `register.py:1402`. If the Keycloak account already exists, `create_account_for_parent()` returns None, and no confirmation email is sent. Re-registering parents get nothing. 2. **`token=None` in profile URL.** The plaintext email body includes `register?token=None` because the parent's `registration_token` was not generated before the email was built. The token generation happens in `registration.py:77-78` but may not fire for existing parents in the promo path. ### Repro Steps 1. Register a player with a promo code using an email that already has a Keycloak account 2. Registration succeeds, on-screen confirmation shows 3. No confirmation email is sent to the parent 4. Admin notification is sent (correctly) ### Expected Behavior - Confirmation email should send for ALL successful registrations, not just when a new Keycloak account is created - Profile URL should always have a valid registration token ### Environment Production, basketball-api, promo registration path. ### File Targets - `src/basketball_api/routes/register.py` — move `send_confirmation_email()` call outside the `if keycloak_credentials:` block (line ~1402). Call it unconditionally after successful registration. Pass `credentials=keycloak_credentials` (which will be None for existing accounts, causing the email to send without the login credentials block). - `src/basketball_api/routes/webhooks.py` — apply the same fix at line ~347. The webhook path has an identical `if keycloak_credentials:` guard around `send_confirmation_email()`. Move it outside the guard so webhook-completed registrations for existing Keycloak accounts also receive confirmation emails. - `src/basketball_api/services/registration.py` — ensure `registration_token` is generated for existing parents (line 77-78) ### Acceptance Criteria - [ ] When I register with a promo code and Keycloak account already exists, confirmation email is still sent (without credentials block) - [ ] When I register with a promo code, profile URL in email has a valid token (not None) - [ ] When I register with card payment (webhook path), same behavior — email sent even if Keycloak account already exists - [ ] When Keycloak account is new, credentials are still included in the email (no regression) ### Test Expectations - [ ] Unit test: promo registration with existing Keycloak account sends confirmation email - [ ] Unit test: confirmation email profile_url contains valid token - [ ] Unit test: webhook path with existing Keycloak account sends confirmation email - Run command: `pytest tests/ -k test_register` ### Constraints - Don't break the credentials block — if Keycloak account is new, credentials should still be included in the email - If Keycloak account already exists, pass `credentials=None` to `send_confirmation_email()` so it sends without the login credentials block - `send_confirmation_email()` already handles `credentials=None` gracefully (the credentials parameter is optional) ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes ### Related - `westside-basketball`
forgejo_admin commented 2026-04-08 20:58:27 +00:00
Author
Owner
Copy link

Scope Review: NEEDS_REFINEMENT

Review note: review-891-2026-04-08

Scope is solid with all bug template sections present and both file targets verified. Two issues found:

  • Blast radius: webhook path has same bug. webhooks.py:347 has identical if keycloak_credentials: guard around send_confirmation_email(). AC #3 implies this should be fixed but it's not listed as a file target. Add src/basketball_api/routes/webhooks.py as a file target.
  • Missing arch note. arch:email label is set but no arch-email note exists in pal-e-docs. Needs creation (separate scope item).
  • Minor: Clarify that existing-account path should pass credentials=None explicitly so email sends without the credentials block.
## Scope Review: NEEDS_REFINEMENT Review note: `review-891-2026-04-08` Scope is solid with all bug template sections present and both file targets verified. Two issues found: - **Blast radius: webhook path has same bug.** `webhooks.py:347` has identical `if keycloak_credentials:` guard around `send_confirmation_email()`. AC #3 implies this should be fixed but it's not listed as a file target. Add `src/basketball_api/routes/webhooks.py` as a file target. - **Missing arch note.** `arch:email` label is set but no `arch-email` note exists in pal-e-docs. Needs creation (separate scope item). - **Minor:** Clarify that existing-account path should pass `credentials=None` explicitly so email sends without the credentials block.
forgejo_admin commented 2026-04-08 21:02:56 +00:00
Author
Owner
Copy link

Scope refinement applied (review-891-2026-04-08):

  1. Added webhooks.py:~347 as file target — identical if keycloak_credentials: guard exists in the webhook path
  2. Clarified credentials=None behavior — explicitly noted that existing accounts pass None to skip the login credentials block
  3. Added AC #3 (webhook path) and AC #4 (no regression for new accounts)
  4. Added webhook path unit test to Test Expectations

Arch note gap (arch:email label with no backing note) tracked as separate backlog item.

**Scope refinement applied (review-891-2026-04-08):** 1. Added `webhooks.py:~347` as file target — identical `if keycloak_credentials:` guard exists in the webhook path 2. Clarified `credentials=None` behavior — explicitly noted that existing accounts pass None to skip the login credentials block 3. Added AC #3 (webhook path) and AC #4 (no regression for new accounts) 4. Added webhook path unit test to Test Expectations Arch note gap (`arch:email` label with no backing note) tracked as separate backlog item.
forgejo_admin commented 2026-04-08 21:04:43 +00:00
Author
Owner
Copy link

Scope Review: READY

Review note: review-891-2026-04-08
Re-review after refinements: all file targets verified (register.py:1402, webhooks.py:347, registration.py:77-78), template complete, traceability solid (story:WS-S22 confirmed, arch-email note tracked separately). 4 ACs are testable, blast radius checked (3rd call site in registration.py:126 is already correct). Single-repo, single-agent scope (~3 min).

## Scope Review: READY Review note: `review-891-2026-04-08` Re-review after refinements: all file targets verified (register.py:1402, webhooks.py:347, registration.py:77-78), template complete, traceability solid (story:WS-S22 confirmed, arch-email note tracked separately). 4 ACs are testable, blast radius checked (3rd call site in registration.py:126 is already correct). Single-repo, single-agent scope (~3 min).
forgejo_admin commented 2026-04-08 22:02:06 +00:00
Author
Owner
Copy link

Validation: PASS

Tiers executed: Tier 1 (local tests), Tier 3 (production health)
Validation note: validation-390-2026-04-08
7 checks: 7 PASS, 0 FAIL

Summary:

  • Local: 85 registration+webhook tests passed (11.72s)
  • CI: Pipeline #416 green (886 passed, 27 skipped)
  • Prod: Pod running (0 restarts), image 2d85242 includes PR #396 commit, API endpoints responding
  • Code review confirms both register.py and webhooks.py fixes applied correctly
## Validation: PASS Tiers executed: Tier 1 (local tests), Tier 3 (production health) Validation note: `validation-390-2026-04-08` 7 checks: 7 PASS, 0 FAIL **Summary:** - Local: 85 registration+webhook tests passed (11.72s) - CI: Pipeline #416 green (886 passed, 27 skipped) - Prod: Pod running (0 restarts), image `2d85242` includes PR #396 commit, API endpoints responding - Code review confirms both register.py and webhooks.py fixes applied correctly
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
510-grant-tournament-tables-to-ro-role
458-skip-proration-v2
458-skip-proration
466-cherry-pick-first-payment
458-merge-first-payment-blast
369-first-payment-email-blast
408-player-nickname
313-local-first-week-email
320-mjml-sponsor-outreach-template
270-contract-reminder-email
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#390
No description provided.