feat: add jersey number selection to checkout flow

forgejo_admin commented

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

Author

Owner

PR #146 Review

DOMAIN REVIEW

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

This PR adds jersey number selection to the checkout flow: a new jersey_number column on the Player model, range + per-division uniqueness validation in POST /jersey/checkout, a new GET /jersey/taken-numbers endpoint, Stripe metadata propagation, webhook persistence, a clean Alembic migration, and 16 new tests.

Model layer (models.py):

jersey_number: Mapped[str | None] = mapped_column(String(2), nullable=True) -- correctly nullable, String(2) accommodates "0", "00", and "1"-"99". Migration 016 matches the model definition exactly. Good.

Validation (jersey.py):

_VALID_JERSEY_NUMBERS uses a set comprehension with {"0", "00"} | {str(n) for n in range(1, 100)} -- correct NFHS rule coverage. Comment references NFHS 2025-2026, which is a nice touch.
Input validation rejects numbers outside this set (including negative, 100+, alphabetic).
Per-division uniqueness check queries correctly: filters on Player.division, Player.jersey_number, and Player.id != player.id. The Player.jersey_number.isnot(None) in the uniqueness filter at line 209 is technically redundant (since == body.number already excludes NULLs), but harmless and arguably defensive.
Opt-out correctly skips number validation.

Taken-numbers endpoint (jersey.py):

Division is validated by attempting Division(division) with a clean 422 on failure.
Query filters on division + non-null jersey_number. Returns a flat list of strings. Clean and correct.
No auth required -- consistent with the existing /jersey/options and /jersey/sizes endpoints being public.

Webhook (webhooks.py):

Reads jersey_number from Stripe metadata and persists directly. Backwards compatible -- missing jersey_number in metadata is handled gracefully (stays null).

Stripe metadata:

jersey_number added to the Stripe checkout session metadata alongside jersey_option and jersey_size. Consistent with the existing pattern.

Migration (016):

Simple add_column / drop_column pair. Nullable column, no data migration needed. Clean.

Test coverage (test_jersey.py):

16 new tests across 3 test classes:
- TestJerseyNumberValidation (8 tests): missing number, invalid number, negative number, duplicate in same division, cross-division success, opt-out no number, metadata passthrough, "0" and "00" edge cases.
- TestJerseyTakenNumbers (4 tests): returns correct numbers, empty list, invalid division, no auth required.
- TestJerseyWebhook additions (2 tests): webhook saves number from metadata, webhook without number still works.
Existing tests updated to include "number" in request payloads. Good backwards-compat hygiene.
Tests cover happy path, edge cases, error handling, cross-division logic, and backwards compatibility. Thorough.

BLOCKERS

None.

All blocker criteria pass:

Test coverage: 16 new tests covering all new functionality paths -- validation, uniqueness, taken-numbers, webhook persistence, edge cases, backwards compat.
Input validation: Jersey number is validated against _VALID_JERSEY_NUMBERS set before any DB operation. Division parameter validated before query.
No secrets: No credentials, API keys, or .env files in the diff.
No DRY violations in auth/security: No duplicated auth logic.

NITS

Webhook does not validate jersey_number from Stripe metadata -- webhooks.py:213-214 writes jersey_number_str directly to the player without checking it against _VALID_JERSEY_NUMBERS or verifying it fits String(2). In practice, the metadata comes from the checkout session this code created, so it should always be valid. However, defense-in-depth would suggest a quick validation or at minimum a length guard, since Stripe metadata is technically mutable by the merchant dashboard. Low risk but worth noting.
Redundant isnot(None) in uniqueness query -- jersey.py:209 (Player.jersey_number.isnot(None)) is redundant with Player.jersey_number == body.number on line 208, since SQL equality with a non-null value already excludes NULLs. Not wrong, just unnecessary.
No tenant scoping on GET /jersey/taken-numbers -- the query filters by division but not tenant. This is consistent with the existing jersey routes (which have zero tenant references) and the plan explicitly says "built for Westside first, multi-tenant only if another program wants in." Not a blocker, but when multi-tenancy lands, this endpoint will need a tenant filter. Track as discovered scope.
Race condition on uniqueness check -- the uniqueness check at checkout time (jersey.py:204-213) is application-level, not a database unique constraint. Two concurrent requests for the same number in the same division could both pass validation. A composite unique index on (division, jersey_number) where jersey_number IS NOT NULL would be the robust solution. Given the current user base (53 players, low concurrency), this is low risk, but worth tracking for when the platform scales.
Triple definition pattern continues -- the epilogue for PR #142 already notes that jersey sizes are defined in three places (enum, route list, validation set). This PR adds _VALID_JERSEY_NUMBERS as yet another standalone definition. The set is algorithmically derived (not repeated data), so this is less fragile than the size case, but the pattern of parallel definitions is growing.

