feat: generic checkout system — products + orders tables (#127) #128
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo_admin/basketball-api!128
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "127-feat-generic-checkout-system-products-ta"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
productsandorderstables (JSONB custom_fields/custom_data)/checkoutAPI routes replace one-off jersey payment flowChanges
src/basketball_api/models.py: Add Product, Order models + ProductType, ProductCategory, OrderStatus enums + JSONB importalembic/versions/013_generic_checkout_system.py: Create tables, seed 3 jersey products with number/size custom_fields, migrate 6 existing jersey orderssrc/basketball_api/routes/checkout.py: New router —GET /products,POST /create-session,GET /orderssrc/basketball_api/routes/webhooks.py: Add_handle_generic_order_completed()before legacy jersey handlersrc/basketball_api/main.py: Register checkout router at/checkoutTest Plan
GET /checkout/productsreturns 3 jersey products with custom_fieldsGET /checkout/products?category=jerseyfilters correctlyPOST /checkout/create-session(opt-out) creates paid order with custom_dataPOST /checkout/create-session($90 jersey) returns real Stripe checkout URLPOST /checkout/create-session(missing required field) returns 422GET /checkout/ordersreturns all orders including migrated legacy dataReview Checklist
Related
plan-wkq— Phase 11 girls jersey + Phase 14 contracts foundationPR #128 Review
DOMAIN REVIEW
Tech stack: Python / FastAPI / SQLAlchemy / Alembic / Stripe / PostgreSQL JSONB
This PR introduces a generic checkout system (products + orders) to replace the one-off jersey payment flow. 5 files changed, 471 additions, 7 deletions. The architecture is sound -- JSONB
custom_fieldson products withcustom_dataon orders is a clean pattern for extensible product types.BLOCKERS
1.
GET /checkout/ordershas NO authentication -- exposes all order data publiclycheckout.pyline 223-249:list_orders()has noDepends(require_admin)orDepends(get_current_user). Every other data-listing endpoint in this codebase (admin, players, teams, subscriptions, coaches_api) requires authentication. This endpoint returnsplayer_id,amount_cents,custom_data(which may contain jersey numbers, sizes, etc.), and order status for ALL orders. Anyone with network access can enumerate every order.This is an authorization bypass -- a BLOCKER per the "Unvalidated user input" and general security criteria.
Fix: Add
user: User = Depends(require_admin)tolist_orders(), consistent with the rest of the admin-facing endpoints in this codebase.2. No test coverage for any new functionality
Zero tests exist for the checkout routes or the updated webhook handler. The existing test suite has
tests/test_jersey.pywith thorough coverage of the analogous jersey flow (options listing, checkout with Stripe mocks, opt-out, webhook handling, customer reuse). The new checkout system needs equivalent coverage:GET /checkout/products-- returns products, category filtering worksPOST /checkout/create-session-- invalid token 404, missing required custom_data 422, opt-out creates paid order, Stripe session creation, customer reuseGET /checkout/orders-- requires auth (once fixed), filters by category/status_handle_generic_order_completed-- marks order as paid, handles missing order_id gracefully, handles nonexistent orderThis is a BLOCKER per the "New functionality with zero test coverage" criteria.
3.
int(order_id_str)in webhook handler can raiseValueErroron malicious metadatawebhooks.pyline 140:Order.id == int(order_id_str)-- if a crafted webhook deliversorder_id: "abc"in metadata, this raises an unhandledValueError, resulting in a 500 response to Stripe. Stripe will then retry the webhook repeatedly. The existing jersey handler (_handle_jersey_checkout_completed) has the same pattern at line 181 withint(player_id_str), but the jersey handler wraps the option parsing in a try/except. The generic handler does not.Fix: Wrap in try/except:
NITS
N1. DRY opportunity: Stripe Customer creation logic duplicated
checkout.pylines 159-170 duplicate the exact same Stripe Customer create-or-reuse pattern fromjersey.pylines 129-141. Both checkplayer.stripe_customer_id, callstripe.Customer.create()with the same metadata shape, and save the ID back. This should be extracted to a shared helper (e.g.,services/stripe.py: get_or_create_customer(player, parent, db)). Not a blocker since this is not in an auth/security path, but it is technical debt worth tracking.N2.
custom_fieldsvalidation only checks key presence, not value typecheckout.pylines 126-133: Forselect-type fields, the validation does not verify that the submitted value is one of the allowedoptions. A parent could submit{"top_size": "XXXL"}and it would pass validation. Consider adding type/options validation for stricter data integrity.N3.
custom_datatype annotation isdict | Nonebutcustom_fieldsstores a listProductResponse.custom_fieldsis typed aslist[dict] | None(correct), butCheckoutRequest.custom_datais typed asdict | None. This is semantically correct (fields are a list, data is a dict), but the model-levelMapped[dict | None]onProduct.custom_fieldsshould beMapped[list | None]since it stores a JSON array. Minor type accuracy issue.N4.
list_productscategory filter does not validate againstProductCategoryenumcheckout.pyline 79:Product.category == categorypasses a raw string to SQLAlchemy enum comparison. If an invalid category is passed (e.g.,?category=invalid), SQLAlchemy may raise aDataErroror return empty results depending on the DB driver. Consider validating againstProductCategoryor using a Pydantic query model.N5. Migration uses hardcoded
tenant_id=1013_generic_checkout_system.pyline 85: Product seed data hardcodestenant_id=1. This is acceptable for a single-tenant system, but if multi-tenancy is ever activated, this migration will only seed products for tenant 1. Worth a comment in the migration noting this assumption.N6. Import ordering in
main.pymain.pyline 17:checkout_routerimport is not in alphabetical order with the other router imports (it appears betweenhealth_routerandjersey_routerrather than aftercoaches_api_router). Minor ruff/isort concern.SOP COMPLIANCE
127-feat-generic-checkout-system-products-tareferences issue #127plan-wkqwith phase referencesPROCESS OBSERVATIONS
jersey.py) and the new checkout routes share significant logic (token-based parent lookup, Stripe customer creation, opt-out handling). A follow-up issue should be created to migrate the jersey flow to use the generic checkout system and deprecate the legacy routes, preventing long-term DRY divergence.players.jersey_optiontoordersdoes not populatecustom_data(jersey_number, sizes). The migrated orders will havecustom_data = NULL. This is acceptable as legacy data but should be documented.VERDICT: NOT APPROVED
Three blockers must be addressed before merge:
GET /checkout/orders-- addrequire_admindependencyValueErrorin webhook -- wrapint(order_id_str)in try/exceptPR #128 Review (Re-review)
Previous review found 3 blockers. Verifying fixes against the updated code.
BLOCKER FIX VERIFICATION
1. Auth bypass on GET /checkout/orders -- FIXED
Confirmed at
src/basketball_api/routes/checkout.py:231:The
require_admindependency is correctly wired using the samerequire_role("admin")pattern used inadmin.py,subscriptions.py,teams.py, etc. The test attests/test_checkout.py:245-247(TestCheckoutOrders.test_requires_auth) verifies that unauthenticated requests to/checkout/ordersreturn 401/403. Fix is correct.2. Zero test coverage -- FIXED
11 tests added in
tests/test_checkout.pyacross 4 test classes:TestCheckoutProducts(4 tests): list all, category filter, empty category, custom_fields structureTestCheckoutCreateSession(5 tests): invalid token 404, invalid product 404, missing required field 422, opt-out skips Stripe, paid product creates Stripe sessionTestCheckoutOrders(2 tests): requires auth, returns orders with admin clientTestCheckoutWebhook(2 tests -- note: the PR body says 11 but I count 11 total across classes): webhook updates order to paid, webhook handles invalid order_id gracefullyCoverage spans happy path, error handling, and edge cases for all 3 endpoints plus the webhook handler. Stripe is properly mocked. Fixture design reuses conftest patterns (
db,tenant,client). Fix is correct.3. Unhandled ValueError in webhook -- FIXED
Confirmed at
src/basketball_api/routes/webhooks.py:140-143:Both
ValueError(non-numeric string) andTypeError(None after metadata corruption) are caught. The function returnsTrueto signal "this was an order event" so the webhook returns 200 and Stripe does not retry. The testtest_webhook_handles_invalid_order_idconfirms this path. Fix is correct.DOMAIN REVIEW (Python/FastAPI/SQLAlchemy)
Auth pattern:
require_admin = require_role("admin")at module level is the established pattern in this codebase (same inadmin.py,subscriptions.py,teams.py). No DRY violation -- each router declares its own module-level dependency, no duplicated auth logic.GET /products is intentionally unauthenticated: This matches the legacy
jersey.pypattern where product/option listings are public (parents browse without auth, then use a registration token for checkout). Correct for this use case.Pydantic models: Using
class Config: from_attributes = True-- this is the Pydantic v1 compatibility style. The rest of the codebase also usesclass Config, so this is consistent. Not a blocker.SQLAlchemy session management:
db.flush()before Stripe session creation (line 186) to getorder.idfor metadata, thendb.commit()after -- correct pattern. Opt-out path commits immediately. No leaked sessions.Migration: Pure SQL approach avoids SQLAlchemy enum double-creation issues (documented in migration docstring). Data migration from
players.jersey_optiontoorderstable is well-structured with proper CASE mapping. Downgrade drops tables and types cleanly.Input validation:
custom_datais validated againstproduct.custom_fieldsspec (required field check at lines 129-136). Product ID validated against DB. Registration token validated. Category filter uses SQLAlchemy enum comparison (not raw SQL). No injection risk.Webhook ordering: Generic order handler runs before legacy jersey handler -- correct priority since new orders will have
order_idmetadata while legacy sessions havejersey_optionmetadata. No ambiguity.BLOCKERS
None. All 3 previous blockers have been resolved correctly.
NITS
Pydantic v2 migration opportunity:
class Config: from_attributes = Truecould becomemodel_config = ConfigDict(from_attributes=True). Not a blocker -- the entire codebase uses v1 style, so this is a codebase-wide migration if done at all.N+1 query on orders listing:
list_ordersat line 234 doesdb.query(Order).join(Product)but then accesseso.product.nameando.product.category.valuein the list comprehension. Thejoinmakes Product available for filtering but SQLAlchemy may still lazy-load the relationship. Adding.options(joinedload(Order.product))would be more explicit. Low risk at current scale.Hardcoded player selection:
player = parent.players[0]at line 138 always picks the first player. This works for single-child families but will need aplayer_idfield inCheckoutRequestwhen multi-player families use checkout. Likely intentional given current scope (jerseys are per-registration-token).SOP COMPLIANCE
127-feat-generic-checkout-system-products-tareferences issue #127plan-wkq-- Phase 11/14_settings.frontend_url, Stripe API key not in code)PROCESS OBSERVATIONS
VERDICT: APPROVED
PR #128 Review
Title: feat: generic checkout system -- products + orders tables (#127)
Branch:
127-feat-generic-checkout-system-products-ta->mainFiles changed: 6 | +829 / -7
DOMAIN REVIEW
Stack: Python / FastAPI / SQLAlchemy / Alembic / Stripe / PostgreSQL (JSONB)
Models (models.py)
custom_fields(product schema) andcustom_data(order values) is the right pattern for dynamic form fields.ProductType,ProductCategory,OrderStatus) are clean and match the migration DDL.custom_fieldstyped asMapped[dict | None]-- technically this islist[dict]in practice (the seed data and the response schema both treat it as a list). Minor type mismatch, but SQLAlchemy JSONB accepts both. The Pydantic response schema (list[dict] | None) is correct.Migration (013_generic_checkout_system.py)
tenant_id=1-- acceptable for a single-tenant system but worth noting for future multi-tenant growth.players.jersey_optiontoordersis well-structured with proper CASE mapping for both product lookup and status conversion.custom_datais NOT migrated for existing jersey orders (jersey_number, sizes are on the player record but not copied to the order'scustom_dataJSONB). This means migrated orders will havecustom_data = NULLwhile new orders will have it populated. Noted below as a nit.Checkout routes (checkout.py)
GET /products-- unauthenticated, returns active products. Appropriate for a public product listing page.POST /create-session-- auth via registration token (query param), consistent with the existing jersey route pattern.GET /orders-- admin-only viarequire_admindependency. Correct.db.flush()before Stripe session creation to getorder.idfor metadata -- correct pattern. Finaldb.commit()after attachingstripe_checkout_session_id.Webhook handler (webhooks.py)
bool) is consistent with the existing_handle_jersey_checkout_completeddesign.order_idreturnsTrue(claims the event) rather than returning 500 to Stripe -- prevents infinite retries on bad data.Router registration (main.py)
checkout_routermounted at/checkout-- clean prefix, no conflict with existing routes.checkoutbeforejersey). Very minor -- matches the same pattern as other imports in the file.BLOCKERS
None. This PR passes all blocker criteria:
custom_fieldsspec (required field check). Registration token validated. Product existence validated. Admin auth on orders endpoint.require_adminreuses the existingrequire_rolefactory. Token-based auth forcreate-sessionfollows the same pattern as the legacy jersey route.NITS
Webhook idempotency gap (medium):
_handle_generic_order_completeddoes not check the current order status before updating. If Stripe retries a webhook (which it does), an order that was alreadypaidwill be re-committed with the same status. No data corruption, but thedb.commit()is unnecessary work. Consider addingif order.status == OrderStatus.paid: return Trueas a guard. The legacy jersey handler has the same pattern, so this is pre-existing, but worth fixing in the new code.Duplicate order prevention (medium):
POST /create-sessiondoes not check whether a pending or paid order already exists for the same player+product combination. A parent could create multiple orders by hitting the endpoint repeatedly. Consider a uniqueness check or a database constraint. This is a business logic decision -- multiple orders for the same product may be intentional (e.g., ordering for multiple seasons), but for jerseys it is likely not.custom_datavalidation only checks presence, not type/value (low): Forselect-type fields, the validation checks that the key exists but does not validate that the value is one of the allowedoptions. A user could submit{"top_size": "XXXL"}and it would pass validation. The frontend should enforce this too, but server-side validation of allowed values would be stronger.player = parent.players[0]always uses the first player (low): If a parent has multiple players, all orders go to the first one. The legacy jersey route has the same behavior. Consider adding aplayer_idfield toCheckoutRequestfor multi-player families, or document the single-player assumption.Migrated orders missing
custom_data(low): The data migration fromplayerstoordersdoes not carry over jersey number or sizes into the order'scustom_dataJSONB. This meansGET /checkout/orderswill returnnullforcustom_dataon migrated records while new orders will have it populated. Consider populating it in the migration if the data exists on the player record.model_configoverclass Config(low): Pydantic v2 prefersmodel_config = ConfigDict(from_attributes=True)overclass Config: from_attributes = True. Both work, but the v2 pattern is canonical.Import ordering in main.py (trivial):
checkout_routerimport is betweenhealth_routerandjersey_router, not alphabetically sorted. The file already has inconsistent ordering, so this is pre-existing.SOP COMPLIANCE
127-feat-generic-checkout-system-products-tareferences #127)plan-wkq)PROCESS OBSERVATIONS
players-- no destructive changes. Safe to run on live DB.order_idmetadata is found.db,client,tenant) and add checkout-specific fixtures. Theadmin_clientfixture properly overrides bothget_dbandrequire_adminand cleans up overrides on teardown.VERDICT: APPROVED
Solid implementation of a generic checkout system. The models, migration, routes, webhook handler, and tests are all well-structured. No blockers. The nits (idempotency guard, duplicate order check, value validation for select fields) are worth addressing in follow-up work but do not block this merge.