Skip to content
Jump to bottom

Automated Test: fix/handle-collective-multiple-host-destinations #359

Closed
wants to merge 1 commit into from
+368 −216
Closed

Automated Test: fix/handle-collective-multiple-host-destinations #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/web/pages/api/cron/bookingReminder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
});

const attendeesList = await Promise.all(attendeesListPromises);

const selectedDestinationCalendar = booking.destinationCalendar || user.destinationCalendar;
const evt: CalendarEvent = {
type: booking.title,
title: booking.title,
Expand All @@ -127,7 +127,7 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
attendees: attendeesList,
uid: booking.uid,
recurringEvent: parseRecurringEvent(booking.eventType?.recurringEvent),
destinationCalendar: booking.destinationCalendar || user.destinationCalendar,
destinationCalendar: selectedDestinationCalendar ? [selectedDestinationCalendar] : [],
};

await sendOrganizerRequestReminderEmail(evt);
Expand Down
2 changes: 1 addition & 1 deletion apps/web/playwright/webhook.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ test.describe("BOOKING_REJECTED", async () => {
},
],
location: "[redacted/dynamic]",
destinationCalendar: null,
destinationCalendar: [],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the webhook.e2e.ts file at the mentioned lines
cat -n apps/web/playwright/webhook.e2e.ts | head -400 | tail -160

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 6096

🏁 Script executed:

# View the beginning of the webhook.e2e.ts file to find BOOKING_CREATED test
head -200 apps/web/playwright/webhook.e2e.ts | tail -100

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 3196

🏁 Script executed:

# Find handlers related to bookings
fd -t f 'confirm.handler' --glob '**' | head -5
fd -t f 'handleNewBooking' --glob '**' | head -5

# Search for where destinationCalendar is set in handlers
rg -l 'destinationCalendar' --type ts --glob '**/handlers' --glob '**/api' | head -10

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 418

🏁 Script executed:

# Find files related to booking handlers
find . -type f -name '*confirm*handler*' 2>/dev/null
find . -type f -name '*handleNewBooking*' 2>/dev/null

# Search for destinationCalendar in booking-related code
rg 'destinationCalendar' --type ts -l | grep -E '(booking|handler|webhook)' | head -15

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1188

🏁 Script executed:

# Examine the confirm handler (BOOKING_REJECTED path)
rg 'destinationCalendar' packages/trpc/server/routers/viewer/bookings/confirm.handler.ts -B3 -A3

# Examine handleNewBooking (BOOKING_CREATED and BOOKING_REQUESTED paths)
rg 'destinationCalendar' packages/features/bookings/lib/handleNewBooking.ts -B3 -A3

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 2189

The handlers intentionally produce different types for destinationCalendar.

The test expectations reflect the actual handler behavior:

  • confirm.handler.ts (BOOKING_REJECTED) defaults to [] when no calendar is found
  • handleNewBooking.ts (BOOKING_CREATED/BOOKING_REQUESTED) defaults to null

While the tests correctly reflect their respective handlers, this type inconsistency across webhook events suggests a potential API contract issue. Consider whether all webhook events should produce the same shape for destinationCalendar (either all [] or all null) for consistency.

🤖 Prompt for AI Agents 
In `@apps/web/playwright/webhook.e2e.ts` at line 249, The handlers return
inconsistent types for destinationCalendar: confirm.handler.ts
(BOOKING_REJECTED) returns [] while handleNewBooking.ts
(BOOKING_CREATED/BOOKING_REQUESTED) returns null; pick a single canonical shape
(either always array or always null), update both handlers (confirm.handler.ts
and handleNewBooking.ts) to return that shape for destinationCalendar
consistently, and adjust the tests in webhook.e2e.ts to expect the unified value
for all webhook event tests (update cases for BOOKING_REJECTED, BOOKING_CREATED,
BOOKING_REQUESTED accordingly).

