pal-e-docs

Review: Mark-as-paid + paid_method migration

review-1679-2026-07-02 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Child of #144, sub-ticket 1/4
  • [x] Repo — ldraney/westside-basketball
  • [x] User Story — present
  • [x] Context — thorough, identifies schema issue and webhook impact
  • [x] File Targets — 6 files listed with clear descriptions
  • [x] Feature Flag — none (appropriate for internal admin feature)
  • [x] Acceptance Criteria — 10 criteria listed
  • [x] Test Expectations — 4 test cases with run command
  • [x] Constraints — 3 constraints listed
  • [x] Checklist — present
  • [x] Related — parent issue and review note referenced
All required sections present per template-issue-feature.

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. Search for "arch-rails-app" returned no results in pal-e-docs.
  • [x] Forgejo issue — ldraney/westside-basketball#145, open

File Targets

  • [x] db/migrate/YYYYMMDD_add_paid_method_and_nullable_stripe.rb — new file. Schema confirms payment_links.stripe_payment_link_id (NOT NULL, line 221) and stripe_payment_link_url (NOT NULL, line 222) need nullable migration. orders table lacks paid_at (confirmed at lines 156-172). paid_method column does not exist yet anywhere in codebase.
  • [x] app/controllers/admin/players_controller.rb — verified exists. Already loads @unpaid_links (line 43), @paid_links (line 44), @paid_orders (line 46), @player_tournaments (line 51). Good insertion point for mark_paid action.
  • [x] app/views/admin/players/show.html.erb — verified exists. Has "Outstanding Balances" section (line 153) and "Tournament History" section (line 92). Renders @unpaid_links (line 175) and @pending_orders (line 159).
  • [x] app/controllers/webhooks_controller.rb — verified exists. Line 75 confirms: payment_link.update!(paid_at: Time.current, status: "paid") — matches ticket's description exactly. Adding paid_method: 'stripe' here is correct.
  • [x] config/routes.rb — verified exists. Admin player routes at lines 24-28 use resources :players with member block. New mark_paid route fits naturally as another member action.
  • [x] app/javascript/controllers/mark_paid_controller.js — new file. Reference pattern inline_fee_controller.js exists and verified — uses Stimulus with fetch PATCH, CSRF token, and status feedback. Good pattern to follow.
All file targets verified. Schema claims match actual codebase state.

Repo Placement

OK. Issue filed on ldraney/westside-basketball, all file targets in same repo. Single-repo change.

Dependencies

  • Board item #1679 has label parent:144 (parent issue: Player invoice builder)
  • Board items #1680 and #1681 both have label depends:145 — they depend on THIS ticket completing first
  • Board item #1682 has depends:147 (invoice builder UI) — sibling but not dependent on #145
  • This ticket (#145) has no depends: label — it can be implemented independently, which is correct since it is the foundation sub-ticket (1/4)
  • The ticket correctly makes stripe_payment_link_id and stripe_payment_link_url nullable, which unblocks #146 (custom charges that create DB-only payment_links without Stripe backing)
Dependency chain is correctly structured: #145 is the foundation, #1680/#1681 depend on it.

Acceptance Criteria

10 acceptance criteria, all verifiable by an agent:
  • AC 1-4: migration assertions — verifiable via schema.rb inspection after rails db:migrate
  • AC 5-7: UI dropdowns — verifiable via view file inspection and manual test
  • AC 8: correct column updates — verifiable via controller test
  • AC 9: webhook writes paid_method: 'stripe' — verifiable via code inspection and test
  • AC 10: subscription webhook not broken — verifiable via existing test suite
Test commands are real (bin/rails test). All criteria are testable.
Note: AC 8 references player_tournaments.paid_at — this column already exists in the create_player_tournaments migration (line 9 of 20260628220000_create_player_tournaments.rb), so the new migration does NOT need to add it. The ticket's migration spec does not mention adding paid_at to player_tournaments, which is correct.

Blast Radius

  • app/controllers/admin/payments_controller.rb — queries orders.select { |o| o.status == "paid" }. Manually-paid orders will now appear here too. This is intended and correct.
  • app/controllers/admin/delinquency_report_controller.rb — queries payment_links.select { |l| l.status == "active" }. Marking paid changes status away from "active", removing the item from the delinquency report. Correct behavior.
  • app/views/admin/roster_report/ — ticket correctly excludes this. No impact.
  • app/services/stripe_client.rb — ticket correctly excludes this. No new Stripe API calls needed.
  • Webhook backward compatibility: existing payment_links with NULL paid_method are treated as legacy/unknown per ticket constraints. Safe.
No unintended blast radius found. Downstream consumers handle the new state correctly.

Decomposition Assessment

6 file targets in 1 repo. 10 acceptance criteria (exceeds threshold of 5). Estimated agent work: ~8 minutes.
However, this ticket was already decomposed from parent #144 (sub-ticket 1/4). The 10 AC are tightly coupled around a single feature (mark-as-paid). Splitting further would create artificial seams between migration/controller/view that must be deployed atomically. No further decomposition recommended.

Recommendation

  • [SCOPE] Create architecture note arch-rails-app for the Rails application component. This label is used across multiple board items (1678, 1679, 1680, 1681, 1682) but has no backing note.