forgejo_admin/basketball-api

Fork 1

fix: don't set jersey_option at checkout — wait for Stripe webhook #140

Merged

forgejo_admin merged 1 commit from 137-fix-jersey-option-timing into main

2026-03-21 21:01:42 +00:00

forgejo_admin commented

2026-03-21 20:58:48 +00:00

Owner

Summary

The checkout endpoint was prematurely setting jersey_option on the player record before Stripe payment was confirmed. If a user abandoned the Stripe checkout session, they appeared as having selected a jersey option they never paid for. This fix removes that premature assignment so jersey_option is only set by the Stripe webhook on payment confirmation.

Changes

src/basketball_api/routes/jersey.py: Removed p.jersey_option = jersey_opt from the checkout path. jersey_order_status is still set to pending so we know a checkout was started. The opt-out path is unchanged.
tests/test_jersey.py: Updated existing tests to assert jersey_option is None after checkout. Updated webhook test to simulate the real flow (jersey_option NULL before webhook). Added 3 new tests covering checkout-without-option, opt-out-sets-immediately, and webhook-sets-from-metadata.

Test Plan

pytest tests/ -k jersey — 28 tests pass
ruff format and ruff check pass
Verify checkout endpoint creates Stripe session but does NOT set jersey_option
Verify jersey_option remains NULL after checkout creation
Verify webhook sets jersey_option on payment confirmation
Verify opt-out still sets jersey_option immediately

Review Checklist

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

Closes #137

## Summary The checkout endpoint was prematurely setting `jersey_option` on the player record before Stripe payment was confirmed. If a user abandoned the Stripe checkout session, they appeared as having selected a jersey option they never paid for. This fix removes that premature assignment so `jersey_option` is only set by the Stripe webhook on payment confirmation. ## Changes - `src/basketball_api/routes/jersey.py`: Removed `p.jersey_option = jersey_opt` from the checkout path. `jersey_order_status` is still set to `pending` so we know a checkout was started. The opt-out path is unchanged. - `tests/test_jersey.py`: Updated existing tests to assert `jersey_option is None` after checkout. Updated webhook test to simulate the real flow (jersey_option NULL before webhook). Added 3 new tests covering checkout-without-option, opt-out-sets-immediately, and webhook-sets-from-metadata. ## Test Plan - [x] `pytest tests/ -k jersey` — 28 tests pass - [x] `ruff format` and `ruff check` pass - [ ] Verify checkout endpoint creates Stripe session but does NOT set `jersey_option` - [ ] Verify `jersey_option` remains NULL after checkout creation - [ ] Verify webhook sets `jersey_option` on payment confirmation - [ ] Verify opt-out still sets `jersey_option` immediately ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #137

forgejo_admin added 1 commit

2026-03-21 20:58:48 +00:00

fix: don't set jersey_option at checkout — wait for Stripe webhook

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

c736d4c9c4

The checkout endpoint was prematurely setting jersey_option on the
player record before payment was confirmed. If a user abandoned the
Stripe session, they'd appear as having selected an option they never
paid for.

Remove the jersey_option assignment from the checkout path. The
jersey_order_status is still set to "pending" so we know a checkout
was started. The existing webhook handler (_handle_jersey_checkout_completed)
already sets jersey_option from Stripe metadata on payment confirmation.

The opt-out path is unchanged — it correctly sets jersey_option
immediately since no payment is involved.

Closes #137

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

forgejo_admin commented

2026-03-21 20:59:14 +00:00

Author

Owner

Self-Review

Verdict: LGTM -- no issues found.

Code change (jersey.py)

Single line removed: p.jersey_option = jersey_opt from the checkout path (line 170)
jersey_order_status = pending still set so we track that a checkout was started
Opt-out path (lines 116-120) correctly untouched -- it sets jersey_option immediately since no payment is involved
Webhook handler in webhooks.py already sets jersey_option from Stripe metadata on checkout.session.completed -- no changes needed there
No dead code left behind (jersey_opt is still used for validation and the opt-out branch)

Tests (test_jersey.py)

