pal-e-docs

Review: Subscription status on admin player detail page (R2)

review-1611-2026-06-27-r2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after major refinement. Previous review (review-1611-2026-06-27) found critical Stripe vs Apple architectural conflict. Owner decided to REPLACE Stripe with Apple. Issue body rewritten. This review validates the rewritten scope.
What improved: The Stripe-vs-Apple coexistence question is resolved — the issue now explicitly says "replace Stripe." Context section documents the decision. Migration approach (repurpose existing column) is sound. File targets are more comprehensive.
What still needs work: The ticket is too large (8 file targets + migration + Stripe removal + Apple badge UI across 2 subsystems), show.html.erb still doesn't exist on main, and several blast radius files are missing from file targets.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — references #65 and #58
  • [x] Repo — ldraney/westside-basketball
  • [x] User Story — admin wants subscription status on player detail page
  • [x] Context — explains Apple delinquency gate and Stripe replacement decision
  • [x] File Targets — 8 modify/create targets, 1 do-not-touch
  • [x] Feature Flag — none (admin-only, acceptable)
  • [x] Acceptance Criteria — 6 criteria
  • [x] Test Expectations — 3 test items with run command
  • [x] Constraints — 3 constraints listed
  • [x] Checklist — present
  • [x] Related — project-westside-basketball, review note, #65

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 in project-westside-basketball user-stories section
  • [ ] arch note MISSING — [SCOPE] No architecture note found for arch-rails-views. Search returned empty results. Create architecture note arch-rails-views for component rails-views.
  • [x] Forgejo issue — ldraney/westside-basketball#66, state: open

