fix: Make identifier conform to RHF field requirement (#10860)

Co-authored-by: Udit Takkar <udit.07814802719@cse.mait.ac.in>

apps/web/playwright/manage-booking-questions.e2e.ts

@@ -202,7 +202,7 @@ async function runTestStepsCommonForTeamAndUserEventType(
     await addQuestionAndSave({
       page,
       question: {
-        name: "how_are_you",
+        name: "how-are-you",
         type: "Address",
         label: "How are you?",
         placeholder: "I'm fine, thanks",
@@ -214,7 +214,7 @@ async function runTestStepsCommonForTeamAndUserEventType(
       const allFieldsLocator = await expectSystemFieldsToBeThereOnBookingPage({ page });
       const userFieldLocator = allFieldsLocator.nth(5);
 
-      await expect(userFieldLocator.locator('[name="how_are_you"]')).toBeVisible();
+      await expect(userFieldLocator.locator('[name="how-are-you"]')).toBeVisible();
       // There are 2 labels right now. Will be one in future. The second one is hidden
       expect(await getLabelText(userFieldLocator)).toBe("How are you?");
       await expect(userFieldLocator.locator("input")).toBeVisible();
@@ -223,18 +223,18 @@ async function runTestStepsCommonForTeamAndUserEventType(
 
   await test.step("Hide Question and see that it's not shown on Booking Page", async () => {
     await toggleQuestionAndSave({
-      name: "how_are_you",
+      name: "how-are-you",
       page,
     });
     await doOnFreshPreview(page, context, async (page) => {
-      const formBuilderFieldLocator = page.locator('[data-fob-field-name="how_are_you"]');
+      const formBuilderFieldLocator = page.locator('[data-fob-field-name="how-are-you"]');
       await expect(formBuilderFieldLocator).toBeHidden();
     });
   });
 
   await test.step("Show Question Again", async () => {
     await toggleQuestionAndSave({
-      name: "how_are_you",
+      name: "how-are-you",
       page,
     });
   });
@@ -242,7 +242,7 @@ async function runTestStepsCommonForTeamAndUserEventType(
   await test.step('Try to book without providing "How are you?" response', async () => {
     await doOnFreshPreview(page, context, async (page) => {
       await bookTimeSlot({ page, name: "Booker", email: "booker@example.com" });
-      await expectErrorToBeThereFor({ page, name: "how_are_you" });
+      await expectErrorToBeThereFor({ page, name: "how-are-you" });
     });
   });
 
@@ -260,18 +260,18 @@ async function runTestStepsCommonForTeamAndUserEventType(
         page,
         context,
         async (page) => {
-          const formBuilderFieldLocator = page.locator('[data-fob-field-name="how_are_you"]');
+          const formBuilderFieldLocator = page.locator('[data-fob-field-name="how-are-you"]');
           await expect(formBuilderFieldLocator).toBeVisible();
           expect(
-            await formBuilderFieldLocator.locator('[name="how_are_you"]').getAttribute("placeholder")
+            await formBuilderFieldLocator.locator('[name="how-are-you"]').getAttribute("placeholder")
           ).toBe("I'm fine, thanks");
           expect(await getLabelText(formBuilderFieldLocator)).toBe("How are you?");
-          await formBuilderFieldLocator.locator('[name="how_are_you"]').fill("I am great!");
+          await formBuilderFieldLocator.locator('[name="how-are-you"]').fill("I am great!");
           await bookTimeSlot({ page, name: "Booker", email: "booker@example.com" });
           await expect(page.locator("[data-testid=success-page]")).toBeVisible();
 
           expect(
-            await page.locator('[data-testid="field-response"][data-fob-field="how_are_you"]').innerText()
+            await page.locator('[data-testid="field-response"][data-fob-field="how-are-you"]').innerText()
           ).toBe("I am great!");
 
           await waitFor(() => {
@@ -287,7 +287,7 @@ async function runTestStepsCommonForTeamAndUserEventType(
               label: "email_address",
               value: "booker@example.com",
             },
-            how_are_you: {
+            "how-are-you": {
               label: "How are you?",
               value: "I am great!",
             },
@@ -303,7 +303,7 @@ async function runTestStepsCommonForTeamAndUserEventType(
           });
 
           expect(payload.userFieldsResponses).toMatchObject({
-            how_are_you: {
+            "how-are-you": {
               label: "How are you?",
               value: "I am great!",
             },

packages/features/bookings/lib/getBookingFields.ts

@@ -3,6 +3,7 @@ import type { z } from "zod";
 
 import { SMS_REMINDER_NUMBER_FIELD } from "@calcom/features/bookings/lib/SystemField";
 import { fieldsThatSupportLabelAsSafeHtml } from "@calcom/features/form-builder/fieldsThatSupportLabelAsSafeHtml";
+import { getFieldIdentifier } from "@calcom/features/form-builder/utils/getFieldIdentifier";
 import { markdownToSafeHTML } from "@calcom/lib/markdownToSafeHTML";
 import slugify from "@calcom/lib/slugify";
 import { EventTypeCustomInputType } from "@calcom/prisma/enums";
@@ -285,7 +286,9 @@ export const ensureBookingInputsHaveSystemFields = ({
 
   const missingSystemBeforeFields = [];
   for (const field of systemBeforeFields) {
-    const existingBookingFieldIndex = bookingFields.findIndex((f) => f.name === field.name);
+    const existingBookingFieldIndex = bookingFields.findIndex(
+      (f) => getFieldIdentifier(f.name) === getFieldIdentifier(field.name)
+    );
     // Only do a push, we must not update existing system fields as user could have modified any property in it,
     if (existingBookingFieldIndex === -1) {
       missingSystemBeforeFields.push(field);
@@ -303,8 +306,13 @@ export const ensureBookingInputsHaveSystemFields = ({
   // Backward Compatibility for SMS Reminder Number
   // Note: We still need workflows in `getBookingFields` due to Backward Compatibility. If we do a one time entry for all event-types, we can remove workflows from `getBookingFields`
   // Also, note that even if Workflows don't explicity add smsReminderNumber field to bookingFields, it would be added as a side effect of this backward compatibility logic
-  if (smsNumberSources.length && !bookingFields.find((f) => f.name !== SMS_REMINDER_NUMBER_FIELD)) {
-    const indexForLocation = bookingFields.findIndex((f) => f.name === "location");
+  if (
+    smsNumberSources.length &&
+    !bookingFields.find((f) => getFieldIdentifier(f.name) !== getFieldIdentifier(SMS_REMINDER_NUMBER_FIELD))
+  ) {
+    const indexForLocation = bookingFields.findIndex(
+      (f) => getFieldIdentifier(f.name) === getFieldIdentifier("location")
+    );
     // Add the SMS Reminder Number field after `location` field always
     bookingFields.splice(indexForLocation + 1, 0, {
       ...getSmsReminderNumberField(),
@@ -340,7 +348,9 @@ export const ensureBookingInputsHaveSystemFields = ({
 
   const missingSystemAfterFields = [];
   for (const field of systemAfterFields) {
-    const existingBookingFieldIndex = bookingFields.findIndex((f) => f.name === field.name);
+    const existingBookingFieldIndex = bookingFields.findIndex(
+      (f) => getFieldIdentifier(f.name) === getFieldIdentifier(field.name)
+    );
     // Only do a push, we must not update existing system fields as user could have modified any property in it,
     if (existingBookingFieldIndex === -1) {
       missingSystemAfterFields.push(field);

packages/features/form-builder/FormBuilder.tsx

@@ -30,6 +30,7 @@ import { fieldTypesConfigMap } from "./fieldTypes";
 import { fieldsThatSupportLabelAsSafeHtml } from "./fieldsThatSupportLabelAsSafeHtml";
 import type { fieldsSchema } from "./schema";
 import { getVariantsConfig } from "./utils";
+import { getFieldIdentifier } from "./utils/getFieldIdentifier";
 
 type RhfForm = {
   fields: z.infer<typeof fieldsSchema>;
@@ -448,6 +449,9 @@ function FieldEditDialog({
                       required
                       {...fieldForm.register("name")}
                       containerClassName="mt-6"
+                      onChange={(e) => {
+                        fieldForm.setValue("name", getFieldIdentifier(e.target.value || ""));
+                      }}
                       disabled={
                         fieldForm.getValues("editable") === "system" ||
                         fieldForm.getValues("editable") === "system-but-optional"

packages/features/form-builder/schema.ts

@@ -1,5 +1,7 @@
 import { z } from "zod";
 
+import { getValidRhfFieldName } from "@calcom/lib/getValidRhfFieldName";
+
 import { fieldTypesConfigMap } from "./fieldTypes";
 import { getVariantsConfig, preprocessNameFieldDataWithVariant } from "./utils";
 
@@ -31,7 +33,7 @@ export const EditableSchema = z.enum([
 ]);
 
 const baseFieldSchema = z.object({
-  name: z.string(),
+  name: z.string().transform(getValidRhfFieldName),
   type: fieldTypeEnum,
   // TODO: We should make at least one of `defaultPlaceholder` and `placeholder` required. Do the same for label.
   label: z.string().optional(),

packages/features/form-builder/utils/getFieldIdentifier.ts

@@ -0,0 +1,5 @@
+import { getValidRhfFieldName } from "@calcom/lib/getValidRhfFieldName";
+
+export const getFieldIdentifier = (name: string) => {
+  return getValidRhfFieldName(name);
+};

packages/lib/getValidRhfFieldName.test.ts

@@ -0,0 +1,38 @@
+import { describe, expect, it } from "vitest";
+
+import { getValidRhfFieldName } from "./getValidRhfFieldName";
+
+describe("getValidRhfFieldName", () => {
+  it("should not convert to lowercase", () => {
+    expect(getValidRhfFieldName("Hello")).toEqual("Hello");
+    expect(getValidRhfFieldName("HELLO")).toEqual("HELLO");
+  });
+
+  it("should convert spaces, _, and any other special character to -", () => {
+    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");
+    expect(getValidRhfFieldName("$hello.there")).toEqual("-hello-there");
+  });
+
+  // So that user can freely add spaces and any other character iteratively and it get's 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-");
+    expect(getValidRhfFieldName("hello-there_")).toEqual("hello-there-");
+    expect(getValidRhfFieldName("_hello-there_")).toEqual("-hello-there-");
+    expect(getValidRhfFieldName("$hello-there_")).toEqual("-hello-there-");
+  });
+
+  it("should remove unicode and emoji characters", () => {
+    expect(getValidRhfFieldName("Hello 📚🕯️®️ There")).toEqual("Hello---------There");
+    expect(getValidRhfFieldName("📚🕯️®️")).toEqual("-------");
+  });
+
+  it("should keep numbers as is", () => {
+    expect(getValidRhfFieldName("hellothere123")).toEqual("hellothere123");
+    expect(getValidRhfFieldName("321hello there123")).toEqual("321hello-there123");
+    expect(getValidRhfFieldName("hello$there")).toEqual("hello-there");
+  });
+});

packages/lib/getValidRhfFieldName.ts

@@ -0,0 +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, "-");
+};

packages/lib/slugify.test.ts

@@ -0,0 +1,36 @@
+import { describe, expect, it } from "vitest";
+
+import { slugify } from "./slugify";
+
+describe("slugify", () => {
+  it("should convert to lowercase", () => {
+    expect(slugify("Hello")).toEqual("hello");
+    expect(slugify("HELLO")).toEqual("hello");
+  });
+
+  it("should convert spaces, _, and any other special character to -", () => {
+    expect(slugify("hello there")).toEqual("hello-there");
+    expect(slugify("hello_there")).toEqual("hello-there");
+    expect(slugify("hello$there")).toEqual("hello-there");
+  });
+
+  it("should keep numbers as is", () => {
+    expect(slugify("hellothere123")).toEqual("hellothere123");
+    expect(slugify("321hello there123")).toEqual("321hello-there123");
+    expect(slugify("hello$there")).toEqual("hello-there");
+  });
+
+  // So that user can freely add spaces and any other character iteratively and it get's converted to - later on.
+  it("should remove dashes from start and end.", () => {
+    expect(slugify("hello-there-")).toEqual("hello-there");
+    expect(slugify("hello-there_")).toEqual("hello-there");
+    expect(slugify("_hello-there_")).toEqual("hello-there");
+    expect(slugify("$hello-there_")).toEqual("hello-there");
+  });
+
+  // This is failing, if we want to fix it, one approach is as used in getValidRhfFieldName
+  it.skip("should remove unicode and emoji characters", () => {
+    expect(slugify("Hello 📚🕯️®️ There")).toEqual("hello---------there");
+    expect(slugify("📚🕯️®️")).toEqual("");
+  });
+});