Updated test_checkout_sets_pending_status -> test_checkout_sets_pending_status_but_not_jersey_option: asserts jersey_option is None after checkout
Updated test_webhook_updates_player_jersey_fields -> test_webhook_sets_jersey_option_and_paid_status: pre-sets jersey_option = None (not reversible) to match real flow
Added test_checkout_creates_stripe_session_without_setting_jersey_option: verifies Stripe session created, jersey_option stays NULL
Added test_opt_out_sets_jersey_option_immediately: confirms opt-out path still sets jersey_option at checkout time
Added test_webhook_sets_jersey_option_from_stripe_metadata: verifies webhook reads option from Stripe metadata, not player record

Verification

28 jersey tests pass
ruff format + check clean
No unrelated file changes

## Self-Review **Verdict: LGTM -- no issues found.** ### Code change (jersey.py) - Single line removed: `p.jersey_option = jersey_opt` from the checkout path (line 170) - `jersey_order_status = pending` still set so we track that a checkout was started - Opt-out path (lines 116-120) correctly untouched -- it sets `jersey_option` immediately since no payment is involved - Webhook handler in `webhooks.py` already sets `jersey_option` from Stripe metadata on `checkout.session.completed` -- no changes needed there - No dead code left behind (`jersey_opt` is still used for validation and the opt-out branch) ### Tests (test_jersey.py) - Updated `test_checkout_sets_pending_status` -> `test_checkout_sets_pending_status_but_not_jersey_option`: asserts `jersey_option is None` after checkout - Updated `test_webhook_updates_player_jersey_fields` -> `test_webhook_sets_jersey_option_and_paid_status`: pre-sets `jersey_option = None` (not `reversible`) to match real flow - Added `test_checkout_creates_stripe_session_without_setting_jersey_option`: verifies Stripe session created, jersey_option stays NULL - Added `test_opt_out_sets_jersey_option_immediately`: confirms opt-out path still sets jersey_option at checkout time - Added `test_webhook_sets_jersey_option_from_stripe_metadata`: verifies webhook reads option from Stripe metadata, not player record ### Verification - 28 jersey tests pass - ruff format + check clean - No unrelated file changes

forgejo_admin commented

2026-03-21 21:01:21 +00:00

Author

Owner

PR #140 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Stripe webhooks

Fix correctness: The change is surgically correct. The checkout endpoint (jersey.py) was prematurely assigning jersey_option on the player record before Stripe payment confirmation. Now it only sets jersey_order_status = pending to track that a checkout was initiated. The jersey_option value is preserved in Stripe session metadata ("jersey_option": body.option at line 162), and the webhook handler (_handle_jersey_checkout_completed in webhooks.py:171-209) reads it back from metadata and sets it on the player only after confirmed payment. The data flow is sound.

Opt-out path: Correctly unchanged. Opt-out sets jersey_option immediately since no payment is involved.

Model compatibility: The Player.jersey_option column is Mapped[JerseyOption | None] with nullable=True, so leaving it as None after checkout is valid at the database level.

Test quality: Three new tests cover the critical scenarios:

Checkout creates Stripe session but jersey_option stays NULL
Opt-out sets jersey_option immediately (regression guard)
Webhook sets jersey_option from Stripe metadata (the real flow)

Existing tests updated to reflect the new behavior (asserting jersey_option is None after checkout, setting up webhook test with NULL initial state). Docstrings added explaining the why behind each assertion. All tests include db.refresh(player) before assertions, which is the correct SQLAlchemy pattern.

No changes to webhooks.py: The webhook handler already reads jersey_option from Stripe metadata and sets it on confirmation. This PR correctly identified that the webhook was already doing the right thing -- the bug was only in the checkout endpoint setting the value too early.

BLOCKERS

None.

New functionality has full test coverage (3 new tests + 2 updated tests)
No unvalidated user input introduced
No secrets or credentials in code
No DRY violations in auth/security paths

NITS

Multi-player family gap (pre-existing, not introduced by this PR): The checkout endpoint sets jersey_order_status = pending on ALL of a parent's players (for p in parent.players), but Stripe metadata only carries player_id: parent.players[0].id. When the webhook fires, only that one player gets jersey_option set and jersey_order_status = paid. Siblings remain stuck at pending with jersey_option = None. This is a pre-existing design gap -- recommend tracking as a separate issue.
PR body Related section: References Closes #137 but does not reference the plan slug plan-wkq. Per SOP template, the Related section should link to the parent plan.

