forgejo_admin/basketball-api

Fork 1

feat: add jersey size selection to checkout flow #142

Merged

forgejo_admin merged 1 commit from 141-jersey-size-selection into main

2026-03-21 21:17:09 +00:00

forgejo_admin commented

2026-03-21 21:10:44 +00:00

Owner

Summary

Add jersey size selection during checkout so parents can specify their child's jersey size. Size is required for paid options (reversible, jersey_warmup), ignored for opt-out, and persisted via Stripe session metadata on payment confirmation.

Changes

src/basketball_api/models.py — Added JerseySize enum (YS, YM, YL, YXL, AS, AM, AL, AXL) and jersey_size nullable column on Player
src/basketball_api/routes/jersey.py — Added GET /jersey/sizes endpoint, added size field to JerseyCheckoutRequest, added size validation (required for paid options, 422 if missing/invalid), pass jersey_size in Stripe session metadata
src/basketball_api/routes/webhooks.py — Webhook reads jersey_size from Stripe metadata and saves to player.jersey_size on payment confirmation; backwards-compatible (missing size = NULL)
alembic/versions/015_add_jersey_size_to_player.py — New migration adding jerseysize enum type and jersey_size column to players table
tests/test_jersey.py — 12 new/updated tests: sizes endpoint, size validation (missing, invalid, opt-out bypass), webhook persistence with size, backwards compat without size

Test Plan

pytest tests/ -k jersey — 35 tests pass
Verify GET /jersey/sizes returns 8 sizes with labels
Verify POST /jersey/checkout rejects paid options without size (422)
Verify opt-out works without size
Verify webhook persists jersey_size from Stripe metadata
Run alembic migration on staging DB

Review Checklist

Passed automated review-fix loop
No secrets or unnecessary files included
Commit messages are descriptive
ruff format and ruff check pass

Closes #141

## Summary Add jersey size selection during checkout so parents can specify their child's jersey size. Size is required for paid options (reversible, jersey_warmup), ignored for opt-out, and persisted via Stripe session metadata on payment confirmation. ## Changes - `src/basketball_api/models.py` — Added `JerseySize` enum (YS, YM, YL, YXL, AS, AM, AL, AXL) and `jersey_size` nullable column on Player - `src/basketball_api/routes/jersey.py` — Added `GET /jersey/sizes` endpoint, added `size` field to `JerseyCheckoutRequest`, added size validation (required for paid options, 422 if missing/invalid), pass `jersey_size` in Stripe session metadata - `src/basketball_api/routes/webhooks.py` — Webhook reads `jersey_size` from Stripe metadata and saves to `player.jersey_size` on payment confirmation; backwards-compatible (missing size = NULL) - `alembic/versions/015_add_jersey_size_to_player.py` — New migration adding `jerseysize` enum type and `jersey_size` column to players table - `tests/test_jersey.py` — 12 new/updated tests: sizes endpoint, size validation (missing, invalid, opt-out bypass), webhook persistence with size, backwards compat without size ## Test Plan - [x] `pytest tests/ -k jersey` — 35 tests pass - [ ] Verify `GET /jersey/sizes` returns 8 sizes with labels - [ ] Verify `POST /jersey/checkout` rejects paid options without size (422) - [ ] Verify opt-out works without size - [ ] Verify webhook persists jersey_size from Stripe metadata - [ ] Run alembic migration on staging DB ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets or unnecessary files included - [x] Commit messages are descriptive - [x] ruff format and ruff check pass ## Related - Closes #141

forgejo_admin added 1 commit

2026-03-21 21:10:44 +00:00

