Skip to content
Jump to bottom

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

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

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

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: [],
// 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> {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Search for createEvent invocations
rg -n "\.createEvent\(" --type=ts -C2 -g '!**/node_modules/**'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 472

🏁 Script executed:

# Check the actual signature at line 87 in CalendarService.ts
cat -n packages/app-store/googlecalendar/lib/CalendarService.ts | sed -n '85,95p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 573

🏁 Script executed:

# Search for all createEvent method definitions
rg -n "createEvent\s*\(" --type=ts -A1 -g '!**/node_modules/**' | grep -E "(async|createEvent)"

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 3606

🏁 Script executed:

# Check the Calendar interface/type definition
cat -n packages/types/Calendar.d.ts | sed -n '215,230p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 666

🏁 Script executed:

# Check if CalendarService implementations extend a base class
rg -n "extends.*Calendar|implements.*Calendar" --type=ts packages/app-store/googlecalendar/lib/CalendarService.ts packages/app-store/office365calendar/lib/CalendarService.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 345

🏁 Script executed:

# Check CalendarManager to see how it instantiates and calls createEvent
cat -n packages/core/CalendarManager.ts | sed -n '230,245p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 841

🏁 Script executed:

# Check which other implementations also declare implements Calendar
rg -n "export.*class.*implements Calendar" --type=ts packages/app-store/

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1708

🏁 Script executed:

# Check a few of the other implementations to see their actual signature
for file in packages/app-store/office365calendar/lib/CalendarService.ts packages/app-store/larkcalendar/lib/CalendarService.ts; do
  echo "=== $file ==="
  sed -n '57,60p;125,130p' "$file" 2>/dev/null || sed -n '50,70p;120,135p' "$file" 2>/dev/null
done

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1094

🏁 Script executed:

# Check EventManager.ts to see if those createEvent references are calls to the Calendar interface method
cat -n packages/core/EventManager.ts | sed -n '365,405p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1785

🏁 Script executed:

# Verify the createEvent function in EventManager (likely a helper function, not the Calendar method)
rg -n "^(async\s+)?function\s+createEvent|^(const|let)\s+createEvent\s*=" packages/core/EventManager.ts -B2 -A5

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 72

🏁 Script executed:

# Search for createEvent function definition in EventManager
rg -n "const createEvent|function createEvent" packages/core/EventManager.ts -A10

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 72

🏁 Script executed:

# Check imports at the top of EventManager.ts
head -50 packages/core/EventManager.ts | cat -n

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 2538

🏁 Script executed:

# Check the createEvent function in CalendarManager.ts
rg -n "^export.*function createEvent|^export const createEvent" packages/core/CalendarManager.ts -A30

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1358

🏁 Script executed:

# Or search more broadly for the createEvent export
cat -n packages/core/CalendarManager.ts | sed -n '1,50p'

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 2345

🏁 Script executed:

# Check if there are TypeScript compilation errors by looking for type errors
# First, let's verify if the other implementations are being called through the Calendar interface
cat -n packages/app-store/_utils/getCalendar.ts | head -50

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1972

🏁 Script executed:

# Check which calendar implementation is loaded by getCalendar
cat -n packages/app-store/_utils/getCalendar.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 1972

Update all calendar provider implementations to match the interface signature with credentialId parameter.

The Calendar interface in packages/types/Calendar.d.ts requires createEvent(event: CalendarEvent, credentialId: number), and GoogleCalendarService correctly implements this. However, 12+ other calendar implementations still have the old signature without credentialId: office365calendar, larkcalendar, zoho-bigin, zohocrm, sendgrid, salesforce, exchangecalendar, exchange2013, exchange2016, hubspot, closecom, and basecamp3. These violate the interface contract and must be updated to include the credentialId parameter.

🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` at line 87,
Multiple calendar provider classes still implement createEvent without the
required credentialId parameter; update each provider's createEvent signature to
match the Calendar interface: createEvent(event: CalendarEvent, credentialId:
number): Promise<NewCalendarEventType>, e.g. in classes/modules for
office365calendar, larkcalendar, zoho-bigin, zohocrm, sendgrid, salesforce,
exchangecalendar, exchange2013, exchange2016, hubspot, closecom, and basecamp3;
then propagate the new parameter into internal calls and any callers/tests
(adjust parameter passing and typings) so implementations and type definitions
align with the Calendar interface.

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",
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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's see the context around lines 254-256 in the current file
head -300 packages/app-store/googlecalendar/lib/CalendarService.ts | tail -100

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 3311

🏁 Script executed:

# Check the git history for the old implementation
git log --oneline -20 -- packages/app-store/googlecalendar/lib/CalendarService.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 168

🏁 Script executed:

# Search for calls to updateEvent to see if externalCalendarId is always provided
rg "updateEvent" --type ts -B 2 -A 2 packages/

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 11261

🏁 Script executed:

# Get the commit that introduced this change
git show bd797c1ea --stat | head -50

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 3015

🏁 Script executed:

# Look at lines 315-317 (deleteEvent) to compare
sed -n '310,325p' packages/app-store/googlecalendar/lib/CalendarService.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 599

🏁 Script executed:

# Check if there's a fallback to "primary" in deleteEvent
sed -n '315,330p' packages/app-store/googlecalendar/lib/CalendarService.ts

Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror

Length of output: 689

Bug: tautological fallback — find will never match when externalCalendarId is falsy.

When externalCalendarId is falsy (the else branch), the code searches for cal.externalId === externalCalendarId, which is effectively cal.externalId === undefined — this will never match a real calendar. As a result, selectedCalendar will be undefined, and calendar.events.update at line 261 receives calendarId: undefined.

The intended logic was likely to fall back to the main host's destination calendar (destructured at line 100-103), similar to the pattern in createEvent at line 154. Use mainHostDestinationCalendar?.externalId || "primary" as the fallback instead.

Note: deleteEvent has an identical bug at lines 315-317 but is partially protected by the fallback at line 322 (calendarId ? calendarId : defaultCalendarId). updateEvent has no such safety net.

🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 254 -
256, The selectedCalendar fallback uses event.destinationCalendar?.find(cal =>
cal.externalId === externalCalendarId) which never matches when
externalCalendarId is falsy; update the logic in the update flow (symbol:
selectedCalendar) to fall back to mainHostDestinationCalendar?.externalId ||
"primary" instead of searching for a matching undefined id, and apply the same
fix to the identical fallback in deleteEvent (and ensure calendar.events.update
/ calendarId is passed a defined value); reference createEvent which already
uses mainHostDestinationCalendar for the intended pattern.


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 fallback bug as updateEvent.

Identical issue: when externalCalendarId is falsy, find((cal) => cal.externalId === externalCalendarId) will never match. However, in deleteEvent the impact is mitigated by the fallback at Line 322 (calendarId ? calendarId : defaultCalendarId), which falls back to "primary".

Still, the find clause is dead code and should be corrected for clarity and correctness.

Proposed fix 
       const calendarId = externalCalendarId
         ? externalCalendarId
-        : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+        : event.destinationCalendar?.find((cal) => cal.credentialId)?.externalId;
📝 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 calendarId = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
const calendarId = externalCalendarId
? externalCalendarId
: event.destinationCalendar?.find((cal) => cal.credentialId)?.externalId;
🤖 Prompt for AI Agents 
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 315 -
317, The current assignment for calendarId in deleteEvent uses a tautological
find (find(cal => cal.externalId === externalCalendarId)) which is dead code
when externalCalendarId is falsy; change the fallback to select a valid
externalId from event.destinationCalendar (e.g.,
event.destinationCalendar?.[0]?.externalId or another appropriate selector) so
calendarId = externalCalendarId ? externalCalendarId :
event.destinationCalendar?.[0]?.externalId; ensure you update the code
referencing calendarId (the variable named calendarId in the deleteEvent logic)
so the fallback returns an actual externalId instead of using the impossible
equality check.


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