pal-e-docs

Review: Remove cash from registration payment method validation

review-860-2026-04-06 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone, companion to westside-landing cash removal
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — clear "As the registration API, I want to reject cash..."
  • [x] Context — thorough background on current behavior and migration rationale
  • [x] File Targets — specific files with line numbers
  • [x] Acceptance Criteria — 4 testable conditions
  • [x] Test Expectations — unit tests + run command
  • [x] Constraints — no migration, explicit card path, companion ticket noted
  • [x] Checklist — PR, tests, no unrelated changes
  • [x] Related — project and companion repo identified

Traceability

  • [x] story:WS-S17 label — "As a parent, I want to register my child online (info + photo + payment + waiver) so that signup is one seamless flow"
  • [x] story note verified — found in project-westside-basketball user-stories section (Parent list)
  • [x] arch:basketball-api label
  • [ ] arch note MISSING — [SCOPE] Create architecture note arch-basketball-api for component basketball-api
  • [x] Forgejo issue — forgejo_admin/basketball-api#354, open

File Targets

  • [x] src/basketball_api/routes/register.py L1142 — verified: schema comment says "card", "cash", or "promo"
  • [x] src/basketball_api/routes/register.py L1162 — verified: docstring mentions cash
  • [x] src/basketball_api/routes/register.py L1182 — verified: validation tuple is ("card", "cash", "promo")
  • [x] src/basketball_api/routes/register.py L1184 — verified: error message includes cash
  • [x] src/basketball_api/routes/register.py L1308-1311 — verified: else/cash branch sets signup_method = "cash"
  • [x] tests/test_promo_registration.py L126-150 — verified: TestCashRegistration class exists
  • [ ] tests/test_promo_registration.py L37 — MISSING FROM TARGETS: _valid_payload default uses payment_method: "cash". Must change to "card"
  • [ ] tests/test_promo_registration.py L304, L327, L336 — MISSING FROM TARGETS: TestAPIRegistrationRecordCreation tests use payment_method="cash" explicitly. Will break
  • [ ] tests/test_country.py L36 — MISSING FROM TARGETS: default payload uses payment_method: "cash". Will break
  • [ ] tests/test_register_upload.py L212 — MISSING FROM TARGETS: default payload uses payment_method: "cash". Will break

Repo Placement

OK. Issue filed on forgejo_admin/basketball-api, all file targets in that repo. Companion frontend ticket exists: board item #859 (westside-landing#223). Single-repo scope is correct.

Dependencies

  • [x] Companion frontend ticket #859 (westside-landing#223) — both in backlog, no hard ordering. Backend can land first safely (frontend won't send "cash" once its ticket lands).
  • [x] No blockers in in_progress column.

Acceptance Criteria

4 AC, all verifiable by an agent via pytest. Test command provided but too narrow — only covers test_promo_registration.py. Blast radius analysis shows test_country.py and test_register_upload.py also need to pass. AC themselves are well-written and specific.

Blast Radius

3 additional test files affected beyond what the issue lists. Grep for "cash" across the repo found:
  • tests/test_promo_registration.py — listed (partially). Default _valid_payload at L37 uses cash. TestAPIRegistrationRecordCreation at L304/327/336 uses cash explicitly.
  • tests/test_country.py L36 — NOT listed. Default payload helper uses cash.
  • tests/test_register_upload.py L212 — NOT listed. Default payload helper uses cash.
All unlisted files will break when cash returns 422. Rollback is straightforward (revert single PR).

Decomposition Assessment

Apply the three-thing limit and five-minute rule:
  • File targets: 2 listed + 2 blast-radius = 4 files, all in 1 repo. Acceptable.
  • Acceptance criteria: 4. Under the 5 threshold.
  • Estimated agent time: under 5 minutes. Straightforward removals and default changes.
  • No independent subtasks — all changes are tightly coupled.
No decomposition needed.

Recommendation

  • [BODY] Add tests/test_country.py to File Targets — change default payload payment_method from "cash" to "card"
  • [BODY] Add tests/test_register_upload.py to File Targets — change default payload payment_method from "cash" to "card"
  • [BODY] Add tests/test_promo_registration.py L37 (_valid_payload default) and L304/327/336 (TestAPIRegistrationRecordCreation) to File Targets — these use "cash" and will break
  • [BODY] Update Test Expectations run command to: pytest tests/test_promo_registration.py tests/test_country.py tests/test_register_upload.py -v
  • [SCOPE] Create architecture note arch-basketball-api for component basketball-api