pal-e-docs

Review: Live DM messaging: real-time client-admin threads

review-1825-2026-07-18c Review

review approved
Project

Verdict: APPROVED

Round 3 re-review. All three issues from round 2 (review-1825-2026-07-18b) are resolved. Ticket is ready for implementation.

Template Completeness

  • [x] Type (Feature)
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets (with DO/DON'T lists)
  • [x] Feature Flag (None — correct, this is an access control fix)
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:messaging label — Epic 4 (US-4.1–4.2)
  • [x] story note verified — found in project-iss user-stories section (key: messaging, role: Lead/Client/Admin)
  • [x] arch:rails label — Rails controller work
  • [ ] arch note MISSING — [SCOPE] Create architecture note arch-rails (systemic gap; does not block this ticket)
  • [x] Forgejo issue — ldraney/intelligentstaffingsystems#55, open

File Targets

  • [x] app/controllers/messages_controller.rb — verified: exists, 103 lines, currently has no lead-blocking guard, has before_action :set_thread_lead and private admin? helper
  • [x] test/controllers/messages_controller_test.rb — verified: exists, 375 lines, 40 tests total, 22 use lead role (21 via sign_in_as :lead + 1 via OmniAuth direct)
  • [x] Files NOT to touch listed — views, model, Stimulus controller, cable.yml all exist and are correctly excluded

Repo Placement

OK. Issue filed on ldraney/intelligentstaffingsystems, all work is in the same repo. No cross-repo concerns.

Dependencies

  • #40 (Message CRUD) — done (board confirms, column: done)
  • #42 (Turbo Streams live delivery) — done (board confirms, column: done)
  • No in-progress blockers
  • Dependencies correctly documented in Lineage section

Acceptance Criteria

5 AC, all machine-verifiable:
  • AC 1: Lead redirect to /communications with flash — testable via integration test (302 + flash check)
  • AC 2: Client access unchanged — testable (existing tests unchanged)
  • AC 3: Admin access unchanged — testable (existing tests unchanged)
  • AC 4: 22 lead tests rewritten to assert 302 — testable (run test file, verify count)
  • AC 5: Client/admin tests pass unchanged — testable (regression baseline)
Test Expectations section adds specific endpoint tests and conversion strategy. Run command is correct: rails test test/controllers/messages_controller_test.rb

Blast Radius

  • Change is confined to MessagesController — no other controller routes leads to /messages
  • /communications route confirmed to exist (routes.rb line 29)
  • ApplicationController has existing require_role class method but ticket correctly uses custom method for non-standard redirect target and flash
  • docs/messaging.md currently documents UI-only gating; ticket correctly notes it must be updated to reflect controller-level enforcement

Decomposition Assessment

No decomposition needed:
  • 2 file targets in 1 repo
  • 5 acceptance criteria (at threshold, not over)
  • Estimated agent work: ~3 minutes (add before_action + rewrite test assertions)
  • Single-concern change: add one guard, update tests to match

Round 2 Issues — Resolution Status

  • [x] AC #4 contradictory ("49 tests still pass" vs "22 lead tests break") — FIXED: AC now reads "22 existing lead-role tests rewritten to assert 302 redirects"
  • [x] Wrong test count (49 vs 40) — FIXED: Context now reads "40 tests passing"
  • [x] Missing test expectation for lead test rewrite — FIXED: Test Expectations includes "Existing lead tests converted: assert redirect instead of 200" and "Lead rendering tests converted to client role"

Recommendation

  • [SCOPE] Create architecture note arch-rails for component rails (systemic gap across board — non-blocking)
No action needed for ticket advancement. Ticket is APPROVED for implementation.