feat: add jersey size selection to checkout flow (#141 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

cd85584a93

Add jersey_size column to players table and require size selection for
paid jersey options at checkout. Size flows through Stripe session
metadata and is persisted by the webhook on payment confirmation.

New endpoints:
- GET /jersey/sizes — returns available sizes with labels
- POST /jersey/checkout — now accepts required `size` param for paid options

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

forgejo_admin commented

2026-03-21 21:11:22 +00:00

Author

Owner

Self-Review: PASS

Files reviewed: 5 changed files, +213/-6 lines

Checklist

Model: JerseySize enum with 8 values, jersey_size nullable column on Player
Migration 015: creates jerseysize enum type and column, downgrade drops both
GET /jersey/sizes: returns 8 sizes with human-readable labels, no auth required
POST /jersey/checkout: size required for paid options (422 if missing/invalid), ignored for opt-out, passed in Stripe metadata
Webhook: reads jersey_size from Stripe metadata, handles missing size (backwards compat) and invalid size (logs warning)
Tests: 35 passing -- sizes endpoint (3), size validation (3), webhook with size (2 updated), backwards compat (1 new), existing tests updated with size param
ruff format + ruff check clean
No secrets or unnecessary files

Notes

Validation order is correct: option validation -> size validation -> token lookup (fail fast before DB queries)
test_invalid_option_returns_422 intentionally omits size -- option validation fires first, which is correct
Backwards compatible: webhook handles sessions created before this change (no jersey_size in metadata = NULL)

## Self-Review: PASS **Files reviewed:** 5 changed files, +213/-6 lines ### Checklist - [x] Model: `JerseySize` enum with 8 values, `jersey_size` nullable column on Player - [x] Migration 015: creates `jerseysize` enum type and column, downgrade drops both - [x] `GET /jersey/sizes`: returns 8 sizes with human-readable labels, no auth required - [x] `POST /jersey/checkout`: size required for paid options (422 if missing/invalid), ignored for opt-out, passed in Stripe metadata - [x] Webhook: reads `jersey_size` from Stripe metadata, handles missing size (backwards compat) and invalid size (logs warning) - [x] Tests: 35 passing -- sizes endpoint (3), size validation (3), webhook with size (2 updated), backwards compat (1 new), existing tests updated with size param - [x] ruff format + ruff check clean - [x] No secrets or unnecessary files ### Notes - Validation order is correct: option validation -> size validation -> token lookup (fail fast before DB queries) - `test_invalid_option_returns_422` intentionally omits `size` -- option validation fires first, which is correct - Backwards compatible: webhook handles sessions created before this change (no `jersey_size` in metadata = NULL)

forgejo_admin commented

2026-03-21 21:13:31 +00:00

Author

Owner

PR #142 Review

DOMAIN REVIEW

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

Python/FastAPI/SQLAlchemy assessment:

Model layer (models.py): JerseySize enum is clean, follows the same pattern as existing enums (JerseyOption, JerseyOrderStatus). The jersey_size column is correctly typed as Mapped[JerseySize | None] with nullable=True. PEP 484 type hints are correct.
Route layer (routes/jersey.py):
- New GET /jersey/sizes endpoint is simple and correct. Returns Pydantic-validated responses.
- JerseyCheckoutRequest adds size: str | None = None -- optional field with correct default.
- Size validation logic is sound: required for paid options, ignored for opt-out, rejects invalid values with 422.
- Input validation uses a pre-built _VALID_SIZES set for O(1) lookup -- good.
- body.size is passed as-is into Stripe metadata (string), which is the correct Stripe pattern.
Webhook layer (routes/webhooks.py):
- Reads jersey_size from Stripe metadata, converts via JerseySize(value) with try/except for invalid values.
- Backwards compatible: missing jersey_size in metadata gracefully results in None (no crash).
- Logging updated to include size -- good observability.
Migration (015_add_jersey_size_to_player.py):
- Correctly chains from 014 (down_revision).
- Creates the postgres enum type with checkfirst=True before adding the column -- safe for re-runs.
- Downgrade drops column then drops enum type -- correct order.
- Column is nullable, so no data migration needed for existing rows.
Pydantic schemas: JerseySizeResponse is properly typed. JerseyCheckoutRequest.size uses str | None which is correct for the validation pattern (validated manually against _VALID_SIZES rather than using Pydantic enum validation, which keeps the error messages user-friendly).

BLOCKERS

None.

Test coverage: 12 new/updated tests covering sizes endpoint (count, values, no-auth), checkout validation (missing size, invalid size, opt-out bypass), Stripe session metadata (size included), webhook persistence (size saved, backwards compat without size). All paths are covered.
Input validation: Size is validated server-side before use. Invalid values return 422 with descriptive error messages. No SQL injection risk -- the value is checked against a hardcoded set before any DB interaction.
No secrets in code: Stripe keys come from settings, not hardcoded.
No DRY violation in auth/security paths: No auth logic was touched.

NITS

Triple definition of size values: The size values appear in (a) JerseySize enum in models.py, (b) JERSEY_SIZES list in jersey.py, and (c) the migration. The migration is a frozen snapshot (correct), but JERSEY_SIZES could be derived from the enum to prevent future drift. Consider:
```
_VALID_SIZES = {s.value for s in JerseySize}
```
and building JERSEY_SIZES with a label map. Not blocking since the values are static and match today.
Comment typo in jersey.py line 125: "rejected for opt-out is N/A" -- reads awkwardly. Minor.
Opt-out with size silently ignored: When a user sends {"option": "opt_out", "size": "YL"}, the size is silently ignored. This is arguably correct (opt-out doesn't need a size), but a stricter approach would reject the request or log a warning. Non-blocking.

SOP COMPLIANCE

Branch named after issue: 141-jersey-size-selection references issue #141
PR body has Summary, Changes, Test Plan, Related sections
Related section references the plan slug: PR body says "Closes #141" but does not reference plan-wkq
No secrets committed
No unnecessary file changes (5 files, all directly related to jersey size feature)
Commit messages are descriptive (PR title follows conventional commits: feat: add jersey size selection to checkout flow)

PROCESS OBSERVATIONS

Deployment frequency: Clean, focused feature addition. Migration is additive (nullable column), so rollback is safe -- just drop the column. Low change failure risk.
Test plan: Checked items show pytest passing. Unchecked items are manual verification steps for staging, which is appropriate for a Stripe integration.
Plan reference gap: The Related section should reference plan-wkq per SOP. This is a process nit, not a code blocker.

VERDICT: APPROVED

## PR #142 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic / Stripe / pytest **Python/FastAPI/SQLAlchemy assessment**: 1. **Model layer** (`models.py`): `JerseySize` enum is clean, follows the same pattern as existing enums (`JerseyOption`, `JerseyOrderStatus`). The `jersey_size` column is correctly typed as `Mapped[JerseySize | None]` with `nullable=True`. PEP 484 type hints are correct. 2. **Route layer** (`routes/jersey.py`): - New `GET /jersey/sizes` endpoint is simple and correct. Returns Pydantic-validated responses. - `JerseyCheckoutRequest` adds `size: str | None = None` -- optional field with correct default. - Size validation logic is sound: required for paid options, ignored for opt-out, rejects invalid values with 422. - Input validation uses a pre-built `_VALID_SIZES` set for O(1) lookup -- good. - `body.size` is passed as-is into Stripe metadata (string), which is the correct Stripe pattern. 3. **Webhook layer** (`routes/webhooks.py`): - Reads `jersey_size` from Stripe metadata, converts via `JerseySize(value)` with try/except for invalid values. - Backwards compatible: missing `jersey_size` in metadata gracefully results in `None` (no crash). - Logging updated to include size -- good observability. 4. **Migration** (`015_add_jersey_size_to_player.py`): - Correctly chains from `014` (down_revision). - Creates the postgres enum type with `checkfirst=True` before adding the column -- safe for re-runs. - Downgrade drops column then drops enum type -- correct order. - Column is nullable, so no data migration needed for existing rows. 5. **Pydantic schemas**: `JerseySizeResponse` is properly typed. `JerseyCheckoutRequest.size` uses `str | None` which is correct for the validation pattern (validated manually against `_VALID_SIZES` rather than using Pydantic enum validation, which keeps the error messages user-friendly). ### BLOCKERS None. - **Test coverage**: 12 new/updated tests covering sizes endpoint (count, values, no-auth), checkout validation (missing size, invalid size, opt-out bypass), Stripe session metadata (size included), webhook persistence (size saved, backwards compat without size). All paths are covered. - **Input validation**: Size is validated server-side before use. Invalid values return 422 with descriptive error messages. No SQL injection risk -- the value is checked against a hardcoded set before any DB interaction. - **No secrets in code**: Stripe keys come from settings, not hardcoded. - **No DRY violation in auth/security paths**: No auth logic was touched. ### NITS 1. **Triple definition of size values**: The size values appear in (a) `JerseySize` enum in `models.py`, (b) `JERSEY_SIZES` list in `jersey.py`, and (c) the migration. The migration is a frozen snapshot (correct), but `JERSEY_SIZES` could be derived from the enum to prevent future drift. Consider: ```python _VALID_SIZES = {s.value for s in JerseySize} ``` and building `JERSEY_SIZES` with a label map. Not blocking since the values are static and match today. 2. **Comment typo** in `jersey.py` line 125: `"rejected for opt-out is N/A"` -- reads awkwardly. Minor. 3. **Opt-out with size silently ignored**: When a user sends `{"option": "opt_out", "size": "YL"}`, the size is silently ignored. This is arguably correct (opt-out doesn't need a size), but a stricter approach would reject the request or log a warning. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `141-jersey-size-selection` references issue #141 - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references the plan slug: PR body says "Closes #141" but does not reference `plan-wkq` - [x] No secrets committed - [x] No unnecessary file changes (5 files, all directly related to jersey size feature) - [x] Commit messages are descriptive (PR title follows conventional commits: `feat: add jersey size selection to checkout flow`) ### PROCESS OBSERVATIONS - **Deployment frequency**: Clean, focused feature addition. Migration is additive (nullable column), so rollback is safe -- just drop the column. Low change failure risk. - **Test plan**: Checked items show `pytest` passing. Unchecked items are manual verification steps for staging, which is appropriate for a Stripe integration. - **Plan reference gap**: The Related section should reference `plan-wkq` per SOP. This is a process nit, not a code blocker. ### VERDICT: APPROVED