fix: event not removed from calendar when reassigned#23099
Conversation
Walkthrough
Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
bookings
anikdhabal
changed the title
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/14/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/lib/EventManager.ts (1)
641-656: Critical: requiresConfirmation flow now bypasses confirmation when skipDeleteEventsAndMeetings is trueReplacing
!changedOrganizerwith!skipDeleteEventsAndMeetingsalters semantics: for bookings that require confirmation, passingskipDeleteEventsAndMeetings = truenow executes the "else" branch and proceeds to create/update events even though confirmation is required. This contradicts the comment and previous behavior that avoided creating/updating until confirmation (except for the changed-organizer branch).Restore gating on
changedOrganizerfor the requires-confirmation path, and useskipDeleteEventsAndMeetingssolely to control whether we delete, not whether we bypass confirmation logic.Suggested fix:
- if (evt.requiresConfirmation && !skipDeleteEventsAndMeetings) { - log.debug("RescheduleRequiresConfirmation: Deleting Event and Meeting for previous booking"); - // As the reschedule requires confirmation, we can't update the events and meetings to new time yet. So, just delete them and let it be handled when organizer confirms the booking. - await this.deleteEventsAndMeetings({ - event: { ...event, destinationCalendar: previousHostDestinationCalendar }, - bookingReferences: booking.references, - }); - } else { + if (evt.requiresConfirmation && !changedOrganizer) { + // As the reschedule requires confirmation, we can't update the events and meetings to new time yet. + // Only perform deletion if not explicitly skipped. + if (!skipDeleteEventsAndMeetings) { + log.debug("RescheduleRequiresConfirmation: Deleting Event and Meeting for previous booking"); + await this.deleteEventsAndMeetings({ + event: { ...event, destinationCalendar: previousHostDestinationCalendar }, + bookingReferences: booking.references, + }); + } + } else { if (changedOrganizer) { - if (!skipDeleteEventsAndMeetings) { + if (!skipDeleteEventsAndMeetings) { log.debug("RescheduleOrganizerChanged: Deleting Event and Meeting for previous booking"); await this.deleteEventsAndMeetings({ event: { ...event, destinationCalendar: previousHostDestinationCalendar }, bookingReferences: booking.references, }); }This maintains legacy behavior (no update/create when confirmation is required and organizer didn’t change) while still letting you skip deletion when you’ve already handled it upstream.
If desired, I can follow up with a focused unit test for:
- requiresConfirmation=true, changedOrganizer=false, skip=true → no update/create nor deletion;
- requiresConfirmation=true, changedOrganizer=false, skip=false → deletion only;
- requiresConfirmation=true, changedOrganizer=true, skip=true → no deletion in reschedule(), but create with new organizer.
🧹 Nitpick comments (1)
packages/lib/EventManager.ts (1)
573-575: Avoid boolean flag parameter; add default and consider options objectAdding a trailing optional boolean increases ambiguity at call sites (boolean trap) and is error-prone for future maintenance.
- Short term: add a default value to make intent explicit.
- Medium term: switch to a typed options object, e.g.
reschedule(event, rescheduleUid, { newBookingId, changedOrganizer, previousHostDestinationCalendar, isBookingRequestedReschedule, skipDeleteEventsAndMeetings }).Apply the minimal change to add an explicit default:
- isBookingRequestedReschedule?: boolean, - skipDeleteEventsAndMeetings?: boolean + isBookingRequestedReschedule?: boolean, + skipDeleteEventsAndMeetings: boolean = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/lib/EventManager.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/lib/EventManager.tspackages/features/bookings/lib/handleNewBooking.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/lib/EventManager.tspackages/features/bookings/lib/handleNewBooking.ts
⏰ 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). (1)
- GitHub Check: Check for E2E label
🔇 Additional comments (1)
packages/features/bookings/lib/handleNewBooking.ts (1)
1705-1707: EventManager.reschedule — skip/delete behavior auditedChecked all call sites of EventManager.reschedule. Findings:
- packages/features/bookings/lib/handleNewBooking.ts — passes skipDeleteEventsAndMeetings = true (7th arg) and explicitly calls originalHostEventManager.deleteEventsAndMeetings(...) immediately before the reschedule call — intentional/correct.
- packages/features/ee/round-robin/handleRescheduleEventManager.ts — calls reschedule(..., changedOrganizer, previousHostDestinationCalendar) without skip flag and does not delete elsewhere — reschedule will handle deletion as needed.
- packages/features/bookings/lib/handleSeats/reschedule/owner/moveSeatedBookingToNewTimeSlot.ts — calls reschedule(copyEvent, rescheduleUid, newBooking.id) with no skip flag and no prior deletion — OK.
- packages/features/bookings/lib/handleSeats/reschedule/owner/combineTwoSeatedBookings.ts — same as above — OK.
No other call sites found that pass the skip flag. No changes required.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
E2E results are ready! |
….com into rr_reassignment_issue
joeauyeung
left a comment
joeauyeung
left a comment
There was a problem hiding this comment.
Thanks for adding the tests
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/roundRobinReassignment.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/ee/round-robin/roundRobinReassignment.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-22T11:42:47.623Z
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/roundRobinReassignment.test.ts
🧬 Code Graph Analysis (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (1)
apps/web/test/utils/bookingScenario/bookingScenario.ts (8)
getOrganizer(1496-1552)TestData(1215-1487)getGoogleCalendarCredential(1168-1176)getDate(1069-1116)createBookingScenario(954-985)getScenarioData(1554-1640)getMockBookingAttendee(2248-2266)mockCalendarToHaveNoBusySlots(1901-1914)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
10-13: Good additions: new test helpers are appropriate for calendar-backed scenarios.Importing getOrganizer, getGoogleCalendarCredential, and mockCalendarToHaveNoBusySlots is the right move for constructing Google Calendar-capable users and mocking the provider.
23-23: LGTM: appStoreMetadata import is correct.Using appStoreMetadata.googlecalendar.type for the booking reference aligns with how other tests/type checks identify app types.
packages/features/ee/round-robin/roundRobinReassignment.test.ts
Outdated
Show resolved
Hide resolved
packages/features/ee/round-robin/roundRobinReassignment.test.ts
Outdated
Show resolved
Hide resolved
packages/features/ee/round-robin/roundRobinReassignment.test.ts
Outdated
Show resolved
Hide resolved
… robin reassignment - Remove existing tests and replace with focused test for organizer change scenario - Verify deleteEventsAndMeetings is called when organizer changes - Mock reschedule method to simulate real behavior while tracking deletion calls - Test verifies original host's calendar events are properly deleted - Ensures booking reassignment and email notifications work correctly Co-Authored-By: anik@cal.com <adhabal2002@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
134-138: Fix parameter name in email expectation: use prevOrganizerThe helper expects prevOrganizer, not previousOrganizer. This will fail at runtime.
Apply this diff:
- expectSuccessfulRoundRobinReschedulingEmails({ - previousOrganizer: originalHost, - newOrganizer: newHost, - emails, - }); + expectSuccessfulRoundRobinReschedulingEmails({ + prevOrganizer: originalHost, + newOrganizer: newHost, + emails, + });
85-96: Avoid hard-coded credentialId; derive it from the organizer’s created credentialHard-coding credentialId: 1 risks flakiness. Use the actual credential ID from originalHost to ensure the correct provider lookup.
Apply this diff:
externalCalendarId: "MOCK_EXTERNAL_CALENDAR_ID", - credentialId: 1, + credentialId: originalHost.credentials?.[0]?.id as number, deleted: null,
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
115-122: Don’t couple deletion to “new host” credentials; relax the assertionAsserting delete uses the new host’s credentials bakes in an implementation detail and may break if the deletion is performed via the original host’s credential (which is arguably more correct). Focus the assertion on the event ID and calendar ID being targeted.
Apply this diff:
- expect(deleteCall.args.event.organizer.email).toBe(newHost.email); // Current implementation uses new host credentials + // Deletion may use either original or new host credentials; avoid coupling to a specific one. + expect([originalHost.email, newHost.email]).toContain(deleteCall.args.event.organizer.email);
106-109: Nit: remove unused update mock to keep setup minimalThe test doesn’t assert on update calls; dropping the unused update UID simplifies the setup.
Apply this diff:
- const calendarMock = mockCalendarToHaveNoBusySlots("googlecalendar", { - create: { uid: "NEW_EVENT_ID" }, - update: { uid: "UPDATED_EVENT_ID" }, - }); + const calendarMock = mockCalendarToHaveNoBusySlots("googlecalendar", { + create: { uid: "NEW_EVENT_ID" }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/roundRobinReassignment.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/ee/round-robin/roundRobinReassignment.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-22T11:42:47.623Z
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/roundRobinReassignment.test.ts
🧬 Code Graph Analysis (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
apps/web/test/utils/bookingScenario/bookingScenario.ts (7)
getOrganizer(1496-1552)TestData(1215-1487)getGoogleCalendarCredential(1168-1176)getDate(1069-1116)createBookingScenario(954-985)getScenarioData(1554-1640)mockCalendarToHaveNoBusySlots(1901-1914)apps/web/test/utils/bookingScenario/expects.ts (2)
expectBookingToBeInDatabase(413-430)expectSuccessfulRoundRobinReschedulingEmails(710-753)
🔇 Additional comments (1)
packages/features/ee/round-robin/roundRobinReassignment.test.ts (1)
31-47: Good use of scenario builders and Google Calendar test utilitiesUsing getOrganizer with getGoogleCalendarCredential and selectedCalendars ensures the calendar mock is exercised realistically for both original and new hosts.
packages/features/ee/round-robin/roundRobinReassignment.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts (5)
128-132: Strengthen expectations: assert no update path was takenGiven this scenario re-creates the event for the new organizer, we shouldn’t see any update calls. This guards against regressions where the system attempts an update instead of creating anew.
// Verify that creation occurred with new host credentials expect(calendarMock.createEventCalls.length).toBe(1); const createCall = calendarMock.createEventCalls[0]; expect(createCall.args.calEvent.organizer.email).toBe(newHost.email); + // Ensure we didn't go down the update path by mistake + expect(calendarMock.updateEventCalls.length).toBe(0);
121-121: Minor: prefer “toBeGreaterThan(0)” over “toBeGreaterThanOrEqual(1)”Both are equivalent here; the former is slightly clearer.
- expect(calendarMock.deleteEventCalls.length).toBeGreaterThanOrEqual(1); + expect(calendarMock.deleteEventCalls.length).toBeGreaterThan(0);
34-35: Prefer static import for roundRobinReassignment unless there’s a solid reason for dynamicStatic imports simplify type-checking, test readability, and toolchain optimizations. Use dynamic import only if you’re avoiding a circular dependency or a heavy module that must be lazily loaded.
- const roundRobinReassignment = (await import("./roundRobinReassignment")).default; + // statically imported at top of fileAdd this at the top-level imports:
import roundRobinReassignment from "./roundRobinReassignment";
133-144: Add DB reference assertions to fully validate cleanup and re-creationRight now we validate calendar client interactions and booking owner change. Consider also asserting:
- The original Google reference is marked deleted/soft-deleted.
- A new Google reference exists for the new event/host.
This ensures data integrity matches the external side-effects.
If there’s a helper like
expectBookingReferencesor access to the booking’s references via your test utils, I can wire up precise assertions. Otherwise, I can add a small helper to fetch the booking by UID and validate references directly.
28-33: Add a companion test for “skipDeleteEventsAndMeetings = true” pathThis PR introduces a flag that controls deletion. We currently cover the “deletion happens” case; please add a test where the flag is true (or where the code path computes it as such) to assert that:
- No delete calls occur,
- The booking is reassigned correctly,
- Email behavior aligns with the non-deletion flow.
If
roundRobinReassignmentdoesn’t expose a way to force the skip flag, I can help craft a focused test that calls the lower-level reschedule API or sets up a scenario that yieldsskipDeleteEventsAndMeetings = true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/roundRobinDeleteEvents.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/ee/round-robin/roundRobinDeleteEvents.test.ts
🧠 Learnings (1)
📚 Learning: 2025-07-22T11:42:47.623Z
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/roundRobinDeleteEvents.test.ts
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts (2)
98-100: Avoid hard-coding credentialId — derive it from the seeded host to prevent flakinessUsing a magic number for credentialId can break if fixture ordering changes; tie the id to the seeded organizer’s actual Google credential so deletion uses the correct account.
- File: packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts — around lines 98–100
Suggested change:
- credentialId: 1, + // Use the actual credential ID of the original host to guarantee correct account usage + credentialId: originalHost.credentials[0].id,If originalHost.credentials[0].id isn’t available in the fixture, derive the ID from your test helpers or adjust fixture utilities to expose the credential ID. I attempted to locate this test file in the repo but couldn't find it — please confirm the path so I can re-verify.
36-53: No action needed — seeded organizers get distinct mocked credentialsgetGoogleCalendarCredential() returns a fresh mocked credential object on each call (apps/web/test/utils/bookingScenario/bookingScenario.ts → getMockedCredential). createBookingScenario inserts users (and their credentials) before inserting bookings (createBookingScenario → addUsers then addBookings), so the booking reference in the test will point at the original host’s created credential. The test (packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts) calls getGoogleCalendarCredential() separately for each organizer, so they will not share the same DB credential.
- Files inspected:
- apps/web/test/utils/bookingScenario/bookingScenario.ts — getMockedCredential / createBookingScenario / addUsers
- packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts — test uses two separate getGoogleCalendarCredential() calls
* fix: event not removed from calendar when reassign * fix test * tweak * add test * test: add comprehensive test for calendar event deletion during round robin reassignment - Remove existing tests and replace with focused test for organizer change scenario - Verify deleteEventsAndMeetings is called when organizer changes - Mock reschedule method to simulate real behavior while tracking deletion calls - Test verifies original host's calendar events are properly deleted - Ensures booking reassignment and email notifications work correctly Co-Authored-By: anik@cal.com <adhabal2002@gmail.com> * update * revert roundRobinReassignment.test.ts * revert --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* fix: event not removed from calendar when reassign * fix test * tweak * add test * test: add comprehensive test for calendar event deletion during round robin reassignment - Remove existing tests and replace with focused test for organizer change scenario - Verify deleteEventsAndMeetings is called when organizer changes - Mock reschedule method to simulate real behavior while tracking deletion calls - Test verifies original host's calendar events are properly deleted - Ensures booking reassignment and email notifications work correctly Co-Authored-By: anik@cal.com <adhabal2002@gmail.com> * update * revert roundRobinReassignment.test.ts * revert --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
4 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist