Replace first/last name with single Name field on profile edit #227

Merged

ldraney merged 4 commits from feature/single-name-field into main 2026-06-15 12:27:12 +00:00

Conversation 1 Commits 4 Files changed 5 +23 -29

ldraney commented 2026-06-15 12:20:40 +00:00

Owner

Copy link

Summary

• Merges separate "First Name" and "Last Name" inputs into a single "Name" field on profile edit
• Landscapers just need a reference label for properties, not a formal name split
• Stores value in Keycloak's existing firstName field — no schema change needed

Closes #226

Changes

• app/views/profile/edit.html.erb -- removed first_name + last_name inputs, replaced with single name input reading from firstName
• app/controllers/profile_controller.rb -- changed params from first_name/last_name to name, removed last_name from Keycloak update call, updated validation message to "Name is required"
• spec/requests/profile_spec.rb -- updated all param references from first_name/last_name to name, updated validation expectation text

Test Plan

• bundle exec rspec spec/requests/profile_spec.rb — 34 examples, 0 failures
• Profile edit form shows single "Name" field
• Submitting a name persists to Keycloak and updates session display name
• Blank name rejected with "Name is required"
• Existing users see their current firstName pre-filled

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No — simple form field consolidation, no new workflow

Related Notes

• ldraney/landscaping-assistant #226 — the Forgejo issue this PR implements
• landscaping-assistant — project this work belongs to

## Summary - Merges separate "First Name" and "Last Name" inputs into a single "Name" field on profile edit - Landscapers just need a reference label for properties, not a formal name split - Stores value in Keycloak's existing `firstName` field — no schema change needed Closes #226 ## Changes - `app/views/profile/edit.html.erb` -- removed first_name + last_name inputs, replaced with single `name` input reading from `firstName` - `app/controllers/profile_controller.rb` -- changed params from `first_name`/`last_name` to `name`, removed `last_name` from Keycloak update call, updated validation message to "Name is required" - `spec/requests/profile_spec.rb` -- updated all param references from `first_name`/`last_name` to `name`, updated validation expectation text ## Test Plan - [x] `bundle exec rspec spec/requests/profile_spec.rb` — 34 examples, 0 failures - [ ] Profile edit form shows single "Name" field - [ ] Submitting a name persists to Keycloak and updates session display name - [ ] Blank name rejected with "Name is required" - [ ] Existing users see their current firstName pre-filled ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — simple form field consolidation, no new workflow ## Related Notes - `ldraney/landscaping-assistant #226` — the Forgejo issue this PR implements - `landscaping-assistant` — project this work belongs to

ldraney added 1 commit 2026-06-15 12:20:40 +00:00

Replace first/last name fields with single Name field on profile edit ... 6ea90a7bf9

Landscapers just need a reference label for properties, not a formal
first/last name split. Merges the two inputs into one, stores the value
in Keycloak's firstName field, and updates validation + specs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney added 1 commit 2026-06-15 12:21:09 +00:00

Merge branch 'main' into feature/single-name-field b4e38b4ffb

ldraney commented 2026-06-15 12:24:01 +00:00

Author

Owner

Copy link

PR #227 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails (ERB views, request specs, controller with Keycloak integration)

Controller changes (app/controllers/profile_controller.rb):

• params[:first_name] / params[:last_name] correctly replaced with params[:name]
• last_name: kwarg removed from kc.update_user call -- only first_name: is sent now
• Session updates correctly reference params[:name] for both :display_name and :first_name
• Validation message updated from "First name is required" to "Name is required"

View changes (app/views/profile/edit.html.erb):

