pal-e-docs

Review: Admin player detail page -- product payment matrix and contact info

review-1609-2026-06-25 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- References #55 and story WS-S9
  • [x] Repo -- ldraney/westside-basketball
  • [x] User Story -- As Marcus (admin), track payment status
  • [x] Context -- Good background on data model and purpose
  • [x] File Targets -- Create and modify targets listed
  • [x] Feature Flag -- none (appropriate for internal admin page)
  • [x] Acceptance Criteria -- 6 criteria listed
  • [x] Test Expectations -- Controller tests with run command
  • [x] Constraints -- 3 constraints listed
  • [x] Checklist -- Standard 3-item checklist
  • [x] Related -- References companion tickets and future sprints

Traceability

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

File Targets

  • [x] app/views/admin/players/show.html.erb -- to be created. Directory app/views/admin/players/ does not yet exist (expected for new file). Verified no collision.
  • [x] app/controllers/admin/players_controller.rb -- to be created (does NOT currently exist). Ticket says "modify" but this file needs to be created. Minor inaccuracy but non-blocking -- agent will create it regardless.
  • [x] config/routes.rb -- verified exists. Current routes have resources :players, only: [] inside admin namespace (line 15) with only a monthly_fee member route. Adding :show is correct approach.
  • [x] app/views/admin/payments/ -- DO NOT TOUCH directive verified. Directory exists with index.html.erb.

Data Model Verification

  • [x] subscription_status -- exists on players table (enum: subscriptionstatus, default: none)
  • [x] jersey_order_status -- exists on players table (enum: jerseyorderstatus: none/pending/paid/shipped)
  • [x] monthly_fee -- exists on players table (integer column)
  • [x] orders.amount_cents -- exists (integer, not null) with status enum (pending/paid/refunded/canceled)
  • [x] orders.product_id -- exists with FK and index
  • [x] product.category -- exists (enum: jersey/contract/tournament/equipment/monthly) -- matches AC categories
  • [x] email_log.email_type -- exists (enum includes payment_request and outstanding_balance) -- matches AC filter
  • [x] Player model associations verified: has_many :orders, :payment_links, :email_logs, :teams (through player_teams), belongs_to :parent

Repo Placement

OK -- issue filed on ldraney/westside-basketball, all file targets are in that repo. Single-repo change.

Dependencies

  • [x] depends:55 (Admin player list page) -- board item #1606, currently in backlog. Correctly documented. Issue #55 depends on #54 (Keycloak auth, now closed/merged). Dependency chain: #54 (done) -> #55 (backlog) -> #58 (backlog). Sprint:5 label on both #55 and #58 is correct.
  • [x] Issue body references #54 as prerequisite -- consistent with board labels.
  • [x] Downstream: issues #65 and #66 (sprint:6) depend on #58 per board data. Not documented in issue body but acceptable -- downstream awareness is sprint-level.

Acceptance Criteria

6 acceptance criteria -- all are verifiable by an agent:
  • AC1-4: Route response, DOM content checks, data correctness -- testable via controller tests and view assertions
  • AC5: Back button -- testable via link presence check
  • AC6: Mobile-responsive -- vague but standard for ERB; could be verified via viewport meta tag presence. Acceptable as-is.
Test commands are real: rails test test/controllers/admin/players_controller_test.rb is the correct Rails test invocation path.

Blast Radius

  • The existing admin/roster_report controller already handles update_fee on the player member route. The new admin/players_controller.rb must coexist with this. The routes currently nest resources :players under admin for the monthly_fee member route via roster_report controller. Adding a separate admin/players_controller.rb with a show action could conflict if not carefully scoped. The ticket should note that the existing member { patch :monthly_fee } routes through roster_report controller and the new show action needs its own controller without disrupting that routing.
  • No similar admin detail pattern exists elsewhere -- this is the first admin detail page, so no blast radius to sibling services.

Decomposition Assessment

3 file targets in 1 repo, 6 acceptance criteria (just over the 5 threshold but all are tightly related view/controller concerns for a single page). Estimated agent work: ~3-4 minutes. No decomposition needed -- the 6th AC (mobile-responsive) is a CSS concern bundled with the view, not a separate system.

Recommendations

  • [SCOPE] Create architecture note arch-rails-views for the rails-views component. This label is used by multiple board items (#55, #58, #66) but has no backing architecture note in pal-e-docs.
  • [BODY] Fix file target description: app/controllers/admin/players_controller.rb is listed under "Files the agent should modify" but this file does not exist yet -- it should be listed under "Files the agent should create."
  • [BODY] Add routing caution note: the existing admin/players resource has a member { patch :monthly_fee } route that dispatches to roster_report controller. The new show action must be added without disrupting this routing. Suggest noting this in Constraints.