pal-e-docs

Review: Refactor email sending to use ActionMailer with Gmail API delivery adapter

review-1598-2026-06-24-r2 Review

review ready
Project

Verdict: APPROVED

Re-review of board item #1598 after scope refinement. Previous review review-1598-2026-06-24 returned NEEDS_REFINEMENT with 6 recommendations. 4 of 5 [BODY] recommendations fully addressed; 1 [SCOPE] item remains but is non-blocking.

Previous Recommendations Status

  • [x] [BODY] Fix Repo field -- FIXED. Now correctly says ldraney/westside-basketball.
  • [x] [BODY] Add Feature Flag section -- FIXED. Section present with "none" and rationale ("internal refactor, same external behavior").
  • [x] [BODY] Rewrite File Targets -- FIXED. Paths align with spike #47 deliverables. Correctly references lib/delivery_methods/gmail_api.rb (namespaced), app/mailers/parent_mailer.rb, marks spike-delivered files as NOT to touch. Includes oauth_tokens migration as "potentially add."
  • [x] [BODY] Rewrite Acceptance Criteria -- FIXED. ACs reference ParentMailer, EmailLogging, deliver_later. Nonexistent TournamentEmailService removed. Token storage AC added.
  • [~] [BODY] Clarify blast/single-send routes -- PARTIALLY FIXED. See minor findings below.
  • [ ] [SCOPE] Create arch note arch-westside-basketball -- NOT ADDRESSED. Non-blocking; tracked as standing recommendation.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Follows Spike #47 / PR #52
  • [x] Repo -- ldraney/westside-basketball (correct)
  • [x] User Story -- Present, well-scoped
  • [x] Context -- Excellent. Explains spike deliverables, what's wired vs not, production concern.
  • [x] File Targets -- Present, accurate (see verification below)
  • [x] Feature Flag -- "none" with rationale
  • [x] Acceptance Criteria -- 8 items, testable
  • [x] Test Expectations -- Present with run command
  • [x] Constraints -- Present (SolidQueue, sanitize raw body, no UI breakage)
  • [x] Checklist -- Present
  • [x] Related -- Present

Traceability

  • [x] story:WS-S7 label -- "As an admin, I want to send branded email announcements so that parent comms are professional and consistent" -- verified in project-westside-basketball user-stories section
  • [~] arch:westside-basketball label -- label present but no backing architecture note arch-westside-basketball exists in pal-e-docs. Standing [SCOPE] recommendation from previous review. Non-blocking for this ticket.
  • [x] Forgejo issue -- ldraney/westside-basketball#14, state: open

File Targets

All file targets verified against main branch and spike branch (origin/47-actionmailer-spike).
Files to modify (verified):
  • [x] config/environments/development.rb -- exists on main. No delivery_method set currently. Correct target.
  • [x] config/environments/production.rb -- exists on main. No delivery_method set currently. Correct target.
  • [x] app/controllers/emails_controller.rb -- exists on main. Confirmed: send_email action calls GmailClient.new / client.send_email directly (lines 28-29). Spike did NOT modify this file. Correct target.
  • [x] app/mailers/parent_mailer.rb -- exists on spike branch. Confirmed: spike's version does NOT include EmailLogging. Adding the concern here is correct.
  • [x] app/mailers/application_mailer.rb -- exists on main, updated by spike. Issue correctly says "no changes expected."
Files potentially added (verified):
  • [x] db/migrate/*_move_oauth_tokens_to_db.rb -- oauth_tokens table already exists in schema with provider, account, token_data columns. OauthToken model exists. Migration would update DeliveryMethods::GmailApi to read from DB instead of config/gmail/token.json.
  • [x] lib/delivery_methods/gmail_api.rb -- exists on spike branch. Currently reads file-based tokens. Modification for DB tokens is correctly scoped.
Files NOT to touch (verified):
  • [x] app/mailers/concerns/email_logging.rb -- exists on spike branch. Complete implementation with after_deliver callback. No changes needed.
  • [x] app/views/parent_mailer/ -- 4 templates exist on spike branch (announcement + payment_reminder, HTML + text). No changes needed.

Repo Placement

Correct. Issue filed on ldraney/westside-basketball, Repo section says ldraney/westside-basketball. All file targets are within this repo. Single-repo scope.

Dependencies

  • depends:47 (Spike: Rails email sending) -- label present and correct. Spike is board item #1579, column in_progress, PR #52 open. This issue CANNOT start until spike merges. Clearly documented in issue Lineage and Context.
  • Board item #1580 (Queens EYBL email, todo) and #1581 (Email #2 TBD, backlog) both carry depends:47. After #47 merges and #14 activates the mechanism, those items become unblocked.
  • SolidQueue dependency noted in Constraints section -- solid_queue gem confirmed in Gemfile. Queue adapter activation for production may need attention (currently commented out in production.rb).

Acceptance Criteria

8 criteria. Assessment:
  • [x] AC 1 (delivery_method in dev/prod) -- Testable. Grep config files.
  • [x] AC 2 (announcement uses ParentMailer.deliver_later) -- Testable. Unit test on controller.
  • [x] AC 3 (payment_reminder uses ParentMailer.deliver_later) -- Testable, but note: EmailsController currently has no payment_reminder action. Agent will need to add one or clarify scope. Minor ambiguity -- acceptable, agent can resolve.
  • [x] AC 4 (EmailLogging in ParentMailer) -- Testable. Check concern inclusion and email_log writes.
  • [~] AC 5 (compose form works e2e) -- Route is /emails/compose, NOT /admin/emails/new as stated. Minor path error but intent is clear. Agent will find the correct route.
  • [x] AC 6 (blast functionality works) -- Current "blast" is the compose form sending to selected team parents. No separate blast endpoint. Intent is clear.
  • [x] AC 7 (GmailClient deprecated) -- Testable. Remove send calls, keep read/inbox methods.
  • [x] AC 8 (token storage) -- "Migrated to DB or documented as follow-up" -- flexible, testable either way.
Test run command: bin/rails test test/controllers/emails_controller_test.rb test/mailers/ -- these files do not exist yet. Agent will create them. This is expected for a feature ticket.

Blast Radius

Low. GmailClient is used in exactly 2 places: emails_controller.rb lines 28-29 (send_email) and line 37 (inbox). The send path gets replaced by ParentMailer; the inbox/read path stays on GmailClient. No other consumers. No cross-repo impact.
If token storage migration is included, blast radius increases slightly to lib/delivery_methods/gmail_api.rb and app/services/gmail_client.rb (both read tokens). Contained within the same repo.

Decomposition Assessment

5 files to modify across 1 repo, 8 acceptance criteria. Post-spike scope is primarily: activate delivery method in 2 config files, rewrite 1 controller action, add 1 concern include, optionally migrate token storage. Estimated agent work: ~5 minutes. Borderline on AC count (8 > 5 threshold) but the individual changes are small and tightly coupled. No decomposition needed.

Recommendations

Minor items -- none blocking. Verdict is APPROVED.
  • [BODY] AC 5: fix route path /admin/emails/new to /emails/compose (routes.rb confirms emails are not under admin namespace).
  • [BODY] AC 3: clarify whether #14 adds a payment_reminder controller action or if that's future work (current controller only has generic send_email).
  • [SCOPE] Create architecture note arch-westside-basketball in pal-e-docs (standing from previous review, non-blocking).
If APPROVED: No action needed. Minor body items can be addressed during implementation.