fix: sync jersey fields to player record from generic checkout webhook #172

Merged

forgejo_admin merged 2 commits from 170-fix-generic-checkout-jersey-sync into main 2026-03-26 22:54:28 +00:00

Conversation 2 Commits 2 Files changed 6 +564 -1

forgejo_admin commented 2026-03-26 22:21:38 +00:00

Owner

Copy link

Summary

The generic checkout system (orders table from migration 013) was not syncing jersey_option, jersey_order_status, jersey_size, or jersey_number back to the players table after payment. Parents who ordered through /checkout/create-session appeared to have no jersey order when querying players. Two confirmed affected players: Anaiyah Fesolai (id=96) and Ayvah Apaisa (id=112).

Changes

• src/basketball_api/services/jersey_sync.py (NEW) -- Shared service module with sync_player_jersey_from_order() and map_product_to_jersey_option(). Maps product name/type to JerseyOption enum, extracts sizing from order.custom_data, and updates player record. Only acts on jersey-category products.
• src/basketball_api/routes/webhooks.py -- _handle_generic_order_completed() now loads the product and calls sync_player_jersey_from_order() after setting order.status = paid.
• src/basketball_api/routes/checkout.py -- Opt-out path in create_checkout_session() now calls sync_player_jersey_from_order() after creating the order. Added duplicate order prevention (409 for existing paid/pending orders for same player+product).
• alembic/versions/023_backfill_player_jersey_from_orders.py (NEW) -- One-time data migration to reconcile existing paid jersey orders back to player records. Idempotent, guarded by WHERE jersey_order_status = 'none' to avoid overwriting legacy-path data.
• tests/test_checkout.py -- 7 new tests: webhook jersey sync (reversible + warmup), non-jersey product skip, opt-out sync, duplicate order prevention (paid, pending, canceled-allows-reorder).

Test Plan

• python -m pytest tests/test_checkout.py tests/test_jersey.py -v -- 66 passed
• Full suite: 554 passed, 1 pre-existing failure (test_temp_team_dedup in admin teams, unrelated -- confirmed fails on main)
• ruff format and ruff check clean

Review Checklist

• Webhook handler syncs jersey fields for jersey-category products
• Opt-out checkout path syncs jersey fields
• Non-jersey products (tournament, equipment) do not touch player jersey fields
• Duplicate order prevention returns 409 for paid/pending orders
• Canceled orders allow re-ordering
• Alembic migration 023 is idempotent (guards with jersey_order_status = 'none')
• Migration does not overwrite legacy-path data
• All new and extended tests pass
• ruff format and ruff check clean

Related

• Closes #170
• Split out: #171 (Baby Betty contradictory state -- manual data fix, not code)
• Introduced by: alembic/versions/013_generic_checkout_system.py

