From 6ce9defecf3f553f4ef362e505d15b244c745468 Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Sat, 7 Dec 2024 08:06:41 +0530 Subject: [PATCH] fix: Do not consider salesforce ownership when making rerouting decision (#18034) --- apps/web/test/lib/getSchedule.test.ts | 4 +- .../features/bookings/lib/handleNewBooking.ts | 18 ++++-- packages/lib/bookings/routing/utils.test.ts | 58 +++++++++++++++++++ packages/lib/bookings/routing/utils.ts | 27 +++++++++ .../trpc/server/routers/viewer/slots/util.ts | 15 +++-- 5 files changed, 111 insertions(+), 11 deletions(-) create mode 100644 packages/lib/bookings/routing/utils.test.ts create mode 100644 packages/lib/bookings/routing/utils.ts diff --git a/apps/web/test/lib/getSchedule.test.ts b/apps/web/test/lib/getSchedule.test.ts index e283882809..5c1debce81 100644 --- a/apps/web/test/lib/getSchedule.test.ts +++ b/apps/web/test/lib/getSchedule.test.ts @@ -2278,7 +2278,7 @@ describe("getSchedule", () => { ); }); - test("Reschedule: should reschedule with same host if rescheduleWithSameRoundRobinHost is true", async () => { + test("Reschedule: should show timeslots of the same host if rescheduleWithSameRoundRobinHost is true", async () => { vi.setSystemTime("2024-05-21T00:00:13Z"); const plus1DateString = "2024-05-22"; @@ -2359,7 +2359,7 @@ describe("getSchedule", () => { ); }); - test("Reschedule: should reschedule as per routedTeamMemberIds(instead of same host) even if rescheduleWithSameRoundRobinHost is true but it is a rerouting scenario", async () => { + test("Reschedule: should show timeslots as per routedTeamMemberIds(instead of same host) even if rescheduleWithSameRoundRobinHost is true but it is a rerouting scenario", async () => { vi.setSystemTime("2024-05-21T00:00:13Z"); const plus1DateString = "2024-05-22"; diff --git a/packages/features/bookings/lib/handleNewBooking.ts b/packages/features/bookings/lib/handleNewBooking.ts index 6ab08585e9..3ffa25123e 100644 --- a/packages/features/bookings/lib/handleNewBooking.ts +++ b/packages/features/bookings/lib/handleNewBooking.ts @@ -46,6 +46,7 @@ import { scheduleTrigger, } from "@calcom/features/webhooks/lib/scheduleTrigger"; import { getVideoCallUrlFromCalEvent } from "@calcom/lib/CalEventParser"; +import { isRerouting, shouldIgnoreContactOwner } from "@calcom/lib/bookings/routing/utils"; import { getDefaultEvent, getUsernameList } from "@calcom/lib/defaultEvents"; import { ErrorCode } from "@calcom/lib/errorCodes"; import { getErrorFromUnknown } from "@calcom/lib/errors"; @@ -464,7 +465,18 @@ async function handler( }); const contactOwnerFromReq = reqBody.teamMemberEmail ?? null; - const skipContactOwner = reqBody.skipContactOwner ?? false; + + const isReroutingCase = isRerouting({ + rescheduleUid: reqBody.rescheduleUid ?? null, + routedTeamMemberIds: routedTeamMemberIds ?? null, + }); + + const skipContactOwner = shouldIgnoreContactOwner({ + skipContactOwner: reqBody.skipContactOwner ?? null, + rescheduleUid: reqBody.rescheduleUid ?? null, + routedTeamMemberIds: routedTeamMemberIds ?? null, + }); + const contactOwnerEmail = skipContactOwner ? null : contactOwnerFromReq; const allHostUsers = await loadAndValidateUsers({ @@ -620,15 +632,13 @@ async function handler( const originalRescheduledBookingUserId = originalRescheduledBooking && originalRescheduledBooking.userId; - const isRouting = !!routedTeamMemberIds; - const isRerouting = originalRescheduledBookingUserId && isRouting; const shouldUseSameRRHost = !!originalRescheduledBookingUserId && eventType.schedulingType === SchedulingType.ROUND_ROBIN && eventType.rescheduleWithSameRoundRobinHost && // If it is rerouting, we should not force reschedule with same host. // It will be unexpected plus could cause unavailable slots as original host might not be part of routedTeamMemberIds - !isRerouting; + !isReroutingCase; const userIdsSet = new Set(users.map((user) => user.id)); diff --git a/packages/lib/bookings/routing/utils.test.ts b/packages/lib/bookings/routing/utils.test.ts new file mode 100644 index 0000000000..61a95faddf --- /dev/null +++ b/packages/lib/bookings/routing/utils.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; + +import { isRerouting, shouldIgnoreContactOwner } from "./utils"; + +describe("isRerouting", () => { + it("should return true when both rescheduleUid and routedTeamMemberIds are present", () => { + const result = isRerouting({ + rescheduleUid: "123", + routedTeamMemberIds: [1, 2], + }); + expect(result).toBe(true); + }); + + it("should return false when rescheduleUid is null", () => { + const result = isRerouting({ + rescheduleUid: null, + routedTeamMemberIds: [1, 2], + }); + expect(result).toBe(false); + }); + + it("should return false when routedTeamMemberIds is null", () => { + const result = isRerouting({ + rescheduleUid: "123", + routedTeamMemberIds: null, + }); + expect(result).toBe(false); + }); +}); + +describe("shouldIgnoreContactOwner", () => { + it("should return true when skipContactOwner is true", () => { + const result = shouldIgnoreContactOwner({ + skipContactOwner: true, + rescheduleUid: null, + routedTeamMemberIds: null, + }); + expect(result).toBe(true); + }); + + it("should return true when rerouting", () => { + const result = shouldIgnoreContactOwner({ + skipContactOwner: false, + rescheduleUid: "123", + routedTeamMemberIds: [1, 2], + }); + expect(result).toBe(true); + }); + + it("should return false when skipContactOwner is false and not rerouting", () => { + const result = shouldIgnoreContactOwner({ + skipContactOwner: false, + rescheduleUid: null, + routedTeamMemberIds: null, + }); + expect(result).toBe(false); + }); +}); diff --git a/packages/lib/bookings/routing/utils.ts b/packages/lib/bookings/routing/utils.ts new file mode 100644 index 0000000000..5eacc5594b --- /dev/null +++ b/packages/lib/bookings/routing/utils.ts @@ -0,0 +1,27 @@ +/** + * It returns true only if there is actually some team members to route to at the moment. + * But I think we should also consider the case where no routedTeamMemberIds is provided or it is null which is when we want to consider all the assigned members of the team event for rerouting. + * So, it could be better to just read cal.rerouting query param instead of routedTeamMemberIds. + */ +export function isRerouting({ + rescheduleUid, + routedTeamMemberIds, +}: { + rescheduleUid: string | null; + routedTeamMemberIds: number[] | null; +}) { + return !!rescheduleUid && !!routedTeamMemberIds; +} + +export function shouldIgnoreContactOwner({ + skipContactOwner, + rescheduleUid, + routedTeamMemberIds, +}: { + skipContactOwner: boolean | null; + rescheduleUid: string | null; + routedTeamMemberIds: number[] | null; +}) { + // During rerouting, we don't want to consider salesforce ownership as it could potentially choose a member that isn't part of routedTeamMemberIds and thus ending up with no available timeslots. + return skipContactOwner || isRerouting({ rescheduleUid, routedTeamMemberIds }); +} diff --git a/packages/trpc/server/routers/viewer/slots/util.ts b/packages/trpc/server/routers/viewer/slots/util.ts index 8f77c9175f..200162f6db 100644 --- a/packages/trpc/server/routers/viewer/slots/util.ts +++ b/packages/trpc/server/routers/viewer/slots/util.ts @@ -18,6 +18,7 @@ import { getRoutedHostsWithContactOwnerAndFixedHosts, findMatchingHostsWithEventSegment, } from "@calcom/lib/bookings/getRoutedUsers"; +import { shouldIgnoreContactOwner, isRerouting } from "@calcom/lib/bookings/routing/utils"; import { RESERVED_SUBDOMAINS } from "@calcom/lib/constants"; import { getUTCOffsetByTimezone } from "@calcom/lib/date-fns"; import { getDefaultEvent } from "@calcom/lib/defaultEvents"; @@ -489,9 +490,15 @@ async function _getAvailableSlots({ input, ctx }: GetScheduleOptions): Promise { const routedTeamMemberIds = input.routedTeamMemberIds ?? null; - const isRouting = !!routedTeamMemberIds; - const isRerouting = input.rescheduleUid && isRouting; if ( input.rescheduleUid && eventType.rescheduleWithSameRoundRobinHost && eventType.schedulingType === SchedulingType.ROUND_ROBIN && // If it is rerouting, we should not force reschedule with same host. // It will be unexpected plus could cause unavailable slots as original host might not be part of routedTeamMemberIds - !isRerouting + !isRerouting({ rescheduleUid: input.rescheduleUid, routedTeamMemberIds }) ) { const originalRescheduledBooking = await prisma.booking.findFirst({ where: {