fix: events not removed from the previous host calendar due to wrong credentials usage#23756
fix: events not removed from the previous host calendar due to wrong credentials usage#23756anikdhabal merged 3 commits intomainfrom
Conversation
WalkthroughWhen the organizer changes during round‑robin reassignment, the code now deletes the original host’s calendar event/meeting using the original host’s credentials and any previousHostDestinationCalendar before proceeding with rescheduling. handleRescheduleEventManager now forwards an additional boolean flag (skipDeleteEventsAndMeetings) into EventManager.reschedule. bookingSelect was changed from using Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. Comment |
dosubot
bot
added
bookings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (1)
315-318: Use Prismaselect(notinclude) per guidelines; also avoid fetching nesteduserhere.Guidelines require using
selectand only the fields you need. The nesteduser.emailis redundant because you already havenewUser.email. Prefer selecting the credential shape your enrichment expects.Apply:
-import { prisma } from "@calcom/prisma"; +import { prisma } from "@calcom/prisma"; +import { credentialForCalendarServiceSelect } from "@calcom/features/bookings/lib/getAllCredentialsForUsersOnEvent/credentialForCalendarServiceSelect"; @@ - const credentials = await prisma.credential.findMany({ - where: { userId: newUser.id }, - include: { user: { select: { email: true } } }, - }); + const credentials = await prisma.credential.findMany({ + where: { userId: newUser.id }, + select: credentialForCalendarServiceSelect, + });If
credentialForCalendarServiceSelectisn’t exported from that path, re-export it from where it’s defined or inline the minimal fields the enrichment needs.
🧹 Nitpick comments (5)
packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
56-56: Coerce to a strict boolean for claritychangedOrganizer is optional; make the intent explicit.
- const skipDeleteEventsAndMeetings = changedOrganizer; + const skipDeleteEventsAndMeetings = !!changedOrganizer;packages/features/ee/round-robin/roundRobinReassignment.ts (2)
361-395: Pre-delete using original host’s credentials is the right fix; refine error handling and destinationCalendar nullability
- Approach looks correct: use previous host creds and remove before reschedule.
- Suggest wrapping deleteEventsAndMeetings with logging and a clear decision on failure (bubble vs. warn-and-continue). Current unconditional await will fail the whole reassignment on any provider hiccup.
- Prefer undefined over [] when no previousHostDestinationCalendar to preserve “unspecified” vs. “explicitly none” semantics some providers/paths rely on.
- const deletionEvent: CalendarEvent = { + const deletionEvent: CalendarEvent = { ...evt, organizer: { id: originalOrganizer.id, name: originalOrganizer.name || "", email: originalOrganizer.email, username: originalOrganizer.username || undefined, timeZone: originalOrganizer.timeZone, language: { translate: originalOrganizerT, locale: originalOrganizer.locale ?? "en" }, timeFormat: getTimeFormatStringFromUserTimeFormat(originalOrganizer.timeFormat), }, - destinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : [], + destinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : undefined, title: booking.title, }; - - await originalHostEventManager.deleteEventsAndMeetings({ - event: deletionEvent, - bookingReferences: booking.references, - }); + try { + await originalHostEventManager.deleteEventsAndMeetings({ + event: deletionEvent, + bookingReferences: booking.references, + }); + } catch (err) { + roundRobinReassignLogger.error("Failed to delete previous host events/meetings", { err }); + // Decide policy: rethrow to keep state consistent, or continue and rely on later cleanup. + throw err; + }Would you like me to add a unit/integration test that asserts deleteEventsAndMeetings is invoked with the original host’s credentialId(s)? I can scaffold it against a mocked EventManager.
402-402: Pass undefined instead of [] when no previous destination calendarMatches the deletionEvent suggestion and avoids signaling “explicit empty set”.
- previousHostDestinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : [], + previousHostDestinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : undefined,packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
330-363: Pre-deleting with the original host’s credentials is the right fix; add guardrails.
- Ensure
booking.user.credentialsare present;getAllCredentialsIncludeServiceAccountKeyseeds from them. If missing, it can crash.- Don’t let a deletion failure block reassign+reschedule; log and continue.
- Minor:
destinationCalendar: []is fine, butundefinedcommunicates “no override” more clearly.Add try/catch around deletion:
- await originalHostEventManager.deleteEventsAndMeetings({ - event: deletionEvent, - bookingReferences: booking.references, - }); + try { + await originalHostEventManager.deleteEventsAndMeetings({ + event: deletionEvent, + bookingReferences: booking.references, + }); + } catch (err) { + roundRobinReassignLogger.warn( + "Failed to delete previous host events/meetings; proceeding with reassignment", + { bookingId, originalHostId: originalOrganizer.id, err } + ); + }Optionally prefer
destinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : undefined.
365-371: Avoid potential double-deletion in reschedule flow.If
handleRescheduleEventManager(or downstreamEventManager.reschedule) can also perform deletions, explicitly skip it here since we already deleted above.If supported by the callee, pass an explicit flag:
const { evtWithAdditionalInfo } = await handleRescheduleEventManager({ evt, rescheduleUid: booking.uid, newBookingId: undefined, changedOrganizer: hasOrganizerChanged, + skipDeleteEventsAndMeetings: true, previousHostDestinationCalendar: previousHostDestinationCalendar ? [previousHostDestinationCalendar] : [], initParams: { user: newUserWithCredentials, eventType, },Please confirm the parameter name (
skipDeleteEventsAndMeetings) in the related changes and wire it here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/features/ee/round-robin/handleRescheduleEventManager.ts(2 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.ts(4 hunks)packages/features/ee/round-robin/roundRobinReassignment.ts(4 hunks)packages/features/ee/round-robin/utils/bookingSelect.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.tspackages/features/ee/round-robin/utils/bookingSelect.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinReassignment.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.tspackages/features/ee/round-robin/utils/bookingSelect.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinReassignment.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/ee/round-robin/roundRobinManualReassignment.tspackages/features/ee/round-robin/utils/bookingSelect.tspackages/features/ee/round-robin/handleRescheduleEventManager.tspackages/features/ee/round-robin/roundRobinReassignment.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ShashwatPS
PR: calcom/cal.com#23638
File: packages/trpc/server/routers/viewer/calendars/setDestinationReminder.handler.test.ts:198-199
Timestamp: 2025-09-06T11:00:34.372Z
Learning: In calcom/cal.com PR #23638, the maintainer ShashwatPS determined that authorization checks in the setDestinationReminder handler are "not applicable" when CodeRabbit suggested adding user-scoped WHERE clauses to prevent users from modifying other users' destination calendar reminder settings.
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Learnt from: vijayraghav-io
PR: calcom/cal.com#16878
File: packages/app-store/larkcalendar/api/callback.ts:109-117
Timestamp: 2025-09-08T10:07:30.026Z
Learning: In office365calendar and zohocalendar callback handlers, vijayraghav-io chose to manually call BookingReferenceRepository.reconnectWithNewCredential() after credential creation instead of using SelectedCalendarRepository.upsert(), to avoid disturbing existing error handling logic in catch blocks while still ensuring booking reference reconnection happens.
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/features/ee/round-robin/utils/bookingSelect.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma select uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.
Applied to files:
packages/features/ee/round-robin/utils/bookingSelect.ts
📚 Learning: 2025-09-03T11:54:05.409Z
Learnt from: supalarry
PR: calcom/cal.com#23514
File: apps/api/v2/src/ee/bookings/2024-08-13/services/bookings.service.ts:579-582
Timestamp: 2025-09-03T11:54:05.409Z
Learning: In calcom/cal.com bookings repository methods, when Prisma include uses `eventType: true`, all eventType fields including seatsShowAttendees are automatically included in the selection. Explicit field selection is not required when using `true` for nested relations.
Applied to files:
packages/features/ee/round-robin/utils/bookingSelect.ts
📚 Learning: 2025-09-01T08:56:14.071Z
Learnt from: nangelina
PR: calcom/cal.com#23486
File: packages/app-store/kyzon-space/lib/tokenManager.ts:25-31
Timestamp: 2025-09-01T08:56:14.071Z
Learning: In token refresh utilities like tokenManager.ts, accessing credential.key from Prisma is legitimate and necessary for OAuth token refresh flows. These internal utilities need stored credentials to refresh tokens and don't expose them in API responses.
Applied to files:
packages/features/ee/round-robin/utils/bookingSelect.ts
🧬 Code graph analysis (2)
packages/features/ee/round-robin/roundRobinManualReassignment.ts (3)
packages/prisma/zod-utils.ts (1)
eventTypeAppMetadataOptionalSchema(86-86)packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts (1)
getAllCredentialsIncludeServiceAccountKey(22-128)packages/types/Calendar.d.ts (1)
CalendarEvent(163-226)
packages/features/ee/round-robin/roundRobinReassignment.ts (3)
packages/prisma/zod-utils.ts (1)
eventTypeAppMetadataOptionalSchema(86-86)packages/features/bookings/lib/getAllCredentialsForUsersOnEvent/getAllCredentials.ts (1)
getAllCredentialsIncludeServiceAccountKey(22-128)packages/types/Calendar.d.ts (1)
CalendarEvent(163-226)
⏰ 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). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Detect changes
🔇 Additional comments (4)
packages/features/ee/round-robin/handleRescheduleEventManager.ts (1)
68-76: No action needed:undefinedcorrectly maps to the sixthisBookingRequestedRescheduleparameter. Optional: refactor to an options object to avoid magic positional arguments.packages/features/ee/round-robin/utils/bookingSelect.ts (1)
17-29: Good shift from include → select; keep an eye on credential surfaceSwitching to select aligns with our Prisma guidance and adds only required user fields plus credentials.
packages/features/ee/round-robin/roundRobinManualReassignment.ts (2)
10-10: New imports look correct and aligned with the new flow.No issues with the added imports for credentials aggregation, EventManager, or the apps metadata schema.
Also applies to: 25-25, 37-37
449-450: Confirm no secrets in credentialForCalendarServiceSelect
Verify that the importedcredentialForCalendarServiceSelect(from@calcom/prisma/selects/credential) does not includekey, service-account private keys, or any other sensitive fields—since it’s used in thecredentialsselection ofbookingSelectinpackages/features/ee/round-robin/utils/bookingSelect.ts.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
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/ee/round-robin/roundRobinManualReassignment.test.ts (1)
149-160: Fix assertion: last reschedule arg must track changedOrganizer, not be hardcoded to trueThe helper currently forces skipDeleteEventsAndMeetings=true, which will fail for scenarios where changedOrganizer=false and can mask regressions.
Apply this diff:
expect(spy).toHaveBeenCalledWith( expectedParams.location ? expect.objectContaining({ location: expectedParams.location }) : expect.any(Object), expectedParams.uid, undefined, expectedParams.changedOrganizer, - expectedParams.destinationCalendars ?? expect.any(Array), - undefined, - true + expectedParams.destinationCalendars ?? expect.any(Array), + undefined, + expectedParams.changedOrganizer );
🧹 Nitpick comments (2)
packages/features/ee/round-robin/roundRobinManualReassignment.test.ts (1)
182-229: Minor: reduce any-usage in the EventManager spy for stronger type-safetyCast EventManager.prototype to unknown and narrow or type the event param instead of any to keep the helper future-proof. Not blocking.
packages/features/ee/round-robin/roundRobinReassignment.test.ts (1)
130-147: Make second booking UID unique
In packages/features/ee/round-robin/roundRobinReassignment.test.ts (around line 133), the extra booking reusesbookingToReassignUid. Sinceuidis marked unique in the Prisma schema, switch it to a distinct literal to avoid constraint conflicts:- uid: bookingToReassignUid, + uid: "rr-fairness-booking",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts(1 hunks)packages/features/ee/round-robin/roundRobinManualReassignment.test.ts(1 hunks)packages/features/ee/round-robin/roundRobinReassignment.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/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinReassignment.test.tspackages/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/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinReassignment.test.tspackages/features/ee/round-robin/roundRobinDeleteEvents.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/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinReassignment.test.tspackages/features/ee/round-robin/roundRobinDeleteEvents.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: ShashwatPS
PR: calcom/cal.com#23638
File: packages/trpc/server/routers/viewer/calendars/setDestinationReminder.handler.test.ts:198-199
Timestamp: 2025-09-06T11:00:34.372Z
Learning: In calcom/cal.com PR #23638, the maintainer ShashwatPS determined that authorization checks in the setDestinationReminder handler are "not applicable" when CodeRabbit suggested adding user-scoped WHERE clauses to prevent users from modifying other users' destination calendar reminder settings.
Learnt from: vijayraghav-io
PR: calcom/cal.com#16878
File: packages/app-store/larkcalendar/api/callback.ts:109-117
Timestamp: 2025-09-08T10:07:30.026Z
Learning: In office365calendar and zohocalendar callback handlers, vijayraghav-io chose to manually call BookingReferenceRepository.reconnectWithNewCredential() after credential creation instead of using SelectedCalendarRepository.upsert(), to avoid disturbing existing error handling logic in catch blocks while still ensuring booking reference reconnection happens.
📚 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/ee/round-robin/roundRobinManualReassignment.test.tspackages/features/ee/round-robin/roundRobinReassignment.test.tspackages/features/ee/round-robin/roundRobinDeleteEvents.test.ts
📚 Learning: 2025-08-27T16:39:38.192Z
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.192Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Applied to files:
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts
⏰ 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). (3)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
🔇 Additional comments (4)
packages/features/ee/round-robin/roundRobinDeleteEvents.test.ts (2)
118-120: LGTM: test passes reassignment metadataAdding reassignedById and orgId aligns with the updated API and improves traceability in assertions.
127-127: LGTM: deletion uses the original host as organizerAsserting organizer.email === originalHost.email correctly verifies deletion under the previous host’s credentials.
packages/features/ee/round-robin/roundRobinReassignment.test.ts (2)
170-177: LGTM: updated reschedule call signature coveredExpectations include destinationCalendars, placeholder undefined, and skipDeleteEventsAndMeetings=true when organizer changes.
279-287: LGTM: fixed-host path asserts skipDeleteEventsAndMeetings=falseMatches the intended behavior when the organizer remains the same.
E2E results are ready! |
3 participants
What does this PR do?
Events not removed from the previous host calendar due to wrong credentials usage