## Summary The generic checkout system (`orders` table from migration 013) was not syncing `jersey_option`, `jersey_order_status`, `jersey_size`, or `jersey_number` back to the `players` table after payment. Parents who ordered through `/checkout/create-session` appeared to have no jersey order when querying players. Two confirmed affected players: Anaiyah Fesolai (id=96) and Ayvah Apaisa (id=112). ## Changes - **`src/basketball_api/services/jersey_sync.py`** (NEW) -- Shared service module with `sync_player_jersey_from_order()` and `map_product_to_jersey_option()`. Maps product name/type to `JerseyOption` enum, extracts sizing from `order.custom_data`, and updates player record. Only acts on jersey-category products. - **`src/basketball_api/routes/webhooks.py`** -- `_handle_generic_order_completed()` now loads the product and calls `sync_player_jersey_from_order()` after setting `order.status = paid`. - **`src/basketball_api/routes/checkout.py`** -- Opt-out path in `create_checkout_session()` now calls `sync_player_jersey_from_order()` after creating the order. Added duplicate order prevention (409 for existing paid/pending orders for same player+product). - **`alembic/versions/023_backfill_player_jersey_from_orders.py`** (NEW) -- One-time data migration to reconcile existing paid jersey orders back to player records. Idempotent, guarded by `WHERE jersey_order_status = 'none'` to avoid overwriting legacy-path data. - **`tests/test_checkout.py`** -- 7 new tests: webhook jersey sync (reversible + warmup), non-jersey product skip, opt-out sync, duplicate order prevention (paid, pending, canceled-allows-reorder). ## Test Plan - `python -m pytest tests/test_checkout.py tests/test_jersey.py -v` -- 66 passed - Full suite: 554 passed, 1 pre-existing failure (`test_temp_team_dedup` in admin teams, unrelated -- confirmed fails on main) - `ruff format` and `ruff check` clean ## Review Checklist - [x] Webhook handler syncs jersey fields for jersey-category products - [x] Opt-out checkout path syncs jersey fields - [x] Non-jersey products (tournament, equipment) do not touch player jersey fields - [x] Duplicate order prevention returns 409 for paid/pending orders - [x] Canceled orders allow re-ordering - [x] Alembic migration 023 is idempotent (guards with `jersey_order_status = 'none'`) - [x] Migration does not overwrite legacy-path data - [x] All new and extended tests pass - [x] ruff format and ruff check clean ## Related - Closes #170 - Split out: #171 (Baby Betty contradictory state -- manual data fix, not code) - Introduced by: `alembic/versions/013_generic_checkout_system.py`

forgejo_admin added 1 commit 2026-03-26 22:21:38 +00:00

fix: sync jersey fields to player record from generic checkout webhook ... f3abf24a76

The generic checkout system (orders table) was not syncing jersey_option,
jersey_order_status, jersey_size, or jersey_number back to the players
table after payment. Parents who ordered through /checkout/create-session
appeared to have no jersey order when querying players.

- Add jersey_sync service module with product-to-JerseyOption mapping
- Wire sync into _handle_generic_order_completed webhook handler
- Wire sync into opt-out path in checkout.py create_checkout_session
- Add duplicate order prevention (409 for existing paid/pending orders)
- Add alembic migration 023 to backfill existing paid jersey orders
- Add 7 new tests covering jersey sync, non-jersey skip, opt-out, and
  duplicate prevention

Closes #170

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

forgejo_admin added 1 commit 2026-03-26 22:22:45 +00:00

fix: add deprecation warning to legacy /jersey/checkout endpoint ... d7d3764e96

Per issue #170 decisions: legacy path stays active but logs a warning
on each call to guide migration toward /checkout/create-session.

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

forgejo_admin commented 2026-03-26 22:23:04 +00:00

Author

Owner

Copy link

Self-Review

Reviewed the full diff against issue #170 acceptance criteria.

Findings

1. Missing deprecation warning on legacy endpoint (FIXED)
Issue spec under Decisions says: "Legacy path (/jersey/checkout): Keep active for now. Add a deprecation logger.warning on each call." This was missing from the initial commit. Fixed in commit d7d3764 -- added logger.warning("DEPRECATED: /jersey/checkout called. Use /checkout/create-session instead.") at the top of jersey_checkout().

2. Missing test: test_webhook_skips_player_sync_for_non_jersey_product (FIXED)
Issue spec listed this as a required test. Added in commit f3abf24 -- creates a tournament-category product, fires a webhook, and asserts player jersey fields remain untouched.

Verified Against Acceptance Criteria

• _handle_generic_order_completed syncs player jersey fields for jersey-category products
• checkout.py opt-out path syncs player jersey fields
• Alembic migration 023 backfills existing paid jersey orders (idempotent, guarded by jersey_order_status = 'none')
• Duplicate order guard returns 409 for existing paid/pending orders
• All 8 new tests pass (7 in test_checkout.py + deprecation warning coverage via existing jersey tests)
• Legacy endpoint deprecation warning added
• Full suite: 554 passed, 1 pre-existing failure (unrelated test_temp_team_dedup)
• ruff format and ruff check clean

No Issues Found

Code is ready for human review.

