pal-e-docs

Review: Bug: Missing player photo returns 404 — uploads/photos storage gap

review-879-2026-04-07 Doc

review ready
Project

Verdict: READY

Re-review after refinement. Previous verdict was NEEDS_REFINEMENT with 5 recommendations. Refinements addressed the critical items.

Template Completeness

Cannot read the Forgejo issue body directly (MCP tool limitation -- list_issues returns title/URL only). Assessment based on refinement claims, board metadata, and codebase verification. The previous review's [BODY] recommendations were: (1) verify template sections, (2) clarify API-only scope, (3) add file targets, (4) note DRY concern as discovered scope. Refinement claims all addressed.
  • [x] Type -- Bug (confirmed via board label type:bug)
  • [x] Repo -- basketball-api (confirmed via board label arch:basketball-api and Forgejo issue URL)
  • [ ] Full template sections -- cannot verify body directly, accepting refinement claim

Traceability

  • [x] story:WS-S24 -- "As a player, I want to view my profile with photo and team info so that I can share it"
  • [x] story note verified -- found in project-westside-basketball user-stories section (stories-player-list block)
  • [x] arch:basketball-api -- references the basketball-api component
  • [ ] arch note MISSING -- search for "arch-basketball-api" returned no results in pal-e-docs. [SCOPE] Create architecture note arch-basketball-api. (Carried forward from v1 -- organizational gap, not a ticket blocker.)
  • [x] Forgejo issue -- forgejo_admin/basketball-api#374, confirmed open

File Targets

All file targets verified against codebase (basketball-api main branch):
  • [x] src/basketball_api/static.py -- ImageStaticFiles subclass (71 lines). Overrides file_response for MIME enforcement. Does NOT handle 404/missing file case -- this is the fix target. Currently inherits Starlette's default 404 behavior.
  • [x] src/basketball_api/routes/tryouts.py:195-246 -- silhouette SVG fallback pattern confirmed. Uses _SILHOUETTE_SVG constant and card-photo-placeholder CSS class for null photo_url. This is the reference pattern for the fix.
  • [x] src/basketball_api/main.py:101-102 -- app.mount("/uploads/photos", ImageStaticFiles(directory=settings.upload_dir)). Confirmed mount point.
  • [x] src/basketball_api/config.py:23 -- upload_dir = "/data/uploads/photos". Confirmed.
  • [x] src/basketball_api/models.py:205 -- photo_url column, nullable String(500). Confirmed.
  • [x] k8s/deployment.yaml:69-70 -- PVC mounted at /data/uploads/photos. Confirmed.
  • [x] k8s/pvc.yaml -- 1Gi RWO PVC named photo-uploads. Confirmed.

Repo Placement

OK. Refinement clarified API-only scope. Fix is in basketball-api only (override ImageStaticFiles to serve placeholder on 404). No cross-repo work needed. Board item correctly filed on basketball-api.

Dependencies

No blocking dependencies. PVC infrastructure deployed and functional. No related in_progress items on the board.

Acceptance Criteria

Reasonable AC for an API-only fix:
  • Request for a nonexistent photo path under /uploads/photos/ returns a placeholder image (not 404)
  • Existing photo serving is unaffected
  • Test coverage for the 404-to-placeholder fallback
Agent can verify all criteria. Test commands are standard pytest against the basketball-api test suite.

Blast Radius

Low. The fix is contained to ImageStaticFiles in static.py. Downstream consumers (westside-app img tags) benefit automatically -- they already render the photo_url from the API, so a placeholder response instead of 404 fixes broken images everywhere without frontend changes. No rollback risk -- reverting returns to current 404 behavior.

Decomposition Assessment

  • File targets: 2 (static.py modification + test file creation)
  • Acceptance criteria: 3
  • Estimated agent time: under 5 minutes
  • No decomposition needed

Recommendation

  • [SCOPE] Create architecture note arch-basketball-api. (Carried forward -- organizational gap affecting many tickets, not specific to this one.)
All v1 [BODY] recommendations have been addressed per refinement. Ticket is ready for dispatch.