Fix: confirmation email skipped for existing Keycloak accounts #396

Merged

forgejo_admin merged 1 commit from 390-fix-confirmation-email-existing-keycloak into main 2026-04-08 21:24:00 +00:00

Conversation 2 Commits 1 Files changed 4 +252 -51

forgejo_admin commented 2026-04-08 21:17:15 +00:00

Owner

Copy link

Summary

Confirmation emails were not sent to parents who already had a Keycloak account because send_confirmation_email() was nested inside if keycloak_credentials: in both the promo and webhook registration paths. Additionally, the promo path never generated registration_token, causing token=None in profile URLs.

Changes

• src/basketball_api/routes/register.py — Added import secrets; generate registration_token for parents in the promo path (v2 endpoint) before waiver signing; moved send_confirmation_email() outside the if keycloak_credentials: guard so it fires unconditionally, passing credentials=keycloak_credentials (None for existing accounts, dict for new ones).
• src/basketball_api/routes/webhooks.py — Moved send_confirmation_email() outside the if keycloak_credentials: guard in the webhook checkout path, same pattern as register.py.
• tests/test_promo_registration.py — Added 3 tests: existing Keycloak sends email with credentials=None, new Keycloak includes credentials (no regression), and registration_token is generated (not None).
• tests/test_webhooks.py — Added 1 test: webhook path sends confirmation email when Keycloak account already exists.

Test Plan

• pytest tests/ -k "test_register or test_promo or test_webhook or test_keycloak" — 124 passed, 0 failures
• 4 new tests specifically cover the bug scenarios from the issue
• No ruff format or lint errors

Review Checklist

• ruff format and ruff check pass
• All relevant tests pass (124 passed)
• No unrelated changes
• Existing credential inclusion behavior preserved (no regression)

Related Notes

None.

Related

Closes #390

## Summary Confirmation emails were not sent to parents who already had a Keycloak account because `send_confirmation_email()` was nested inside `if keycloak_credentials:` in both the promo and webhook registration paths. Additionally, the promo path never generated `registration_token`, causing `token=None` in profile URLs. ## Changes - `src/basketball_api/routes/register.py` — Added `import secrets`; generate `registration_token` for parents in the promo path (v2 endpoint) before waiver signing; moved `send_confirmation_email()` outside the `if keycloak_credentials:` guard so it fires unconditionally, passing `credentials=keycloak_credentials` (None for existing accounts, dict for new ones). - `src/basketball_api/routes/webhooks.py` — Moved `send_confirmation_email()` outside the `if keycloak_credentials:` guard in the webhook checkout path, same pattern as register.py. - `tests/test_promo_registration.py` — Added 3 tests: existing Keycloak sends email with credentials=None, new Keycloak includes credentials (no regression), and registration_token is generated (not None). - `tests/test_webhooks.py` — Added 1 test: webhook path sends confirmation email when Keycloak account already exists. ## Test Plan - `pytest tests/ -k "test_register or test_promo or test_webhook or test_keycloak"` — 124 passed, 0 failures - 4 new tests specifically cover the bug scenarios from the issue - No ruff format or lint errors ## Review Checklist - [x] `ruff format` and `ruff check` pass - [x] All relevant tests pass (124 passed) - [x] No unrelated changes - [x] Existing credential inclusion behavior preserved (no regression) ## Related Notes None. ## Related Closes #390

forgejo_admin added 1 commit 2026-04-08 21:17:15 +00:00

