pal-e-docs

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

review-1825-2026-07-18b Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after full rewrite. The ticket is now correctly scoped as a delta (controller-level lead blocking). One critical AC issue remains that would block an implementing agent.

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — delta on #40 + #42
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag — None (correct, this is a security gate)
  • [x] Acceptance Criteria — 4 items
  • [x] Test Expectations — 4 items + run command
  • [x] Constraints
  • [x] Checklist
  • [x] Related

Traceability

  • [x] story:messaging label — verified in project-iss user-stories section (Epic 4, US-4.1–4.2)
  • [ ] arch note MISSING — [SCOPE] No arch-rails note found in pal-e-docs. However, arch:rails is a framework-level label used across many tickets; a dedicated note is acceptable to defer.
  • [x] Forgejo issue — ldraney/intelligentstaffingsystems#55, state: open

File Targets

  • [x] app/controllers/messages_controller.rb — verified: 103 lines, no lead-blocking guard exists. Uses current_user_has_role?("admin") pattern. An existing restrict_to_roles class method in ApplicationController (line 26) provides the same pattern.
  • [x] test/controllers/messages_controller_test.rb — verified: 375 lines, 40 test methods. Contains 22 tests using sign_in_as :lead that assert successful access (200 OK, message content, forms, etc.).

Repo Placement

OK — issue filed in ldraney/intelligentstaffingsystems, all targets are in that repo.

Dependencies

  • #40 Message CRUD — done (board item 1817)
  • #42 Turbo Streams live delivery — done (board item 1818)
  • No active blockers. All upstream work is complete.

Acceptance Criteria

CRITICAL: AC #4 is contradictory and would block implementation.
AC #4 states: "Existing 49 message tests still pass (no regressions)." However:
  • The test file has 40 test methods (not 49)
  • 22 of those 40 tests use sign_in_as :lead and assert successful access (200, content rendering, form presence, turbo streams, etc.)
  • Adding a before_action that redirects leads will cause all 22 to fail with 302 instead of 200
  • An implementing agent following AC #4 literally would be stuck in an impossible state — it cannot both block leads AND keep lead-success tests passing
AC #1–3 are clear and testable. The Test Expectations section correctly specifies new redirect tests but does not acknowledge that existing lead tests must be rewritten.

Blast Radius

  • docs/messaging.md line 1 says "At the controller level, both leads and clients can access their own thread" — must be updated (ticket's Related section acknowledges this)
  • docs/security.md references the controller role map — would need updating
  • No other controllers use the same pattern for leads — the change is isolated to MessagesController
  • The existing restrict_to_roles method in ApplicationController could be reused instead of a custom require_client_or_admin

Decomposition Assessment

2 file targets in 1 repo, 4 AC. The actual work (1 before_action + test rewrites) fits in a single agent pass well under 5 minutes. No decomposition needed.

Recommendation

  • [BODY] Replace AC #4 with: "Lead-role tests in messages_controller_test.rb rewritten to assert redirect (22 tests converted from success assertions to 302 redirect assertions); client and admin tests unchanged (no regressions in non-lead tests)"
  • [BODY] Fix test count: file has 40 test methods, not 49
  • [BODY] Add to Test Expectations: "Rewrite existing lead tests (sign_in_as :lead) to assert 302 redirect to /communications. Convert tests that verify lead-specific rendering (compose form, turbo streams, etc.) to use client role instead."