fix: coach Stripe retry + backfill scripts #40

Merged

forgejo_admin merged 4 commits from fix/coach-stripe-retry into main 2026-03-10 05:28:32 +00:00

Conversation 1 Commits 4 Files changed 6 +454 -21

forgejo_admin commented 2026-03-10 05:22:23 +00:00

Owner

Copy link

Summary

• Fix coach onboarding redirect loop when Stripe Connect account creation fails
• Add backfill scripts for importing 41 historical Stripe checkout sessions + sending registration emails

Changes

• src/basketball_api/routes/coach.py: GET /coach/onboard now detects agreement_signed + no stripe_connect_account_id and retries Stripe account creation instead of redirect-looping to status
• tests/test_coach.py: Updated existing test, added test for retry-creates-account path
• scripts/backfill_stripe.py: New script — imports Stripe checkout sessions into DB (idempotent, dry-run by default)
• scripts/send_registration_emails.py: New script — sends branded registration emails to un-emailed paid parents (dry-run by default, --test-email for preview)

Test Plan

• Coach with agreement_signed + no Stripe account visits /coach/onboard?token=... → Stripe account created → redirected to Stripe onboarding
• Marcus unblocked: https://basketball-api.tail5b443a.ts.net/coach/onboard?token=PZzOc6aDt5X5LqoUor0HhNkPI4XfxolkuTmY9qbRUtM
• Dry-run backfill script against production DB (verified: 39 created, 2 skipped)
• Test email send to Lucas before blast to real families

Review Checklist

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

Related

• Closes #20 — Phase 3b email blast
• plan-2026-03-08-tryout-prep — Phase 3b: Email blast (subphases 3b-iii + 3b-iv)

## Summary - Fix coach onboarding redirect loop when Stripe Connect account creation fails - Add backfill scripts for importing 41 historical Stripe checkout sessions + sending registration emails ## Changes - `src/basketball_api/routes/coach.py`: GET `/coach/onboard` now detects `agreement_signed` + no `stripe_connect_account_id` and retries Stripe account creation instead of redirect-looping to status - `tests/test_coach.py`: Updated existing test, added test for retry-creates-account path - `scripts/backfill_stripe.py`: New script — imports Stripe checkout sessions into DB (idempotent, dry-run by default) - `scripts/send_registration_emails.py`: New script — sends branded registration emails to un-emailed paid parents (dry-run by default, `--test-email` for preview) ## Test Plan - [ ] Coach with `agreement_signed` + no Stripe account visits `/coach/onboard?token=...` → Stripe account created → redirected to Stripe onboarding - [ ] Marcus unblocked: `https://basketball-api.tail5b443a.ts.net/coach/onboard?token=PZzOc6aDt5X5LqoUor0HhNkPI4XfxolkuTmY9qbRUtM` - [ ] Dry-run backfill script against production DB (verified: 39 created, 2 skipped) - [ ] Test email send to Lucas before blast to real families ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related - Closes #20 — Phase 3b email blast - `plan-2026-03-08-tryout-prep` — Phase 3b: Email blast (subphases 3b-iii + 3b-iv)

forgejo_admin added 2 commits 2026-03-10 05:22:23 +00:00

fix: retry Stripe Connect account creation on coach onboard revisit ... 73c3873ab4

When a coach signed the contractor agreement but Stripe Connect account
creation failed, the retry path was stuck in a redirect loop:
/coach/onboard → (already signed) → redirect to /status → "Retry Setup"
→ back to /coach/onboard → loop.

Now GET /coach/onboard detects agreement_signed + no stripe_connect_account_id
and retries creating the Stripe account before redirecting.

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

feat: add Stripe backfill + registration email blast scripts ... 901c5baf07

- scripts/backfill_stripe.py: imports historical Stripe checkout sessions
  into the DB (idempotent, dry-run by default)
- scripts/send_registration_emails.py: sends branded registration emails
  to all paid parents who haven't been emailed yet (dry-run by default,
  supports --test-email for preview)

Phase 3b-iii tooling for the 41 paid families backfill.

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

forgejo_admin commented 2026-03-10 05:24:00 +00:00

