fix: improve cancellation email of reschedule RR booking #17768
Conversation
|
@kart1ka is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (11/20/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (11/20/24)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (11/21/24)1 label was added to this PR based on Keith Williams's automation. |
|
Hi @Praashh Screenshot of cancelled mail sent to the old host (after the round robin booking has been rescheduled) with old date and reassigned to displayed
Screenshot of scheduled mail sent to the new host (after the round robin booking has been rescheduled) with new date displayed
|
i rescheduled and cancel the meet, it was same but i'll check it again |
Here are the steps that i am using to test my solution:
|
|
A quick question.
|
yes add guest field is supposed to be enabled while rescheduling a booking |
E2E results are ready! |
|
@kart1ka can you fix the conflicts🙏 |
Hi @anikdhabal Conflicts resolved. |
|
Hey @anikdhabal Just checking on the status of this PR. |
|
Hey @anikdhabal, I’ve updated the title and subtitle as discussed and verified that everything else is working as expected. Note: While merging main into this branch, some linting fixes were automatically applied, resulting in whitespace or newline only changes in a few files. Let me know if you’d like me to revert those. |
|
@kart1ka yeah let's keep only the main changes |
|
@anikdhabal I have merged the main branch again and those unwanted changes seem to have gone. Pls review. |
WalkthroughThis change set modifies the logic and templates for email and SMS notifications related to round robin (RR) booking rescheduling and reassignment. It updates the cancellation email to clarify when a booking was rescheduled to a different host, introduces a new email/SMS function for explicit reassignment scenarios, and adjusts the relevant templates and payloads to support a "reassigned" context. Test utilities and cases are updated to reflect the new logic and to ensure that the correct notifications are sent and verified during both automated and manual round robin host changes. Filtering logic for hosts is also refined to better match attendee emails. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~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 improving the clarity and accuracy of notifications for rescheduled round robin bookings, as described in the linked issues. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/emails/email-manager.ts (1)
261-285: Well-implemented reassignment function with proper parameter structure.The function correctly uses object destructuring for named parameters as requested in past reviews. The logic properly handles:
- Host email disable checks
- Promise aggregation for concurrent sending
- SMS notifications for team members with phone numbers
However, there's significant code duplication between this function and
sendRoundRobinCancelledEmailsAndSMS. Consider extracting the common SMS/email sending logic into a shared helper function to follow DRY principles.+// Helper function to reduce duplication +const sendRoundRobinNotificationEmails = async ( + calEvent: CalendarEvent, + members: Person[], + emailFactory: (calendarEvent: CalendarEvent, teamMember: Person) => BaseEmail, + eventTypeMetadata?: EventTypeMetadata +) => { + if (eventTypeDisableHostEmail(eventTypeMetadata)) return; + const calendarEvent = formatCalEvent(calEvent); + const emailsAndSMSToSend: Promise<unknown>[] = []; + const smsService = new EventCancelledSMS(calEvent); + + for (const teamMember of members) { + emailsAndSMSToSend.push(sendEmail(() => emailFactory(calendarEvent, teamMember))); + if (teamMember.phoneNumber) { + emailsAndSMSToSend.push(smsService.sendSMSToAttendee(teamMember)); + } + } + + await Promise.all(emailsAndSMSToSend); +};Also, based on past review comments about SMS/email behavior alignment, please verify that both functions should use the same SMS class (
EventCancelledSMS).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/test/utils/bookingScenario/expects.ts(2 hunks)packages/emails/email-manager.ts(1 hunks)packages/emails/src/templates/BaseScheduledEmail.tsx(1 hunks)packages/emails/src/templates/OrganizerCancelledEmail.tsx(1 hunks)packages/emails/templates/organizer-cancelled-email.ts(2 hunks)packages/features/bookings/lib/handleNewBooking.ts(2 hunks)packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(3 hunks)packages/features/ee/round-robin/roundRobinReassignment.ts(3 hunks)packages/lib/bookings/filterHostsBySameRoundRobinHost.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/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsBySameRoundRobinHost.tspackages/features/bookings/lib/handleNewBooking/test/reschedule.test.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.tsapps/web/test/utils/bookingScenario/expects.tspackages/emails/email-manager.tspackages/emails/templates/organizer-cancelled-email.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/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/bookings/lib/handleNewBooking.tspackages/emails/src/templates/OrganizerCancelledEmail.tsxpackages/emails/src/templates/BaseScheduledEmail.tsxpackages/lib/bookings/filterHostsBySameRoundRobinHost.tspackages/features/bookings/lib/handleNewBooking/test/reschedule.test.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.tsapps/web/test/utils/bookingScenario/expects.tspackages/emails/email-manager.tspackages/emails/templates/organizer-cancelled-email.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/emails/src/templates/OrganizerCancelledEmail.tsxpackages/emails/src/templates/BaseScheduledEmail.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
📚 Learning: the filterhostsbysameroundrobinhost function in packages/lib/bookings/filterhostsbysameroundrobinhos...
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Applied to files:
packages/features/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsBySameRoundRobinHost.tspackages/features/bookings/lib/handleNewBooking/test/reschedule.test.tspackages/features/ee/round-robin/roundRobinReassignment.tspackages/features/ee/round-robin/roundRobinManualReassignment.tsapps/web/test/utils/bookingScenario/expects.tspackages/emails/email-manager.ts
📚 Learning: in the failedbookingsbyfield component (packages/features/insights/components/failedbookingsbyfield....
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsBySameRoundRobinHost.tspackages/emails/email-manager.ts
📚 Learning: in the insightsbookingservice (packages/lib/server/service/insightsbooking.ts), the constructor stor...
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/features/bookings/lib/handleNewBooking.tspackages/lib/bookings/filterHostsBySameRoundRobinHost.ts
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#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.
Applied to files:
packages/emails/src/templates/BaseScheduledEmail.tsx
📚 Learning: in packages/app-store/office365calendar/lib/calendarservice.ts, the fetcher method in office365calen...
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
apps/web/test/utils/bookingScenario/expects.tspackages/emails/templates/organizer-cancelled-email.ts
📚 Learning: prefer composition over prop drilling in react; use react children feature instead of passing props ...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Prefer composition over prop drilling in React; use React children feature instead of passing props through many layers
Applied to files:
packages/emails/email-manager.ts
🧬 Code Graph Analysis (2)
packages/features/ee/round-robin/roundRobinReassignment.ts (2)
packages/emails/email-manager.ts (1)
sendRoundRobinReassignedEmailsAndSMS(261-285)packages/prisma/zod-utils.ts (1)
EventTypeMetadata(146-146)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
packages/emails/email-manager.ts (1)
sendRoundRobinReassignedEmailsAndSMS(261-285)packages/prisma/zod-utils.ts (1)
EventTypeMetadata(146-146)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (19)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (2)
261-264: LGTM: Updated to use the new reassignment email functionThe change from
sendRoundRobinCancelledEmailsAndSMStosendRoundRobinReassignedEmailsAndSMSaligns perfectly with the PR objective of improving email clarity for Round Robin reassignments. This makes the test more semantically accurate by explicitly testing reassignment email functionality rather than cancellation emails.
325-325: LGTM: Consistent expectation for the new email functionThe expectation correctly verifies that the new
sendRoundRobinReassignedEmailsAndSMSfunction is called once during manual reassignment, which is the expected behavior.packages/emails/src/templates/BaseScheduledEmail.tsx (1)
110-112: LGTM: Enhanced reassignment reason display with consistent patternThe addition of conditional rendering for reassignment reason when
byUseris falsy creates symmetry with the existing logic for whenbyUseris truthy. This ensures users receive reassignment context in both scenarios, improving email clarity as intended by the PR objectives.The implementation follows established patterns:
- Proper optional chaining and conditional rendering
- Consistent use of the
Infocomponent- Appropriate localization with
t("reason")packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts (1)
2161-2161: LGTM! Correctly tests booker-initiated reschedule functionality.The addition of
bookerReschedule: trueproperly aligns with the test scenario where the booker is rescheduling the round robin booking (as indicated byrescheduledBy: booker.emailon line 2103). This change ensures the test validates the new reassignment email functionality introduced in this PR.packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
7-7: LGTM! Import updated to reflect reassignment semantics.The change from
sendRoundRobinCancelledEmailsAndSMStosendRoundRobinReassignedEmailsAndSMSbetter reflects the actual operation being performed.
352-365: LGTM! Function call updated with improved parameter structure.The change to named parameters and the new
reassignedToparameter structure improves readability and provides clearer context about who the booking was reassigned to. This aligns well with the PR objective to improve reassignment email communication.packages/features/bookings/lib/handleNewBooking.ts (2)
81-81: Good organizational improvement.Grouping imports from the same module improves code readability.
1766-1780: LGTM! Reassignment detection and context passing implemented correctly.The logic properly identifies when a reassignment has occurred by finding non-fixed users in the new booked members and passes the appropriate context to the cancellation email function. The conditional "Booker Rescheduled" reason provides valuable clarity about who initiated the change.
packages/features/ee/round-robin/roundRobinReassignment.ts (2)
7-7: LGTM! Improved function naming aligns with PR objectives.The import change from
sendRoundRobinCancelledEmailsAndSMStosendRoundRobinReassignedEmailsAndSMSbetter reflects the actual operation and supports the PR's goal of clarifying that a booking was reassigned rather than cancelled.
415-428: Function call updated to use clearer named parameters.The refactor from positional arguments to named object properties improves code readability and maintainability. The new
reassignedToparameter clearly communicates who the booking was reassigned to, supporting better email content.packages/lib/bookings/filterHostsBySameRoundRobinHost.ts (3)
9-9: Appropriate expansion of generic constraint.Adding the
37-41: Enhanced query to support email-based host identification.The Prisma query enhancement to include attendee emails enables more sophisticated host matching, which is essential for the improved filtering logic that follows.
46-54: Improved filtering logic with email-based host identification.The two-step filtering approach is more robust:
- First identifies the current RR host by matching attendee emails with non-fixed hosts
- Then filters hosts to include those matching either the found RR host ID or the original booking user ID
This addresses scenarios where hosts might be identified through different means while maintaining backward compatibility.
packages/emails/src/templates/OrganizerCancelledEmail.tsx (1)
3-19: Well-structured conditional email content for reassigned vs cancelled events.The component effectively handles both scenarios:
- Uses appropriate translation keys (
event_request_reassignedvsevent_request_cancelled)- Includes subtitle only for reassigned events to provide additional context
- Maintains robust translation fallback from
teamMembertoorganizerlanguageThis directly supports the PR's objective of clarifying whether a booking was reassigned or cancelled.
packages/emails/templates/organizer-cancelled-email.ts (2)
12-12: Conditional subject line improves email clarity.The subject now accurately reflects the actual operation - using "event_reassigned_subject" when reassigned vs "event_cancelled_subject" for true cancellations. This directly addresses the PR's objective of clarifying communication.
26-26: Enhanced template rendering with reassignment context.The updated
getHtmlmethod signature and call properly passes the reassignment context to the email rendering logic, enabling the template to display appropriate content and messaging based on the operation type.Also applies to: 31-37
apps/web/test/utils/bookingScenario/expects.ts (2)
714-719: Appropriate test enhancement for booker reschedule scenarios.The optional
bookerRescheduleparameter allows tests to specify when reassignment emails should be expected, providing more precise test coverage for different reschedule scenarios.
744-755: Conditional reassignment email expectation aligns with new logic.The test correctly expects reassignment emails only when:
bookerRescheduleis true (reschedule initiated by booker)- Organizer actually changes (different from previous organizer)
This properly validates the improved email notification system that distinguishes between cancellations and reassignments.
packages/emails/email-manager.ts (1)
240-240: LGTM - Parameter type enhancement.The addition of the optional
reasonfield to thereassignedToparameter correctly supports the enhanced email messaging for clearer cancellation context.
| emailsAndSMSToSend.push( | ||
| sendEmail( | ||
| () => new OrganizerCancelledEmail({ calEvent: calendarEvent, teamMember, reassigned: reassignedTo }) | ||
| ) | ||
| ); | ||
|
|
||
| if (teamMember.phoneNumber) { | ||
| emailsAndSMSToSend.push(successfullyReScheduledSMS.sendSMSToAttendee(teamMember)); | ||
| } | ||
| } | ||
|
|
||
| await Promise.all(emailsAndSMSToSend); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Approve email logic, but verify SMS class usage.
The simplified email logic correctly aligns with PR objectives - always sending cancellation emails with optional reassignment context. However, there's an inconsistency in SMS handling:
- Variable name suggests
successfullyReScheduledSMSbut the class instantiated isEventCancelledSMS - This appears intentional based on the context, but please verify this is the correct SMS type for cancelled/reassigned bookings.
🏁 Script executed:
#!/bin/bash
# Verify SMS class usage consistency across round robin functions
rg -A 3 -B 1 "EventCancelledSMS|EventSuccessfullyReScheduledSMS" packages/emails/email-manager.tsLength of output: 2178
Clarify SMS class and variable naming in cancellation flow
The use of EventCancelledSMS in the round-robin cancellation function is correct, but the variable name (successfullyReScheduledSMS) implies a “reschedule” SMS and will confuse future readers. Please rename the variable to match the class and its intent:
• File: packages/emails/email-manager.ts
Location: inside sendRoundRobinCancelledEmailsAndSMS (around lines 247–259)
Replace:
- const successfullyReScheduledSMS = new EventCancelledSMS(calEvent);
+ const eventCancelledSms = new EventCancelledSMS(calEvent);
…
- emailsAndSMSToSend.push(
- successfullyReScheduledSMS.sendSMSToAttendee(teamMember)
- );
+ emailsAndSMSToSend.push(
+ eventCancelledSms.sendSMSToAttendee(teamMember)
+ );This ensures the variable name accurately reflects that it’s sending a cancellation SMS.
🤖 Prompt for AI Agents
In packages/emails/email-manager.ts around lines 247 to 259, the variable named
successfullyReScheduledSMS is misleading because it suggests sending a
reschedule SMS, but it actually sends a cancellation SMS using the
EventCancelledSMS class. Rename the variable successfullyReScheduledSMS to a
name that clearly indicates it is for cancellation SMS, such as
eventCancelledSMS, to accurately reflect its purpose and avoid confusion.
What does this PR do?
Screencast.from.21-11-24.12.49.55.AM.IST.webm
How should this be tested?
Mandatory Tasks (DO NOT REMOVE)