From 51caa6834a1e3de7495b4c07708e5ec198239b3b Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Mon, 21 Aug 2023 22:41:47 +0530 Subject: [PATCH] fix: Make identifier conform to RHF field requirement (#10860) Co-authored-by: Udit Takkar --- .../manage-booking-questions.e2e.ts | 24 ++++++------ .../features/bookings/lib/getBookingFields.ts | 18 +++++++-- .../features/form-builder/FormBuilder.tsx | 4 ++ packages/features/form-builder/schema.ts | 4 +- .../form-builder/utils/getFieldIdentifier.ts | 5 +++ packages/lib/getValidRhfFieldName.test.ts | 38 +++++++++++++++++++ packages/lib/getValidRhfFieldName.ts | 5 +++ packages/lib/slugify.test.ts | 36 ++++++++++++++++++ 8 files changed, 117 insertions(+), 17 deletions(-) create mode 100644 packages/features/form-builder/utils/getFieldIdentifier.ts create mode 100644 packages/lib/getValidRhfFieldName.test.ts create mode 100644 packages/lib/getValidRhfFieldName.ts create mode 100644 packages/lib/slugify.test.ts diff --git a/apps/web/playwright/manage-booking-questions.e2e.ts b/apps/web/playwright/manage-booking-questions.e2e.ts index 0d06a2e9d2..a680b44414 100644 --- a/apps/web/playwright/manage-booking-questions.e2e.ts +++ b/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!", }, diff --git a/packages/features/bookings/lib/getBookingFields.ts b/packages/features/bookings/lib/getBookingFields.ts index faf5aeb6bf..cab4a34ecd 100644 --- a/packages/features/bookings/lib/getBookingFields.ts +++ b/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); diff --git a/packages/features/form-builder/FormBuilder.tsx b/packages/features/form-builder/FormBuilder.tsx index bcd49b9bad..2c0b5ecfd0 100644 --- a/packages/features/form-builder/FormBuilder.tsx +++ b/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; @@ -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" diff --git a/packages/features/form-builder/schema.ts b/packages/features/form-builder/schema.ts index 7eddf7bf8c..f873cd818a 100644 --- a/packages/features/form-builder/schema.ts +++ b/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(), diff --git a/packages/features/form-builder/utils/getFieldIdentifier.ts b/packages/features/form-builder/utils/getFieldIdentifier.ts new file mode 100644 index 0000000000..eebc9ca4e4 --- /dev/null +++ b/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); +}; diff --git a/packages/lib/getValidRhfFieldName.test.ts b/packages/lib/getValidRhfFieldName.test.ts new file mode 100644 index 0000000000..7d3d673809 --- /dev/null +++ b/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"); + }); +}); diff --git a/packages/lib/getValidRhfFieldName.ts b/packages/lib/getValidRhfFieldName.ts new file mode 100644 index 0000000000..a052381390 --- /dev/null +++ b/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, "-"); +}; diff --git a/packages/lib/slugify.test.ts b/packages/lib/slugify.test.ts new file mode 100644 index 0000000000..7a7b2a3501 --- /dev/null +++ b/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(""); + }); +});