pal-e-docs

Review: Admin player list page -- scrollable roster with payment status badges

review-1606-2026-06-25 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- references #54, story WS-S9
  • [x] Repo -- ldraney/westside-basketball
  • [x] User Story -- As Marcus (admin), scrollable player list with payment status
  • [x] Context -- explains current disjointed views, motivation for unified list
  • [x] File Targets -- 2 create, 1 modify, 2 do-not-touch
  • [x] Feature Flag -- "none" (correct for admin-only internal view)
  • [x] Acceptance Criteria -- 7 criteria
  • [x] Test Expectations -- 2 controller tests with run command
  • [x] Constraints -- 4 constraints listed
  • [x] Checklist -- standard 3-item checklist
  • [x] Related -- references #54 and #58
All required sections present. Template is complete.

Traceability

  • [x] story:WS-S9 label -- "As an admin, I want to track payment status per player so that I know who owes what"
  • [x] story note verified -- found in project-westside-basketball user-stories section under Admin (Marcus)
  • [x] arch:rails-views label -- present on board item
  • [ ] arch note MISSING -- [SCOPE] Create architecture note arch-rails-views for component rails-views. No matching note found in pal-e-docs.
  • [x] Forgejo issue -- ldraney/westside-basketball#55, state: open

File Targets

  • [x] app/controllers/admin/players_controller.rb -- verified: does NOT exist yet, correct to CREATE
  • [x] app/views/admin/players/index.html.erb -- verified: does NOT exist yet, correct to CREATE
  • [x] config/routes.rb -- verified: contains resources :players, only: [] inside admin namespace with a nested member route for monthly_fee. Ticket correctly says to expand the only: [] to include :index.
  • [x] app/controllers/admin/payments_controller.rb -- verified: EXISTS, correctly listed as do-not-touch
  • [x] app/controllers/admin/roster_report_controller.rb -- verified: EXISTS, correctly listed as do-not-touch
All file targets verified. No inaccuracies found.

Repo Placement

OK. Issue filed on ldraney/westside-basketball, all file targets are in that repo. No cross-repo work needed.

Dependencies

  • depends:54 (Keycloak auth) -- board item #1605, column: done. Dependency is satisfied.
  • Blocks #58 (Admin player detail page) -- board item #1609, column: backlog, has depends:55. Correctly documented in both issues' Related sections.
Dependency chain is clean. No undocumented dependencies found.

Acceptance Criteria

7 acceptance criteria are testable:
  • AC1 (/admin/players shows list) -- verifiable via controller test, HTTP 200 + content check
  • AC2 (grouped/filterable by team) -- verifiable via controller test with team params
  • AC3 (row shows player name, parent name, email, status badge) -- verifiable via view test or system test checking DOM elements
  • AC4 (visual indicator for current vs behind) -- verifiable via CSS class check on badge
  • AC5 (click navigates to /admin/players/:id) -- verifiable but NOTE: the detail page (#58) doesn't exist yet. The link will 404 until #58 ships. This is acceptable since #58 explicitly depends on #55.
  • AC6 (mobile-responsive) -- requires manual/visual check or system test with viewport
  • AC7 (admin role required) -- verifiable via auth test (Keycloak gating from #54)
Test expectations list 2 controller tests with run command rails test test/controllers/admin/players_controller_test.rb. No existing admin players test file -- agent must create it. This is expected for a new controller.

Blast Radius

  • Status badge pattern already exists in app/views/admin/roster_report/index.html.erb and app/views/admin/payments/index.html.erb. Both use the status-badge status-badge--{variant} CSS class pattern. The new view MUST reuse this existing pattern for visual consistency. The ticket's Constraints section says "Style after landscaping-assistant Today view" but should reference the existing badge pattern in this repo's own views.
  • No other downstream consumers affected. The route addition is additive.

Decomposition Assessment

  • File targets: 3 files across 1 repo -- under threshold
  • Acceptance criteria: 7 ACs -- above the 5-AC threshold, but all are tightly coupled to a single controller+view. The work is cohesive: one controller, one view template, one route line.
  • Estimated agent time: ~3-4 minutes (scaffold controller, build view, add route, write 2 tests)
No decomposition needed. The ACs are facets of a single page, not independent features.

Recommendation

  • [SCOPE] Create architecture note arch-rails-views for the rails-views component. The arch:rails-views label is used on both #55 and #58 but no backing architecture note exists in pal-e-docs. This note should document the admin view conventions, layout structure, and shared patterns (e.g., the status-badge CSS classes).
  • [BODY] Add a constraint to the issue body: "Reuse existing status-badge status-badge--{variant} CSS class pattern from roster_report/index.html.erb and payments/index.html.erb" -- this ensures the agent discovers and follows the existing pattern rather than inventing a new one.