Author

Owner

Copy link

PR #40 Review

BLOCKERS

None. The core fix is correct and the scripts are well-structured with safe defaults.

NITS

1. File handle leak in scripts/backfill_stripe.py line 168.
   args.stripe_key = open(key_file).read().strip() -- should use a with statement or pathlib.Path(key_file).read_text().strip(). Not dangerous for a one-shot CLI script, but poor practice.

2. Unused import in scripts/backfill_stripe.py line 34.
   Base is imported from basketball_api.models but never used. Ruff would flag this.

3. Fragile email mutation in scripts/send_registration_emails.py lines 118-122.
   The --test-email path temporarily overwrites parent.email on a SQLAlchemy-tracked object, sends the email, then restores it. If send_confirmation_email (or anything it calls) triggers a session flush, the test address could be persisted to the DB. Currently safe because send_confirmation_email does not commit, but this is brittle. Safer approach: pass the email override directly or use db.expunge(parent) / work on a copy.

4. No tests for the two new scripts.
   scripts/backfill_stripe.py and scripts/send_registration_emails.py are non-trivial (221 and 148 lines respectively) with real business logic (backfill_session, get_unsent_parents). The backfill_session function and extract_custom_fields would benefit from unit tests -- they are pure-ish functions that could be tested without Stripe API access. Acceptable as operational scripts for a time-sensitive launch, but worth a follow-up.

5. Potential duplicate players across sessions for the same parent.
   In backfill_stripe.py, if two Stripe sessions exist for the same parent email but with different stripe_checkout_session_id values (e.g., a parent who paid for two kids), a new Player is always created. This is likely correct behavior for multi-child families, but if the same child appears in two sessions (e.g., a failed-then-retried checkout), a duplicate Player record will be created. The dedup is only on stripe_checkout_session_id, not on parent+player_name. Worth noting for the backfill operator.

6. Code duplication in coach.py.
   The Stripe account creation + redirect + error handling block (lines 158-172 in the new code) is nearly identical to the same block in signup_submit (lines 130-143) and accept_agreement (lines 220-233). Three copies of the same try/except pattern. A helper like _create_stripe_and_redirect(db, coach, token) would reduce this. Non-blocking.

SOP COMPLIANCE

