feat: save card registration as pending before Stripe redirect #111

Merged

forgejo_admin merged 3 commits from 110-feat-stripe-card-payment-saves-registrat into main 2026-03-19 01:38:34 +00:00

Conversation 3 Commits 3 Files changed 3 +197 -6

forgejo_admin commented 2026-03-19 01:23:50 +00:00

Contributor

Copy link

Summary

• Card payments now create a DB record (pending/stripe) before redirecting to Stripe Payment Link
• Webhook handler matches checkout.session.completed events to pending registrations by parent email
• Updates registration to paid with Stripe session/payment intent IDs stored

Changes

• routes/register.py: Moved card redirect after player/registration creation, sets pending/stripe
• routes/webhooks.py: New pending registration matching block before jersey checkout handler
• tests/test_promo_registration.py: Updated test comment for new behavior

Test Plan

• pytest tests/test_promo_registration.py -v passes
• Card registration creates pending record in DB
• Webhook updates pending → paid on checkout.session.completed

Review Checklist

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

Related

• Closes #110
• plan-wkq — Phase 1 (Commit Uncommitted Work)
• Note: Phase 4 will replace email-based webhook matching with metadata-based Checkout Sessions

## Summary - Card payments now create a DB record (`pending/stripe`) before redirecting to Stripe Payment Link - Webhook handler matches `checkout.session.completed` events to pending registrations by parent email - Updates registration to `paid` with Stripe session/payment intent IDs stored ## Changes - `routes/register.py`: Moved card redirect after player/registration creation, sets `pending/stripe` - `routes/webhooks.py`: New pending registration matching block before jersey checkout handler - `tests/test_promo_registration.py`: Updated test comment for new behavior ## Test Plan - [ ] `pytest tests/test_promo_registration.py -v` passes - [ ] Card registration creates pending record in DB - [ ] Webhook updates pending → paid on checkout.session.completed ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related - Closes #110 - `plan-wkq` — Phase 1 (Commit Uncommitted Work) - Note: Phase 4 will replace email-based webhook matching with metadata-based Checkout Sessions

forgejo_admin added 1 commit 2026-03-19 01:23:50 +00:00