## Self-Review Reviewed the full diff against issue #170 acceptance criteria. ### Findings **1. Missing deprecation warning on legacy endpoint (FIXED)** Issue spec under Decisions says: "Legacy path (`/jersey/checkout`): Keep active for now. Add a deprecation `logger.warning` on each call." This was missing from the initial commit. Fixed in commit d7d3764 -- added `logger.warning("DEPRECATED: /jersey/checkout called. Use /checkout/create-session instead.")` at the top of `jersey_checkout()`. **2. Missing test: `test_webhook_skips_player_sync_for_non_jersey_product` (FIXED)** Issue spec listed this as a required test. Added in commit f3abf24 -- creates a tournament-category product, fires a webhook, and asserts player jersey fields remain untouched. ### Verified Against Acceptance Criteria - [x] `_handle_generic_order_completed` syncs player jersey fields for jersey-category products - [x] `checkout.py` opt-out path syncs player jersey fields - [x] Alembic migration 023 backfills existing paid jersey orders (idempotent, guarded by `jersey_order_status = 'none'`) - [x] Duplicate order guard returns 409 for existing paid/pending orders - [x] All 8 new tests pass (7 in test_checkout.py + deprecation warning coverage via existing jersey tests) - [x] Legacy endpoint deprecation warning added - [x] Full suite: 554 passed, 1 pre-existing failure (unrelated `test_temp_team_dedup`) - [x] ruff format and ruff check clean ### No Issues Found Code is ready for human review.

forgejo_admin commented 2026-03-26 22:25:51 +00:00

Author

Owner

Copy link

PR #172 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Alembic / Stripe webhooks

Architecture (jersey_sync.py service)

The new services/jersey_sync.py module correctly centralizes the jersey-field sync logic shared between the webhook handler (paid orders) and the checkout route (opt-outs). Both call sites (webhooks.py:167, checkout.py:181) pass (db, order, product) cleanly. The service guards on ProductCategory.jersey before touching player records, preventing unintended side effects on tournament/equipment products.

map_product_to_jersey_option() uses name-based matching ("warmup", "reversible", "jersey" substring checks) which is fragile if product names change. However, this mirrors existing product naming conventions and is the pragmatic approach given that ProductType only distinguishes one_time/subscription/opt_out -- there is no JerseyOption field on the Product model itself. Acceptable for now.

Webhook handler (webhooks.py)

The _handle_generic_order_completed() function correctly loads the product and calls sync_player_jersey_from_order() after setting order.status = paid. The product fetch (db.query(Product).filter(Product.id == order.product_id).first()) is guarded with if product:. Good defensive coding.

Note: The legacy _handle_jersey_checkout_completed() (lines 180-235) still contains inline jersey-sync logic that reads from Stripe metadata rather than order.custom_data. This is intentionally kept for backward compatibility with existing checkout sessions created via the legacy /jersey/checkout endpoint. The deprecation warning on that endpoint (jersey.py:182) makes the migration path clear.

Checkout route (checkout.py)

The duplicate order prevention logic (lines 147-164) is well-scoped -- checks for paid or pending status on the same (player_id, product_id) pair. Returns 409 with an informative message including the existing order ID. Canceled orders correctly allow re-ordering.

The db.flush() before sync_player_jersey_from_order() in the opt-out path (line 178) is correct -- ensures order.id is available before the sync function queries for the player.

Migration 023

Idempotent backfill migration. Correctly uses sa.table() inline references (no ORM dependency). The WHERE jersey_order_status = 'none' guard prevents overwriting data set by the legacy jersey checkout flow. The migration's _map_name_to_jersey_option() duplicates the service-layer mapping, but this is correct practice for migrations -- they must be self-contained.

No-op downgrade is documented with rationale. Chain is correct: 022 -> 023.

Type hints and docstrings: All new functions have PEP 484 type hints and PEP 257 docstrings. Good.

Error handling: JerseySize validation uses try/except ValueError with a warning log on invalid values (jersey_sync.py:76). jersey_number is coerced to string (line 81). Both are defensive and correct.

BLOCKERS

None.

NITS