• Branch named after issue number -- branch is fix/coach-stripe-retry, not 20-* or issue-20-*. This is a combined fix + feature PR so the descriptive name is understandable, but it does not follow the issue-number convention.
• PR body follows template -- has Summary, Changes, Test Plan, Related sections.
• Related references plan slug -- plan-2026-03-08-tryout-prep referenced.
• No secrets committed -- scripts read keys from env vars and file paths only. No hardcoded values.
• No .env files committed.
• Scope is reasonable -- coach fix + backfill scripts are tightly related to Phase 3b email blast (Issue #20).
• Tests exist for the coach.py fix -- two tests added (happy path redirect-to-status with existing Stripe account, and retry-creates-account path). Both properly mock Stripe calls and verify DB state.

VERDICT: APPROVED

The core fix in coach.py is correct and well-tested. When contractor_agreement_signed is true but stripe_connect_account_id is null, the route now retries Stripe account creation instead of redirecting to a status page that would bounce back -- breaking the loop. The error path gracefully renders a status page with an error message rather than crashing.

The backfill scripts are operationally sound with dry-run defaults, idempotent processing, and clear logging. The nits above are all non-blocking quality improvements that can be addressed in a follow-up.

## PR #40 Review ### BLOCKERS None. The core fix is correct and the scripts are well-structured with safe defaults. ### NITS 1. **File handle leak in `scripts/backfill_stripe.py` line 168.** `args.stripe_key = open(key_file).read().strip()` -- should use a `with` statement or `pathlib.Path(key_file).read_text().strip()`. Not dangerous for a one-shot CLI script, but poor practice. 2. **Unused import in `scripts/backfill_stripe.py` line 34.** `Base` is imported from `basketball_api.models` but never used. Ruff would flag this. 3. **Fragile email mutation in `scripts/send_registration_emails.py` lines 118-122.** The `--test-email` path temporarily overwrites `parent.email` on a SQLAlchemy-tracked object, sends the email, then restores it. If `send_confirmation_email` (or anything it calls) triggers a session flush, the test address could be persisted to the DB. Currently safe because `send_confirmation_email` does not commit, but this is brittle. Safer approach: pass the email override directly or use `db.expunge(parent)` / work on a copy. 4. **No tests for the two new scripts.** `scripts/backfill_stripe.py` and `scripts/send_registration_emails.py` are non-trivial (221 and 148 lines respectively) with real business logic (`backfill_session`, `get_unsent_parents`). The `backfill_session` function and `extract_custom_fields` would benefit from unit tests -- they are pure-ish functions that could be tested without Stripe API access. Acceptable as operational scripts for a time-sensitive launch, but worth a follow-up. 5. **Potential duplicate players across sessions for the same parent.** In `backfill_stripe.py`, if two Stripe sessions exist for the same parent email but with different `stripe_checkout_session_id` values (e.g., a parent who paid for two kids), a new Player is always created. This is likely correct behavior for multi-child families, but if the same child appears in two sessions (e.g., a failed-then-retried checkout), a duplicate Player record will be created. The dedup is only on `stripe_checkout_session_id`, not on parent+player_name. Worth noting for the backfill operator. 6. **Code duplication in coach.py.** The Stripe account creation + redirect + error handling block (lines 158-172 in the new code) is nearly identical to the same block in `signup_submit` (lines 130-143) and `accept_agreement` (lines 220-233). Three copies of the same try/except pattern. A helper like `_create_stripe_and_redirect(db, coach, token)` would reduce this. Non-blocking. ### SOP COMPLIANCE - [ ] Branch named after issue number -- branch is `fix/coach-stripe-retry`, not `20-*` or `issue-20-*`. This is a combined fix + feature PR so the descriptive name is understandable, but it does not follow the issue-number convention. - [x] PR body follows template -- has Summary, Changes, Test Plan, Related sections. - [x] Related references plan slug -- `plan-2026-03-08-tryout-prep` referenced. - [x] No secrets committed -- scripts read keys from env vars and file paths only. No hardcoded values. - [x] No .env files committed. - [x] Scope is reasonable -- coach fix + backfill scripts are tightly related to Phase 3b email blast (Issue #20). - [x] Tests exist for the coach.py fix -- two tests added (happy path redirect-to-status with existing Stripe account, and retry-creates-account path). Both properly mock Stripe calls and verify DB state. ### VERDICT: APPROVED The core fix in `coach.py` is correct and well-tested. When `contractor_agreement_signed` is true but `stripe_connect_account_id` is null, the route now retries Stripe account creation instead of redirecting to a status page that would bounce back -- breaking the loop. The error path gracefully renders a status page with an error message rather than crashing. The backfill scripts are operationally sound with dry-run defaults, idempotent processing, and clear logging. The nits above are all non-blocking quality improvements that can be addressed in a follow-up.

forgejo_admin added 1 commit 2026-03-10 05:25:21 +00:00

fix: address review nits — unused import, file handle leak ... 9e3901b4e1

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

forgejo_admin added 1 commit 2026-03-10 05:25:32 +00:00

chore: gitignore worktrees + playwright artifacts ... e8087b571e

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

forgejo_admin merged commit c953a347f5 into main 2026-03-10 05:28:32 +00:00

forgejo_admin deleted branch fix/coach-stripe-retry 2026-03-10 05:28:32 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-10 05:28:33 +00:00

fix: coach Stripe retry + backfill scripts (#40)

forgejo_admin referenced this pull request from a commit 2026-03-10 06:31:38 +00:00

chore: bump image to 7166559 (dedup fix + coach interstitial + retry)

forgejo_admin referenced this pull request 2026-03-10 06:31:56 +00:00

chore: bump image to 7166559 (dedup fix + coach interstitial + retry) #43

forgejo_admin referenced this pull request 2026-03-10 06:32:44 +00:00

chore: bump image to 7166559 (dedup fix + coach interstitial + retry) #43

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

No description provided.