// hideCalendarNotes: false,
requiresConfirmation: "[redacted/dynamic]",
eventTypeId: "[redacted/dynamic]",
Expand Down
33 changes: 22 additions & 11 deletions packages/app-store/googlecalendar/lib/CalendarService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export default class GoogleCalendarService implements Calendar {
};
};

async createEvent(calEventRaw: CalendarEvent): Promise<NewCalendarEventType> {
async createEvent(calEventRaw: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> {
const eventAttendees = calEventRaw.attendees.map(({ id: _id, ...rest }) => ({
...rest,
responseStatus: "accepted",
Expand All @@ -97,6 +97,10 @@ export default class GoogleCalendarService implements Calendar {
responseStatus: "accepted",
})) || [];
return new Promise(async (resolve, reject) => {
const [mainHostDestinationCalendar] =
calEventRaw?.destinationCalendar && calEventRaw?.destinationCalendar.length > 0
? calEventRaw.destinationCalendar
: [];
const myGoogleAuth = await this.auth.getToken();
const payload: calendar_v3.Schema$Event = {
summary: calEventRaw.title,
Expand All @@ -115,8 +119,8 @@ export default class GoogleCalendarService implements Calendar {
id: String(calEventRaw.organizer.id),
responseStatus: "accepted",
organizer: true,
email: calEventRaw.destinationCalendar?.externalId
? calEventRaw.destinationCalendar.externalId
email: mainHostDestinationCalendar?.externalId
? mainHostDestinationCalendar.externalId
: calEventRaw.organizer.email,
},
...eventAttendees,
Expand All @@ -138,13 +142,16 @@ export default class GoogleCalendarService implements Calendar {
const calendar = google.calendar({
version: "v3",
});
const selectedCalendar = calEventRaw.destinationCalendar?.externalId
? calEventRaw.destinationCalendar.externalId
: "primary";
// Find in calEventRaw.destinationCalendar the one with the same credentialId

const selectedCalendar = calEventRaw.destinationCalendar?.find(
(cal) => cal.credentialId === credentialId
)?.externalId;

calendar.events.insert(
{
auth: myGoogleAuth,
calendarId: selectedCalendar,
calendarId: selectedCalendar || "primary",
Comment on lines +145 to +154
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

calendarId in the subsequent patch call (Line 168) lacks the || "primary" fallback.

Line 154 correctly falls back to "primary" when no matching credential is found, but the patch call on Line 168 uses selectedCalendar directly — which could be undefined if calEventRaw.destinationCalendar is empty/null and no match is found.

Proposed fix 
             calendar.events.patch({
               // Update the same event but this time we know the hangout link
-              calendarId: selectedCalendar,
+              calendarId: selectedCalendar || "primary",
               auth: myGoogleAuth,
🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 145 -
154, The patch call in calendar.events.patch uses selectedCalendar directly
which can be undefined; update the calendarId passed to calendar.events.patch
(in CalendarService.ts where calendar.events.patch is invoked) to use the same
fallback as the insert call by changing calendarId: selectedCalendar to
calendarId: selectedCalendar || "primary" so it defaults to "primary" when no
matching destinationCalendar entry exists.

requestBody: payload,
conferenceDataVersion: 1,
sendUpdates: "none",
Expand Down Expand Up @@ -188,6 +195,8 @@ export default class GoogleCalendarService implements Calendar {

async updateEvent(uid: string, event: CalendarEvent, externalCalendarId: string): Promise<any> {
return new Promise(async (resolve, reject) => {
const [mainHostDestinationCalendar] =
event?.destinationCalendar && event?.destinationCalendar.length > 0 ? event.destinationCalendar : [];
const myGoogleAuth = await this.auth.getToken();
const eventAttendees = event.attendees.map(({ ...rest }) => ({
...rest,
Expand Down Expand Up @@ -216,8 +225,8 @@ export default class GoogleCalendarService implements Calendar {
id: String(event.organizer.id),
organizer: true,
responseStatus: "accepted",
email: event.destinationCalendar?.externalId
? event.destinationCalendar.externalId
email: mainHostDestinationCalendar?.externalId
? mainHostDestinationCalendar.externalId
: event.organizer.email,
},
...(eventAttendees as any),
Expand All @@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {

const selectedCalendar = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.externalId;
: event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
Comment on lines 254 to +256
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Bug: tautological find — the else-branch always returns undefined.

When externalCalendarId is falsy, the code searches for cal.externalId === externalCalendarId, which compares against an undefined/null/empty value. This will never find a meaningful calendar entry, so selectedCalendar will always be undefined in the else-branch, causing the calendar.events.update call to receive undefined for calendarId.

The likely intent was to look up by credentialId or use the first destination calendar as a fallback.

Possible fix (mirror the createEvent pattern) 
-     const selectedCalendar = externalCalendarId
-       ? externalCalendarId
-       : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+     const selectedCalendar = externalCalendarId
+       ? externalCalendarId
+       : event.destinationCalendar?.[0]?.externalId || "primary";
📝 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.

Suggested change
const selectedCalendar = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.externalId;
: event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
const selectedCalendar = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.[0]?.externalId || "primary";
🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 254 -
256, selectedCalendar is being computed tautologically by comparing
cal.externalId to externalCalendarId, so when externalCalendarId is falsy the
find always returns undefined and calendar.events.update gets undefined; change
the else-branch to locate the externalId from event.destinationCalendar (e.g.,
find by credentialId or fall back to the first destinationCalendar's externalId)
— update the expression that assigns selectedCalendar (and any related logic
used later in calendar.events.update) to mirror the createEvent pattern: use
externalCalendarId if present, otherwise use event.destinationCalendar.find(cal
=> cal.credentialId === credentialId)?.externalId ??
event.destinationCalendar[0]?.externalId.


calendar.events.update(
{
Expand Down Expand Up @@ -303,7 +312,9 @@ export default class GoogleCalendarService implements Calendar {
});

const defaultCalendarId = "primary";
const calendarId = externalCalendarId ? externalCalendarId : event.destinationCalendar?.externalId;
const calendarId = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
Comment on lines +315 to +317
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same tautological find bug as in updateEvent.

Identical issue: when externalCalendarId is falsy, searching for cal.externalId === externalCalendarId is a no-op. The defaultCalendarId fallback on Line 322 mitigates the impact here (unlike in updateEvent), but the find logic is still dead code.

Proposed fix 
      const calendarId = externalCalendarId
        ? externalCalendarId
-       : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+       : event.destinationCalendar?.[0]?.externalId;
🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 315 -
317, The current assignment to calendarId uses a tautological find: when
externalCalendarId is falsy the predicate cal.externalId === externalCalendarId
can never match, making the find dead code; change the logic in CalendarService
where calendarId is computed so that if externalCalendarId is provided use it,
otherwise select a fallback by searching event.destinationCalendar for a
suitable default (e.g., first calendar or one matching another criterion) before
falling back to defaultCalendarId—update the expression that references
externalCalendarId, event.destinationCalendar?.find(...), and defaultCalendarId
to perform the find only when externalCalendarId is falsy and the list exists.


calendar.events.delete(
{
Expand Down
12 changes: 8 additions & 4 deletions packages/app-store/larkcalendar/lib/CalendarService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ export default class LarkCalendarService implements Calendar {
async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> {
let eventId = "";
let eventRespData;
const calendarId = event.destinationCalendar?.externalId;
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
const calendarId = mainHostDestinationCalendar?.externalId;
if (!calendarId) {
throw new Error("no calendar id");
}
Expand Down Expand Up @@ -160,7 +161,8 @@ export default class LarkCalendarService implements Calendar {
}

private createAttendees = async (event: CalendarEvent, eventId: string) => {
const calendarId = event.destinationCalendar?.externalId;
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
const calendarId = mainHostDestinationCalendar?.externalId;
if (!calendarId) {
this.log.error("no calendar id provided in createAttendees");
throw new Error("no calendar id provided in createAttendees");
Expand All @@ -187,7 +189,8 @@ export default class LarkCalendarService implements Calendar {
async updateEvent(uid: string, event: CalendarEvent, externalCalendarId?: string) {
const eventId = uid;
let eventRespData;
const calendarId = externalCalendarId || event.destinationCalendar?.externalId;
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
const calendarId = externalCalendarId || mainHostDestinationCalendar?.externalId;
if (!calendarId) {
this.log.error("no calendar id provided in updateEvent");
throw new Error("no calendar id provided in updateEvent");
Expand Down Expand Up @@ -231,7 +234,8 @@ export default class LarkCalendarService implements Calendar {
* @returns
*/
async deleteEvent(uid: string, event: CalendarEvent, externalCalendarId?: string) {
const calendarId = externalCalendarId || event.destinationCalendar?.externalId;
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
const calendarId = externalCalendarId || mainHostDestinationCalendar?.externalId;
if (!calendarId) {
this.log.error("no calendar id provided in deleteEvent");
throw new Error("no calendar id provided in deleteEvent");
Expand Down
5 changes: 3 additions & 2 deletions packages/app-store/office365calendar/lib/CalendarService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ export default class Office365CalendarService implements Calendar {
}

async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> {
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
try {
const eventsUrl = event.destinationCalendar?.externalId
? `/me/calendars/${event.destinationCalendar?.externalId}/events`
const eventsUrl = mainHostDestinationCalendar?.externalId
? `/me/calendars/${mainHostDestinationCalendar?.externalId}/events`
: "/me/calendar/events";

const response = await this.fetcher(eventsUrl, {
Expand Down
43 changes: 24 additions & 19 deletions packages/core/CalendarManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ export const getBusyCalendarTimes = async (

export const createEvent = async (
credential: CredentialWithAppName,
calEvent: CalendarEvent
calEvent: CalendarEvent,
externalId?: string
): Promise<EventResult<NewCalendarEventType>> => {
const uid: string = getUid(calEvent);
const calendar = await getCalendar(credential);
Expand All @@ -226,29 +227,31 @@ export const createEvent = async (

// Check if the disabledNotes flag is set to true
if (calEvent.hideCalendarNotes) {
calEvent.additionalNotes = "Notes have been hidden by the organiser"; // TODO: i18n this string?
calEvent.additionalNotes = "Notes have been hidden by the organizer"; // TODO: i18n this string?
}

// TODO: Surface success/error messages coming from apps to improve end user visibility
const creationResult = calendar
? await calendar.createEvent(calEvent).catch(async (error: { code: number; calError: string }) => {
success = false;
/**
* There is a time when selectedCalendar externalId doesn't match witch certain credential
* so google returns 404.
* */
if (error?.code === 404) {
? await calendar
.createEvent(calEvent, credential.id)
.catch(async (error: { code: number; calError: string }) => {
success = false;
/**
* There is a time when selectedCalendar externalId doesn't match witch certain credential
* so google returns 404.
* */
if (error?.code === 404) {
return undefined;
}
if (error?.calError) {
calError = error.calError;
}
log.error("createEvent failed", JSON.stringify(error), calEvent);
// @TODO: This code will be off till we can investigate an error with it
//https://github.com/calcom/cal.com/issues/3949
// await sendBrokenIntegrationEmail(calEvent, "calendar");
return undefined;
}
if (error?.calError) {
calError = error.calError;
}
log.error("createEvent failed", JSON.stringify(error), calEvent);
// @TODO: This code will be off till we can investigate an error with it
//https://github.com/calcom/cal.com/issues/3949
// await sendBrokenIntegrationEmail(calEvent, "calendar");
return undefined;
})
})
: undefined;

return {
Expand All @@ -261,6 +264,8 @@ export const createEvent = async (
originalEvent: calEvent,
calError,
calWarnings: creationResult?.additionalInfo?.calWarnings || [],
externalId,
credentialId: credential.id,
};
};

Expand Down