• Two form fields (first_name + last_name) consolidated into one name field
• Label, for attribute, name attribute, and id attribute all consistently use "name"
• value still reads from @user_data["firstName"] (Keycloak's attribute) -- correct, since Keycloak stores it as firstName
• required attribute and * indicator retained on the single field
• Net deletion of 6 lines -- clean removal with no orphaned markup

Spec changes (spec/requests/profile_spec.rb):

• All 10 test cases updated from first_name:/last_name: params to name: param
• Validation test correctly expects "Name is required" instead of "First name is required"
• Session assertion updated: session[:user][:first_name] now expected to equal "Updated Name" (full name string)
• Happy path, error path, validation edge cases, and session persistence all covered

Codebase impact: Exhaustive search confirms zero remaining references to last_name or lastName outside the three changed files. No orphaned code.

BLOCKERS

1. Stale lastName data in Keycloak (data integrity concern)

The PR removes last_name: from the kc.update_user call but does not explicitly clear it (e.g., last_name: ""). Existing users who previously saved a last name will retain that stale value in Keycloak's lastName attribute forever. This is not a crash risk, but it is a data cleanliness issue.

Severity assessment: This is a NIT, not a true blocker -- the lastName field is simply no longer displayed or used by the application. However, if any future Keycloak-side reporting or external integration reads lastName, it will contain stale data. Recommend adding last_name: "" to the update call to explicitly clear it, or documenting the decision to leave it.

No true blockers found.

NITS

1. Session key mismatch -- session[:user][:first_name] now stores a full name (e.g., "Updated Name"). The key name :first_name is misleading when it holds a combined name. Consider renaming to :name for consistency. Low priority since it is internal-only, but future developers will be confused by session[:user][:first_name] containing "Jane Doe".

2. Stale Keycloak lastName -- As noted above, consider sending last_name: "" in the update_user call to explicitly clear the field for users who previously had a last name saved. One-line addition to the controller.

3. Test naming -- The spec description at line 205 tests session update with name: "Updated Name" and asserts session[:user][:first_name] equals "Updated Name". A comment or more descriptive it block noting the intentional key reuse would help readability.

SOP COMPLIANCE

• PR body has: Summary, Changes, Test Plan, Related -- all present and well-structured
• Tests exist and pass (34 examples, 0 failures per Test Plan)
• No secrets, .env files, or credentials committed
• No unnecessary file changes -- exactly 3 files, all directly related to the name field consolidation
• Commit messages are descriptive
• Scope is tight -- no feature creep, no unrelated changes
• Closes #226 links to parent issue

PROCESS OBSERVATIONS

• Change failure risk: LOW. This is a pure simplification -- removing a field, not adding complexity. The Keycloak firstName attribute is reused as the storage backend, so no schema migration is needed.
• Deployment frequency impact: NONE. No feature flag needed for a form field consolidation.
• Documentation gap: The session key :first_name now holds a semantically different value. If there is any developer documentation describing session structure, it should be updated.

VERDICT: APPROVED

Clean, well-scoped PR. Tests are comprehensive. The stale lastName and session key naming are nits worth addressing in a follow-up but do not block this merge.

## PR #227 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails (ERB views, request specs, controller with Keycloak integration) **Controller changes** (`app/controllers/profile_controller.rb`): - `params[:first_name]` / `params[:last_name]` correctly replaced with `params[:name]` - `last_name:` kwarg removed from `kc.update_user` call -- only `first_name:` is sent now - Session updates correctly reference `params[:name]` for both `:display_name` and `:first_name` - Validation message updated from "First name is required" to "Name is required" **View changes** (`app/views/profile/edit.html.erb`): - Two form fields (first_name + last_name) consolidated into one `name` field - Label, `for` attribute, `name` attribute, and `id` attribute all consistently use "name" - `value` still reads from `@user_data["firstName"]` (Keycloak's attribute) -- correct, since Keycloak stores it as `firstName` - `required` attribute and `*` indicator retained on the single field - Net deletion of 6 lines -- clean removal with no orphaned markup **Spec changes** (`spec/requests/profile_spec.rb`): - All 10 test cases updated from `first_name:`/`last_name:` params to `name:` param - Validation test correctly expects "Name is required" instead of "First name is required" - Session assertion updated: `session[:user][:first_name]` now expected to equal "Updated Name" (full name string) - Happy path, error path, validation edge cases, and session persistence all covered **Codebase impact:** Exhaustive search confirms zero remaining references to `last_name` or `lastName` outside the three changed files. No orphaned code. ### BLOCKERS **1. Stale `lastName` data in Keycloak (data integrity concern)** The PR removes `last_name:` from the `kc.update_user` call but does not explicitly clear it (e.g., `last_name: ""`). Existing users who previously saved a last name will retain that stale value in Keycloak's `lastName` attribute forever. This is not a crash risk, but it is a data cleanliness issue. **Severity assessment:** This is a NIT, not a true blocker -- the `lastName` field is simply no longer displayed or used by the application. However, if any future Keycloak-side reporting or external integration reads `lastName`, it will contain stale data. Recommend adding `last_name: ""` to the update call to explicitly clear it, or documenting the decision to leave it. **No true blockers found.** ### NITS 1. **Session key mismatch** -- `session[:user][:first_name]` now stores a full name (e.g., "Updated Name"). The key name `:first_name` is misleading when it holds a combined name. Consider renaming to `:name` for consistency. Low priority since it is internal-only, but future developers will be confused by `session[:user][:first_name]` containing "Jane Doe". 2. **Stale Keycloak `lastName`** -- As noted above, consider sending `last_name: ""` in the `update_user` call to explicitly clear the field for users who previously had a last name saved. One-line addition to the controller. 3. **Test naming** -- The spec description at line 205 tests session update with `name: "Updated Name"` and asserts `session[:user][:first_name]` equals `"Updated Name"`. A comment or more descriptive `it` block noting the intentional key reuse would help readability. ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related -- all present and well-structured - [x] Tests exist and pass (34 examples, 0 failures per Test Plan) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- exactly 3 files, all directly related to the name field consolidation - [x] Commit messages are descriptive - [x] Scope is tight -- no feature creep, no unrelated changes - [x] `Closes #226` links to parent issue ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** This is a pure simplification -- removing a field, not adding complexity. The Keycloak `firstName` attribute is reused as the storage backend, so no schema migration is needed. - **Deployment frequency impact: NONE.** No feature flag needed for a form field consolidation. - **Documentation gap:** The session key `:first_name` now holds a semantically different value. If there is any developer documentation describing session structure, it should be updated. ### VERDICT: APPROVED Clean, well-scoped PR. Tests are comprehensive. The stale `lastName` and session key naming are nits worth addressing in a follow-up but do not block this merge.

ldraney added 2 commits 2026-06-15 12:25:32 +00:00

Address review nits: rename session key, clear stale lastName ... 8419a67f01

- Rename session[:user][:first_name] to session[:user][:name] in both
  profile and sessions controllers
- Send last_name: "" on profile update to clear stale Keycloak data
- Update keycloak service to use key? check for lastName so empty
  string clears the field

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Merge branch 'feature/single-name-field' of https://forgejo.tail5b443a.ts.net/ldraney/landscaping-assistant into feature/single-name-field e2667a4e99

ldraney merged commit c7355d8ce3 into main 2026-06-15 12:27:12 +00:00

ldraney deleted branch feature/single-name-field 2026-06-15 12:27:12 +00:00

ldraney referenced this pull request from a commit 2026-06-15 12:27:12 +00:00

Replace first/last name with single Name field on profile edit (#227)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/landscaping-assistant!227

No description provided.