Review: Invoice builder UI: select items and generate tokenized link

review-1681-2026-07-02 Review

review needs-refinement

Project

Board item #1681 — Forgejo issue ldraney/westside-basketball#147. Feature type. Points: 2.

Template Completeness

[x] Type — Feature
[x] Lineage — child of #144, depends on #145
[x] Repo — ldraney/westside-basketball
[x] User Story — complete (admin generates shareable invoice link)
[x] Context — thorough; documents MessageVerifier token approach
[x] File Targets — 5 files to modify/create, 3 files to avoid
[x] Feature Flag — "none" (appropriate for admin-only UI)
[x] Acceptance Criteria — 9 items
[x] Test Expectations — 3 tests + run command
[x] Constraints — 3 listed (secret_key_base, payload size, Stimulus patterns)
[x] Checklist — present
[x] Related — 3 related issues

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-app label — Rails application component
[ ] arch note MISSING — [SCOPE] Create architecture note arch-rails-app for the Rails application component
[x] Forgejo issue — ldraney/westside-basketball#147, open

File Targets

[x] app/controllers/admin/players_controller.rb — verified: exists with index and show actions; generate_invoice action will be added
[x] app/views/admin/players/show.html.erb — verified: 287 lines, shows outstanding balances (payment_links, orders) and tournament history sections. Checkboxes will span multiple sections.
[x] config/routes.rb — verified: admin namespace has resources :players, only: [:index, :show] with member block; POST route for generate-invoice will be added
[x] app/javascript/controllers/invoice_builder_controller.js — new file; directory exists with 4 existing controllers (application, index, inline_fee, program). Stimulus patterns established.
[x] app/javascript/controllers/clipboard_controller.js — new file; ticket says "(or inline in invoice_builder)" so may be optional

Repo Placement

OK. Issue filed on ldraney/westside-basketball, Repo section says ldraney/westside-basketball, all file targets within this repo. No cross-repo concerns.

Dependencies

#144 (parent, board ID 1678) — backlog, 5pts — parent issue for the invoice builder epic
#145 (dependency, board ID 1679) — backlog, 2pts — "Mark-as-paid + paid_method migration". Currently in backlog. This must be completed first: without manual mark-as-paid, the invoice builder would show items as unpaid that may have been paid via cash/Venmo, producing inaccurate invoices. Dependency is correctly documented in both labels and issue body.
#146 (sibling, board ID 1680) — backlog, 1pt — "Custom charges on player detail page". Not a direct dependency but adds custom charge items that would appear on invoices. The issue body mentions #146 in the Context section.
#148 (downstream, board ID 1682) — backlog, 2pts, depends:147 — "Public invoice page (tokenized, no-login)". Consumes the tokens this ticket generates. Correctly listed as downstream.

All dependencies correctly documented. Ordering on the board is correct: #145 must be done before #147, and #148 depends on #147.

Acceptance Criteria

9 AC items. Most are verifiable via controller tests and DOM assertions. Two issues:

AC #9 ("Token is tamper-proof — modifying the URL returns an error on the public page (#148)") — this AC tests #148's behavior, not #147's. The tamper-proof property can be verified in isolation with a unit test (MessageVerifier raises InvalidSignature on modified tokens), which is already covered in Test Expectations. AC #9 should be reworded to reference the unit test, not the public page.
AC #7 ("Clicking Copy copies the URL to clipboard") — clipboard API tests can be flaky in CI due to browser permissions, but the Stimulus controller can be tested with mocked navigator.clipboard. Acceptable.

Blast Radius

No existing MessageVerifier usage in the codebase — this introduces a new pattern. Low risk since MessageVerifier is a standard Rails API.
No existing clipboard functionality — new Stimulus controller adds no risk to existing features.
Checkboxes modify the player detail show page visual layout. The show page is admin-only, so blast radius is limited to admin users.
Route changes are scoped to admin namespace — no impact on public routes.
Note: @total_outstanding_cents currently excludes unpaid tournament amounts (only sums payment_links + orders). The invoice builder will include tournaments as selectable items but the "Total Owed" stat card will not match. This is a pre-existing data gap, not introduced by this ticket.

Decomposition Assessment

5 file targets across 1 repo, 9 acceptance criteria, estimated agent work ~25-35 minutes.

>3 file targets across >2 repos — NO (1 repo)
>5 acceptance criteria — YES (9 AC)
Estimated agent work >5 minutes — YES

The 9 AC count triggers the decomposition threshold. However, the work is highly cohesive: the Stimulus controller, the controller action, the view changes, and the route are tightly coupled and cannot be meaningfully separated. The backend token generation only has value with the frontend checkboxes, and vice versa. Decomposing would create artificial boundaries and increase integration risk. Recommendation: keep as single ticket but reduce AC count by removing AC #9 (belongs to #148) and potentially merging AC #4/#5 into one criterion. If AC count drops to 7, it is borderline acceptable.

Recommendations

[SCOPE] Create architecture note arch-rails-app for the Rails application component in pal-e-docs
[BODY] Reword AC #9: replace "modifying the URL returns an error on the public page (#148)" with "Unit test verifies MessageVerifier raises InvalidSignature on tampered tokens" — the public page error handling belongs to #148
[BODY] Clarify that checkboxes span two view sections (Outstanding Balances for payment_links/orders, Tournament History for player_tournaments with pending status) since unpaid items are currently displayed in separate sections