fix: handle collective multiple host on destinationCalendar#8
Conversation
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis refactor converts destination calendar handling from single-value to array-based architecture across the codebase. Calendar services now extract the first array element as the primary calendar, credential IDs are propagated through event creation methods, and team booking logic accumulates member destination calendars for collective scheduling scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Booking as Booking Handler
participant EM as EventManager
participant CM as CalendarManager
participant CS as Calendar Service
participant DB as Database
Note over Booking,DB: Old Flow: Single Destination Calendar
Booking->>EM: createAllCalendarEvents(evt: destinationCalendar?)
EM->>CM: createEvent(evt, externalId?)
CM->>CS: createEvent(evt)
CS->>CS: Use destinationCalendar.externalId
CS-->>CM: Result
CM-->>EM: EventResult
EM-->>Booking: SingleResult
Note over Booking,DB: New Flow: Multi-Destination Calendar Array
Booking->>EM: createAllCalendarEvents(evt: destinationCalendar[])
EM->>EM: For each destination in array
alt destination.credentialId exists
EM->>DB: Fetch credential if missing
EM->>CM: createEvent(evt, externalId, credentialId)
else No credentialId
EM->>DB: Find all credentials for integration
EM->>CM: createEvent(evt, externalId) for each
end
CM->>CS: createEvent(evt, credentialId)
CS->>CS: Match calendar by credentialId
CS-->>CM: Result + credentialId + externalId
CM-->>EM: EventResult with metadata
EM->>EM: Collect per-reference results
EM-->>Booking: ResultArray[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring close attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/lib/CalendarService.ts (1)
125-198: mainHostDestinationCalendar extraction is correct; guard against “no matching calendar” edge caseDeriving
mainHostDestinationCalendarviaevent.destinationCalendar ?? []and using itsexternalIdboth to filter target calendars and to markprimaryinlistCalendarscorrectly adapts this CalDAV service to the new array form without changing intent.One edge case to consider: if
mainHostDestinationCalendar?.externalIdis set but does not match any fetched calendar URLs, the.filterincreateEventwill yield an empty array, so nocreateCalendarObjectcalls are made while the function still returns a “successful” UID. It may be worth logging or surfacing a warning/error whenmainHostDestinationCalendar.externalIdis defined but no calendars match, to make silent misconfiguration easier to detect.Also applies to: 496-521
packages/core/EventManager.ts (1)
116-123: Null-safety bug in Google Meet fallback when no destinationCalendar is setHere:
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { evt["location"] = "integrations:daily"; }If
evt.destinationCalendarisnullor an empty array (which is valid with the new types),mainHostDestinationCalendarisundefined. Accessing.integrationonundefinedwill throw at runtime whenevt.location === MeetLocationType.This needs an optional chain or equivalent guard:
- if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { + if ( + evt.location === MeetLocationType && + mainHostDestinationCalendar?.integration !== "google_calendar" + ) { evt["location"] = "integrations:daily"; }Without this change, bookings with Google Meet location and no destination calendar will crash in
EventManager.create.packages/app-store/googlecalendar/lib/CalendarService.ts (2)
196-203: Fix the.findlogic to properly handle missingexternalCalendarIdinupdateEventanddeleteEventThe review comment is confirmed. Both methods contain a critical logic flaw:
- Lines 254-256 (updateEvent) and 315-317 (deleteEvent) search
event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)in the fallback branch.- When
externalCalendarIdis falsy (undefined/null/empty), this predicate never matches any calendar, returning undefined.- For
deleteEvent, undefined then silently falls back to"primary", ignoring available destination calendars.- For
updateEvent, undefined may be passed directly to the API call.Required changes:
- Only attempt
.find()whenexternalCalendarIdis truthy.- Fall back to
event.destinationCalendar?.[0]?.externalIdbefore falling back to"primary"ordefaultCalendarId.This ensures legacy bookings with missing
externalCalendarIdbut populateddestinationCalendaruse the available calendar instead of silently defaulting to primary.
87-104: Calendar selection logic passesundefinedcalendarId to Google API calls and mishandles fallbacksThe review comment is accurate. All three issues are confirmed in the source code:
createEventpatch lacks fallback (line 168): Thecalendar.events.patch()call usescalendarId: selectedCalendarwithout the|| "primary"fallback thatcalendar.events.insert()correctly applies on line 153. IfselectedCalendaris undefined (no matching destination calendar), the patch will fail.
updateEventanddeleteEventcompare against undefined (lines 250–252, 315–317): Both methods use.find((cal) => cal.externalId === externalCalendarId)whereexternalCalendarIdmay be falsy/undefined. This predicate will never match any calendar, soevent.destinationCalendaris ignored as a fallback. InupdateEvent,selectedCalendaris then passed directly tocalendar.events.update()on line 256 without a fallback, risking undefined.
deleteEventhas a fallback but flawed logic: While line 322 does fall back todefaultCalendarId, the find logic is still incorrect and should not compare against undefined.The fixes in the review comment are correct and necessary to ensure robust calendar selection across all three methods.
🧹 Nitpick comments (12)
packages/emails/src/templates/BrokenIntegrationEmail.tsx (1)
88-91: Simplify the optional chaining and ensure type consistency.Line 90 contains redundant optional chaining (
mainHostDestinationCalendar?.integration) sincemainHostDestinationCalendaris already checked in the ternary condition. Additionally, the fallback value"calendar"is a string, whilesplit("_")returns an array, creating a type inconsistency that makes the code harder to reason about.Consider this refactor for clarity:
const [mainHostDestinationCalendar] = calEvent.destinationCalendar ?? []; let calendar = mainHostDestinationCalendar - ? mainHostDestinationCalendar?.integration.split("_") - : "calendar"; + ? mainHostDestinationCalendar.integration.split("_") + : ["calendar"];This ensures
calendaris always an array initially, making the subsequentArray.isArraycheck on line 93 more predictable.packages/app-store/larkcalendar/lib/CalendarService.ts (1)
125-161: Array-based destinationCalendar handling is correct; logic is duplicated in a few placesUsing
[mainHostDestinationCalendar] = event.destinationCalendar ?? []and thenmainHostDestinationCalendar?.externalIdsafely adapts to the new array shape and avoids null access across create/update/delete and attendee creation. To keep behavior perfectly in sync if this ever changes, consider extracting a small helper likegetMainHostDestinationCalendar(event)instead of repeating the destructuring in each method.Also applies to: 163-182, 189-229, 236-252
packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts (1)
68-92: destinationCalendar normalization to array matches the new CalendarEvent contractWrapping
booking.destinationCalendar(or the user’s destinationCalendar fallback) in a single‑element array, and defaulting to[], cleanly aligns this handler with the updateddestinationCalendar: DestinationCalendar[]shape while preserving existing selection logic. If you keep adding similar logic in other handlers, consider a shared helper (e.g.getSelectedDestinationCalendar(booking)) to avoid future drift.packages/app-store/office365calendar/lib/CalendarService.ts (1)
72-92: Office365 createEvent correctly switches to array-based destinationCalendarUsing
[mainHostDestinationCalendar] = event.destinationCalendar ?? []and then targeting/me/calendars/{id}/eventswhenexternalIdis present keeps existing behavior while supportingDestinationCalendar[], with a safe fallback to/me/calendar/eventswhen none is selected. As a tiny optional cleanup, the second?.in${mainHostDestinationCalendar?.externalId}isn’t strictly needed since you already guard on it above.packages/features/ee/payments/api/webhook.ts (1)
100-121: Stripe payment flows now normalize destinationCalendar to an array consistentlyIn both
getBookingandhandlePaymentSuccess, computingselectedDestinationCalendar = booking.destinationCalendar || user.destinationCalendarand then settingdestinationCalendar: selectedDestinationCalendar ? [selectedDestinationCalendar] : []preserves the previous fallback logic while conforming to the newDestinationCalendar[]shape used byCalendarEvent.Since this selection rule is now duplicated (and similar patterns exist in other handlers), you might consider a small shared helper (e.g.
resolveDestinationCalendar(booking, user)) to keep behavior consistent across all payment and booking flows.Also applies to: 207-231
packages/core/CalendarManager.ts (1)
218-269: createEvent refactor cleanly propagates credentialId/externalId; error handling looks reasonablePassing
credential.idintocalendar.createEvent(calEvent, credential.id)and returning bothexternalIdandcredentialIdfrom this helper gives downstream code enough context to build accurate booking references for multi‑destination scenarios. ThehideCalendarNotesbehavior and updated copy (“organizer”) are also clear.The catch handler marking
success = false, special‑casingcode === 404, and optionally capturingcalErrorprovides a controlled failure path without throwing for the “mismatched externalId/credential” case. Just ensure all calendar adapters’createEventsignatures are compatible with the extracredentialIdparameter (even if some ignore it), and that callers supply the appropriateexternalIdwhere you rely on it for later correlation.packages/features/bookings/lib/handleCancelBooking.ts (1)
250-256: destinationCalendar array handling is consistent; consider de-duplicating the constructionThe two places where
evt.destinationCalendaris built now both follow:destinationCalendar: bookingToDelete?.destinationCalendar ? [bookingToDelete.destinationCalendar] : bookingToDelete?.user.destinationCalendar ? [bookingToDelete.user.destinationCalendar] : [];This matches the new
DestinationCalendar[] | nullshape and works fine with downstream checks that look atlength > 0.If this pattern is going to be reused further, consider extracting a small helper (e.g.,
getBookingDestinationCalendars(booking)) to avoid subtle divergence between the paid/non-paid paths in future edits.Also applies to: 528-533
packages/core/EventManager.ts (2)
336-383: Multi-destination create flow looks correct; consider sharing credential-fetch helperThe new
createAllCalendarEventslogic:
- Iterates
event.destinationCalendar[].- When
destination.credentialIdis present, resolves that specific credential (with DB fallback if not already inthis.calendarCredentials) and callscreateEvent(credential, event, destination.externalId).- When not, falls back to all credentials whose
typematchesdestination.integration.This matches the new per-destination model and should behave correctly.
You may want to factor the “fetch credential from DB and build
CredentialWithAppName” code into a small helper, since the same pattern also appears inupdateAllCalendarEvents.
474-590: updateAllCalendarEvents correctly iterates calendar references, but error fallback loses credential contextThe refactor to:
- Derive
calendarReferenceas an array of all_calendarreferences (fromnewBookingwhen merging, otherwise frombooking),- Resolve a credential per reference (with DB fallback), and
- Push
updateEvent(credential, event, bookingRefUid, calenderExternalId),is consistent with the new multi-reference model and should handle most cases well.
In the catch block, though:
return Promise.resolve( calendarReference?.map((reference) => ({ appName: "none", type: reference?.type || "calendar", success: false, uid: "", originalEvent: event, credentialId: 0, })) ?? [] );you intentionally drop any association with the actual credential ID that failed. If you care about observability, you could surface
reference.credentialId ?? 0instead of a hard-coded0to make debugging easier:- credentialId: 0, + credentialId: reference?.credentialId ?? 0,Functionally everything still works; this is just a small improvement to error reporting.
packages/features/bookings/lib/handleNewBooking.ts (3)
724-769: loadUsers refactor improves clarity and error handlingThe new
loadUsersencapsulation:
- Handles dynamic groups (no
eventTypeId) by loading users by username list.- Handles eventType-based bookings by mapping
hostsinto user-like objects, falling back toeventType.userswhen no hosts are defined.- Wraps Prisma and HttpError failures into consistent
HttpErrorresponses, with a generic 500 for unexpected errors.This makes the user-loading logic easier to follow and isolates error mapping to one place. The only minor suggestion is to preserve more detail from non-Prisma errors if you care about debugging, but functionally this is solid.
987-1009: teamDestinationCalendars + evt.destinationCalendar push correctly aggregate host calendars for COLLECTIVEYou now:
Collect each non-organizer host’s
user.destinationCalendarfor COLLECTIVE team events:const teamDestinationCalendars: DestinationCalendar[] = []; ... if (isTeamEventType && eventType.schedulingType === "COLLECTIVE" && user.destinationCalendar) { teamDestinationCalendars.push(user.destinationCalendar); }Initialize
evt.destinationCalendarfrom the event type or organizer:destinationCalendar: eventType.destinationCalendar ? [eventType.destinationCalendar] : organizerUser.destinationCalendar ? [organizerUser.destinationCalendar] : null,Append team members’ calendars for COLLECTIVE:
if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { evt.destinationCalendar?.push(...teamDestinationCalendars); }This matches the goal of having all hosts’ calendars represented on the
CalendarEventfor collective scheduling.One edge case to be aware of: if neither the event type nor organizer has a
destinationCalendar,evt.destinationCalendarwill benull, and the?.pushwill be a no-op, even ifteamDestinationCalendarsis non-empty. If you want “collective hosts only” to still get events, you could initializeevt.destinationCalendartoteamDestinationCalendarsin that scenario, but that’s a product decision rather than a bug.Also applies to: 1077-1079
1874-1880: BookingCreateInput now connects only the primary destination calendar; behavior is clear but could be documenteddestinationCalendar: evt.destinationCalendar && evt.destinationCalendar.length > 0 ? { connect: { id: evt.destinationCalendar[0].id }, } : undefined,This “first element only” connection is consistent with:
- The DB schema (
Booking.destinationCalendarbeing a single relation), and- Using the first item in
evt.destinationCalendar[]as the canonical/primary calendar elsewhere.If multi-destination bookings are expected to show up in the booking row itself in the future, it might be worth a short comment here stating that only the primary destination calendar is persisted on the booking, and others are tracked via references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/web/pages/api/cron/bookingReminder.ts(2 hunks)apps/web/playwright/webhook.e2e.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(8 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(4 hunks)packages/app-store/office365calendar/lib/CalendarService.ts(1 hunks)packages/core/CalendarManager.ts(3 hunks)packages/core/EventManager.ts(8 hunks)packages/core/builders/CalendarEvent/builder.ts(1 hunks)packages/core/builders/CalendarEvent/class.ts(1 hunks)packages/emails/src/templates/BrokenIntegrationEmail.tsx(1 hunks)packages/features/bookings/lib/handleCancelBooking.ts(3 hunks)packages/features/bookings/lib/handleNewBooking.ts(8 hunks)packages/features/ee/payments/api/paypal-webhook.ts(1 hunks)packages/features/ee/payments/api/webhook.ts(4 hunks)packages/lib/CalendarService.ts(2 hunks)packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/confirm.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/create.handler.ts(1 hunks)packages/types/Calendar.d.ts(4 hunks)packages/types/EventManager.d.ts(1 hunks)
🔇 Additional comments (15)
apps/web/playwright/webhook.e2e.ts (1)
249-249: LGTM! Test expectation updated correctly.The test expectation correctly reflects the new array-based destinationCalendar structure. When no destination calendar is present, an empty array is now the expected value instead of
null.packages/core/builders/CalendarEvent/class.ts (1)
26-26: LGTM! Core type updated to support array-based architecture.The type change from
DestinationCalendar | nulltoDestinationCalendar[] | nullis the foundation for the array-based destination calendar handling introduced in this PR. This enables collective scheduling scenarios with multiple host destination calendars.packages/core/builders/CalendarEvent/builder.ts (1)
1-2: LGTM! Import optimization applied correctly.Converting
Bookingto a type-only import while keepingPrismaas a value import is appropriate, asBookingis only used at the type level in this file. This improves tree-shaking and build efficiency.packages/types/EventManager.d.ts (1)
26-26: LGTM! EventResult extended to support external ID tracking.Adding the optional
externalIdfield enables downstream flows to track external calendar identifiers alongside credential IDs, supporting the enhanced calendar event creation and update workflows.apps/web/pages/api/cron/bookingReminder.ts (1)
107-130: LGTM! Destination calendar array construction is correct.The implementation properly:
- Derives the destination calendar from booking or user fallback (line 107)
- Wraps the selected calendar in an array when present, or uses an empty array (line 130)
This aligns with the array-based destinationCalendar architecture consistently applied across the PR.
packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
175-179: LGTM! Destination calendar resolution follows the correct pattern.The ternary chain correctly prioritizes
booking.destinationCalendar, falls back touser.destinationCalendar, and defaults to an empty array. Each branch properly wraps the calendar in an array, ensuring consistent array-based handling throughout the booking confirmation flow.packages/features/ee/payments/api/paypal-webhook.ts (1)
152-156: LGTM! Consistent destination calendar handling in payment flow.The implementation mirrors the pattern used in other booking flows, ensuring consistent array-based destinationCalendar handling across payment webhook processing. The ternary chain correctly prioritizes booking, falls back to user, and defaults to an empty array.
packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (1)
221-244: destinationCalendar wrapping preserves prior semantics under the new array typeSetting
destinationCalendarto[bookingToReschedule.destinationCalendar]when present (otherwise[]) updates the shape without changing this handler’s logic, which previously relied solely on the booking’s destination calendar. If you ever want reschedule webhooks to fall back to the owner’s default calendar when the booking’s destinationCalendar is null, this would be the place to align that behavior with other flows.packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts (1)
266-298: Cancelled‑email CalendarEvent now aligns with array-based destinationCalendarThe new
destinationCalendarexpression (booking.destinationCalendarfallback tobooking.user.destinationCalendar, wrapped in an array or[]) is consistent with other updated flows and withDestinationCalendar[]typing, while preserving the previous selection behavior.packages/types/Calendar.d.ts (1)
11-16: Types look consistent with new multi-destination model; verify all Calendar implementations were updated
CalendarEvent.destinationCalendarasDestinationCalendar[] | nulland the new optionalschedulingType?: SchedulingType | nullalign with how other files now treat destination calendars (arrays with “first element as primary”) and scheduling logic.- Updating
Calendar.createEvent(event: CalendarEvent, credentialId: number)matches the new credential-aware flow; implementations (e.g.,GoogleCalendarService) in this PR do follow it.Please just confirm all other
Calendarimplementors (e.g., Office365, Lark, genericCalendarService) have also been updated to this new signature so type-checking and runtime calls stay in sync.Also applies to: 145-190, 220-222
packages/core/EventManager.ts (1)
152-172: Reference mapping now correctly uses result.externalId/credentialIdThe updated
referencesToCreatemapping:externalCalendarId: isCalendarType ? result.externalId : undefined, credentialId: isCalendarType ? result.credentialId : undefined,aligns with the new flow where calendar services return both the target external calendar ID and the credential used. This is the right direction for multi-calendar references and should make downstream updates/deletes much more reliable.
packages/features/bookings/lib/handleNewBooking.ts (4)
1-3: New imports and eventType fetching support destination calendars cleanlyIntroducing
DestinationCalendarin the imports and includingdestinationCalendaringetEventTypesFromDB’s select keeps Prisma types and the higher-levelCalendarEventin sync. This is the right foundation for pushing destination calendar info through booking creation and into the event payload.Also applies to: 233-281
358-384: Using original booking duration when rescheduling is a sensible enhancementThe
ensureAvailableUserstweak:const originalBookingDuration = input.originalRescheduledBooking ? dayjs(input.originalRescheduledBooking.endTime).diff( dayjs(input.originalRescheduledBooking.startTime), "minutes" ) : undefined; ... duration: originalBookingDuration,makes reschedule-availability checks respect the actual duration of the original booking, instead of always using
eventType.length. AssuminggetUserAvailabilitytreatsdurationas optional, this is a good behavioral improvement and won’t affect non-reschedule flows (whereoriginalRescheduledBookingis undefined).
688-706: Team-event detection via schedulingType is clearer and more explicitconst isTeamEventType = !!eventType.schedulingType && ["COLLECTIVE", "ROUND_ROBIN"].includes(eventType.schedulingType);Basing
isTeamEventTypeonschedulingTypebeingCOLLECTIVEorROUND_ROBINmakes the intent explicit and lines up with later checks that branch oneventType.schedulingType. This should be easier to reason about than implicit host/team heuristics.
1040-1075: CalendarEvent enrichment with schedulingType and destinationCalendar is consistent with downstream usageThe constructed
evt:
- Includes
destinationCalendaras an array (ornull) and- Sets
schedulingType: eventType.schedulingType.This ensures downstream services (
EventManager, calendar services, webhooks) have enough context to:
- Decide whether to treat this as a team/collective booking.
- Resolve destination calendars in a way that’s compatible with the new array-based types.
Given the rest of the PR, this shape looks correct.
| const bookingCalendarReference = bookingToDelete.references.filter((reference) => | ||
| reference.type.includes("_calendar") | ||
| ); | ||
|
|
||
| if (bookingCalendarReference) { | ||
| const { credentialId, uid, externalCalendarId } = bookingCalendarReference; | ||
| // If the booking calendar reference contains a credentialId | ||
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| const calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| const promises = bookingToDelete.user.credentials | ||
| .filter((credential) => credential.type.endsWith("_calendar")) | ||
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); | ||
| if (bookingRef) { | ||
| const { uid, externalCalendarId } = bookingRef; | ||
| const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId); | ||
| apiDeletes.push(deletedEvent); | ||
| if (bookingCalendarReference.length > 0) { | ||
| for (const reference of bookingCalendarReference) { | ||
| const { credentialId, uid, externalCalendarId } = reference; | ||
| // If the booking calendar reference contains a credentialId | ||
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| let calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (!calendarCredential) { | ||
| // get credential from DB | ||
| const foundCalendarCredential = await prisma.credential.findUnique({ | ||
| where: { | ||
| id: credentialId, | ||
| }, | ||
| }); | ||
| if (foundCalendarCredential) { | ||
| calendarCredential = foundCalendarCredential; | ||
| } | ||
| } | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| const promises = bookingToDelete.user.credentials | ||
| .filter((credential) => credential.type.endsWith("_calendar")) | ||
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); | ||
| if (bookingRef) { | ||
| const { uid, externalCalendarId } = bookingRef; | ||
| const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId); | ||
| apiDeletes.push(deletedEvent); | ||
| } | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| } else { | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } else { | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behavior of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behaviour of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } |
There was a problem hiding this comment.
Recurring cancellations only delete the first calendar reference per booking
In the allRemainingBookings recurring-cancellation path:
const promises = bookingToDelete.user.credentials
.filter((credential) => credential.type.endsWith("_calendar"))
.map(async (credential) => {
const calendar = await getCalendar(credential);
for (const updBooking of updatedBookings) {
const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
if (bookingRef) {
const { uid, externalCalendarId } = bookingRef;
const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
apiDeletes.push(deletedEvent);
}
}
});Issues:
updBooking.references.find(...)returns only the first_calendarreference per booking. With the new multi-destination model, a single booking can have multiple_calendarreferences (one per host/calendar), so additional events are never deleted.- Because this block runs inside the outer
for (const reference of bookingCalendarReference), you may also end up scheduling the same deletes multiple times.
You likely want to iterate all calendar references for each updated booking and delete each one:
- for (const updBooking of updatedBookings) {
- const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
- if (bookingRef) {
- const { uid, externalCalendarId } = bookingRef;
- const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
- apiDeletes.push(deletedEvent);
- }
- }
+ for (const updBooking of updatedBookings) {
+ const calendarRefs = updBooking.references.filter((ref) => ref.type.includes("_calendar"));
+ for (const { uid, externalCalendarId } of calendarRefs) {
+ const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
+ apiDeletes.push(deletedEvent);
+ }
+ }You can then consider moving this “delete all calendar refs for updatedBookings” logic outside the outer for (const reference of bookingCalendarReference) loop to avoid repeating the same work per reference.
Right now, recurring cancellations in multi-calendar scenarios will leave some provider events undeleted.
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | ||
| metadata: { | ||
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}), |
There was a problem hiding this comment.
Critical logic reversal breaks organization slug assignment.
Line 151 inverts the original condition from !IS_TEAM_BILLING_ENABLED to IS_TEAM_BILLING_ENABLED. This completely reverses when the slug is assigned:
Previous behavior:
- Billing disabled → slug set directly
- Billing enabled → slug stored as
requestedSlugin metadata (pending approval)
Current behavior:
- Billing disabled → slug not set at all (organizations will have no slug!)
- Billing enabled → slug set directly AND as
requestedSlug
This breaks organization creation in non-billing environments, as organizations will be created without a slug, breaking URL generation and routing.
Apply this diff to restore the original logic:
create: {
name,
- ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}),
+ ...(!IS_TEAM_BILLING_ENABLED && { slug }),
metadata: {
- ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}),
+ ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }),
isOrganization: true,
isOrganizationVerified: false,
isOrganizationConfigured,📝 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.
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | |
| metadata: { | |
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), | |
| ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}), | |
| create: { | |
| name, | |
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | |
| metadata: { | |
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), | |
| isOrganization: true, | |
| isOrganizationVerified: false, | |
| isOrganizationConfigured, |
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/organizations/create.handler.ts around
lines 151 to 153, the boolean condition was inverted so slug is set only when
IS_TEAM_BILLING_ENABLED is true; revert the logic so that when billing is
disabled (!IS_TEAM_BILLING_ENABLED) the top-level slug is assigned, and when
billing is enabled (IS_TEAM_BILLING_ENABLED) the slug is stored under
metadata.requestedSlug instead; update the two ternary expressions accordingly
to restore previous behavior (slug: !(IS_TEAM_BILLING_ENABLED) ? { slug } : {}
and metadata: { ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}) }).
🧩 Analysis chain
File appears unrelated to PR objectives.
This PR is titled "fix: handle collective multiple host on destinationCalendar" and the AI summary describes changes to destination calendar array handling. However, this file only contains organization creation logic with no destination calendar references.
The changes here modify organization slug assignment based on billing flags, which appears completely unrelated to destination calendar handling for collective scheduling.
Please verify whether these changes were intentionally included in this PR or if they were introduced accidentally during a merge/rebase.
🏁 Script executed:
#!/bin/bash
# Verify if this file has any destination calendar references
rg -n "destinationCalendar" packages/trpc/server/routers/viewer/organizations/create.handler.ts
# Check git history to see if these changes were meant for a different PR
git log --oneline --follow -10 packages/trpc/server/routers/viewer/organizations/create.handler.tsLength of output: 662
I need to see the actual changes made to this file in the PR to better understand if they're related:
Let me check the actual changes made to this file in the PR:
Let me examine the actual git diff to see what was changed in this file:
This will help me see if there are any destination calendar references in the file context around those lines.
Let me also search for usage of `requestedSlug` and IS_TEAM_BILLING_ENABLED in the codebase to understand their purpose:
[/run_scripts]
The scripts require the repository to be cloned. Let me check what information is actually needed:
Actually, let me verify whether this file should even be part of the destination calendar fix by checking the specific git diff:
Based on the evidence gathered so far:
1. **No destination calendar references found** - The `rg` search returned zero results for "destinationCalendar" in this file
2. **File is part of the PR commit** - Git history shows it was modified in commit bd797c1e "fix: handle collective multiple host on destinationCalendar (#10967)"
3. **Web search shows slug affects event type routing** - but not directly destination calendar handling
The original review comment's suspicion appears valid: this file contains changes to organization slug assignment based on team billing flags, which appears tangential to the stated PR objective of handling collective multiple hosts on destination calendars.
**Organization slug assignment is independent of destination calendar logic.** The slug is a URL identifier for organizations; destination calendars are where calendar events get synced. These are separate concerns.
---
**This file appears unrelated to the PR's stated objective of fixing collective multiple host destination calendar handling.**
The changes modify organization slug assignment based on `IS_TEAM_BILLING_ENABLED`, which affects the organization's URL identifier. This is unrelated to destination calendar array handling for collective scheduling. The file contains no destination calendar logic or references.
Verify whether these changes were intentionally bundled with the destination calendar fix or if they should be addressed in a separate PR focused on billing/organization setup.
---
🏁 Script executed:
```shell
#!/bin/bash
# Show the exact changes made to this file in the PR commit
git show bd797c1e:packages/trpc/server/routers/viewer/organizations/create.handler.ts | head -160 | tail -20
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Search for how requestedSlug is used in destination calendar logic
rg -n "requestedSlug" --type ts -C 3 -g '!**/*.test.ts' | head -50
# Check if IS_TEAM_BILLING_ENABLED affects destination calendar
rg -n "IS_TEAM_BILLING_ENABLED" --type ts -C 2 | head -50Length of output: 8926
🏁 Script executed:
#!/bin/bash
# Show the exact changes made to this file in the PR commit
git log --oneline -1 --format="%H" | xargs -I {} git show {}:packages/trpc/server/routers/viewer/organizations/create.handler.ts 2>/dev/null | sed -n '145,160p'Length of output: 464
🏁 Script executed:
#!/bin/bash
# Get the full diff for this specific file from the PR commit
git show bd797c1e -- packages/trpc/server/routers/viewer/organizations/create.handler.ts 2>/dev/null | head -100Length of output: 1097
Let me check if there are any tests or documentation explaining when this slug logic should apply:
This will help me understand whether the logic inversion was intentional or if it introduces a bug.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for tests related to organization creation and IS_TEAM_BILLING_ENABLED
fd -e spec.ts -e test.ts | xargs rg -l "create.*organization\|organization.*create" 2>/dev/null | head -5
# Look for comments around the changed lines explaining the logic
git show bd797c1e:packages/trpc/server/routers/viewer/organizations/create.handler.ts | sed -n '140,160p'
# Check if there's any prior version to understand the change
git show bd797c1e^:packages/trpc/server/routers/viewer/organizations/create.handler.ts 2>/dev/null | sed -n '140,160p' || echo "Previous version not available"
Length of output: 1264
Critical issue: Logic inversion on line 151 that changes when organization slugs are assigned.
The code inverts the condition for when the organization slug field is set:
- Before:
...(!IS_TEAM_BILLING_ENABLED && { slug })→ slug assigned when billing disabled - After:
...(IS_TEAM_BILLING_ENABLED ? { slug } : {})→ slug assigned when billing enabled
This is a behavioral change that contradicts how teams handle slug assignment. When team billing is enabled, teams only set requestedSlug in metadata (not the slug field directly), following a two-step publishing workflow. Organizations should be consistent.
Additionally, this change is unrelated to the PR's stated objective of fixing collective multiple host destination calendar handling. The file contains no destination calendar logic. Verify whether this logic inversion is intentional or a bug introduced during commit.
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/organizations/create.handler.ts around
lines 151-153, the ternary condition for assigning slug/metadata is inverted:
change the logic so that slug is set only when team billing is disabled and when
team billing is enabled only set metadata.requestedSlug (i.e., revert to using
!IS_TEAM_BILLING_ENABLED for the direct slug assignment and use
IS_TEAM_BILLING_ENABLED to populate requestedSlug in metadata); ensure the code
matches the original two-step publishing workflow for teams and double-check
this change is intentional relative to the PR.
2 participants
Test 4
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Replicated from ai-code-review-evaluation/cal.com-coderabbit#4