pal-e-docs

Review: Appointment scheduling: 24hr advance, text and attachments

review-1824-2026-07-18 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] ### Type — Feature
  • [x] ### Lineage — Related to #15
  • [x] ### Repo — ldraney/intelligentstaffingsystems
  • [x] ### User Story — Present (As a lead or client...)
  • [x] ### Context — Present, good background
  • [x] ### File Targets — Present (6 files to create/modify, 1 exclusion)
  • [x] ### Feature Flag — None (acceptable for core workflow)
  • [x] ### Acceptance Criteria — Present (10 items)
  • [x] ### Test Expectations — Present (9 items + run command)
  • [x] ### Constraints — Present (4 items)
  • [x] ### Checklist — Present
  • [x] ### Related — Present

Traceability

  • [x] story:communications label — present on board item
  • [ ] story note MISSING — [SCOPE] "communications" is not registered as a story key in project-iss user-stories section. Only "messaging" exists (Epic 4). Create user story entry on project-iss for "communications" or clarify whether this should use story:messaging.
  • [x] arch:rails label — present on board item
  • [ ] arch note MISSING — [SCOPE] Search for "arch-rails" returned no results. Known gap (also flagged in review-1820). Create architecture note arch-rails for the Rails component.
  • [x] Forgejo issue — #54, open

File Targets

  • [x] app/controllers/appointments_controller.rb — verified: does not exist, to be created (correct)
  • [x] app/views/appointments/new.html.erb — verified: does not exist, to be created (correct)
  • [x] app/views/appointments/_confirmation.html.erb — verified: does not exist, to be created (correct)
  • [x] app/models/appointment.rb — verified: does not exist, to be created (correct)
  • [x] app/assets/stylesheets/appointments.css — verified: directory exists, file to be created (correct, matches existing convention)
  • [ ] db/migrate/XXX_add_topic_and_attachments_to_appointments.rb — ISSUE: Migration description says "add topic, notes columns if not present" implying ALTER TABLE, but no appointments table exists in schema. Migration should CREATE the full appointments table (id, lead_id, scheduled_at, topic, notes, status, timestamps).
  • [ ] MISSING: config/routes.rb — Currently has placeholder get "booking", to: "leads#booking". Must be updated to route to the new appointments controller. Without this, the new controller is unreachable.
  • [ ] MISSING: app/controllers/leads_controller.rb — The booking action (line 31) becomes dead code after route change. Should be addressed (remove or redirect).
  • [ ] MISSING: app/views/leads/booking.html.erb — Placeholder view ("Appointment booking is coming soon") should be removed after the real implementation ships.
  • [x] app/controllers/communications_controller.rb — correctly excluded (button linking to booking_path already exists in _contact_cards.html.erb)

Repo Placement

OK — issue filed on ldraney/intelligentstaffingsystems, all file targets are in this repo. Single-repo change.

Dependencies

  • #52 (Communications tab, board item #1822) — DONE. The "Set an Appointment" button exists in _contact_cards.html.erb line 50, linking to booking_path. Dependency satisfied.
  • #15 (Appointment booking and calendar) — Referenced in lineage. The current booking_path placeholder was created for #15. This ticket (#54) implements the authenticated booking flow that #15 originally scoped.
  • ActiveStorage — Already configured (migration 20260706183426_create_active_storage_tables exists in schema). No blocker.
  • No in_progress items block this ticket.

Acceptance Criteria

All 10 criteria are testable by an agent. The test expectations align well with the acceptance criteria. The run command is specific: rails test test/controllers/appointments_controller_test.rb test/models/appointment_test.rb.
Minor gap: No AC covers the route change or what happens to the existing booking_path placeholder. An agent implementing this may not realize the route needs updating.

Blast Radius

  • test/controllers/leads_controller_test.rb line 79 — tests the current booking placeholder. Will need removal or update.
  • test/controllers/communications_controller_test.rb line 111 — tests that appointment card links to booking_path. If the route name changes from booking_path to e.g. new_appointment_path, this test breaks.
  • app/controllers/leads_controller.rb line 23 — redirects to booking_path after lead creation. If route name changes, this redirect breaks.
  • Recommendation: keep booking_path as an alias or redirect to minimize blast radius.

Decomposition Assessment

6 file targets in 1 repo. 10 acceptance criteria (exceeds 5 threshold). However, all work is cohesive (single controller + model + views + migration), and the user preference is to keep tickets inclusive. No decomposition needed — single agent pass is feasible given the cohesive scope.

Recommendation

  • [BODY] Add config/routes.rb to File Targets — replace placeholder get "booking", to: "leads#booking" with appointment resource routes. Recommend keeping booking_path as a named route alias to avoid blast radius on existing tests and redirects.
  • [BODY] Add app/controllers/leads_controller.rb and app/views/leads/booking.html.erb to File Targets (cleanup of dead placeholder code).
  • [BODY] Fix migration description — change "add topic, notes columns if not present" to "create appointments table with lead_id, scheduled_at, topic, notes, status columns".
  • [SCOPE] Register "communications" as a story key in project-iss user-stories section, or reclassify as story:messaging if the Communications tab falls under Epic 4.
  • [SCOPE] Create architecture note arch-rails for the Rails component (known gap, applies to many ISS tickets).