feat: save card registration as pending before Stripe redirect (#110) ... 7bc77c4670

Card payments now create a DB record (pending/stripe) before redirecting
to the Stripe Payment Link. Webhook matches completed payments to pending
registrations by parent email and updates status to paid.

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

forgejo_admin commented 2026-03-19 01:26:18 +00:00

Author

Contributor

Copy link

PR #111 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Stripe webhooks

register.py changes -- The save-before-redirect logic is structurally correct. Card payments now flow through full player/registration creation with payment_status=pending and signup_method="stripe", then return the Stripe Payment Link URL. The placement after db.commit() at line 1273 ensures the record is persisted before the user is redirected. This is the right pattern.

webhooks.py changes -- The new pending-registration matching block at lines 198-229 queries for a pending registration by joining Registration -> Player -> Parent and matching on lowercased parent email. When found, it updates payment_status to paid and stores Stripe session/payment IDs. The logic is functionally sound for the happy path, but has edge case issues documented below.

BLOCKERS

1. Card registration test does not verify DB state (missing test coverage)

test_card_payment_returns_redirect_url (line 149 of test_promo_registration.py) was updated with a new docstring ("should save registration as pending") but the test body was NOT updated. It only asserts:

• response.status_code == 200
• "redirect_url" in body
• "stripe.com" in body["redirect_url"]

It does NOT verify that a Registration record was created with payment_status=pending and signup_method="stripe". The docstring claims the test covers DB persistence, but it does not. This is the core new behavior of this PR and it is untested.

2. Zero test coverage for webhook pending-registration matching

The new webhook handler block (33 lines of new logic in webhooks.py) has no tests at all. Required test cases:

• Happy path: pending registration exists, webhook marks it paid with correct Stripe IDs
• No match: email has no pending registration, falls through to jersey/registration handlers
• No email in webhook payload: handler skips gracefully
• Multiple pending registrations for same email (see edge case below)

This is new functionality with zero test coverage -- a BLOCKER per review criteria.

3. Webhook handler ordering can intercept jersey checkouts (correctness bug)

The pending-registration check (line 197-229) runs BEFORE the jersey checkout check (line 232). The pending-registration handler matches on customer_email + payment_status == pending. The jersey handler matches on metadata.jersey_option.

If a parent has a pending card registration AND later buys a jersey, the jersey checkout.session.completed event will have their email in customer_details.email. The pending-registration handler will match first, incorrectly marking the registration as paid with the jersey checkout session ID, and the jersey order will never be processed.

Fix: Either check for jersey metadata first (swap ordering), or add signup_method == "stripe" to the webhook filter, or check that the checkout session ID does NOT have jersey metadata before matching as a registration payment.

NITS

1. Inline import in hot path -- from basketball_api.models import Parent, PaymentStatus, Registration at line 199 inside stripe_webhook(). Move these to the module-level imports at the top of webhooks.py where Player and other models are already imported. The inline import adds per-request overhead and is inconsistent with the rest of the file.

2. func import ordering -- from sqlalchemy import func at line 9 is placed between the basketball_api.config and basketball_api.models imports, breaking the standard import grouping (stdlib, third-party, local). Should be grouped with the sqlalchemy.orm import on line 5.

3. Multiple pending registrations ambiguity -- .first() at line 216 picks an arbitrary pending registration when a parent has multiple children registered with card payment. The PR body acknowledges Phase 4 will fix this with metadata-based matching. Consider adding an ORDER BY created_at DESC so at least the most recent registration is matched, or logging a warning when multiple matches exist.

SOP COMPLIANCE

• Branch named after issue (110-feat-stripe-card-payment-saves-registrat -> issue #110)
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug (plan-wkq)
• No secrets committed (Stripe link is a public Payment Link URL, not a secret key)
• Tests exist and verify new behavior -- FAIL: card test does not verify DB state, webhook handler has zero tests
• No unnecessary file changes (3 files, all relevant)
• Commit messages are descriptive

PROCESS OBSERVATIONS

• Change failure risk: HIGH -- The webhook handler has no tests and a correctness bug (jersey checkout interception). Deploying this could silently misroute Stripe webhook events, marking registrations as paid from jersey purchases or vice versa.
• Acknowledged tech debt -- The PR body correctly notes that Phase 4 will replace email-based matching with metadata-based Checkout Sessions. This is good traceability. However, the interim solution needs tests and the ordering bug fixed before it can ship.
• Test pattern exists -- test_subscriptions.py and test_jersey.py both have webhook test classes that mock stripe.Webhook.construct_event. The same pattern should be used to test the new pending-registration webhook handler.

VERDICT: NOT APPROVED

Three blockers must be resolved:

1. Add DB assertions to test_card_payment_returns_redirect_url (verify pending/stripe record created)
2. Add webhook tests for the pending-registration matching handler (happy path, no-match fallthrough, no-email skip)
3. Fix webhook handler ordering so jersey checkouts with metadata are checked BEFORE email-based pending-registration matching

## PR #111 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Stripe webhooks **register.py changes** -- The save-before-redirect logic is structurally correct. Card payments now flow through full player/registration creation with `payment_status=pending` and `signup_method="stripe"`, then return the Stripe Payment Link URL. The placement after `db.commit()` at line 1273 ensures the record is persisted before the user is redirected. This is the right pattern. **webhooks.py changes** -- The new pending-registration matching block at lines 198-229 queries for a pending registration by joining `Registration -> Player -> Parent` and matching on lowercased parent email. When found, it updates `payment_status` to `paid` and stores Stripe session/payment IDs. The logic is functionally sound for the happy path, but has edge case issues documented below. ### BLOCKERS **1. Card registration test does not verify DB state (missing test coverage)** `test_card_payment_returns_redirect_url` (line 149 of `test_promo_registration.py`) was updated with a new docstring ("should save registration as pending") but the test body was NOT updated. It only asserts: - `response.status_code == 200` - `"redirect_url" in body` - `"stripe.com" in body["redirect_url"]` It does NOT verify that a `Registration` record was created with `payment_status=pending` and `signup_method="stripe"`. The docstring claims the test covers DB persistence, but it does not. This is the core new behavior of this PR and it is untested. **2. Zero test coverage for webhook pending-registration matching** The new webhook handler block (33 lines of new logic in `webhooks.py`) has no tests at all. Required test cases: - Happy path: pending registration exists, webhook marks it paid with correct Stripe IDs - No match: email has no pending registration, falls through to jersey/registration handlers - No email in webhook payload: handler skips gracefully - Multiple pending registrations for same email (see edge case below) This is new functionality with zero test coverage -- a BLOCKER per review criteria. **3. Webhook handler ordering can intercept jersey checkouts (correctness bug)** The pending-registration check (line 197-229) runs BEFORE the jersey checkout check (line 232). The pending-registration handler matches on `customer_email` + `payment_status == pending`. The jersey handler matches on `metadata.jersey_option`. If a parent has a pending card registration AND later buys a jersey, the jersey `checkout.session.completed` event will have their email in `customer_details.email`. The pending-registration handler will match first, incorrectly marking the registration as paid with the jersey checkout session ID, and the jersey order will never be processed. Fix: Either check for jersey metadata first (swap ordering), or add `signup_method == "stripe"` to the webhook filter, or check that the checkout session ID does NOT have jersey metadata before matching as a registration payment. ### NITS **1. Inline import in hot path** -- `from basketball_api.models import Parent, PaymentStatus, Registration` at line 199 inside `stripe_webhook()`. Move these to the module-level imports at the top of `webhooks.py` where `Player` and other models are already imported. The inline import adds per-request overhead and is inconsistent with the rest of the file. **2. `func` import ordering** -- `from sqlalchemy import func` at line 9 is placed between the `basketball_api.config` and `basketball_api.models` imports, breaking the standard import grouping (stdlib, third-party, local). Should be grouped with the `sqlalchemy.orm` import on line 5. **3. Multiple pending registrations ambiguity** -- `.first()` at line 216 picks an arbitrary pending registration when a parent has multiple children registered with card payment. The PR body acknowledges Phase 4 will fix this with metadata-based matching. Consider adding an `ORDER BY created_at DESC` so at least the most recent registration is matched, or logging a warning when multiple matches exist. ### SOP COMPLIANCE - [x] Branch named after issue (`110-feat-stripe-card-payment-saves-registrat` -> issue #110) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references plan slug (`plan-wkq`) - [x] No secrets committed (Stripe link is a public Payment Link URL, not a secret key) - [ ] Tests exist and verify new behavior -- **FAIL**: card test does not verify DB state, webhook handler has zero tests - [x] No unnecessary file changes (3 files, all relevant) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk: HIGH** -- The webhook handler has no tests and a correctness bug (jersey checkout interception). Deploying this could silently misroute Stripe webhook events, marking registrations as paid from jersey purchases or vice versa. - **Acknowledged tech debt** -- The PR body correctly notes that Phase 4 will replace email-based matching with metadata-based Checkout Sessions. This is good traceability. However, the interim solution needs tests and the ordering bug fixed before it can ship. - **Test pattern exists** -- `test_subscriptions.py` and `test_jersey.py` both have webhook test classes that mock `stripe.Webhook.construct_event`. The same pattern should be used to test the new pending-registration webhook handler. ### VERDICT: NOT APPROVED Three blockers must be resolved: 1. Add DB assertions to `test_card_payment_returns_redirect_url` (verify `pending/stripe` record created) 2. Add webhook tests for the pending-registration matching handler (happy path, no-match fallthrough, no-email skip) 3. Fix webhook handler ordering so jersey checkouts with metadata are checked BEFORE email-based pending-registration matching

forgejo_admin added 1 commit 2026-03-19 01:30:38 +00:00

fix: address QA blockers — test coverage + webhook ordering (#110) ... bcaf345577

1. Card registration test now verifies DB state (Registration with
   payment_status=pending, signup_method=stripe, and Player record)
2. Added webhook tests for pending-registration matching: happy path,
   case-insensitive email, and no-match fallthrough
3. Fixed webhook handler ordering: jersey metadata check now runs before
   email-based pending registration check to prevent jersey checkouts
   from being intercepted by the registration matcher

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

forgejo_admin commented 2026-03-19 01:33:01 +00:00

Author

Contributor

Copy link

PR #111 Review

Re-review after fixes for 3 previously reported blockers.

BLOCKER RESOLUTION STATUS

Blocker 1: Card registration test should assert DB state -- FIXED

TestCardRegistration.test_card_payment_returns_redirect_url now accepts a db: Session fixture and asserts:

• reg.payment_status == PaymentStatus.pending
• reg.signup_method == "stripe"
• Player record exists with correct name

Clean, comprehensive fix.

Blocker 2: New webhook tests for pending registration email matching -- FIXED

Three new tests added in TestRegistrationWebhookCompletion:

• test_webhook_marks_pending_registration_as_paid -- full end-to-end: register card -> webhook -> verify paid + Stripe IDs stored
• test_webhook_email_matching_is_case_insensitive -- registers with mixed-case email, webhook fires with different casing, still matches
• test_webhook_no_match_falls_through -- webhook with unknown email does not crash, falls through to process_checkout_completed

All three tests use the correct pattern: mock stripe.Webhook.construct_event, create real DB state via the registration endpoint, then assert DB mutations. Well-structured.

Blocker 3: Webhook handler ordering (jersey metadata first, then email matching) -- FIXED

In webhooks.py line 200, _handle_jersey_checkout_completed(db, data_object) runs first. Only if it returns False (no jersey_option in metadata) does the code proceed to email-based pending registration matching (line 203+). The comment at line 197-199 explicitly documents the rationale. Correct ordering.

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe webhooks

Registration flow (register.py):

• Card payment path now creates Parent -> Player -> Registration (pending/stripe) before returning redirect_url. Previously it returned the redirect immediately without saving any DB state. The fix correctly places the redirect after db.commit() at line 1278.
• The elif body.payment_method == "card" branch at line 1243 correctly sets payment_status = PaymentStatus.pending and signup_method = "stripe".

Webhook flow (webhooks.py):

• The email extraction chain (lines 206-214) handles customer_details.email, customer_email, and empty string fallback. Normalized with .strip().lower().
• The DB query (lines 216-225) joins Registration -> Player -> Parent, filters by lowercased email and pending status. Uses func.lower(Parent.email) for DB-side case-insensitive comparison. Correct.
• On match: sets payment_status = paid, stores stripe_checkout_session_id, stripe_payment_intent_id, amount_cents. All fields exist on the Registration model and are nullable. Correct.

Import placement: The from basketball_api.models import Parent, PaymentStatus, Registration at line 204 is a function-level import inside the webhook handler. Player is already imported at module level (line 10). This works but is slightly inconsistent -- Parent and Registration could be at module level too. Non-blocking.

BLOCKERS

One new issue found:

The webhook pending registration query (lines 216-225) filters only on PaymentStatus.pending but does NOT filter on Registration.signup_method == "stripe". Both cash and card registrations create pending records. If a parent registers a child with cash (pending/cash), then later registers another child with card (pending/stripe), the webhook could match the wrong registration -- whichever .first() returns.

The fix is adding one line to the filter:

Registration.signup_method == "stripe",

This is a correctness bug. While it may be unlikely in the current user base, the email-based matching is already acknowledged as a temporary approach (PR body notes Phase 4 will use metadata-based matching). Adding the signup_method filter costs nothing and prevents a real data integrity issue.

Severity: BLOCKER -- unfiltered query could update the wrong registration record.

NITS

1. Function-level import (webhooks.py line 204): Parent, PaymentStatus, and Registration are imported inside the function body. Consider moving to module-level imports alongside the existing Player import on line 10. This is consistent with the rest of the file.

2. Test file naming: The webhook completion tests live in test_promo_registration.py, which was originally scoped to registration endpoint tests. The webhook tests are conceptually distinct. Consider whether they should eventually move to a dedicated test_webhook.py. Non-blocking for this PR.

3. PR body Test Plan checkboxes: All three items are unchecked (- [ ]). These should be checked off before merge to reflect that testing was actually performed.

SOP COMPLIANCE

• Branch named after issue (110-feat-stripe-card-payment-saves-registrat references issue #110)
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug (plan-wkq)
• No secrets committed (Stripe link is a public payment link, not a secret key)
• No unnecessary file changes (3 files changed, all directly related)
• Commit messages are descriptive

PROCESS OBSERVATIONS

• The previous review-fix cycle worked well: all 3 original blockers are cleanly resolved.
• The PR body correctly notes that Phase 4 will replace email-based webhook matching with metadata-based Checkout Sessions. This is good forward-looking documentation.
• Change failure risk is moderate due to the email-matching approach, but the explicit Phase 4 plan mitigates this being a long-term concern.

VERDICT: NOT APPROVED

One remaining blocker: the webhook query must also filter on signup_method == "stripe" to avoid matching cash registrations for the same parent email. Single-line fix.

## PR #111 Review Re-review after fixes for 3 previously reported blockers. ### BLOCKER RESOLUTION STATUS **Blocker 1: Card registration test should assert DB state** -- FIXED `TestCardRegistration.test_card_payment_returns_redirect_url` now accepts a `db: Session` fixture and asserts: - `reg.payment_status == PaymentStatus.pending` - `reg.signup_method == "stripe"` - Player record exists with correct name Clean, comprehensive fix. **Blocker 2: New webhook tests for pending registration email matching** -- FIXED Three new tests added in `TestRegistrationWebhookCompletion`: - `test_webhook_marks_pending_registration_as_paid` -- full end-to-end: register card -> webhook -> verify paid + Stripe IDs stored - `test_webhook_email_matching_is_case_insensitive` -- registers with mixed-case email, webhook fires with different casing, still matches - `test_webhook_no_match_falls_through` -- webhook with unknown email does not crash, falls through to `process_checkout_completed` All three tests use the correct pattern: mock `stripe.Webhook.construct_event`, create real DB state via the registration endpoint, then assert DB mutations. Well-structured. **Blocker 3: Webhook handler ordering (jersey metadata first, then email matching)** -- FIXED In `webhooks.py` line 200, `_handle_jersey_checkout_completed(db, data_object)` runs first. Only if it returns `False` (no `jersey_option` in metadata) does the code proceed to email-based pending registration matching (line 203+). The comment at line 197-199 explicitly documents the rationale. Correct ordering. ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe webhooks **Registration flow** (`register.py`): - Card payment path now creates Parent -> Player -> Registration (pending/stripe) before returning `redirect_url`. Previously it returned the redirect immediately without saving any DB state. The fix correctly places the redirect after `db.commit()` at line 1278. - The `elif body.payment_method == "card"` branch at line 1243 correctly sets `payment_status = PaymentStatus.pending` and `signup_method = "stripe"`. **Webhook flow** (`webhooks.py`): - The email extraction chain (lines 206-214) handles `customer_details.email`, `customer_email`, and empty string fallback. Normalized with `.strip().lower()`. - The DB query (lines 216-225) joins Registration -> Player -> Parent, filters by lowercased email and pending status. Uses `func.lower(Parent.email)` for DB-side case-insensitive comparison. Correct. - On match: sets `payment_status = paid`, stores `stripe_checkout_session_id`, `stripe_payment_intent_id`, `amount_cents`. All fields exist on the Registration model and are nullable. Correct. **Import placement**: The `from basketball_api.models import Parent, PaymentStatus, Registration` at line 204 is a function-level import inside the webhook handler. `Player` is already imported at module level (line 10). This works but is slightly inconsistent -- `Parent` and `Registration` could be at module level too. Non-blocking. ### BLOCKERS **One new issue found:** The webhook pending registration query (lines 216-225) filters only on `PaymentStatus.pending` but does NOT filter on `Registration.signup_method == "stripe"`. Both `cash` and `card` registrations create `pending` records. If a parent registers a child with cash (pending/cash), then later registers another child with card (pending/stripe), the webhook could match the wrong registration -- whichever `.first()` returns. The fix is adding one line to the filter: ```python Registration.signup_method == "stripe", ``` This is a correctness bug. While it may be unlikely in the current user base, the email-based matching is already acknowledged as a temporary approach (PR body notes Phase 4 will use metadata-based matching). Adding the `signup_method` filter costs nothing and prevents a real data integrity issue. **Severity: BLOCKER** -- unfiltered query could update the wrong registration record. ### NITS 1. **Function-level import** (`webhooks.py` line 204): `Parent`, `PaymentStatus`, and `Registration` are imported inside the function body. Consider moving to module-level imports alongside the existing `Player` import on line 10. This is consistent with the rest of the file. 2. **Test file naming**: The webhook completion tests live in `test_promo_registration.py`, which was originally scoped to registration endpoint tests. The webhook tests are conceptually distinct. Consider whether they should eventually move to a dedicated `test_webhook.py`. Non-blocking for this PR. 3. **PR body Test Plan checkboxes**: All three items are unchecked (`- [ ]`). These should be checked off before merge to reflect that testing was actually performed. ### SOP COMPLIANCE - [x] Branch named after issue (`110-feat-stripe-card-payment-saves-registrat` references issue #110) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references plan slug (`plan-wkq`) - [x] No secrets committed (Stripe link is a public payment link, not a secret key) - [x] No unnecessary file changes (3 files changed, all directly related) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - The previous review-fix cycle worked well: all 3 original blockers are cleanly resolved. - The PR body correctly notes that Phase 4 will replace email-based webhook matching with metadata-based Checkout Sessions. This is good forward-looking documentation. - Change failure risk is moderate due to the email-matching approach, but the explicit Phase 4 plan mitigates this being a long-term concern. ### VERDICT: NOT APPROVED One remaining blocker: the webhook query must also filter on `signup_method == "stripe"` to avoid matching cash registrations for the same parent email. Single-line fix.

forgejo_admin added 1 commit 2026-03-19 01:33:33 +00:00

fix: filter webhook matcher by signup_method=stripe (#110) ... 158c599042

Prevents a Stripe checkout webhook from incorrectly marking a
cash registration as paid when both share the same parent email.

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

forgejo_admin commented 2026-03-19 01:37:07 +00:00

Author

Contributor

Copy link

PR #111 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe webhooks / pytest

register.py -- card payment flow

The old code returned the Stripe redirect URL immediately (before creating any DB records). The new code correctly moves the redirect after player/parent/registration creation, setting payment_status=pending and signup_method="stripe". The control flow is clean:

1. Promo validation (unchanged)
2. Player/parent creation (unchanged)
3. Payment status determination -- now includes the card branch (pending/stripe)
4. DB commit
5. Early return with redirect_url for card payments (before Keycloak account creation, which only applies to paid registrations)

The early return at line 1278 is correctly placed after db.commit() and before the Keycloak block, which is guarded by payment_status == PaymentStatus.paid anyway. No logic leak.

webhooks.py -- pending registration matching

The handler ordering is correct and well-commented:

1. Jersey checkout (deterministic metadata match) runs first -- returns early if matched
2. Pending registration match (email-based) runs second
3. Generic process_checkout_completed is the fallback

The query at lines 216-226 is the focus of this round. It filters on three conditions:

• func.lower(Parent.email) == customer_email (case-insensitive email match)
• Registration.payment_status == PaymentStatus.pending
• Registration.signup_method == "stripe"

This is the fix from round 2. The signup_method == "stripe" filter correctly prevents matching cash-pending registrations, which was the previous blocker. The join chain Registration -> Player -> Parent follows the established relationships in the model.

Email extraction handles both customer_details.email and customer_email with .strip().lower() normalization. Empty-email case is guarded (if customer_email:).

Inline import: from basketball_api.models import Parent, PaymentStatus, Registration at line 204 is inside the function body. Player is already imported at the module level (line 10). Parent, PaymentStatus, and Registration are not in the top-level imports. This is a minor style inconsistency but not a blocker -- likely done to avoid circular imports or to keep the diff minimal.

test_promo_registration.py -- test coverage

The test suite covers:

• test_card_payment_returns_redirect_url -- verifies redirect URL returned AND DB state (pending/stripe), including Player record existence. This was the round 1 blocker fix.
• test_webhook_marks_pending_registration_as_paid -- full end-to-end: creates pending registration, fires webhook, asserts paid status + Stripe session/payment intent IDs + amount_cents. This was the round 1 blocker fix.
• test_webhook_email_matching_is_case_insensitive -- registers with mixed-case email, webhook arrives with different case, still matches. Good edge case.
• test_webhook_no_match_falls_through -- webhook for unknown email does not crash, falls through to process_checkout_completed. Good error path coverage.

All tests properly mock stripe.Webhook.construct_event and use the _clean fixture for DB isolation.

BLOCKERS

None.

All three previous blockers have been verified as resolved:

1. Round 1 -- Missing DB assertions in card test: Fixed. test_card_payment_returns_redirect_url now asserts reg.payment_status == PaymentStatus.pending and reg.signup_method == "stripe".
2. Round 1 -- No webhook tests: Fixed. Three webhook tests added covering happy path, case-insensitive matching, and no-match fallthrough.
3. Round 2 -- Missing signup_method == "stripe" filter: Fixed. The webhook query at line 223 includes Registration.signup_method == "stripe", preventing false matches on cash-pending registrations.

NITS

1. Inline import in webhook handler (webhooks.py:204): Parent, PaymentStatus, and Registration could be moved to the module-level imports alongside Player for consistency. Not a blocker -- the code works correctly as-is.

2. PR body test plan uses manual checkboxes: The test plan lists pytest tests/test_promo_registration.py -v but the checkboxes are unchecked. Minor -- CI validates this.

SOP COMPLIANCE

• Branch named after issue (110-feat-stripe-card-payment-saves-registrat references issue #110)
• PR body has Summary, Changes, Test Plan, Related sections
• Related references plan slug (plan-wkq)
• Tests exist (4 new tests covering card registration + webhook completion)
• No secrets committed (all secrets via settings object)
• No unnecessary file changes (3 files, all directly related to the feature)
• Commit messages are descriptive

PROCESS OBSERVATIONS

• Deployment frequency: Clean feature addition. No migration needed -- the signup_method and payment_status columns already exist in the model. Low-risk deploy.
• Change failure risk: Low. The webhook handler ordering (jersey first, then registration, then fallback) is well-structured. The signup_method == "stripe" filter prevents cross-contamination with cash registrations.
• Phase 4 note: The PR body correctly notes that Phase 4 will replace email-based webhook matching with Stripe Checkout Session metadata. This is good forward documentation -- the current email-based approach works but is acknowledged as interim.
• Review-fix loop: Three rounds to get clean. Round 1 caught missing test assertions and webhook coverage. Round 2 caught the signup_method filter gap. Round 3 confirms all fixes landed correctly.

VERDICT: APPROVED

## PR #111 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe webhooks / pytest **register.py -- card payment flow** The old code returned the Stripe redirect URL immediately (before creating any DB records). The new code correctly moves the redirect after player/parent/registration creation, setting `payment_status=pending` and `signup_method="stripe"`. The control flow is clean: 1. Promo validation (unchanged) 2. Player/parent creation (unchanged) 3. Payment status determination -- now includes the `card` branch (`pending/stripe`) 4. DB commit 5. Early return with `redirect_url` for card payments (before Keycloak account creation, which only applies to `paid` registrations) The early return at line 1278 is correctly placed after `db.commit()` and before the Keycloak block, which is guarded by `payment_status == PaymentStatus.paid` anyway. No logic leak. **webhooks.py -- pending registration matching** The handler ordering is correct and well-commented: 1. Jersey checkout (deterministic metadata match) runs first -- returns early if matched 2. Pending registration match (email-based) runs second 3. Generic `process_checkout_completed` is the fallback The query at lines 216-226 is the focus of this round. It filters on three conditions: - `func.lower(Parent.email) == customer_email` (case-insensitive email match) - `Registration.payment_status == PaymentStatus.pending` - `Registration.signup_method == "stripe"` This is the fix from round 2. The `signup_method == "stripe"` filter correctly prevents matching cash-pending registrations, which was the previous blocker. The join chain `Registration -> Player -> Parent` follows the established relationships in the model. Email extraction handles both `customer_details.email` and `customer_email` with `.strip().lower()` normalization. Empty-email case is guarded (`if customer_email:`). **Inline import**: `from basketball_api.models import Parent, PaymentStatus, Registration` at line 204 is inside the function body. `Player` is already imported at the module level (line 10). `Parent`, `PaymentStatus`, and `Registration` are not in the top-level imports. This is a minor style inconsistency but not a blocker -- likely done to avoid circular imports or to keep the diff minimal. **test_promo_registration.py -- test coverage** The test suite covers: - `test_card_payment_returns_redirect_url` -- verifies redirect URL returned AND DB state (pending/stripe), including Player record existence. This was the round 1 blocker fix. - `test_webhook_marks_pending_registration_as_paid` -- full end-to-end: creates pending registration, fires webhook, asserts `paid` status + Stripe session/payment intent IDs + amount_cents. This was the round 1 blocker fix. - `test_webhook_email_matching_is_case_insensitive` -- registers with mixed-case email, webhook arrives with different case, still matches. Good edge case. - `test_webhook_no_match_falls_through` -- webhook for unknown email does not crash, falls through to `process_checkout_completed`. Good error path coverage. All tests properly mock `stripe.Webhook.construct_event` and use the `_clean` fixture for DB isolation. ### BLOCKERS None. All three previous blockers have been verified as resolved: 1. **Round 1 -- Missing DB assertions in card test**: Fixed. `test_card_payment_returns_redirect_url` now asserts `reg.payment_status == PaymentStatus.pending` and `reg.signup_method == "stripe"`. 2. **Round 1 -- No webhook tests**: Fixed. Three webhook tests added covering happy path, case-insensitive matching, and no-match fallthrough. 3. **Round 2 -- Missing `signup_method == "stripe"` filter**: Fixed. The webhook query at line 223 includes `Registration.signup_method == "stripe"`, preventing false matches on cash-pending registrations. ### NITS 1. **Inline import in webhook handler** (webhooks.py:204): `Parent`, `PaymentStatus`, and `Registration` could be moved to the module-level imports alongside `Player` for consistency. Not a blocker -- the code works correctly as-is. 2. **PR body test plan uses manual checkboxes**: The test plan lists `pytest tests/test_promo_registration.py -v` but the checkboxes are unchecked. Minor -- CI validates this. ### SOP COMPLIANCE - [x] Branch named after issue (`110-feat-stripe-card-payment-saves-registrat` references issue #110) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references plan slug (`plan-wkq`) - [x] Tests exist (4 new tests covering card registration + webhook completion) - [x] No secrets committed (all secrets via `settings` object) - [x] No unnecessary file changes (3 files, all directly related to the feature) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean feature addition. No migration needed -- the `signup_method` and `payment_status` columns already exist in the model. Low-risk deploy. - **Change failure risk**: Low. The webhook handler ordering (jersey first, then registration, then fallback) is well-structured. The `signup_method == "stripe"` filter prevents cross-contamination with cash registrations. - **Phase 4 note**: The PR body correctly notes that Phase 4 will replace email-based webhook matching with Stripe Checkout Session metadata. This is good forward documentation -- the current email-based approach works but is acknowledged as interim. - **Review-fix loop**: Three rounds to get clean. Round 1 caught missing test assertions and webhook coverage. Round 2 caught the signup_method filter gap. Round 3 confirms all fixes landed correctly. ### VERDICT: APPROVED

forgejo_admin merged commit 6e1c7ce185 into main 2026-03-19 01:38:34 +00:00

forgejo_admin deleted branch 110-feat-stripe-card-payment-saves-registrat 2026-03-19 01:38:34 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-19 01:38:34 +00:00

feat: save card registration as pending before Stripe redirect (#111)

forgejo_admin referenced this pull request 2026-03-19 01:40:40 +00:00

feat: dynamic Stripe Checkout Sessions for tryout registration #112

forgejo_admin referenced this pull request 2026-03-29 22:31:34 +00:00

Bug: /public/teams endpoint may not filter by is_public #254

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

ldraney/basketball-api!111

No description provided.