SOP COMPLIANCE

Branch named after issue: 143-jersey-number-selection matches issue #143
PR body has: Summary, Changes, Test Plan, Related -- all present and well-structured
Related section references plan-wkq Phase 11, links to prior PRs #140 and #142
Tests exist: 16 new tests, existing tests updated
No secrets committed
No unnecessary file changes -- exactly 5 files, all directly related to jersey number selection
Commit messages are descriptive (per PR title)

PROCESS OBSERVATIONS

Deployment frequency: This is the third jersey enhancement in sequence (#140 timing fix, #142 size selection, #146 number selection). Clean incremental delivery -- each PR is independently deployable and backwards compatible. Positive DORA signal.

Change failure risk: Low. The migration is additive (nullable column), the webhook is backwards compatible, and validation is thorough. The only persistence concern is the webhook accepting unvalidated metadata (nit 1), but the blast radius is limited to a single player record.

Documentation: The PR body is exemplary -- clear summary, change list, test plan with checkboxes, and related context. The NFHS rules citation in the code comment adds context for future maintainers.

VERDICT: APPROVED

## PR #146 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy / Alembic / Stripe / pytest This PR adds jersey number selection to the checkout flow: a new `jersey_number` column on the `Player` model, range + per-division uniqueness validation in `POST /jersey/checkout`, a new `GET /jersey/taken-numbers` endpoint, Stripe metadata propagation, webhook persistence, a clean Alembic migration, and 16 new tests. **Model layer** (`models.py`): - `jersey_number: Mapped[str | None] = mapped_column(String(2), nullable=True)` -- correctly nullable, `String(2)` accommodates "0", "00", and "1"-"99". Migration 016 matches the model definition exactly. Good. **Validation** (`jersey.py`): - `_VALID_JERSEY_NUMBERS` uses a set comprehension with `{"0", "00"} | {str(n) for n in range(1, 100)}` -- correct NFHS rule coverage. Comment references NFHS 2025-2026, which is a nice touch. - Input validation rejects numbers outside this set (including negative, 100+, alphabetic). - Per-division uniqueness check queries correctly: filters on `Player.division`, `Player.jersey_number`, and `Player.id != player.id`. The `Player.jersey_number.isnot(None)` in the uniqueness filter at line 209 is technically redundant (since `== body.number` already excludes NULLs), but harmless and arguably defensive. - Opt-out correctly skips number validation. **Taken-numbers endpoint** (`jersey.py`): - Division is validated by attempting `Division(division)` with a clean 422 on failure. - Query filters on division + non-null jersey_number. Returns a flat list of strings. Clean and correct. - No auth required -- consistent with the existing `/jersey/options` and `/jersey/sizes` endpoints being public. **Webhook** (`webhooks.py`): - Reads `jersey_number` from Stripe metadata and persists directly. Backwards compatible -- missing `jersey_number` in metadata is handled gracefully (stays null). **Stripe metadata**: - `jersey_number` added to the Stripe checkout session metadata alongside `jersey_option` and `jersey_size`. Consistent with the existing pattern. **Migration** (016): - Simple `add_column` / `drop_column` pair. Nullable column, no data migration needed. Clean. **Test coverage** (`test_jersey.py`): - 16 new tests across 3 test classes: - `TestJerseyNumberValidation` (8 tests): missing number, invalid number, negative number, duplicate in same division, cross-division success, opt-out no number, metadata passthrough, "0" and "00" edge cases. - `TestJerseyTakenNumbers` (4 tests): returns correct numbers, empty list, invalid division, no auth required. - `TestJerseyWebhook` additions (2 tests): webhook saves number from metadata, webhook without number still works. - Existing tests updated to include `"number"` in request payloads. Good backwards-compat hygiene. - Tests cover happy path, edge cases, error handling, cross-division logic, and backwards compatibility. Thorough. ### BLOCKERS None. All blocker criteria pass: - **Test coverage**: 16 new tests covering all new functionality paths -- validation, uniqueness, taken-numbers, webhook persistence, edge cases, backwards compat. - **Input validation**: Jersey number is validated against `_VALID_JERSEY_NUMBERS` set before any DB operation. Division parameter validated before query. - **No secrets**: No credentials, API keys, or .env files in the diff. - **No DRY violations in auth/security**: No duplicated auth logic. ### NITS 1. **Webhook does not validate `jersey_number` from Stripe metadata** -- `webhooks.py:213-214` writes `jersey_number_str` directly to the player without checking it against `_VALID_JERSEY_NUMBERS` or verifying it fits `String(2)`. In practice, the metadata comes from the checkout session this code created, so it should always be valid. However, defense-in-depth would suggest a quick validation or at minimum a length guard, since Stripe metadata is technically mutable by the merchant dashboard. Low risk but worth noting. 2. **Redundant `isnot(None)` in uniqueness query** -- `jersey.py:209` (`Player.jersey_number.isnot(None)`) is redundant with `Player.jersey_number == body.number` on line 208, since SQL equality with a non-null value already excludes NULLs. Not wrong, just unnecessary. 3. **No tenant scoping on `GET /jersey/taken-numbers`** -- the query filters by division but not tenant. This is consistent with the existing jersey routes (which have zero tenant references) and the plan explicitly says "built for Westside first, multi-tenant only if another program wants in." Not a blocker, but when multi-tenancy lands, this endpoint will need a tenant filter. Track as discovered scope. 4. **Race condition on uniqueness check** -- the uniqueness check at checkout time (`jersey.py:204-213`) is application-level, not a database unique constraint. Two concurrent requests for the same number in the same division could both pass validation. A composite unique index on `(division, jersey_number)` where `jersey_number IS NOT NULL` would be the robust solution. Given the current user base (53 players, low concurrency), this is low risk, but worth tracking for when the platform scales. 5. **Triple definition pattern continues** -- the epilogue for PR #142 already notes that jersey sizes are defined in three places (enum, route list, validation set). This PR adds `_VALID_JERSEY_NUMBERS` as yet another standalone definition. The set is algorithmically derived (not repeated data), so this is less fragile than the size case, but the pattern of parallel definitions is growing. ### SOP COMPLIANCE - [x] Branch named after issue: `143-jersey-number-selection` matches issue #143 - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and well-structured - [x] Related section references `plan-wkq` Phase 11, links to prior PRs #140 and #142 - [x] Tests exist: 16 new tests, existing tests updated - [x] No secrets committed - [x] No unnecessary file changes -- exactly 5 files, all directly related to jersey number selection - [x] Commit messages are descriptive (per PR title) ### PROCESS OBSERVATIONS **Deployment frequency**: This is the third jersey enhancement in sequence (#140 timing fix, #142 size selection, #146 number selection). Clean incremental delivery -- each PR is independently deployable and backwards compatible. Positive DORA signal. **Change failure risk**: Low. The migration is additive (nullable column), the webhook is backwards compatible, and validation is thorough. The only persistence concern is the webhook accepting unvalidated metadata (nit 1), but the blast radius is limited to a single player record. **Documentation**: The PR body is exemplary -- clear summary, change list, test plan with checkboxes, and related context. The NFHS rules citation in the code comment adds context for future maintainers. ### VERDICT: APPROVED

2026-03-21 21:41:40 +00:00

Summary

Add jersey number selection to the checkout flow. Parents pick a number (0, 00, or 1-99) during jersey purchase, validated for uniqueness within their child's division. Numbers are persisted through the full Stripe checkout-to-webhook pipeline.

Changes

src/basketball_api/models.py: add jersey_number column (String(2), nullable) to Player model
src/basketball_api/routes/jersey.py: accept number param in POST /jersey/checkout with range + per-division uniqueness validation; add GET /jersey/taken-numbers?division= endpoint
src/basketball_api/routes/webhooks.py: read jersey_number from Stripe metadata and persist to player
alembic/versions/016_add_jersey_number_to_player.py: new migration adding column
tests/test_jersey.py: 16 new tests covering number validation, duplicate rejection, cross-division success, taken-numbers endpoint, and webhook persistence

Test Plan

pytest tests/ -k jersey -- 50 tests pass (34 existing updated + 16 new)
Duplicate number in same division returns 409
Same number in different division succeeds
Opt-out does not require number
Taken-numbers returns correct list per division
Webhook saves jersey_number from metadata
ruff format + ruff check pass

Review Checklist

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

Closes #143
plan-wkq -- Phase 11
Prior: #140 (jersey_option timing fix), #142 (jersey size support)

## Summary Add jersey number selection to the checkout flow. Parents pick a number (0, 00, or 1-99) during jersey purchase, validated for uniqueness within their child's division. Numbers are persisted through the full Stripe checkout-to-webhook pipeline. ## Changes - `src/basketball_api/models.py`: add `jersey_number` column (String(2), nullable) to Player model - `src/basketball_api/routes/jersey.py`: accept `number` param in `POST /jersey/checkout` with range + per-division uniqueness validation; add `GET /jersey/taken-numbers?division=` endpoint - `src/basketball_api/routes/webhooks.py`: read `jersey_number` from Stripe metadata and persist to player - `alembic/versions/016_add_jersey_number_to_player.py`: new migration adding column - `tests/test_jersey.py`: 16 new tests covering number validation, duplicate rejection, cross-division success, taken-numbers endpoint, and webhook persistence ## Test Plan - [x] `pytest tests/ -k jersey` -- 50 tests pass (34 existing updated + 16 new) - [x] Duplicate number in same division returns 409 - [x] Same number in different division succeeds - [x] Opt-out does not require number - [x] Taken-numbers returns correct list per division - [x] Webhook saves jersey_number from metadata - [x] ruff format + ruff check pass ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #143 - `plan-wkq` -- Phase 11 - Prior: #140 (jersey_option timing fix), #142 (jersey size support)

forgejo_admin added 1 commit

feat: add jersey number selection to checkout flow (#143 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

1966c85e79

Add jersey_number (String, nullable) to Player model with per-division
uniqueness validation. POST /jersey/checkout now requires a number param
for paid options (0, 00, 1-99) and rejects duplicates within the same
division. New GET /jersey/taken-numbers endpoint returns claimed numbers.
Webhook persists jersey_number from Stripe metadata.

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

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

Self-Review

Reviewed the full diff (5 files, +465/-10). No issues found.

Checklist:

Model: jersey_number String(2), nullable -- correct for "0"/"00"/"1"-"99"
Migration 016: clean add/drop, correct revision chain (015 -> 016)
Checkout: number required for paid options, range validated, per-division uniqueness enforced (409 on conflict)
GET /jersey/taken-numbers: filters by division, excludes NULL
Stripe metadata: jersey_number passed through checkout and persisted in webhook
Backwards compat: webhook handles missing jersey_number gracefully
Tests: 50 pass (34 existing updated + 16 new), ruff clean
No unrelated changes, no secrets

## Self-Review Reviewed the full diff (5 files, +465/-10). No issues found. **Checklist:** - [x] Model: `jersey_number` String(2), nullable -- correct for "0"/"00"/"1"-"99" - [x] Migration 016: clean add/drop, correct revision chain (015 -> 016) - [x] Checkout: number required for paid options, range validated, per-division uniqueness enforced (409 on conflict) - [x] `GET /jersey/taken-numbers`: filters by division, excludes NULL - [x] Stripe metadata: `jersey_number` passed through checkout and persisted in webhook - [x] Backwards compat: webhook handles missing `jersey_number` gracefully - [x] Tests: 50 pass (34 existing updated + 16 new), ruff clean - [x] No unrelated changes, no secrets