feat: add GET /checkout/first-payment endpoint with Stripe redirect #375

Merged

forgejo_admin merged 2 commits from 368-checkout-first-payment into main 2026-04-07 20:13:17 +00:00

Conversation 2 Commits 2 Files changed 2 +508

forgejo_admin commented 2026-04-07 17:13:04 +00:00

Owner

Copy link

Summary

Adds GET /checkout/first-payment?token={contract_token} endpoint that validates a signed contract token, calculates the prorated first monthly payment, creates a Stripe Checkout Session with setup_future_usage: off_session to save the card for recurring billing, and returns a 307 redirect to Stripe's hosted checkout page.

Changes

• src/basketball_api/routes/checkout.py -- added _calculate_prorated_cents() helper and GET /first-payment endpoint. Imports added: ContractStatus, RedirectResponse. Follows existing Stripe Customer create/reuse and Order creation patterns.
• tests/test_first_payment.py -- 7 tests covering valid redirect, invalid token (404), unsigned contract (404), duplicate order (409), $180 proration ($15000 cents), null monthly_fee default ($16500 cents), and setup_future_usage verification.

Test Plan

• pytest tests/test_first_payment.py -v -- 7/7 pass
• pytest tests/ -x -q -- 758 pass, 1 pre-existing failure in test_templated_email.py::TestEmailTypeEnumValues::test_all_expected_values (from PR #370 adding first_payment to EmailType without updating the test; confirmed on main before this branch)

Review Checklist

• ruff format and ruff check pass
• 7 new tests pass
• No unrelated file changes (email.py, admin.py, webhooks.py untouched)
• Proration formula matches westside-contracts +page.svelte:9
• payment_intent_data.setup_future_usage included in Stripe call
• Duplicate order prevention (409)
• Null monthly_fee defaults to $200

Related Notes

• Pre-existing test failure in test_templated_email.py from PR #370 (not introduced by this PR)

Related

• Closes #368
• Parent: #366
• Depends on: #367 (migration, merged as PR #370)

## Summary Adds `GET /checkout/first-payment?token={contract_token}` endpoint that validates a signed contract token, calculates the prorated first monthly payment, creates a Stripe Checkout Session with `setup_future_usage: off_session` to save the card for recurring billing, and returns a 307 redirect to Stripe's hosted checkout page. ## Changes - `src/basketball_api/routes/checkout.py` -- added `_calculate_prorated_cents()` helper and `GET /first-payment` endpoint. Imports added: `ContractStatus`, `RedirectResponse`. Follows existing Stripe Customer create/reuse and Order creation patterns. - `tests/test_first_payment.py` -- 7 tests covering valid redirect, invalid token (404), unsigned contract (404), duplicate order (409), $180 proration ($15000 cents), null monthly_fee default ($16500 cents), and `setup_future_usage` verification. ## Test Plan - `pytest tests/test_first_payment.py -v` -- 7/7 pass - `pytest tests/ -x -q` -- 758 pass, 1 pre-existing failure in `test_templated_email.py::TestEmailTypeEnumValues::test_all_expected_values` (from PR #370 adding `first_payment` to `EmailType` without updating the test; confirmed on main before this branch) ## Review Checklist - [x] `ruff format` and `ruff check` pass - [x] 7 new tests pass - [x] No unrelated file changes (email.py, admin.py, webhooks.py untouched) - [x] Proration formula matches westside-contracts `+page.svelte:9` - [x] `payment_intent_data.setup_future_usage` included in Stripe call - [x] Duplicate order prevention (409) - [x] Null monthly_fee defaults to $200 ## Related Notes - Pre-existing test failure in `test_templated_email.py` from PR #370 (not introduced by this PR) ## Related - Closes #368 - Parent: #366 - Depends on: #367 (migration, merged as PR #370)

forgejo_admin added 1 commit 2026-04-07 17:13:04 +00:00

feat: add GET /checkout/first-payment endpoint with Stripe redirect ... 5edc0d4e99

Implements token-based checkout for prorated first monthly payment.
Looks up player by contract_token (must be signed), calculates prorated
amount from monthly_fee using the westside-contracts formula, creates a
Stripe Checkout Session with setup_future_usage to save the card for
recurring billing, and returns a 307 redirect to Stripe's hosted page.

Includes 7 tests covering valid flow, invalid/unsigned tokens, duplicate
order prevention, proration math ($180->$150, null->$200 default), and
setup_future_usage verification.

Closes #368

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

forgejo_admin commented 2026-04-07 17:13:46 +00:00

Author

Owner

Copy link

QA Review

Verdict: APPROVED (with nits)

What looks good

• Proration formula is correct and matches westside-contracts +page.svelte:9 -- verified: $200->$165, $180->$150, $160->$135
• payment_intent_data={"setup_future_usage": "off_session"} is present -- critical for saving cards for recurring billing
• Duplicate order prevention follows existing pattern exactly
• Stripe Customer create/reuse follows existing create_checkout_session pattern
• 307 redirect with RedirectResponse as specified
• Token lookup correctly requires both contract_token match AND contract_status == signed
• All 7 tests pass and cover every acceptance criterion from issue #368
• No unrelated files touched (email.py, admin.py, webhooks.py clean)
• ruff format and ruff check pass
• 1 pre-existing test failure (test_templated_email.py::TestEmailTypeEnumValues) confirmed on main -- not introduced by this PR

Nits (non-blocking)

1. No null guard on parent lookup (line 364): parent = db.query(Parent).filter(Parent.id == player.parent_id).first() -- if parent is somehow None, parent.email on line 369 crashes with AttributeError. The FK constraint makes this near-impossible with valid data, but a defensive if not parent: raise HTTPException(500, ...) would match the product null guard pattern above it.

2. Missing test for existing stripe_customer_id reuse path: All tests create players without stripe_customer_id, so the if player.stripe_customer_id branch (line 365) is never exercised. Not blocking since it's the same pattern as the existing create_checkout_session endpoint.

VERDICT: APPROVED

## QA Review ### Verdict: APPROVED (with nits) ### What looks good - Proration formula is correct and matches westside-contracts `+page.svelte:9` -- verified: $200->$165, $180->$150, $160->$135 - `payment_intent_data={"setup_future_usage": "off_session"}` is present -- critical for saving cards for recurring billing - Duplicate order prevention follows existing pattern exactly - Stripe Customer create/reuse follows existing `create_checkout_session` pattern - 307 redirect with `RedirectResponse` as specified - Token lookup correctly requires both `contract_token` match AND `contract_status == signed` - All 7 tests pass and cover every acceptance criterion from issue #368 - No unrelated files touched (email.py, admin.py, webhooks.py clean) - `ruff format` and `ruff check` pass - 1 pre-existing test failure (`test_templated_email.py::TestEmailTypeEnumValues`) confirmed on main -- not introduced by this PR ### Nits (non-blocking) 1. **No null guard on parent lookup** (line 364): `parent = db.query(Parent).filter(Parent.id == player.parent_id).first()` -- if parent is somehow None, `parent.email` on line 369 crashes with `AttributeError`. The FK constraint makes this near-impossible with valid data, but a defensive `if not parent: raise HTTPException(500, ...)` would match the product null guard pattern above it. 2. **Missing test for existing `stripe_customer_id` reuse path**: All tests create players without `stripe_customer_id`, so the `if player.stripe_customer_id` branch (line 365) is never exercised. Not blocking since it's the same pattern as the existing `create_checkout_session` endpoint. ### VERDICT: APPROVED

forgejo_admin added 1 commit 2026-04-07 17:14:22 +00:00

fix: address QA nits -- parent null guard + stripe customer reuse test ... 9b445090f6

Adds defensive null check on parent lookup (line 364) and a test
verifying existing stripe_customer_id is reused without calling
Customer.create.

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

forgejo_admin commented 2026-04-07 17:17:09 +00:00

Author

Owner

Copy link

PR #375 Review

DOMAIN REVIEW

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

Endpoint correctness (checkout.py):

• GET /first-payment?token={contract_token} -- correctly looks up Player by contract_token + contract_status == ContractStatus.signed, returns 404 for invalid/unsigned tokens.
• _calculate_prorated_cents() formula: round(monthly_fee * 25 / 30 / 5) * 5 * 100 -- matches the westside-contracts reference as documented.
• Null monthly_fee defaults to _DEFAULT_MONTHLY_FEE = 200 -- correct.
• Duplicate prevention via Order.status.in_([OrderStatus.paid, OrderStatus.pending]) -- matches existing create-session pattern exactly.
• Stripe Customer create/reuse pattern -- correct, follows existing convention.
• Order creation with db.flush() before Stripe session (to get order.id) then db.commit() after storing stripe_checkout_session_id -- correct pattern.
• RedirectResponse(url=checkout_session.url, status_code=307) -- correct.

CRITICAL: setup_future_usage verification (requirement #8) -- PRESENT and correct:

payment_intent_data={"setup_future_usage": "off_session"},

This is in the stripe.checkout.Session.create() call. Card will be saved for future monthly billing. Verified.

Test coverage (test_first_payment.py -- 359 lines, 8 tests):

1. test_valid_token_redirects_to_stripe -- verifies 307, Location header, Order amount (16500 cents), session ID stored
2. test_invalid_token_returns_404 -- invalid token path
3. test_unsigned_contract_returns_404 -- offered status rejection
4. test_duplicate_order_returns_409 -- duplicate prevention
5. test_180_fee_prorates_to_150 -- $180 tier ($15000 cents)
6. test_null_monthly_fee_defaults_to_200 -- null default ($16500 cents)
7. test_setup_future_usage_passed_to_stripe -- explicitly verifies payment_intent_data kwargs
8. test_existing_stripe_customer_reused -- verifies Customer.create NOT called when stripe_customer_id exists

All 10 requirements from the issue are covered. Test count is 8, which exceeds the 7 stated in the PR body (the 8th test for customer reuse is a bonus).

Stripe mocking approach: @patch("basketball_api.routes.checkout.stripe") patches at the module level. Tests verify both the happy path redirect and the kwargs passed to Stripe. Solid.

BLOCKERS

None. All critical requirements met:

• setup_future_usage: "off_session" is present and tested
• Tests cover all specified scenarios plus a bonus customer-reuse test
• No hardcoded secrets or credentials
• Input is validated (token lookup with signed status check)
• No SQL injection risk (SQLAlchemy ORM queries with parameterized filters)

NITS

1. ProductCategory import -- The diff adds ContractStatus to the models import but does not show ProductCategory being added. The endpoint uses ProductCategory.monthly on the product lookup query. The PR states it depends on PR #370 (merged), which likely adds monthly to ProductCategory and may also add the import. If the local main I'm reading is stale, this is fine. However, verify that ProductCategory is imported in the merged branch's checkout.py. If missing, it would be a NameError at runtime.

2. DRY: Stripe Customer create/reuse -- The create-or-reuse-customer block is now duplicated in 4 places across the codebase: checkout.py (existing create-session), checkout.py (new first-payment), subscriptions.py, and jersey.py. This is a pre-existing pattern, not introduced by this PR, but worth noting for a future extraction into a shared helper like get_or_create_stripe_customer(player, parent, db). Consider creating a backlog ticket.

3. No active monthly product (500) -- The endpoint returns HTTP 500 if no active monthly product is found. This is a server misconfiguration error, so 500 is arguably correct. An alternative would be 503 (Service Unavailable) to signal a temporary config issue, but 500 is acceptable.

4. Unicode em-dash in product name -- The Stripe line item uses f"First Monthly Payment \u2013 {player.name}" (em-dash). Minor -- works fine in Stripe, just noting for consistency if other product names use hyphens.

5. Logging -- Good structured logging with player name, ID, amount, order ID, and session ID. No concerns.

SOP COMPLIANCE

• Branch named after issue: 368-checkout-first-payment follows {issue-number}-{kebab-case-purpose}
• PR body has: Summary, Changes, Test Plan, Related -- all present and thorough
• Related section references parent issue (#366), dependency (#367/PR #370), and closes #368
• Tests exist: 8 tests covering happy path, error paths, edge cases, and the critical setup_future_usage verification
• No secrets committed -- Stripe keys are accessed via _settings, not hardcoded
• No unnecessary file changes -- only checkout.py and test_first_payment.py modified
• Commit messages are descriptive (PR title: feat: add GET /checkout/first-payment endpoint with Stripe redirect)

PROCESS OBSERVATIONS

• Change failure risk: LOW. The endpoint is additive (new route, no modifications to existing routes). Stripe integration follows established patterns. Test coverage is comprehensive.
• Dependency chain: This PR correctly identifies its dependency on PR #370 (migration). The ProductCategory.monthly enum value must exist for the product lookup to work. Verify this is on main before merging.
• Pre-existing test failure: The PR notes 1 pre-existing failure in test_templated_email.py from PR #370 adding first_payment to EmailType without updating the test assertion. This should be tracked and fixed separately.

VERDICT: APPROVED

## PR #375 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Stripe SDK / pytest **Endpoint correctness** (`checkout.py`): - `GET /first-payment?token={contract_token}` -- correctly looks up Player by `contract_token` + `contract_status == ContractStatus.signed`, returns 404 for invalid/unsigned tokens. - `_calculate_prorated_cents()` formula: `round(monthly_fee * 25 / 30 / 5) * 5 * 100` -- matches the westside-contracts reference as documented. - Null `monthly_fee` defaults to `_DEFAULT_MONTHLY_FEE = 200` -- correct. - Duplicate prevention via `Order.status.in_([OrderStatus.paid, OrderStatus.pending])` -- matches existing `create-session` pattern exactly. - Stripe Customer create/reuse pattern -- correct, follows existing convention. - Order creation with `db.flush()` before Stripe session (to get `order.id`) then `db.commit()` after storing `stripe_checkout_session_id` -- correct pattern. - `RedirectResponse(url=checkout_session.url, status_code=307)` -- correct. **CRITICAL: `setup_future_usage` verification (requirement #8)** -- PRESENT and correct: ```python payment_intent_data={"setup_future_usage": "off_session"}, ``` This is in the `stripe.checkout.Session.create()` call. Card will be saved for future monthly billing. Verified. **Test coverage** (`test_first_payment.py` -- 359 lines, 8 tests): 1. `test_valid_token_redirects_to_stripe` -- verifies 307, Location header, Order amount (16500 cents), session ID stored 2. `test_invalid_token_returns_404` -- invalid token path 3. `test_unsigned_contract_returns_404` -- offered status rejection 4. `test_duplicate_order_returns_409` -- duplicate prevention 5. `test_180_fee_prorates_to_150` -- $180 tier ($15000 cents) 6. `test_null_monthly_fee_defaults_to_200` -- null default ($16500 cents) 7. `test_setup_future_usage_passed_to_stripe` -- explicitly verifies `payment_intent_data` kwargs 8. `test_existing_stripe_customer_reused` -- verifies `Customer.create` NOT called when `stripe_customer_id` exists All 10 requirements from the issue are covered. Test count is 8, which exceeds the 7 stated in the PR body (the 8th test for customer reuse is a bonus). **Stripe mocking approach**: `@patch("basketball_api.routes.checkout.stripe")` patches at the module level. Tests verify both the happy path redirect and the kwargs passed to Stripe. Solid. ### BLOCKERS **None.** All critical requirements met: - `setup_future_usage: "off_session"` is present and tested - Tests cover all specified scenarios plus a bonus customer-reuse test - No hardcoded secrets or credentials - Input is validated (token lookup with signed status check) - No SQL injection risk (SQLAlchemy ORM queries with parameterized filters) ### NITS 1. **`ProductCategory` import** -- The diff adds `ContractStatus` to the models import but does not show `ProductCategory` being added. The endpoint uses `ProductCategory.monthly` on the product lookup query. The PR states it depends on PR #370 (merged), which likely adds `monthly` to `ProductCategory` and may also add the import. If the local main I'm reading is stale, this is fine. However, verify that `ProductCategory` is imported in the merged branch's `checkout.py`. If missing, it would be a `NameError` at runtime. 2. **DRY: Stripe Customer create/reuse** -- The create-or-reuse-customer block is now duplicated in 4 places across the codebase: `checkout.py` (existing `create-session`), `checkout.py` (new `first-payment`), `subscriptions.py`, and `jersey.py`. This is a pre-existing pattern, not introduced by this PR, but worth noting for a future extraction into a shared helper like `get_or_create_stripe_customer(player, parent, db)`. Consider creating a backlog ticket. 3. **No active monthly product (500)** -- The endpoint returns HTTP 500 if no active monthly product is found. This is a server misconfiguration error, so 500 is arguably correct. An alternative would be 503 (Service Unavailable) to signal a temporary config issue, but 500 is acceptable. 4. **Unicode em-dash in product name** -- The Stripe line item uses `f"First Monthly Payment \u2013 {player.name}"` (em-dash). Minor -- works fine in Stripe, just noting for consistency if other product names use hyphens. 5. **Logging** -- Good structured logging with player name, ID, amount, order ID, and session ID. No concerns. ### SOP COMPLIANCE - [x] Branch named after issue: `368-checkout-first-payment` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and thorough - [x] Related section references parent issue (#366), dependency (#367/PR #370), and closes #368 - [x] Tests exist: 8 tests covering happy path, error paths, edge cases, and the critical `setup_future_usage` verification - [x] No secrets committed -- Stripe keys are accessed via `_settings`, not hardcoded - [x] No unnecessary file changes -- only `checkout.py` and `test_first_payment.py` modified - [x] Commit messages are descriptive (PR title: `feat: add GET /checkout/first-payment endpoint with Stripe redirect`) ### PROCESS OBSERVATIONS - **Change failure risk**: LOW. The endpoint is additive (new route, no modifications to existing routes). Stripe integration follows established patterns. Test coverage is comprehensive. - **Dependency chain**: This PR correctly identifies its dependency on PR #370 (migration). The `ProductCategory.monthly` enum value must exist for the product lookup to work. Verify this is on main before merging. - **Pre-existing test failure**: The PR notes 1 pre-existing failure in `test_templated_email.py` from PR #370 adding `first_payment` to `EmailType` without updating the test assertion. This should be tracked and fixed separately. ### VERDICT: APPROVED

forgejo_admin merged commit a1990821d5 into main 2026-04-07 20:13:17 +00:00

forgejo_admin deleted branch 368-checkout-first-payment 2026-04-07 20:13:17 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-07 20:13:18 +00:00

feat: add GET /checkout/first-payment endpoint with Stripe redirect (#375)

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

No description provided.