1. Legacy handler DRY (webhooks.py:180-235): _handle_jersey_checkout_completed duplicates jersey-sync logic inline. When the legacy endpoint is eventually removed, this handler should also be removed or refactored to delegate to jersey_sync.py. Consider opening a cleanup issue to track this.

2. Name-based product mapping fragility (jersey_sync.py:33-37): The map_product_to_jersey_option() function relies on substring matching against product names. If someone creates a product named "Tournament Jersey Giveaway" in the equipment category, and it somehow had category=jersey, the mapping would incorrectly return reversible. The category guard in sync_player_jersey_from_order mitigates this, but a future improvement could add a jersey_option column to the Product model for explicit mapping.

3. Migration print() statement (023:119): print(f"Backfilled jersey fields for {updated} player(s)...") -- consider using op.get_context().config logger or just dropping it. print() in migrations works but is noisy in automated runs. Very minor.

SOP COMPLIANCE

• Branch named after issue (170-fix-generic-checkout-jersey-sync -> issue #170)
• PR body has: Summary, Changes, Test Plan, Related
• Related section references parent issue (Closes #170) and discovered scope (#171)
• Tests exist and pass (7 new tests, 554 total passed per PR body)
• No secrets, .env files, or credentials committed
• No unnecessary file changes (6 files, all on-topic)
• Commit messages are descriptive
• Related section does not reference a plan slug -- acceptable for board-driven bug fix (no plan)

PROCESS OBSERVATIONS

• Discovered scope handled correctly: Issue #171 (Baby Betty contradictory state) was split out as a separate manual data fix issue, not bundled into this code PR. Good discipline.
• Legacy deprecation path: The /jersey/checkout endpoint now has a deprecation warning and docstring notice. The legacy webhook handler is preserved for in-flight sessions. Clean migration path.
• Duplicate order prevention is a defensive improvement beyond the original bug scope (jersey sync). This is a reasonable scope addition since it prevents the same class of data inconsistency that caused the bug.
• Change failure risk: Low. The jersey sync service is additive -- it writes player fields that were previously left as defaults. The migration guard (jersey_order_status = 'none') prevents overwriting legitimate data. Tests cover happy path, edge cases, and negative cases.

VERDICT: APPROVED

## PR #172 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Alembic / Stripe webhooks **Architecture (jersey_sync.py service)** The new `services/jersey_sync.py` module correctly centralizes the jersey-field sync logic shared between the webhook handler (paid orders) and the checkout route (opt-outs). Both call sites (`webhooks.py:167`, `checkout.py:181`) pass `(db, order, product)` cleanly. The service guards on `ProductCategory.jersey` before touching player records, preventing unintended side effects on tournament/equipment products. `map_product_to_jersey_option()` uses name-based matching (`"warmup"`, `"reversible"`, `"jersey"` substring checks) which is fragile if product names change. However, this mirrors existing product naming conventions and is the pragmatic approach given that `ProductType` only distinguishes `one_time`/`subscription`/`opt_out` -- there is no `JerseyOption` field on the `Product` model itself. Acceptable for now. **Webhook handler (webhooks.py)** The `_handle_generic_order_completed()` function correctly loads the product and calls `sync_player_jersey_from_order()` after setting `order.status = paid`. The product fetch (`db.query(Product).filter(Product.id == order.product_id).first()`) is guarded with `if product:`. Good defensive coding. Note: The legacy `_handle_jersey_checkout_completed()` (lines 180-235) still contains inline jersey-sync logic that reads from Stripe metadata rather than `order.custom_data`. This is intentionally kept for backward compatibility with existing checkout sessions created via the legacy `/jersey/checkout` endpoint. The deprecation warning on that endpoint (`jersey.py:182`) makes the migration path clear. **Checkout route (checkout.py)** The duplicate order prevention logic (lines 147-164) is well-scoped -- checks for `paid` or `pending` status on the same `(player_id, product_id)` pair. Returns 409 with an informative message including the existing order ID. Canceled orders correctly allow re-ordering. The `db.flush()` before `sync_player_jersey_from_order()` in the opt-out path (line 178) is correct -- ensures `order.id` is available before the sync function queries for the player. **Migration 023** Idempotent backfill migration. Correctly uses `sa.table()` inline references (no ORM dependency). The `WHERE jersey_order_status = 'none'` guard prevents overwriting data set by the legacy jersey checkout flow. The migration's `_map_name_to_jersey_option()` duplicates the service-layer mapping, but this is correct practice for migrations -- they must be self-contained. No-op downgrade is documented with rationale. Chain is correct: `022 -> 023`. **Type hints and docstrings**: All new functions have PEP 484 type hints and PEP 257 docstrings. Good. **Error handling**: `JerseySize` validation uses try/except ValueError with a warning log on invalid values (jersey_sync.py:76). `jersey_number` is coerced to string (line 81). Both are defensive and correct. ### BLOCKERS None. ### NITS 1. **Legacy handler DRY** (`webhooks.py:180-235`): `_handle_jersey_checkout_completed` duplicates jersey-sync logic inline. When the legacy endpoint is eventually removed, this handler should also be removed or refactored to delegate to `jersey_sync.py`. Consider opening a cleanup issue to track this. 2. **Name-based product mapping fragility** (`jersey_sync.py:33-37`): The `map_product_to_jersey_option()` function relies on substring matching against product names. If someone creates a product named "Tournament Jersey Giveaway" in the equipment category, and it somehow had `category=jersey`, the mapping would incorrectly return `reversible`. The category guard in `sync_player_jersey_from_order` mitigates this, but a future improvement could add a `jersey_option` column to the `Product` model for explicit mapping. 3. **Migration `print()` statement** (`023:119`): `print(f"Backfilled jersey fields for {updated} player(s)...")` -- consider using `op.get_context().config` logger or just dropping it. `print()` in migrations works but is noisy in automated runs. Very minor. ### SOP COMPLIANCE - [x] Branch named after issue (`170-fix-generic-checkout-jersey-sync` -> issue #170) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references parent issue (`Closes #170`) and discovered scope (`#171`) - [x] Tests exist and pass (7 new tests, 554 total passed per PR body) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (6 files, all on-topic) - [x] Commit messages are descriptive - [ ] Related section does not reference a plan slug -- acceptable for board-driven bug fix (no plan) ### PROCESS OBSERVATIONS - **Discovered scope handled correctly**: Issue #171 (Baby Betty contradictory state) was split out as a separate manual data fix issue, not bundled into this code PR. Good discipline. - **Legacy deprecation path**: The `/jersey/checkout` endpoint now has a deprecation warning and docstring notice. The legacy webhook handler is preserved for in-flight sessions. Clean migration path. - **Duplicate order prevention** is a defensive improvement beyond the original bug scope (jersey sync). This is a reasonable scope addition since it prevents the same class of data inconsistency that caused the bug. - **Change failure risk**: Low. The jersey sync service is additive -- it writes player fields that were previously left as defaults. The migration guard (`jersey_order_status = 'none'`) prevents overwriting legitimate data. Tests cover happy path, edge cases, and negative cases. ### VERDICT: APPROVED

forgejo_admin merged commit 7ef6ebf197 into main 2026-03-26 22:54:28 +00:00

forgejo_admin deleted branch 170-fix-generic-checkout-jersey-sync 2026-03-26 22:54:29 +00:00

forgejo_admin referenced this pull request from a commit 2026-03-26 22:54:30 +00:00

fix: sync jersey fields to player record from generic checkout webhook (#172)

forgejo_admin referenced this pull request 2026-03-26 23:15:07 +00:00

Bug: teams/save endpoint appends teams instead of replacing assignment #173

forgejo_admin referenced this pull request 2026-03-26 23:42:01 +00:00

fix: clear team assignments before appending in teams/save #174

forgejo_admin referenced this pull request 2026-03-26 23:43:49 +00:00

fix: clear team assignments before appending in teams/save #174

forgejo_admin referenced this pull request 2026-03-27 03:42:31 +00:00

Bug: Alembic migration crash on deploy — forked chain from revision 018 #179

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

No description provided.