fix: email and phone fields validation#24432
Conversation
WalkthroughAdds four new English i18n keys for booking field validation messages related to Email and Attendee Phone Number visibility/requirement. Simplifies a boolean expression in the bookings response schema (no semantic change). Adds and exports Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/bookings/lib/getBookingResponsesSchema.ts (1)
176-210: Email: validate optional values when provided (align with phone).Currently, optional email is not validated unless checkOptional=true. This can accept invalid emails when a value is entered. Mirror the phone logic: validate if required OR non-empty value.
- if (bookingField.type === "email") { - if (!bookingField.hidden && (checkOptional || bookingField.required)) { + if (bookingField.type === "email") { + const needsValidation = + isRequired || (typeof value === "string" && value.trim() !== ""); + if (!bookingField.hidden && needsValidation) { // Email RegExp to validate if the input is a valid email if (!emailSchema.safeParse(value).success) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("email_validation_error"), }); } @@ const requiredEmailsMatch = requiredEmails.find((email) => bookerEmail.includes(email)); if (requiredEmails.length > 0 && !requiredEmailsMatch) { ctx.addIssue({ code: z.ZodIssueCode.custom, message: m("require_emails_no_match_found_error_message"), }); } }
🧹 Nitpick comments (1)
apps/web/public/static/locales/en/common.json (1)
3789-3792: New keys added correctly.Copy reads fine. Optional: consider “At least one of Email or Attendee Phone Number must be required” for 3791.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/bookings/lib/getBookingResponsesSchema.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/util.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/trpc/server/routers/viewer/eventTypes/util.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/trpc/server/routers/viewer/eventTypes/util.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/lib/getBookingResponsesSchema.tspackages/trpc/server/routers/viewer/eventTypes/util.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/eventTypes/util.ts (1)
274-296: Validation branches look correct; good use of static i18n keys.Covers both-hidden, at-least-one-required, and hidden→other-required cases. Please add unit/e2e tests to lock this logic.
github-actions
bot
added
booking-page
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/features/bookings/lib/getBookingResponsesSchema.test.ts (2)
179-208: Good regression coverage for hidden required emailCovers the bug: hidden required email shouldn't be validated when phone is provided.
Optionally assert the parsed data to lock intended output shape:
const parsedResponses = await schema.safeParseAsync({ name: "John", email: "", attendeePhoneNumber: "+919999999999", }); expect(parsedResponses.success).toBe(true); +if (parsedResponses.success) { + expect(parsedResponses.data).toEqual( + expect.objectContaining({ + name: "John", + attendeePhoneNumber: "+919999999999", + }) + ); +}
209-238: Mirrors phone-hidden case wellHidden required phone is not validated when email is present. Looks correct.
Optionally assert the returned data similarly:
const parsedResponses = await schema.safeParseAsync({ name: "John", email: "john@example.com", attendeePhoneNumber: "", }); expect(parsedResponses.success).toBe(true); +if (parsedResponses.success) { + expect(parsedResponses.data).toEqual( + expect.objectContaining({ + name: "John", + email: "john@example.com", + }) + ); +}packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts (1)
517-674: Comprehensive validation matrix for email/phone presenceThe scenarios cover both-hidden, neither-required, and mixed hidden/required states with correct BAD_REQUEST messages. Solid coverage.
Avoid double invocation in toThrow assertions to reduce noise:
- expect(() => ensureEmailOrPhoneNumberIsPresent(fields)).toThrow(TRPCError); - expect(() => ensureEmailOrPhoneNumberIsPresent(fields)).toThrow( + const act = () => ensureEmailOrPhoneNumberIsPresent(fields); + expect(act).toThrow(TRPCError); + expect(act).toThrow( expect.objectContaining({ code: "BAD_REQUEST", message: "booking_fields_email_and_phone_both_hidden", }) );You can apply the same pattern to other tests in this block.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/bookings/lib/getBookingResponsesSchema.test.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.tspackages/features/bookings/lib/getBookingResponsesSchema.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.tspackages/features/bookings/lib/getBookingResponsesSchema.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.tspackages/features/bookings/lib/getBookingResponsesSchema.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/getBookingResponsesSchema.test.ts
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts (1)
10-10: Named exports import LGTMImporting ensureEmailOrPhoneNumberIsPresent as a named export aligns with our no-default-exports guideline.
As per coding guidelines
apps/web/public/static/locales/en/common.json (1)
3791-3794: New i18n keys are clear and consistentKeys match test expectations and copy style. Approved.
Optionally verify these keys exist (or add placeholders) in other locales to avoid missing translations at runtime:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/bookings/lib/getBookingResponsesSchema.test.ts (1)
179-208: Covers hidden required email correctly; add a prefilled-hidden case (matches the bug).Good assertion that a hidden required email doesn’t block submission when empty and phone is present. Please also add a test where email is hidden but prefilled (non-empty), to directly cover the reported regression.
Example to add near this block:
test(`hidden required email with prefilled value should not be validated`, async () => { const schema = getBookingResponsesSchema({ bookingFields: [ { name: "name", type: "name", required: true }, { name: "email", type: "email", required: true, hidden: true }, { name: "attendeePhoneNumber", type: "phone", required: true }, ] as z.infer<typeof eventTypeBookingFields> & z.BRAND<"HAS_SYSTEM_FIELDS">, view: "ALL_VIEWS", }); const parsedResponses = await schema.safeParseAsync({ name: "John", email: "john@example.com", // prefilled while hidden attendeePhoneNumber: "+919999999999", }); expect(parsedResponses.success).toBe(true); });packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts (1)
517-674: Great coverage; add edge cases for empty/missing fields to lock behavior.Consider adding:
- fields = [] or undefined → should be a no-op (current impl returns early).
- Only email present (no phone field) with email.required=false → should throw email_or_phone_required.
- Only phone present (no email field) with phone.required=false → should throw email_or_phone_required.
These make the contract explicit and guard future regressions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/public/static/locales/en/common.json(1 hunks)packages/features/bookings/lib/getBookingResponsesSchema.test.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/lib/getBookingResponsesSchema.test.tspackages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/lib/getBookingResponsesSchema.test.tspackages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/lib/getBookingResponsesSchema.test.tspackages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/getBookingResponsesSchema.test.ts
🧬 Code graph analysis (2)
packages/features/bookings/lib/getBookingResponsesSchema.test.ts (1)
packages/features/bookings/lib/getBookingResponsesSchema.ts (1)
getBookingResponsesSchema(34-37)
packages/trpc/server/routers/viewer/eventTypes/__tests__/util.test.ts (1)
packages/trpc/server/routers/viewer/eventTypes/util.ts (1)
ensureEmailOrPhoneNumberIsPresent(265-298)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build Docs
- GitHub Check: Tests / Unit
- GitHub Check: Production builds / Build API v1
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
🔇 Additional comments (2)
packages/features/bookings/lib/getBookingResponsesSchema.test.ts (1)
209-238: LGTM: hidden required phone is handled.Behavior matches intent; parsing succeeds when phone is hidden and email is present/required.
apps/web/public/static/locales/en/common.json (1)
3791-3794: i18n keys align with server messages.Looks good. Please ensure:
- Keys are added (or intentionally fall back) across other locales to avoid missing translations.
- All server/UI references use these static keys.
3 participants
What does this PR do?
Refactors a boolean condition in bookings response schema to an equivalent OR check. Updates ensureEmailOrPhoneNumberIsPresent to add explicit validation branches: error if both fields are hidden; require phone when email is hidden; require email when phone is hidden; and enforce at least one of email or phone as required. Replaces inline messages with the new static translation keys
Fixes #24369