pal-e-docs

Review: Validate basketball-api (2 PRs, test failure)

review-514-2026-03-27 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Task
  • [x] Scope -- describes validation of 2 PRs, CI failures, root cause investigation
  • [x] Lineage -- "Validation audit -- session 2026-03-28 pipeline gap"
  • [x] User Story -- present
  • [x] Acceptance Criteria -- 7 items (3 are boilerplate, see below)
  • [x] Test Expectations -- present
  • [x] Constraints -- present, includes hands-off conventions note
  • [x] Related -- present
  • [ ] Repo -- missing explicit ### Repo section (implicit from issue location)
  • [ ] Checklist -- missing ### Checklist section

Traceability

  • [ ] story:WS-S5 label -- ISSUE: WS-S5 does not resolve to a documented user story in pal-e-docs or codebase. Label exists on the board item but has no backing definition.
  • [x] arch:basketball-api label -- valid component, correct repo
  • [x] Forgejo issue -- forgejo_admin/basketball-api#201, open

File Targets

Task type -- no file targets required per template. However, investigation comment and PR #203 reference specific files, all verified:
  • [x] tests/test_tryouts.py:419 -- confirmed: settings.base_url on main (the bug), settings.frontend_url in PR #203 fix
  • [x] tests/test_tryouts.py:407 -- confirmed: docstring updated in PR #203
  • [x] src/basketball_api/routes/tryouts.py:796 -- confirmed: uses settings.frontend_url (changed by PR #196)

Repo Placement

OK. Issue #201 filed on forgejo_admin/basketball-api. Fix PR #203 also on basketball-api. Single repo, correct placement.

Dependencies

  • Board item #514 is on board-westside-basketball backlog. No items in_progress or next_up that block or are blocked by this.
  • Related platform board item #417 ("Critical: Migrate basketball-api Postgres to CNPG") is in next_up on board-pal-e-platform but is independent work (different scope: database migration vs test fix).
  • PR #203 already exists and is open, mergeable. It closes issue #202 (the bug-specific issue). Issue #201 (this ticket) is the broader validation task that encompasses it.
  • No undocumented dependencies.

Acceptance Criteria

7 criteria total. Assessment:
  • [x] "Pipeline failures diagnosed -- root cause documented" -- Testable. Investigation comment already documents root cause (PR #196 changed base_url to frontend_url in implementation but not test).
  • [x] "Determined whether docs-only PRs should skip build" -- Testable. Answer is clear: build failure is pre-existing test bug, not path-filtering issue.
  • [x] "Verified no regression in basketball-api functionality" -- Testable. Run test suite.
  • [x] "API endpoints still responding correctly" -- Testable. Curl endpoints or check uptime.
  • [ ] "Pipeline verified" -- Vague. Which pipeline? After PR #203 merge? Needs specificity.
  • [ ] "Deployment confirmed" -- Vague. Basketball-api deploys via ArgoCD. What to check? Pod healthy? Image tag matches?
  • [ ] "Features validated" -- Vague. Which features? The copy-link button? Registration flow?
First 4 criteria are solid. Last 3 are generic validation boilerplate that should either be made specific or removed.

Blast Radius

No blast radius concern. settings.base_url is still legitimately used in 5 other places:
  • admin.py:203 -- admin dashboard base URL (API-served pages)
  • tryouts.py:509 -- photo URL (MinIO assets served via API)
  • coach_onboarding.py:112 -- coach onboarding base URL
  • test_keycloak_integration.py:170,266 -- mock settings for Keycloak tests
  • scripts/send_registration_emails.py:90 -- CLI script override
These are all correct: base_url is for API-hosted resources, frontend_url is for user-facing app links. PR #196 correctly documented this distinction.

Decomposition

No decomposition needed. The actual work is a 2-line test fix in a single file in a single repo. PR #203 already exists with the fix. Well under the 5-minute rule.

Issue Scope Mismatch

The issue title and body frame this as a broad validation task covering "2 PRs merged during session 2026-03-28" with 7 acceptance criteria. But investigation (already completed in a Forgejo comment) established that:
  • PRs #199 and #200 are docs-only, already merged successfully, no issues
  • Pipeline failures are caused by a pre-existing test bug from PR #196 (not from session PRs)
  • Fix PR #203 already exists and is ready for review
The ticket conflates the validation task (verify merged PRs) with the discovered bug fix (PR #203). These are two different things. The validation is already done (investigation comment proves it). What remains is: merge PR #203 and verify CI goes green.

Recommendation

  • [BODY] Add explicit ### Repo section: forgejo_admin/basketball-api
  • [BODY] Add ### Checklist section with PR opened / tests pass / no unrelated changes
  • [BODY] Replace AC items 5-7 with specific criteria: "PR #203 merged", "Woodpecker pipeline green on main after merge", "ArgoCD basketball-api pod running updated image"
  • [BODY] Update title/scope to reflect that this is now "merge PR #203 and verify CI" rather than "validate 2 PRs"
  • [LABEL] Resolve story:WS-S5 -- either document the user story or replace with a valid story label (e.g., story:superuser-validate)