SOP COMPLIANCE

Branch named after issue (137-fix-jersey-option-timing references #137)
PR body has Summary, Changes, Test Plan, Related
Related section references plan slug (missing plan-wkq reference)
Tests exist and cover new behavior (3 new + 2 updated)
No secrets committed
No unnecessary file changes (2 files, both directly relevant)
Commit messages are descriptive

PROCESS OBSERVATIONS

Change failure risk: LOW. Single-line removal in production code, with the webhook handler already correctly handling the jersey_option assignment from metadata. The fix reduces data inconsistency risk (abandoned checkouts no longer leave stale jersey_option values).
Discovered scope: Multi-player webhook gap (nit #1) should become a Forgejo issue and board item per convention.
Test plan thoroughness: The unchecked items in the PR Test Plan (manual verification steps) are appropriate for integration testing that requires a live Stripe session. Unit tests cover the logic paths.

VERDICT: APPROVED

## PR #140 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Stripe webhooks **Fix correctness:** The change is surgically correct. The checkout endpoint (`jersey.py`) was prematurely assigning `jersey_option` on the player record before Stripe payment confirmation. Now it only sets `jersey_order_status = pending` to track that a checkout was initiated. The `jersey_option` value is preserved in Stripe session metadata (`"jersey_option": body.option` at line 162), and the webhook handler (`_handle_jersey_checkout_completed` in `webhooks.py:171-209`) reads it back from metadata and sets it on the player only after confirmed payment. The data flow is sound. **Opt-out path:** Correctly unchanged. Opt-out sets `jersey_option` immediately since no payment is involved. **Model compatibility:** The `Player.jersey_option` column is `Mapped[JerseyOption | None]` with `nullable=True`, so leaving it as `None` after checkout is valid at the database level. **Test quality:** Three new tests cover the critical scenarios: 1. Checkout creates Stripe session but `jersey_option` stays NULL 2. Opt-out sets `jersey_option` immediately (regression guard) 3. Webhook sets `jersey_option` from Stripe metadata (the real flow) Existing tests updated to reflect the new behavior (asserting `jersey_option is None` after checkout, setting up webhook test with NULL initial state). Docstrings added explaining the why behind each assertion. All tests include `db.refresh(player)` before assertions, which is the correct SQLAlchemy pattern. **No changes to webhooks.py:** The webhook handler already reads `jersey_option` from Stripe metadata and sets it on confirmation. This PR correctly identified that the webhook was already doing the right thing -- the bug was only in the checkout endpoint setting the value too early. ### BLOCKERS None. - New functionality has full test coverage (3 new tests + 2 updated tests) - No unvalidated user input introduced - No secrets or credentials in code - No DRY violations in auth/security paths ### NITS 1. **Multi-player family gap (pre-existing, not introduced by this PR):** The checkout endpoint sets `jersey_order_status = pending` on ALL of a parent's players (`for p in parent.players`), but Stripe metadata only carries `player_id: parent.players[0].id`. When the webhook fires, only that one player gets `jersey_option` set and `jersey_order_status = paid`. Siblings remain stuck at `pending` with `jersey_option = None`. This is a pre-existing design gap -- recommend tracking as a separate issue. 2. **PR body Related section:** References `Closes #137` but does not reference the plan slug `plan-wkq`. Per SOP template, the Related section should link to the parent plan. ### SOP COMPLIANCE - [x] Branch named after issue (`137-fix-jersey-option-timing` references #137) - [x] PR body has Summary, Changes, Test Plan, Related - [ ] Related section references plan slug (missing `plan-wkq` reference) - [x] Tests exist and cover new behavior (3 new + 2 updated) - [x] No secrets committed - [x] No unnecessary file changes (2 files, both directly relevant) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** Single-line removal in production code, with the webhook handler already correctly handling the `jersey_option` assignment from metadata. The fix reduces data inconsistency risk (abandoned checkouts no longer leave stale `jersey_option` values). - **Discovered scope:** Multi-player webhook gap (nit #1) should become a Forgejo issue and board item per convention. - **Test plan thoroughness:** The unchecked items in the PR Test Plan (manual verification steps) are appropriate for integration testing that requires a live Stripe session. Unit tests cover the logic paths. ### VERDICT: APPROVED