feat: disable recording emails for guests #22457
Conversation
|
nehaaaa8 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
@naaa760 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
There was a problem hiding this comment.
cubic reviewed 14 files and found no issues. Review PR in cubic.dev.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/13/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/13/25)1 label was added to this PR based on Keith Williams's automation. |
|
Hey @naaa760, thanks for the pr. Can you pls attach a loom video? There also seems to be a type check failing, can you fix that as well? |
let me update you with loom video in some time. |
quick-fix.sql
Outdated
| SELECT t.id as team_id, t.name as team_name, t."isPlatform" | ||
| FROM "Team" t | ||
| JOIN "Membership" m ON t.id = m."teamId" | ||
| WHERE m.role IN ('ADMIN', 'OWNER'); | ||
|
|
turbo.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "$schema": "https://turborepo.org/schema.json", | |||
| "pipeline": { | |||
| "tasks": { | |||
There was a problem hiding this comment.
Uncommit this change. This is not a part of the issue
|
Hi @anikdhabal Type Error: FIXED 📹 Loom Video Update: Could you please:
The implementation is complete and type-safe. I can provide:
|
|
This PR is being marked as stale due to inactivity. |
WalkthroughA new feature was implemented to allow disabling the sending of recording download emails to guests in the Cal Video integration. This includes updates across the backend, frontend, and database schema: a new boolean field ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All modifications are directly related to the objective of allowing users to disable recording download emails for guests in Cal Video. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/app/api/recorded-daily-video/route.ts(1 hunks)packages/emails/daily-video-emails.ts(1 hunks)packages/emails/email-manager.ts(1 hunks)packages/features/eventtypes/components/Locations.tsx(1 hunks)packages/features/eventtypes/lib/types.ts(1 hunks)packages/lib/server/locales/en/common.json(1 hunks)packages/lib/server/repository/calVideoSettings.ts(3 hunks)packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts(1 hunks)packages/prisma/migrations/20250128000000_add_disable_recording_emails_for_guests/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/prisma/zod/custom/eventtype.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/create.handler.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/types.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/eventtypes/lib/types.tspackages/trpc/server/routers/viewer/eventTypes/create.handler.tspackages/trpc/server/routers/viewer/eventTypes/types.tsapps/web/app/api/recorded-daily-video/route.tspackages/prisma/zod/custom/eventtype.tspackages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.tspackages/emails/email-manager.tspackages/lib/server/repository/calVideoSettings.tspackages/emails/daily-video-emails.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/eventtypes/lib/types.tspackages/trpc/server/routers/viewer/eventTypes/create.handler.tspackages/trpc/server/routers/viewer/eventTypes/types.tsapps/web/app/api/recorded-daily-video/route.tspackages/prisma/zod/custom/eventtype.tspackages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.tspackages/emails/email-manager.tspackages/lib/server/repository/calVideoSettings.tspackages/features/eventtypes/components/Locations.tsxpackages/emails/daily-video-emails.ts
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/eventtypes/components/Locations.tsx
🧠 Learnings (4)
packages/prisma/migrations/20250128000000_add_disable_recording_emails_for_guests/migration.sql (1)
Learnt from: vijayraghav-io
PR: #16579
File: packages/prisma/schema.prisma:149-153
Timestamp: 2025-07-16T07:14:49.248Z
Learning: In Cal.com's schema design, the team prefers to keep Boolean fields nullable (Boolean?) with defaults rather than making them non-nullable (Boolean) to avoid expensive database migrations that would update existing rows.
packages/prisma/schema.prisma (1)
Learnt from: vijayraghav-io
PR: #16579
File: packages/prisma/schema.prisma:149-153
Timestamp: 2025-07-16T07:14:49.248Z
Learning: In Cal.com's schema design, the team prefers to keep Boolean fields nullable (Boolean?) with defaults rather than making them non-nullable (Boolean) to avoid expensive database migrations that would update existing rows.
packages/lib/server/locales/en/common.json (1)
Learnt from: bandhan-majumder
PR: #22359
File: packages/lib/server/locales/en/common.json:1336-1339
Timestamp: 2025-07-14T16:31:45.233Z
Learning: When making localization changes for new features, it's often safer to add new strings rather than modify existing ones to avoid breaking existing functionality that depends on the original strings. This approach allows for feature-specific customization while maintaining backward compatibility.
packages/features/eventtypes/components/Locations.tsx (3)
Learnt from: alishaz-polymath
PR: #22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the currentLink.maxUsageCount ?? 1 fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: hariombalhara
PR: #22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.759Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
🪛 GitHub Actions: PR Update
apps/web/app/api/recorded-daily-video/route.ts
[error] 142-142: TypeScript error TS2322: Type 'number | null' is not assignable to type 'number | undefined'. Type 'null' is not assignable to type 'number | undefined' for property 'eventTypeId'.
packages/features/eventtypes/components/Locations.tsx
[error] 428-428: TypeScript error TS2551: Property 'disableRecordingEmailsForGuests' does not exist on type of 'calVideoSettings'. Did you mean 'disableRecordingForGuests'?
🔇 Additional comments (15)
packages/lib/server/locales/en/common.json (1)
29-29: New i18n key correctly addedGood call introducing a fresh key (
disable_recording_emails_for_guests) rather than overloading the existingdisable_recording_for_guests. This maintains backward compatibility and aligns with our localization guidelines.packages/features/eventtypes/lib/types.ts (1)
165-165: LGTM!The new optional boolean property is correctly typed and well-positioned within the
calVideoSettingsobject. This provides proper TypeScript support for the new feature.packages/trpc/server/routers/viewer/eventTypes/create.handler.ts (1)
75-75: LGTM!The implementation correctly handles the new
disableRecordingEmailsForGuestsfield with appropriate null coalescing to default tofalse. The pattern is consistent with other properties in the same object.packages/prisma/zod/custom/eventtype.ts (1)
13-13: LGTM!The Zod schema definition correctly validates the new field as an optional and nullable boolean, providing appropriate validation flexibility while maintaining consistency with the existing schema pattern.
packages/platform/types/event-types/event-types_2024_06_14/inputs/create-event-type.input.ts (1)
142-147: LGTM! Property addition follows established patterns.The new
disableRecordingEmailsForGuestsproperty is properly decorated with validation and documentation annotations, consistent with other boolean properties in theCalVideoSettingsclass.apps/web/app/api/recorded-daily-video/route.ts (2)
139-147: Good use of select instead of include.The database query correctly follows the coding guideline by using
selectto only fetch the requireddisableRecordingEmailsForGuestsfield instead of including all fields.
150-154: LGTM! Proper parameter passing.The function call correctly passes the fetched setting with a proper fallback to
false, maintaining backward compatibility.packages/trpc/server/routers/viewer/eventTypes/types.ts (1)
36-36: LGTM! Schema update follows established patterns.The new
disableRecordingEmailsForGuestsfield is properly added with consistent typing (optional and nullable boolean) matching other fields in the schema.packages/emails/email-manager.ts (2)
731-735: LGTM! Function signature updated with proper defaults.The new
disableRecordingEmailsForGuestsparameter is properly added with a default value offalse, ensuring backward compatibility for existing callers.
739-751: Excellent implementation of conditional email sending.The logic correctly implements the feature requirements:
- Organizers always receive recording emails (line 740-742)
- Guests only receive emails when
disableRecordingEmailsForGuestsisfalse(lines 745-751)This ensures the organizer is never excluded while providing the option to disable guest notifications.
packages/lib/server/repository/calVideoSettings.ts (3)
22-22: LGTM! Consistent with existing pattern.The new property follows the same optional boolean | null pattern as other Cal Video settings, which is appropriate for configuration flags.
34-34: LGTM! Proper null handling and placement.The nullish coalescing operator ensures a concrete boolean value is stored in the database, preventing potential null-related issues. Good placement with other recording-related settings.
44-44: LGTM! Maintains create/update consistency.The create operation mirrors the update operation exactly, ensuring consistent behavior regardless of whether the record exists. The false default preserves backward compatibility.
packages/emails/daily-video-emails.ts (2)
35-39: LGTM! Well-designed function signature.The optional parameter with a default value of
falsemaintains backward compatibility while clearly expressing the feature's intent. Good parameter naming and placement.
43-55: LGTM! Clear conditional logic with good separation of concerns.The implementation correctly ensures organizers always receive recording emails while respecting the guest setting. Clear comments enhance readability.
Note: The
sendDailyVideoTranscriptEmailsfunction doesn't have similar conditional logic - verify if this is intentional or if transcript emails should also respect guest preferences.
| const calVideoSettings = await prisma.calVideoSettings.findUnique({ | ||
| where: { | ||
| eventTypeId: booking.eventTypeId, | ||
| }, |
There was a problem hiding this comment.
Fix TypeScript error: Handle null eventTypeId.
The pipeline failure indicates that booking.eventTypeId can be null, but the Prisma query expects a number. This will cause a runtime error.
Apply this fix to handle the null case:
+ if (!booking.eventTypeId) {
+ // If no eventTypeId, default to false (send emails to guests)
+ await sendDailyVideoRecordingEmails(evt, downloadLink, false);
+ return NextResponse.json({ message: "Success" });
+ }
+
// Get Cal Video settings to check if recording emails for guests are disabled
const calVideoSettings = await prisma.calVideoSettings.findUnique({
where: {
eventTypeId: booking.eventTypeId,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const calVideoSettings = await prisma.calVideoSettings.findUnique({ | |
| where: { | |
| eventTypeId: booking.eventTypeId, | |
| }, | |
| if (!booking.eventTypeId) { | |
| // If no eventTypeId, default to false (send emails to guests) | |
| await sendDailyVideoRecordingEmails(evt, downloadLink, false); | |
| return NextResponse.json({ message: "Success" }); | |
| } | |
| // Get Cal Video settings to check if recording emails for guests are disabled | |
| const calVideoSettings = await prisma.calVideoSettings.findUnique({ | |
| where: { | |
| eventTypeId: booking.eventTypeId, | |
| }, |
🧰 Tools
🪛 GitHub Actions: PR Update
[error] 142-142: TypeScript error TS2322: Type 'number | null' is not assignable to type 'number | undefined'. Type 'null' is not assignable to type 'number | undefined' for property 'eventTypeId'.
🤖 Prompt for AI Agents
In apps/web/app/api/recorded-daily-video/route.ts around lines 140 to 143, the
Prisma query uses booking.eventTypeId which can be null, causing a TypeScript
error and potential runtime issue. Fix this by adding a check before the query
to ensure booking.eventTypeId is not null; if it is null, handle that case
appropriately (e.g., skip the query or return early). This prevents passing null
to the Prisma where clause expecting a number.
| <Controller | ||
| name="calVideoSettings.disableRecordingEmailsForGuests" | ||
| defaultValue={!!eventType.calVideoSettings?.disableRecordingEmailsForGuests} | ||
| render={({ field: { onChange, value } }) => { | ||
| return ( | ||
| <SettingsToggle | ||
| title={t("disable_recording_emails_for_guests")} | ||
| labelClassName="text-sm leading-6 whitespace-normal break-words" | ||
| checked={value} | ||
| onCheckedChange={onChange} | ||
| /> | ||
| ); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Good implementation pattern but fix TypeScript error.
The new toggle follows the established pattern of other CalVideo settings toggles and correctly uses t() for localization as required by the coding guidelines.
However, the pipeline failure indicates a TypeScript error where disableRecordingEmailsForGuests doesn't exist on the calVideoSettings type.
Ensure that the FormValues type in packages/features/eventtypes/lib/types.ts includes this new property in the calVideoSettings interface, or verify that all related type definitions have been updated consistently across the codebase.
🧰 Tools
🪛 GitHub Actions: PR Update
[error] 428-428: TypeScript error TS2551: Property 'disableRecordingEmailsForGuests' does not exist on type of 'calVideoSettings'. Did you mean 'disableRecordingForGuests'?
🤖 Prompt for AI Agents
In packages/features/eventtypes/components/Locations.tsx around lines 426 to
439, the TypeScript error occurs because the property
disableRecordingEmailsForGuests is missing from the calVideoSettings type. To
fix this, update the FormValues type definition in
packages/features/eventtypes/lib/types.ts by adding
disableRecordingEmailsForGuests as a boolean property to the calVideoSettings
interface. Also, verify that any other related type definitions referencing
calVideoSettings are updated accordingly to maintain consistency across the
codebase.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "CalVideoSettings" ADD COLUMN "disableRecordingEmailsForGuests" BOOLEAN NOT NULL DEFAULT false; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using nullable Boolean field following team preferences.
Based on Cal.com's established schema design patterns, the team prefers nullable Boolean fields (Boolean?) with defaults rather than non-nullable fields to avoid expensive database migrations that update existing rows.
Consider changing the migration to:
-ALTER TABLE "CalVideoSettings" ADD COLUMN "disableRecordingEmailsForGuests" BOOLEAN NOT NULL DEFAULT false;
+ALTER TABLE "CalVideoSettings" ADD COLUMN "disableRecordingEmailsForGuests" BOOLEAN DEFAULT false;This approach aligns with the team's preference and avoids the cost of updating all existing rows during migration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ALTER TABLE "CalVideoSettings" ADD COLUMN "disableRecordingEmailsForGuests" BOOLEAN NOT NULL DEFAULT false; | |
| ALTER TABLE "CalVideoSettings" ADD COLUMN "disableRecordingEmailsForGuests" BOOLEAN DEFAULT false; |
🤖 Prompt for AI Agents
In
packages/prisma/migrations/20250128000000_add_disable_recording_emails_for_guests/migration.sql
at line 1, the new column disableRecordingEmailsForGuests is added as a
non-nullable Boolean with a default false. To align with team preferences and
avoid costly updates on existing rows, change the column to be nullable Boolean
(allow NULL values) with a default false instead. Modify the ALTER TABLE
statement to add the column as nullable with the default, so existing rows are
not updated during migration.
| @@ -78,6 +78,7 @@ model CalVideoSettings { | |||
| redirectUrlOnExit String? | |||
| disableTranscriptionForGuests Boolean @default(false) | |||
| disableTranscriptionForOrganizer Boolean @default(false) | |||
| disableRecordingEmailsForGuests Boolean @default(false) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow team preference for nullable Boolean fields.
The new field uses a non-nullable Boolean, but Cal.com's established schema design pattern prefers nullable Boolean fields with defaults to avoid expensive database migrations.
Consider changing to:
- disableRecordingEmailsForGuests Boolean @default(false)
+ disableRecordingEmailsForGuests Boolean? @default(false)This approach aligns with the team's preference and avoids costly row updates during migration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| disableRecordingEmailsForGuests Boolean @default(false) | |
| disableRecordingEmailsForGuests Boolean? @default(false) |
🤖 Prompt for AI Agents
In packages/prisma/schema.prisma at line 81, the field
disableRecordingEmailsForGuests is defined as a non-nullable Boolean with a
default value. To follow the team's preference and avoid expensive database
migrations, change this field to a nullable Boolean with a default value
instead. Update the schema to make the field Boolean? with @default(false) so it
aligns with the established pattern and reduces migration costs.
|
@naaa760 Can you pls resolve the merge conflicts and attach a loom video? |
What does this PR do?
This PR adds the ability to disable recording download emails for guests in Cal Video meetings while still sending them to organizers. This gives event organizers more control over email notifications sent to attendees.
The new setting "Disable recording emails for guests" is added to the Cal Video settings section, allowing organizers to control whether guests receive recording download emails.
Mandatory Tasks
How should this be tested?
Testing Steps:
Expected Behavior:
Test Cases:
Implementation Details
The feature includes:
disableRecordingEmailsForGuestsin CalVideoSettingsSummary by cubic
Added a new setting to disable recording download emails for guests in Cal Video meetings, so only organizers receive them if enabled.