From 349027281526adc0376f67b2e3a46aa9c2f1080b Mon Sep 17 00:00:00 2001 From: Peer Richelsen Date: Tue, 18 Nov 2025 06:54:33 +0100 Subject: [PATCH] feat: normalize routing form identifier field with URL-safe format (#25128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What does this PR do? - Fixes #25116 - Fixes [CAL-6741](https://linear.app/calcom/issue/CAL-6741/make-routing-edit-question-form-less-intrusive-on-how-it-handles-the) https://github.com/user-attachments/assets/d78c4c62-2af5-44d8-b25c-e938cc8ddf6f Improves the routing form field identifier auto-fill behavior to generate URL-safe identifiers from labels. Previously, the identifier field simply duplicated the label text. Now it normalizes the input to be URL-friendly. **Changes:** - Converts labels to lowercase with hyphens (e.g., "What is your name?" → "what-is-your-name") - Limits identifiers to the first 5 words (e.g., "What do you work on and how can we help?" → "what-do-you-work-on") - Updates the Identifier label to clarify it's a URL parameter with an example - Preserves manual identifier edits when the label changes **Link to Devin run:** https://app.devin.ai/sessions/f569297990fe4436bab1fec89d8b71ac **Requested by:** @PeerRich (peer@cal.com) ## Key Implementation Details The `normalizeIdentifier` function: - Strips special characters and replaces them with spaces - Converts to lowercase - Splits on whitespace and takes first 5 words - Joins with hyphens The auto-fill logic only updates the identifier if: 1. The identifier field is empty, OR 2. The identifier matches the normalized version of the previous label This preserves manual edits while still providing helpful auto-fill for new fields. ## Mandatory Tasks (DO NOT REMOVE) - [x] I have self-reviewed the code (A decent size PR without self-review might be rejected). - [x] I have updated the developer docs in /docs if this PR makes changes that would require a [documentation change](https://cal.com/docs). If N/A, write N/A here and check the checkbox. **N/A** - This is a UI behavior change that doesn't require documentation updates. - [ ] I confirm automated tests are in place that prove my fix is effective or that my feature works. **Note:** No automated tests were added for this change. Manual testing is recommended. ## How should this be tested? **Test Cases:** 1. **Basic normalization:** - Create a new routing form field - Enter label: "What is your name?" - Verify identifier auto-fills to: "what-is-your-name" 2. **5-word limit:** - Enter label: "What do you work on and how can we help?" - Verify identifier becomes: "what-do-you-work-on" 3. **Special characters:** - Enter label: "Email (required)!" - Verify identifier becomes: "email-required" 4. **Manual edit preservation:** - Enter label: "Test Field" - Manually change identifier to: "custom-id" - Change label to: "New Test Field" - Verify identifier stays: "custom-id" (not auto-updated) 5. **Updated label text:** - Verify the Identifier field label shows: "Identifier (URL Parameter, i.e.: &what-is-your-name=john-doe)" **Environment:** - No special environment variables needed - Test in the routing forms builder at `/apps/routing-forms/[formId]` ## Important Review Points ⚠️ **No automated tests**: The normalization function and auto-fill behavior lack test coverage. Consider adding unit tests for edge cases. 🔍 **Auto-fill logic**: The identifier only auto-updates if it's empty or matches the previous normalized label. This preserves manual edits but may be surprising to users in some scenarios. 🌍 **Unicode handling**: Non-ASCII characters are replaced with spaces. This may not be ideal for international users with non-English labels. ## Checklist - [x] I have read the [contributing guide](https://github.com/calcom/cal.com/blob/main/CONTRIBUTING.md) - [x] My code follows the style guidelines of this project - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have checked if my changes generate no new warnings --- .../routing-forms/[...pages]/FormEdit.tsx | 16 ++++++++++-- apps/web/public/static/locales/en/common.json | 1 + .../playwright/tests/basic.e2e.ts | 26 +++++++++---------- packages/lib/getValidRhfFieldName.test.ts | 25 ++++++++++++++---- packages/lib/getValidRhfFieldName.ts | 2 +- 5 files changed, 49 insertions(+), 21 deletions(-) diff --git a/apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/FormEdit.tsx b/apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/FormEdit.tsx index 2939aedd26..c193242b71 100644 --- a/apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/FormEdit.tsx +++ b/apps/web/app/(use-page-wrapper)/apps/routing-forms/[...pages]/FormEdit.tsx @@ -8,6 +8,7 @@ import { v4 as uuidv4 } from "uuid"; import { FieldTypes } from "@calcom/app-store/routing-forms/lib/FieldTypes"; import type { RoutingFormWithResponseCount } from "@calcom/app-store/routing-forms/types/types"; +import { getFieldIdentifier } from "@calcom/features/form-builder/utils/getFieldIdentifier"; import { useLocale } from "@calcom/lib/hooks/useLocale"; import classNames from "@calcom/ui/classNames"; import { Button } from "@calcom/ui/components/button"; @@ -102,14 +103,25 @@ function Field({ required {...hookForm.register(`${hookFieldNamespace}.label`)} onChange={(e) => { - hookForm.setValue(`${hookFieldNamespace}.label`, e.target.value, { shouldDirty: true }); + const newLabel = e.target.value; + // Use label from useWatch which is guaranteed to be the previous value + // since useWatch updates reactively (after re-render), not synchronously + const previousLabel = label || ""; + hookForm.setValue(`${hookFieldNamespace}.label`, newLabel, { shouldDirty: true }); + const currentIdentifier = hookForm.getValues(`${hookFieldNamespace}.identifier`); + // Only auto-update identifier if it was auto-generated from the previous label + // This preserves manual identifier changes + const isIdentifierGeneratedFromPreviousLabel = currentIdentifier === getFieldIdentifier(previousLabel); + if (!currentIdentifier || isIdentifierGeneratedFromPreviousLabel) { + hookForm.setValue(`${hookFieldNamespace}.identifier`, getFieldIdentifier(newLabel), { shouldDirty: true }); + } }} />
{ label: "Test Field", }); const queryString = - "firstField=456&Test Field Number=456&Test Field Single Selection=456&Test Field Multiple Selection=456&Test Field Multiple Selection=789&Test Field Phone=456&Test Field Email=456@example.com"; + "firstField=456&Test-Field-Number=456&Test-Field-Single-Selection=456&Test-Field-Multiple-Selection=456&Test-Field-Multiple-Selection=789&Test-Field-Phone=456&Test-Field-Email=456@example.com"; await gotoRoutingLink({ page, queryString }); - await page.fill('[data-testid="form-field-Test Field Long Text"]', "manual-fill"); + await page.fill('[data-testid="form-field-Test-Field-Long-Text"]', "manual-fill"); await expect(page.locator('[data-testid="form-field-firstField"]')).toHaveValue("456"); - await expect(page.locator('[data-testid="form-field-Test Field Number"]')).toHaveValue("456"); + await expect(page.locator('[data-testid="form-field-Test-Field-Number"]')).toHaveValue("456"); // TODO: Verify select and multiselect has prefilled values. - // expect(await page.locator(`[data-testid="form-field-Test Field Select"]`).inputValue()).toBe("456"); - // expect(await page.locator(`[data-testid="form-field-Test Field MultiSelect"]`).inputValue()).toBe("456"); + // expect(await page.locator(`[data-testid="form-field-Test-Field-Select"]`).inputValue()).toBe("456"); + // expect(await page.locator(`[data-testid="form-field-Test-Field-MultiSelect"]`).inputValue()).toBe("456"); - await expect(page.locator('[data-testid="form-field-Test Field Phone"]')).toHaveValue("456"); - await expect(page.locator('[data-testid="form-field-Test Field Email"]')).toHaveValue( + await expect(page.locator('[data-testid="form-field-Test-Field-Phone"]')).toHaveValue("456"); + await expect(page.locator('[data-testid="form-field-Test-Field-Email"]')).toHaveValue( "456@example.com" ); @@ -300,12 +300,12 @@ test.describe("Routing Forms", () => { expect(url.searchParams.get("firstField")).toBe("456"); // All other params come from prefill URL - expect(url.searchParams.get("Test Field Number")).toBe("456"); - expect(url.searchParams.get("Test Field Long Text")).toBe("manual-fill"); - expect(url.searchParams.get("Test Field Multiple Selection")).toBe("456"); - expect(url.searchParams.getAll("Test Field Multiple Selection")).toMatchObject(["456", "789"]); - expect(url.searchParams.get("Test Field Phone")).toBe("456"); - expect(url.searchParams.get("Test Field Email")).toBe("456@example.com"); + expect(url.searchParams.get("Test-Field-Number")).toBe("456"); + expect(url.searchParams.get("Test-Field-Long-Text")).toBe("manual-fill"); + expect(url.searchParams.get("Test-Field-Multiple-Selection")).toBe("456"); + expect(url.searchParams.getAll("Test-Field-Multiple-Selection")).toMatchObject(["456", "789"]); + expect(url.searchParams.get("Test-Field-Phone")).toBe("456"); + expect(url.searchParams.get("Test-Field-Email")).toBe("456@example.com"); }); // TODO: How to install the app just once? diff --git a/packages/lib/getValidRhfFieldName.test.ts b/packages/lib/getValidRhfFieldName.test.ts index 77319cea24..01c6b113fc 100644 --- a/packages/lib/getValidRhfFieldName.test.ts +++ b/packages/lib/getValidRhfFieldName.test.ts @@ -16,11 +16,11 @@ describe("getValidRhfFieldName", () => { }); // So that user can freely add spaces and any other character iteratively and it gets converted to - and he can add more characters. - // We don't really care about a hyphen in the end - it("should not remove dashes from start and end.", () => { - expect(getValidRhfFieldName("hello-there-")).toEqual("hello-there-"); + // Trailing dashes are removed to ensure valid field names + it("should remove trailing dashes but keep leading dashes.", () => { + expect(getValidRhfFieldName("hello-there-")).toEqual("hello-there"); expect(getValidRhfFieldName("-hello-there")).toEqual("-hello-there"); - expect(getValidRhfFieldName("$hello-there-")).toEqual("-hello-there-"); + expect(getValidRhfFieldName("$hello-there-")).toEqual("-hello-there"); }); it("should not remove underscore from start and end.", () => { @@ -31,7 +31,8 @@ describe("getValidRhfFieldName", () => { it("should remove unicode and emoji characters", () => { expect(getValidRhfFieldName("Hello 📚🕯️®️ There")).toEqual("Hello---------There"); - expect(getValidRhfFieldName("📚🕯️®️")).toEqual("-------"); + // When all characters are emojis/special chars, they become dashes which are then removed + expect(getValidRhfFieldName("📚🕯️®️")).toEqual(""); }); it("should keep numbers as is", () => { @@ -39,4 +40,18 @@ describe("getValidRhfFieldName", () => { expect(getValidRhfFieldName("321hello there123")).toEqual("321hello-there123"); expect(getValidRhfFieldName("hello$there")).toEqual("hello-there"); }); + + it("should not modify system field names", () => { + // System fields should remain unchanged as they don't contain invalid characters or trailing dashes + expect(getValidRhfFieldName("name")).toEqual("name"); + expect(getValidRhfFieldName("email")).toEqual("email"); + expect(getValidRhfFieldName("location")).toEqual("location"); + expect(getValidRhfFieldName("title")).toEqual("title"); + expect(getValidRhfFieldName("notes")).toEqual("notes"); + expect(getValidRhfFieldName("guests")).toEqual("guests"); + expect(getValidRhfFieldName("rescheduleReason")).toEqual("rescheduleReason"); + expect(getValidRhfFieldName("smsReminderNumber")).toEqual("smsReminderNumber"); + expect(getValidRhfFieldName("attendeePhoneNumber")).toEqual("attendeePhoneNumber"); + expect(getValidRhfFieldName("aiAgentCallPhoneNumber")).toEqual("aiAgentCallPhoneNumber"); + }); }); diff --git a/packages/lib/getValidRhfFieldName.ts b/packages/lib/getValidRhfFieldName.ts index bb2a38fe2c..0749441a64 100644 --- a/packages/lib/getValidRhfFieldName.ts +++ b/packages/lib/getValidRhfFieldName.ts @@ -1,5 +1,5 @@ export const getValidRhfFieldName = (fieldName: string) => { // Remember that any transformation that you do here would run on System Field names as well. So, be careful and avoiding doing anything here that would modify the SystemField names. // e.g. SystemField name currently have uppercases in them. So, no need to lowercase unless absolutely needed. - return fieldName.replace(/[^a-zA-Z0-9-_]/g, "-"); + return fieldName.replace(/[^a-zA-Z0-9-_]/g, "-").replace(/-+$/, ""); };