File Targets

  • [x] db/migrate/*_replace_stripe_with_apple_subscription.rb — new migration. Sound approach: repurpose existing subscription_status enum column, drop stripe_subscription_id, add Apple fields. Note: db/migrate/ directory does not exist in repo (schema-only). Agent will need to create directory.
  • [x] app/models/player.rb — verified: exists. Currently has no explicit enum declaration (relies on Postgres enum). Agent will need to add explicit enum declaration for Apple states.
  • [ ] app/views/admin/players/show.html.erb — ISSUE: File does NOT exist on main. Issue #58 (player detail page) is marked done on board (item 1609) but code has NOT been merged. No branch for it found. No show action in controller. Routes only define :index. This file target is invalid until #58 is actually merged.
  • [x] app/views/admin/players/_subscription_badge.html.erb — new partial. Acceptable — will be created.
  • [x] app/views/admin/players/index.html.erb — verified: exists (113 lines). Currently uses subscription_badge_variant and subscription_badge_label helpers with Stripe states. Needs Apple state update.
  • [x] app/helpers/application_helper.rb — verified: exists (22 lines). Defines subscription_badge_variant and subscription_badge_label with Stripe states (active/past_due/canceled/none). Needs Apple state update.
  • [x] app/controllers/webhooks_controller.rb — verified: exists (119 lines). Full Stripe webhook handling: checkout.session.completed, customer.subscription.updated, customer.subscription.deleted. To be removed.
  • [x] config/routes.rb — verified: has post "webhooks/stripe" => "webhooks#stripe" at line 50. To be removed.

Repo Placement

OK — issue is filed on ldraney/westside-basketball, which is the correct repo for all affected files.

Dependencies

  • depends:65 — item 1610 is in todo column. Issue #65 (App Store Connect configuration) is open on Forgejo and in todo on the board. This ticket's AC #5 ("When webhook updates subscription state, badge reflects new state") depends on #65 providing the webhook endpoint. However, the issue acknowledges this dependency in Lineage and explicitly says webhook verification logic is in #65's scope (do-not-touch). The migration and badge display work can proceed independently — only the webhook integration AC requires #65 first.
  • depends:58 (implicit) — STILL BROKEN. Issue #58 (admin player detail page) is marked done on board (item 1609) but show.html.erb does NOT exist on main. Git log shows no merge commit for a show view. No branch found. The admin/players controller has no show action. Routes define only :index. This ticket targets show.html.erb which cannot be modified if it doesn't exist.
  • Stripe removal blast radius undocumented. The issue lists 8 file targets but misses several files that reference Stripe subscription fields and will break when the enum values change or stripe_subscription_id is removed (see Blast Radius).

Acceptance Criteria

  • [x] AC1: "Stripe subscription fields and webhook code removed" — testable, clear scope
  • [x] AC2: "Player model subscription_status enum updated to Apple states" — testable, enum values specified
  • [ ] AC3: "Player detail page shows color-coded subscription badge" — NOT testable: show.html.erb doesn't exist. Depends on #58 being merged first.
  • [x] AC4: "Player list page badge helper updated to reflect Apple states" — testable
  • [ ] AC5: "When webhook updates subscription state, badge reflects new state on next page load" — NOT fully testable in isolation: depends on #65 webhook. However, the subscription_status column update can be tested independently.
  • [x] AC6: "Players without subscriptions show 'No subscription' gracefully" — testable

Blast Radius

SIGNIFICANT. The issue's file targets miss several files that hard-code Stripe subscription_status enum values and will break when enum values change:
  • app/controllers/admin/players_controller.rb (33 lines) — Lines 27-29 hard-code "active", "past_due", "none", "canceled" for summary counts. NOT in file targets.
  • app/controllers/admin/roster_report_controller.rb (137 lines) — Lines 14, 24, 29-30, 58, 121 reference "active", "none", "canceled" and stripe_subscription_id. Line 58-60 calls stripe.update_subscription_price using stripe_subscription_id. NOT in file targets.
  • app/controllers/admin/delinquency_report_controller.rb (89 lines) — Lines 34, 44, 57 reference "past_due" and subscription_status. NOT in file targets.
  • app/views/admin/roster_report/index.html.erb — Lines 51, 79 reference subscription_status. NOT in file targets.
  • app/services/stripe_client.rb (249 lines) — Full Stripe API client with subscription methods. NOT in file targets (should it be deleted or kept for non-subscription Stripe use like payment links?).
  • app/views/admin/payments/index.html.erb — References Stripe. NOT in file targets.
  • test/helpers/application_helper_test.rb — 10 tests hard-coding Stripe enum values. NOT in file targets.
  • test/controllers/webhooks_controller_test.rb — Full Stripe webhook test suite. NOT in file targets.
  • test/controllers/admin/roster_report_controller_test.rb — References subscription_status. NOT in file targets.
  • test/services/stripe_client_test.rb — Stripe client tests. NOT in file targets.
Total: 10 files reference Stripe subscription fields that are NOT in the issue's file targets. If the agent only modifies the 8 listed files, the app will have broken enum references, failing tests, and a non-functional roster report.

Decomposition Assessment

NEEDS DECOMPOSITION.
  • 8 file targets listed + 10 unlisted blast radius files = 18 files potentially affected
  • 6 acceptance criteria
  • Two distinct subsystems: (1) data migration (enum change + field removal), (2) UI/badge updates
  • Estimated agent work: well beyond 5 minutes. The migration touches the database schema, 6+ controllers, 4+ views, 1 service class, and 4+ test files.
  • Recommended split:
[DECOMPOSE] — 18 files across 2 subsystems, 6 AC. Route to skill-decompose-ticket.

Recommendation

  • [DECOMPOSE] 18 files (8 listed + 10 unlisted) across 2 subsystems (data migration + UI). Exceeds 5-minute rule. Route to skill-decompose-ticket. Recommended split: (A) enum migration + Stripe removal + blast radius cleanup, (B) show page badge UI (blocked on #58).
  • [BODY] show.html.erb does not exist on main. Issue #58 is marked done on board but code is NOT merged. Either: (a) merge #58 first, or (b) include show view creation in this ticket's scope, or (c) split badge-on-show into a separate ticket blocked on #58.
  • [BODY] Add missing blast radius files to file targets: app/controllers/admin/players_controller.rb, app/controllers/admin/roster_report_controller.rb, app/controllers/admin/delinquency_report_controller.rb, app/views/admin/roster_report/index.html.erb, app/services/stripe_client.rb (decide: delete or keep for non-subscription Stripe use?), and all 4 affected test files.
  • [BODY] Clarify stripe_client.rb disposition: it handles payment links and checkout sessions beyond subscriptions. Should it be deleted entirely or only have subscription methods removed?
  • [SCOPE] Create architecture note arch-rails-views for component rails-views.
  • [SCOPE] Verify #58 (player detail page) is actually merged/deployed before this ticket can target show.html.erb. Board says done but code is missing from main.