pal-e-docs

Review: In-app registration: phone number field + email verification

review-1831-2026-07-17 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type
  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Feature Flag
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

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
  • [x] arch:auth label — authentication component
  • [ ] arch note MISSING — [SCOPE] Create architecture note arch-auth for the auth component
  • [x] Forgejo issue — #59, open

File Targets

  • [ ] db/migrate/XXX_add_phone_to_leads.rb — ISSUE: phone column already exists in schema (created in 20260705000002_create_leads.rb). No migration needed. The field is named phone, not phone_number.
  • [x] app/models/lead.rb — verified: exists, already has validates :phone, presence: true. No model changes needed for phone field.
  • [x] app/controllers/sessions_controller.rb — verified: exists, walk-in creation uses phone: "Not provided" placeholder at line 109. Needs update to extract phone from Keycloak userinfo/token claims.
  • [ ] keycloak-theme/iss/login/ — ISSUE: directory exists but the current theme is CSS-only with parent=keycloak (inherits default registration form). Adding a custom phone attribute to the registration form requires a register.ftl FreeMarker template, not just CSS. The ticket does not mention creating this template.
  • [ ] Keycloak realm config (pal-e-services tfvars) — ISSUE: this file lives in the pal-e-services repo, not intelligentstaffingsystems. Cross-repo dependency not explicitly called out.
  • [ ] app/views/pages/home.html.erb (listed as “not to touch”) — ISSUE: file does not exist. Minor inaccuracy in the “don’t touch” list.

Repo Placement

The Forgejo issue is filed on ldraney/intelligentstaffingsystems, but two file targets live in other repos:
  • Keycloak realm config (email verification, phone attribute) — lives in pal-e-services tfvars
  • keycloak-theme/iss/login/ — lives in THIS repo (OK)
The pal-e-services changes (enabling email verification, adding phone as a required user attribute) should either be a separate Forgejo issue on pal-e-services, or the cross-repo dependency should be explicitly documented in this issue’s Constraints section.

Dependencies

  • #6 (Keycloak OIDC auth) — done (closed), prerequisite satisfied
  • #8 (Lead model + registration) — done (closed), prerequisite satisfied
  • #58 (Landing page) — backlog, open. Not a blocker; related but independent
  • #16 (Postmark email integration) — backlog, sprint 5. Potential undocumented dependency: AC #5 says “Keycloak sends verification email via Postmark SMTP”. Keycloak needs SMTP credentials configured in realm settings to send verification emails. If Postmark SMTP is not yet configured in Keycloak, this ticket is blocked. Clarify whether Keycloak SMTP is already configured or if this depends on #16.

Acceptance Criteria

9 acceptance criteria. All are generally testable but several concerns:
  • AC #5 (“Keycloak sends verification email via Postmark SMTP”) — conflates Keycloak realm SMTP config with the Rails Postmark integration (#16). Keycloak has its own SMTP config independent of Rails. Clarify which layer owns this.
  • AC #8 (“Lead record created/claimed on first login per existing resolution ladder”) — this already works today. Not a new criterion but a regression guard. OK as written.
  • Test commands are valid: rails test test/controllers/sessions_controller_test.rb test/models/lead_test.rb — both test files exist.

Blast Radius

  • The existing web registration form (leads/new.html.erb, LeadsController) still exists. The ticket says “The website has NO registration form — all registration is in-app” but does not scope removing or disabling the existing web form. This creates a contradictory state. Should the existing web form be removed in this ticket or a follow-up?
  • The keycloak-theme change (adding register.ftl) could affect login page appearance if not carefully scoped to registration only.
  • Existing tests reference phone field extensively (model tests, fixture data, profile tests, leads controller tests). Changes to phone handling in sessions_controller should not break these.

Decomposition Assessment

File targets span 2 repos (intelligentstaffingsystems + pal-e-services). 9 acceptance criteria exceeds the 5 AC threshold. The work naturally splits into:
  • (a) Keycloak infrastructure: enable email verification + add phone user attribute (pal-e-services issue)
  • (b) Rails code: extract phone from OIDC token in sessions_controller + update tests (ISS issue, small)
  • (c) Keycloak theme: add register.ftl with phone field + ISS branding (ISS issue, moderate)
Borderline for decomposition. If the pal-e-services work is split out as a separate issue, the remaining ISS work (b + c) is ~5 min. Recommend splitting pal-e-services config into its own issue rather than full decomposition via skill-decompose-ticket.

Recommendations

  • [BODY] Remove file target db/migrate/XXX_add_phone_to_leads.rb — phone column already exists in schema. No migration needed.
  • [BODY] Fix field name inconsistency: the column is phone, not phone_number. Update issue title or body to match.
  • [BODY] Add file target keycloak-theme/iss/login/register.ftl — a FreeMarker template is needed to add the phone field to Keycloak registration. CSS-only theme cannot add form fields.
  • [BODY] Clarify cross-repo dependency: pal-e-services tfvars changes (enable email verification, add phone attribute) should be a separate Forgejo issue on pal-e-services, or explicitly documented as a prerequisite.
  • [BODY] Remove app/views/pages/home.html.erb from the “not to touch” list — file does not exist.
  • [BODY] Clarify AC #5: does Keycloak SMTP need to be configured as part of this ticket, or is it already done? If not done, this ticket depends on Keycloak SMTP setup (which may or may not be #16).
  • [BODY] Address the existing web registration form (leads/new.html.erb): should it be removed/disabled in this ticket or tracked separately?
  • [SCOPE] Create architecture note arch-auth for the authentication component in pal-e-docs.