pal-e-docs

Review: In-app registration: post-Keycloak flow, phone validation, LeadsController reconciliation (re-review)

review-1831-2026-07-17-r2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after major scope rewrite. The original ticket was decomposed: #59 (this) covers Rails-side only; pal-e-services#184 covers Keycloak realm config. The previous review (review-1831-2026-07-17) returned NEEDS_REFINEMENT with 8 recommendations. 7 of 8 have been addressed. One carried-forward issue and one new issue remain.

Previous Issues -- Resolved

  • [x] [BODY] Removed unnecessary phone migration -- phone column already exists in schema
  • [x] [BODY] Fixed phone/phone_number naming -- issue consistently uses phone
  • [x] [BODY] Keycloak theme work split to pal-e-services#184 -- no longer in scope
  • [x] [BODY] Cross-repo dependency documented -- pal-e-services#184 created, noted as blocker
  • [x] [BODY] Removed nonexistent app/views/pages/home.html.erb reference
  • [x] [BODY] Clarified Postmark independence -- "Does NOT depend on #16 -- Keycloak handles its own SMTP"
  • [x] [BODY] Addressed LeadsController reconciliation in AC #3 -- "repurposed as pre-registration capture form or removed if redundant"

Template Completeness

  • [x] Type (Feature)
  • [x] Story (key: registration)
  • [x] Summary / Context
  • [x] File Targets (6 targets)
  • [x] Acceptance Criteria (6 items)
  • [x] Blast Radius
  • [x] Constraints
  • [x] Blockers
  • [x] Decomposition
  • [x] Repo
  • [ ] Feature Flag -- missing, but acceptable: no docs/feature-flags.md exists in this project yet
  • [ ] Test Expectations -- missing explicit section, but covered implicitly by AC #6

Traceability

  • [x] story:registration label -- Epic 1 (US-1.2--1.4), Visitor/Lead
  • [x] story note verified -- found in project-iss user-stories section (key: registration, backing: Epic 1)
  • [x] arch:auth label -- authentication component
  • [ ] arch note MISSING -- [SCOPE] Neither arch-auth nor arch-auth-iss exists in pal-e-docs. Carried forward from R1.
  • [x] Forgejo issue -- ldraney/intelligentstaffingsystems#59, open

File Targets

  • [x] app/controllers/sessions_controller.rb -- verified: OmniAuth callback with 3-path lead resolution. Walk-in path (line 104) creates Lead with phone: "Not provided". Needs update to extract phone from Keycloak token.
  • [x] app/controllers/leads_controller.rb -- verified: public registration form with skip_before_action :authenticate_user!. Permits phone in lead_params.
  • [x] app/views/leads/new.html.erb -- verified: full registration form (74 lines) with phone telephone_field.
  • [x] app/models/lead.rb -- verified: has validates :phone, presence: true but no format validation. Ticket correctly scopes adding format validation.
  • [x] config/routes.rb -- verified: resources :leads, only: %i[new create] and booking route present.
  • [x] test/controllers/sessions_controller_test.rb -- verified: comprehensive tests for all 3 lead resolution paths (by sub, email claim, walk-in).

Blast Radius

  • [x] app/controllers/application_controller.rb -- verified: authenticate_user! checks session[:user], compatible with self-registered users via walk-in path.
  • [ ] app/helpers/sessions_helper.rb -- ISSUE: file does not exist. Actual file is app/helpers/session_helper.rb (singular, module SessionHelper). [BODY] Fix filename.

Repo Placement

Correct. All file targets are in intelligentstaffingsystems. Keycloak realm config has been properly split to pal-e-services#184 as a separate ticket. Both items are on board-iss with sprint:A label.

Dependencies

  • #1864 (pal-e-services#184, "Keycloak ISS realm: self-registration, phone field, SMTP") -- sprint:A, backlog. Prerequisite for this ticket. Documented as blocker. Correct.
  • #6 (Keycloak OIDC auth) -- done. Prerequisite satisfied.
  • #8 (Lead model + registration) -- done. Prerequisite satisfied.
  • #16 (Postmark email) -- explicitly NOT a dependency. Correct.
  • No undocumented blockers found.

Acceptance Criteria

6 acceptance criteria. All testable. Notes:
  • AC #1 (post-registration redirect to dashboard) -- partially satisfied by existing code: sessions_controller.rb:56 already redirects to dashboard_path. The new work is ensuring Keycloak's post-registration flow reaches this callback, which depends on pal-e-services#184.
  • AC #3 (LeadsController reconciliation) -- gives agent appropriate flexibility with "either repurposed or removed". Acceptable.
  • AC #4 (OmniAuth callback for first-login) -- walk-in path already exists (lines 101-114). Enhancement needed for phone sync, not a greenfield implementation.
  • AC #5 (phone sync from Keycloak) -- clear and testable. Requires Keycloak to provide phone in token (pal-e-services#184).
  • AC #6 (tests) -- test file exists with good coverage. Incremental test additions needed.

Decomposition Assessment

6 file targets in 1 repo. 6 ACs (slightly over 5-AC threshold). However, AC #1 and #4 are incremental enhancements to existing code, and AC #6 is a meta-criterion. Effective new work is ~3 discrete changes (phone validation, phone sync, LeadsController reconciliation). Single agent pass under 5 minutes. No decomposition needed.

Recommendations

  • [SCOPE] Create architecture note arch-auth (or arch-auth-iss) for the authentication component in pal-e-docs. Carried forward from R1 -- this is a traceability requirement for the arch:auth label.
  • [BODY] Fix blast radius file path: app/helpers/sessions_helper.rb should be app/helpers/session_helper.rb (singular, module SessionHelper).