pal-e-docs

Review: Message model, migration, controller, and views (CRUD)

review-1817-2026-07-06 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature (child of #12)
  • [x] Lineage — parent #12, decomp:1792, dependencies listed
  • [x] Repo — ldraney/intelligentstaffingsystems
  • [x] User Story — "As a lead or admin, I can view and send messages..."
  • [x] Context — CRUD foundation for Epic 4, US-4.1/4.2, one thread per lead
  • [x] File Targets — 5 create targets listed with descriptions
  • [x] Feature Flag — "None" (correct for internal CRUD, no flag doc exists)
  • [x] Acceptance Criteria — 6 ACs from parent #12 AC 1-5, 8
  • [x] Test Expectations — model, controller, integration tests; run command provided
  • [x] Constraints — docs-in-PR rule, security.md line references, ISS brand palette
  • [x] Checklist — 8 items
  • [x] Related — project, parent, sibling, blockers, user stories all listed

Traceability

  • [x] story:messaging label — present on board item #1817
  • [x] story note verified — found in project-iss user-stories section: "messaging", Epic 4 (US-4.1–4.2), Role Lead/Client/Admin, "One thread per lead; admin sees all"
  • [x] arch:rails label — present on board item #1817
  • [ ] arch note MISSING — [SCOPE] No arch-rails note exists in pal-e-docs. This is a known board-wide gap flagged in prior ISS reviews (review-1791, review-1815, review-1814, review-1800). Create architecture note arch-rails-iss or a shared arch-rails note for the Rails application component.
  • [x] Forgejo issue — https://forgejo.tail5b443a.ts.net/ldraney/intelligentstaffingsystems/issues/40, open

File Targets

  • [x] app/models/message.rb — verified: does not exist yet (create target). Lead model at app/models/lead.rb confirmed present with keycloak_sub field for sender identification. No existing has_many associations on Lead — adding :messages is safe.
  • [x] db/migrate/xxx_create_messages.rb — verified: no existing messages migration. Migration directory exists.
  • [x] app/controllers/messages_controller.rb — verified: does not exist yet. require_role macro confirmed in ApplicationController (line 23). docs/security.md line 112 documents the expected role gate: "lead, client, admin" with data scoping.
  • [x] app/views/messages/ — verified: directory does not exist yet. Existing view directories (leads, pages, sessions) confirm the convention.
  • [x] app/assets/stylesheets/messages.css — verified: does not exist yet. Existing stylesheets (application.css, forms.css, pages.css) confirm the naming convention.

Constraint Verification

  • [x] docs/security.md line 112 — verified: "MessagesController | lead, client, admin | Role gate + data scoping: leads/clients see only their own thread; admin sees all (#12)"
  • [x] docs/security.md line 200 — verified: "Rails CSRF protection is enabled by default... Turbo injects tokens automatically via meta tags, including for Turbo Frame submissions (message create, #12)"
  • [x] ISS brand palette — Navy #1b2a4a + gold #996d13 specified in constraints

Repo Placement

OK. Issue filed on ldraney/intelligentstaffingsystems. All file targets are in the same repo. Single-repo scope, no cross-repo concerns.

Dependencies

  • #6 Keycloak OIDC auth (board item 1786) — validation column. Auth integration is implemented; ApplicationController has authenticate_user! and require_role. Satisfied.
  • #8 Lead model (board item 1788) — validation column. app/models/lead.rb exists with keycloak_sub, role enum, and scopes. Satisfied.
  • #10 Bottom tab bar (board item 1790) — backlog. Documented as dependency in the issue. The CRUD itself (model, migration, controller, views) does not require the tab bar. Only the navigation entry to reach /messages depends on #10. No TAB_ROLES or tab navigation code exists in the codebase yet. Advisory: CRUD can proceed, but AC #1 ("Messages tab shows...") implies tab bar existence.
  • #42 Turbo Streams (board item 1818) — backlog, sibling ticket. Not a blocker; real-time delivery is explicitly out of scope for this ticket.

Acceptance Criteria

6 ACs, all testable and verifiable by an agent:
  • AC1 (Messages tab shows thread) — integration test, verifiable via page content assertions
  • AC2 (Lead scoped by lead_id) — controller test with different user sessions
  • AC3 (Admin sees all) — controller test with admin session
  • AC4 (Chronological order) — model/controller test with ordered records
  • AC5 (Compose and submit) — integration test, form submission
  • AC6 (has_many :messages on Lead) — model test, association assertion
All ACs are concrete, testable, and have clear pass/fail criteria. Test commands are real (rails test).

Blast Radius

Low. No existing messaging code in the codebase. No existing associations on the Lead model. The has_many :messages addition is additive-only and cannot break existing functionality. No sibling services affected. The Turbo Streams sibling (#42) builds on top of this CRUD, so this ticket's schema and model decisions will constrain #42's implementation.

Decomposition Assessment

  • 5 file targets in 1 repo (threshold: >3 across >2 repos) — PASS
  • 6 ACs (threshold: >5) — borderline, but all ACs are tightly coupled standard CRUD operations. This ticket is already a decomposition of parent #12 (which was 8 points). Further splitting would create artificial boundaries between model/controller/views that are typically scaffolded together.
  • Estimated agent work: ~5-7 minutes for standard Rails CRUD with role scoping, tests, docs, and styles. Borderline but feasible in a single pass.
No decomposition needed. The 6 ACs are a cohesive CRUD unit that an agent handles as one operation.

Recommendation

  • [SCOPE] Create architecture note arch-rails-iss (or shared arch-rails) for the Rails application component. This is a board-wide gap previously flagged in reviews for items 1791, 1815, 1814, and 1800. Resolving it once clears the traceability gap for all arch:rails-labeled ISS items.