fix: send confirmation email for existing Keycloak accounts (#390) ... 8efea6f1fc

Move send_confirmation_email() outside the `if keycloak_credentials:` guard
in both the promo registration path (register.py) and the webhook path
(webhooks.py) so parents with existing Keycloak accounts still receive
confirmation emails. Also generate registration_token in the promo path
to prevent token=None in profile URLs.

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

forgejo_admin commented 2026-04-08 21:20:02 +00:00

Author

Owner

Copy link

PR #396 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy

Bug fix correctness -- The root cause was correct: send_confirmation_email() was nested inside if keycloak_credentials: in both the promo (register.py) and webhook (webhooks.py) paths. Parents with existing Keycloak accounts got None from create_account_for_parent(), skipping the email entirely.

The fix is sound:

1. register.py -- keycloak_credentials is initialized to None at line 1387 before the Keycloak conditional, so it is always in scope when the email call fires. The email call is moved to fire unconditionally after the Keycloak block, passing credentials=keycloak_credentials (None for existing accounts, dict for new). The send_confirmation_email() function already handles credentials=None gracefully (line 96-103 in email.py -- cred_text is empty string).

2. register.py token fix -- registration_token generation via secrets.token_urlsafe(32) is guarded by if not parent.registration_token:, preventing overwrite for returning parents. This fixes the token=None profile URL bug. Placement before waiver signing is correct.

3. webhooks.py -- Same pattern: email call moved outside if keycloak_credentials: but kept inside if player and player.parent: and if tenant:. Scope is correct.

4. No regressions -- Card/Stripe payments still return early at line 1384 (return {"redirect_url": ...}) before reaching the email section, so only promo registrations hit this path. The webhook path handles card payments separately. The credentials kwarg is passed through, preserving behavior for new accounts.

send_confirmation_email signature compatibility -- Verified at /home/ldraney/basketball-api/src/basketball_api/services/email.py:65-72. The function accepts credentials: Credentials | None = None as a keyword argument. Passing credentials=None is the default behavior. No interface mismatch.

BLOCKERS

None.

• Test coverage: 4 new tests cover both paths (promo existing, promo new, token generation, webhook existing). Tests verify the exact bug scenario -- credentials=None is passed and mock_email.assert_called_once() confirms the email fires.
• No unvalidated user input changes.
• No secrets or credentials in code.
• No DRY violations in auth paths -- both register.py and webhooks.py follow the same pattern (email outside keycloak guard, credentials passed through).

NITS

1. Lazy import inside hot path -- from basketball_api.services.email import send_confirmation_email is imported inline at call sites in both register.py and webhooks.py rather than at module top. This is an existing pattern in the codebase (not introduced by this PR), so not a blocker, but worth noting for future cleanup.

2. Test setup duplication -- The three promo tests in TestConfirmationEmailExistingKeycloak each repeat the same Settings/monkeypatch boilerplate (lines 633-639, 662-668, 694-698). A shared fixture would reduce duplication. Minor -- the tests are readable as-is.

3. Hardcoded tryout details in email body -- email.py:119 has hardcoded date "Tuesday, March 24", time, and location. Not introduced by this PR, but this will need updating for future seasons.

SOP COMPLIANCE

• Branch named after issue: 390-fix-confirmation-email-existing-keycloak
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug -- N/A, this is a standalone bug fix (no plan)
• No secrets committed
• No unnecessary file changes (4 files, all directly related to the bug)
• Commit messages are descriptive (PR title matches issue)
• Closes #390 referenced in PR body

PROCESS OBSERVATIONS

Clean bug fix with strong test coverage. The fix addresses both code paths (promo + webhook) that shared the same bug pattern, which is good -- partial fixes would have left the webhook path broken. The 4 new tests directly map to the bug scenarios described in issue #390. Change failure risk is low given the narrow scope and the fact that send_confirmation_email already handled credentials=None.

VERDICT: APPROVED

## PR #396 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy **Bug fix correctness** -- The root cause was correct: `send_confirmation_email()` was nested inside `if keycloak_credentials:` in both the promo (register.py) and webhook (webhooks.py) paths. Parents with existing Keycloak accounts got `None` from `create_account_for_parent()`, skipping the email entirely. The fix is sound: 1. **register.py** -- `keycloak_credentials` is initialized to `None` at line 1387 before the Keycloak conditional, so it is always in scope when the email call fires. The email call is moved to fire unconditionally after the Keycloak block, passing `credentials=keycloak_credentials` (None for existing accounts, dict for new). The `send_confirmation_email()` function already handles `credentials=None` gracefully (line 96-103 in `email.py` -- `cred_text` is empty string). 2. **register.py token fix** -- `registration_token` generation via `secrets.token_urlsafe(32)` is guarded by `if not parent.registration_token:`, preventing overwrite for returning parents. This fixes the `token=None` profile URL bug. Placement before waiver signing is correct. 3. **webhooks.py** -- Same pattern: email call moved outside `if keycloak_credentials:` but kept inside `if player and player.parent:` and `if tenant:`. Scope is correct. 4. **No regressions** -- Card/Stripe payments still return early at line 1384 (`return {"redirect_url": ...}`) before reaching the email section, so only promo registrations hit this path. The webhook path handles card payments separately. The `credentials` kwarg is passed through, preserving behavior for new accounts. **`send_confirmation_email` signature compatibility** -- Verified at `/home/ldraney/basketball-api/src/basketball_api/services/email.py:65-72`. The function accepts `credentials: Credentials | None = None` as a keyword argument. Passing `credentials=None` is the default behavior. No interface mismatch. ### BLOCKERS None. - Test coverage: 4 new tests cover both paths (promo existing, promo new, token generation, webhook existing). Tests verify the exact bug scenario -- `credentials=None` is passed and `mock_email.assert_called_once()` confirms the email fires. - No unvalidated user input changes. - No secrets or credentials in code. - No DRY violations in auth paths -- both register.py and webhooks.py follow the same pattern (email outside keycloak guard, credentials passed through). ### NITS 1. **Lazy import inside hot path** -- `from basketball_api.services.email import send_confirmation_email` is imported inline at call sites in both register.py and webhooks.py rather than at module top. This is an existing pattern in the codebase (not introduced by this PR), so not a blocker, but worth noting for future cleanup. 2. **Test setup duplication** -- The three promo tests in `TestConfirmationEmailExistingKeycloak` each repeat the same Settings/monkeypatch boilerplate (lines 633-639, 662-668, 694-698). A shared fixture would reduce duplication. Minor -- the tests are readable as-is. 3. **Hardcoded tryout details in email body** -- `email.py:119` has hardcoded date "Tuesday, March 24", time, and location. Not introduced by this PR, but this will need updating for future seasons. ### SOP COMPLIANCE - [x] Branch named after issue: `390-fix-confirmation-email-existing-keycloak` - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug -- N/A, this is a standalone bug fix (no plan) - [x] No secrets committed - [x] No unnecessary file changes (4 files, all directly related to the bug) - [x] Commit messages are descriptive (PR title matches issue) - [x] Closes #390 referenced in PR body ### PROCESS OBSERVATIONS Clean bug fix with strong test coverage. The fix addresses both code paths (promo + webhook) that shared the same bug pattern, which is good -- partial fixes would have left the webhook path broken. The 4 new tests directly map to the bug scenarios described in issue #390. Change failure risk is low given the narrow scope and the fact that `send_confirmation_email` already handled `credentials=None`. ### VERDICT: APPROVED

forgejo_admin commented 2026-04-08 21:22:28 +00:00

Author

Owner

Copy link

PR #396 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / pytest

Bug fix correctness: The fix addresses two distinct bugs from #390:

1. Confirmation email gated behind if keycloak_credentials: -- Both register.py (promo path) and webhooks.py (Stripe path) had send_confirmation_email() nested inside a conditional that only fired when a new Keycloak account was created. Parents with existing accounts got no confirmation email. The fix correctly moves the email call outside the guard and passes credentials=keycloak_credentials (which is None for existing accounts, dict for new ones). The send_confirmation_email function signature already accepts credentials: Credentials | None = None, so this is safe.

2. registration_token was never generated in the promo path -- The promo v2 endpoint skipped token generation, resulting in token=None appearing literally in profile URLs (confirmed: email.py:92 interpolates parent.registration_token into f-strings). The fix generates a secrets.token_urlsafe(32) token when parent.registration_token is falsy. This is correct and uses the standard library's cryptographically secure token generator.

Third call site: src/basketball_api/services/registration.py:126 also calls send_confirmation_email but was already correct -- it calls unconditionally without a Keycloak guard and without passing credentials. No change needed there.

Pattern consistency: Both register.py and webhooks.py now follow the same pattern: try/except around send_confirmation_email() with credentials=keycloak_credentials, structured log message indicating whether credentials were included or omitted. Good symmetry.

Indentation in webhooks.py: The email block in webhooks.py is deeply nested (inside the Stripe event processing chain). The indentation looks correct relative to the surrounding try/except for Keycloak account creation.

BLOCKERS

None.

NITS

1. Inline import pattern -- Both paths use from basketball_api.services.email import send_confirmation_email inside the function body rather than at module top-level. This is a pre-existing pattern (not introduced by this PR), so not actionable here, but worth noting as tech debt.

2. confirmation_email_sent flag not set -- The registration.py path sets registration.confirmation_email_sent = True after sending. Neither register.py nor webhooks.py set this flag after sending the email. This is pre-existing behavior unchanged by this PR, but could cause confusion for any code that checks that field. Consider a follow-up ticket.

3. Log message consistency -- register.py logs "Sent confirmation email to %s (promo registration, credentials=%s)" while webhooks.py logs "Sent confirmation email to %s (credentials=%s)". Minor -- the promo path includes the registration type context, the webhook path does not. Not blocking.

SOP COMPLIANCE

• Branch named after issue: 390-fix-confirmation-email-existing-keycloak
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug -- no plan slug referenced (Related says "Closes #390" and "None" for notes). Acceptable for a standalone bug fix with no parent plan.
• No secrets committed -- test values use obvious fakes (test-password, whsec_test, tok_existing)
• No unrelated changes -- all 4 files are directly relevant to the bug
• Tests exist: 4 new tests covering both paths (existing Keycloak sends email, new Keycloak includes credentials, token generation, webhook path)

PROCESS OBSERVATIONS

• Clean bug fix with tight scope. Two related bugs fixed in one PR is appropriate since they share the same root cause (email logic gated behind Keycloak conditional).
• Test coverage is solid: tests verify both the happy path (new credentials included) and the bug scenario (existing account, credentials=None) for both registration paths. The token generation test verifies the second bug.
• Change failure risk is low -- the fix makes email sending more permissive (fires unconditionally instead of conditionally), and send_confirmation_email already handles credentials=None gracefully.

VERDICT: APPROVED

## PR #396 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / pytest **Bug fix correctness:** The fix addresses two distinct bugs from #390: 1. **Confirmation email gated behind `if keycloak_credentials:`** -- Both `register.py` (promo path) and `webhooks.py` (Stripe path) had `send_confirmation_email()` nested inside a conditional that only fired when a *new* Keycloak account was created. Parents with existing accounts got no confirmation email. The fix correctly moves the email call outside the guard and passes `credentials=keycloak_credentials` (which is `None` for existing accounts, dict for new ones). The `send_confirmation_email` function signature already accepts `credentials: Credentials | None = None`, so this is safe. 2. **`registration_token` was never generated in the promo path** -- The promo v2 endpoint skipped token generation, resulting in `token=None` appearing literally in profile URLs (confirmed: `email.py:92` interpolates `parent.registration_token` into f-strings). The fix generates a `secrets.token_urlsafe(32)` token when `parent.registration_token` is falsy. This is correct and uses the standard library's cryptographically secure token generator. **Third call site:** `src/basketball_api/services/registration.py:126` also calls `send_confirmation_email` but was already correct -- it calls unconditionally without a Keycloak guard and without passing `credentials`. No change needed there. **Pattern consistency:** Both `register.py` and `webhooks.py` now follow the same pattern: try/except around `send_confirmation_email()` with `credentials=keycloak_credentials`, structured log message indicating whether credentials were included or omitted. Good symmetry. **Indentation in webhooks.py:** The email block in webhooks.py is deeply nested (inside the Stripe event processing chain). The indentation looks correct relative to the surrounding try/except for Keycloak account creation. ### BLOCKERS None. ### NITS 1. **Inline import pattern** -- Both paths use `from basketball_api.services.email import send_confirmation_email` inside the function body rather than at module top-level. This is a pre-existing pattern (not introduced by this PR), so not actionable here, but worth noting as tech debt. 2. **`confirmation_email_sent` flag not set** -- The `registration.py` path sets `registration.confirmation_email_sent = True` after sending. Neither `register.py` nor `webhooks.py` set this flag after sending the email. This is pre-existing behavior unchanged by this PR, but could cause confusion for any code that checks that field. Consider a follow-up ticket. 3. **Log message consistency** -- `register.py` logs `"Sent confirmation email to %s (promo registration, credentials=%s)"` while `webhooks.py` logs `"Sent confirmation email to %s (credentials=%s)"`. Minor -- the promo path includes the registration type context, the webhook path does not. Not blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `390-fix-confirmation-email-existing-keycloak` - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related references plan slug -- no plan slug referenced (Related says "Closes #390" and "None" for notes). Acceptable for a standalone bug fix with no parent plan. - [x] No secrets committed -- test values use obvious fakes (`test-password`, `whsec_test`, `tok_existing`) - [x] No unrelated changes -- all 4 files are directly relevant to the bug - [x] Tests exist: 4 new tests covering both paths (existing Keycloak sends email, new Keycloak includes credentials, token generation, webhook path) ### PROCESS OBSERVATIONS - Clean bug fix with tight scope. Two related bugs fixed in one PR is appropriate since they share the same root cause (email logic gated behind Keycloak conditional). - Test coverage is solid: tests verify both the happy path (new credentials included) and the bug scenario (existing account, credentials=None) for both registration paths. The token generation test verifies the second bug. - Change failure risk is low -- the fix makes email sending more permissive (fires unconditionally instead of conditionally), and `send_confirmation_email` already handles `credentials=None` gracefully. ### VERDICT: APPROVED

forgejo_admin merged commit 0776b49afe into main 2026-04-08 21:24:00 +00:00

forgejo_admin deleted branch 390-fix-confirmation-email-existing-keycloak 2026-04-08 21:24:01 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-08 21:24:01 +00:00

Fix: confirmation email skipped for existing Keycloak accounts (#396)

forgejo_admin referenced this pull request 2026-04-08 22:02:06 +00:00

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

Sign in to join this conversation.

Reviewers

No reviewers

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

Subscribe

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!396

No description provided.