From 2408337c784d03c11cf6911a01d25154276d1436 Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Thu, 5 Dec 2024 20:00:22 +0530 Subject: [PATCH] fix: Don't force reschedule with same RR host if reschedule is actually rerouting (#17511) * Dont force reschedule with same RR host if routed there * Add unit tests --- apps/web/test/lib/getSchedule.test.ts | 283 +++++++++++++++++- apps/web/test/lib/getSchedule/expects.ts | 26 ++ .../utils/bookingScenario/bookingScenario.ts | 3 + .../features/bookings/lib/handleNewBooking.ts | 12 +- .../handleNewBooking/test/reschedule.test.ts | 159 ++++++++++ packages/lib/isOutOfBounds.tsx | 4 +- .../trpc/server/routers/viewer/slots/util.ts | 31 +- 7 files changed, 499 insertions(+), 19 deletions(-) diff --git a/apps/web/test/lib/getSchedule.test.ts b/apps/web/test/lib/getSchedule.test.ts index 7ba0fd7295..e283882809 100644 --- a/apps/web/test/lib/getSchedule.test.ts +++ b/apps/web/test/lib/getSchedule.test.ts @@ -19,7 +19,7 @@ import dayjs from "@calcom/dayjs"; import { SchedulingType, type BookingStatus } from "@calcom/prisma/enums"; import { getAvailableSlots as getSchedule } from "@calcom/trpc/server/routers/viewer/slots/util"; -import { expect } from "./getSchedule/expects"; +import { expect, expectedSlotsForSchedule } from "./getSchedule/expects"; import { setupAndTeardown } from "./getSchedule/setupAndTeardown"; import { timeTravelToTheBeginningOfToday } from "./getSchedule/utils"; @@ -89,7 +89,8 @@ describe("getSchedule", () => { }); }); - describe("Round robin lead skip - CRM", async () => { + // TODO: Move these inside describe('Team Event') + describe("Round robin lead skip(i.e. use contact owner specified by teamMemberEmail) - CRM", async () => { test("correctly get slots for event with only round robin hosts", async () => { vi.setSystemTime("2024-05-21T00:00:13Z"); @@ -367,7 +368,7 @@ describe("getSchedule", () => { } ); }); - test("correctly get slots for event with only round robin hosts", async () => { + test("correctly get slots for event with only round robin hosts - When no availability is found", async () => { vi.setSystemTime("2024-05-21T00:00:13Z"); const plus1DateString = "2024-05-22"; @@ -496,6 +497,119 @@ describe("getSchedule", () => { } ); }); + test("Negative case(i.e. skipContactOwner is true) - when teamMemberEmail is provided but not used", async () => { + vi.setSystemTime("2024-05-21T00:00:13Z"); + + const plus1DateString = "2024-05-22"; + const plus2DateString = "2024-05-23"; + + const crmCredential = { + id: 1, + type: "salesforce_crm", + key: { + clientId: "test-client-id", + }, + userId: 1, + teamId: null, + appId: "salesforce", + invalid: false, + user: { email: "test@test.com" }, + }; + + await createCredentials([crmCredential]); + + mockCrmApp("salesforce", { + getContacts: [ + { + id: "contact-id", + email: "test@test.com", + ownerEmail: "example@example.com", + }, + ], + createContacts: [{ id: "contact-id", email: "test@test.com" }], + }); + + await createBookingScenario({ + eventTypes: [ + { + id: 1, + slotInterval: 60, + length: 60, + hosts: [ + { + userId: 101, + isFixed: false, + }, + { + userId: 102, + isFixed: false, + }, + ], + schedulingType: "ROUND_ROBIN", + metadata: { + apps: { + salesforce: { + enabled: true, + appCategories: ["crm"], + roundRobinLeadSkip: true, + }, + }, + }, + }, + ], + users: [ + { + ...TestData.users.example, + email: "example@example.com", + id: 101, + schedules: [TestData.schedules.IstEveningShift], + }, + { + ...TestData.users.example, + email: "example1@example.com", + id: 102, + schedules: [TestData.schedules.IstMorningShift], + defaultScheduleId: 2, + }, + ], + bookings: [], + }); + + const scheduleWhenContactOwnerIsSkipped = await getSchedule({ + input: { + eventTypeId: 1, + eventTypeSlug: "", + startTime: `${plus1DateString}T18:30:00.000Z`, + endTime: `${plus2DateString}T18:29:59.999Z`, + timeZone: Timezones["+5:30"], + isTeamEvent: true, + teamMemberEmail: "example@example.com", + skipContactOwner: true, + orgSlug: null, + }, + }); + + // Both users slot would be available as contact owner(example@example.com) is skipped and we fallback to all users of RR + expect(scheduleWhenContactOwnerIsSkipped).toHaveTimeSlots( + [ + `04:30:00.000Z`, + `05:30:00.000Z`, + `06:30:00.000Z`, + `07:30:00.000Z`, + `08:30:00.000Z`, + `09:30:00.000Z`, + `10:30:00.000Z`, + `11:30:00.000Z`, + `12:30:00.000Z`, + `13:30:00.000Z`, + `14:30:00.000Z`, + `15:30:00.000Z`, + ], + { + dateString: plus2DateString, + } + ); + }); }); describe("User Event", () => { @@ -2163,5 +2277,168 @@ describe("getSchedule", () => { } ); }); + + test("Reschedule: should reschedule with same host if rescheduleWithSameRoundRobinHost is true", async () => { + vi.setSystemTime("2024-05-21T00:00:13Z"); + + const plus1DateString = "2024-05-22"; + const plus2DateString = "2024-05-23"; + + // An event with common schedule + await createBookingScenario({ + eventTypes: [ + { + id: 1, + slotInterval: 60, + length: 60, + rescheduleWithSameRoundRobinHost: true, + hosts: [ + { + userId: 101, + isFixed: false, + }, + { + userId: 102, + isFixed: false, + }, + ], + schedulingType: "ROUND_ROBIN", + }, + ], + users: [ + { + ...TestData.users.example, + email: "example@example.com", + id: 101, + schedules: [TestData.schedules.IstEveningShift], + defaultScheduleId: 1, + }, + { + ...TestData.users.example, + email: "example1@example.com", + id: 102, + schedules: [TestData.schedules.IstMorningShift], + defaultScheduleId: 2, + }, + ], + bookings: [ + { + uid: "BOOKING_TO_RESCHEDULE_UID", + userId: 101, + attendees: [ + { + email: "IntegrationTestUser102@example.com", + }, + ], + eventTypeId: 1, + status: "ACCEPTED", + startTime: `${plus2DateString}T04:00:00.000Z`, + endTime: `${plus2DateString}T04:15:00.000Z`, + }, + ], + }); + + const schedule = await getSchedule({ + input: { + eventTypeId: 1, + eventTypeSlug: "", + startTime: `${plus1DateString}T18:30:00.000Z`, + endTime: `${plus2DateString}T18:29:59.999Z`, + timeZone: Timezones["+5:30"], + isTeamEvent: true, + rescheduleUid: "BOOKING_TO_RESCHEDULE_UID", + }, + }); + + // expect only slots of IstEveningShift as this is the slots for the original host of the booking + expect(schedule).toHaveTimeSlots( + [`11:30:00.000Z`, `12:30:00.000Z`, `13:30:00.000Z`, `14:30:00.000Z`, `15:30:00.000Z`], + { + dateString: plus2DateString, + } + ); + }); + + test("Reschedule: should reschedule 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"; + const plus2DateString = "2024-05-23"; + + // An event with common schedule + await createBookingScenario({ + eventTypes: [ + { + id: 1, + slotInterval: 60, + length: 60, + rescheduleWithSameRoundRobinHost: true, + hosts: [ + { + userId: 101, + isFixed: false, + }, + { + userId: 102, + isFixed: false, + }, + ], + schedulingType: "ROUND_ROBIN", + }, + ], + users: [ + { + ...TestData.users.example, + email: "example@example.com", + id: 101, + schedules: [TestData.schedules.IstEveningShift], + defaultScheduleId: 1, + }, + { + ...TestData.users.example, + email: "example1@example.com", + id: 102, + schedules: [TestData.schedules.IstMorningShift], + defaultScheduleId: 2, + }, + ], + bookings: [ + { + uid: "BOOKING_TO_RESCHEDULE_UID", + userId: 101, + attendees: [ + { + email: "IntegrationTestUser102@example.com", + }, + ], + eventTypeId: 1, + status: "ACCEPTED", + startTime: `${plus2DateString}T04:00:00.000Z`, + endTime: `${plus2DateString}T04:15:00.000Z`, + }, + ], + }); + + const schedule = await getSchedule({ + input: { + eventTypeId: 1, + eventTypeSlug: "", + startTime: `${plus1DateString}T18:30:00.000Z`, + endTime: `${plus2DateString}T18:29:59.999Z`, + timeZone: Timezones["+5:30"], + isTeamEvent: true, + rescheduleUid: "BOOKING_TO_RESCHEDULE_UID", + routedTeamMemberIds: [102], + }, + }); + + // expect only slots of IstEveningShift as this is the slots for the original host of the booking + expect(schedule).toHaveTimeSlots( + expectedSlotsForSchedule.IstMorningShift.interval["1hr"].allPossibleSlotsStartingAt430, + { + dateString: plus2DateString, + } + ); + }); }); }); diff --git a/apps/web/test/lib/getSchedule/expects.ts b/apps/web/test/lib/getSchedule/expects.ts index 168564c818..8c84c99f08 100644 --- a/apps/web/test/lib/getSchedule/expects.ts +++ b/apps/web/test/lib/getSchedule/expects.ts @@ -30,6 +30,32 @@ export const expectedSlotsForSchedule = { }, }, }, + IstMorningShift: { + interval: { + "1hr": { + allPossibleSlotsStartingAt430: [ + "04:30:00.000Z", + "05:30:00.000Z", + "06:30:00.000Z", + "07:30:00.000Z", + "08:30:00.000Z", + "09:30:00.000Z", + "10:30:00.000Z", + "11:30:00.000Z", + ], + allPossibleSlotsStartingAt4: [ + "04:00:00.000Z", + "05:00:00.000Z", + "06:00:00.000Z", + "07:00:00.000Z", + "08:00:00.000Z", + "09:00:00.000Z", + "10:00:00.000Z", + "11:00:00.000Z", + ], + }, + }, + }, }; declare global { diff --git a/apps/web/test/utils/bookingScenario/bookingScenario.ts b/apps/web/test/utils/bookingScenario/bookingScenario.ts index 0b29fa3c6a..858ea012d2 100644 --- a/apps/web/test/utils/bookingScenario/bookingScenario.ts +++ b/apps/web/test/utils/bookingScenario/bookingScenario.ts @@ -1283,6 +1283,9 @@ export function getOrganizer({ export function getScenarioData( { + /** + * organizer has no special meaning. It is a regular user. It is supposed to be deprecated along with `usersApartFromOrganizer` and we should introduce a new `users` field instead + */ organizer, eventTypes, usersApartFromOrganizer = [], diff --git a/packages/features/bookings/lib/handleNewBooking.ts b/packages/features/bookings/lib/handleNewBooking.ts index 13dca85e4a..6ab08585e9 100644 --- a/packages/features/bookings/lib/handleNewBooking.ts +++ b/packages/features/bookings/lib/handleNewBooking.ts @@ -619,10 +619,16 @@ async function handler( // freeUsers is ensured const originalRescheduledBookingUserId = originalRescheduledBooking && originalRescheduledBooking.userId; - const isSameRoundRobinHost = + + const isRouting = !!routedTeamMemberIds; + const isRerouting = originalRescheduledBookingUserId && isRouting; + const shouldUseSameRRHost = !!originalRescheduledBookingUserId && eventType.schedulingType === SchedulingType.ROUND_ROBIN && - eventType.rescheduleWithSameRoundRobinHost; + 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; const userIdsSet = new Set(users.map((user) => user.id)); @@ -646,7 +652,7 @@ async function handler( }); } - const newLuckyUser = isSameRoundRobinHost + const newLuckyUser = shouldUseSameRRHost ? freeUsers.find((user) => user.id === originalRescheduledBookingUserId) : await getLuckyUser({ // find a lucky user that is not already in the luckyUsers array diff --git a/packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts b/packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts index 115be3aae5..4bd5892e51 100644 --- a/packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts +++ b/packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts @@ -2546,6 +2546,165 @@ describe("handleNewBooking", () => { }, timeout ); + + test( + "should reschedule as per routedTeamMemberIds(instead of same host) even if rescheduleWithSameRoundRobinHost is true but it is a rerouting scenario", + async ({ emails }) => { + const handleNewBooking = (await import("@calcom/features/bookings/lib/handleNewBooking")).default; + const booker = getBooker({ + email: "booker@example.com", + name: "Booker", + }); + + const otherHost = getOrganizer({ + name: "RR Host 1", + email: "rrhost1@example.com", + id: 101, + schedules: [TestData.schedules.IstWorkHours], + credentials: [getGoogleCalendarCredential()], + selectedCalendars: [TestData.selectedCalendars.google], + }); + + const hostOfOriginalBooking = getOrganizer({ + name: "RR Host 2", + email: "rrhost2@example.com", + id: 102, + schedules: [TestData.schedules.IstWorkHours], + credentials: [getGoogleCalendarCredential()], + selectedCalendars: [TestData.selectedCalendars.google], + }); + + const { dateString: plus1DateString } = getDate({ dateIncrement: 1 }); + const uidOfBookingToBeRescheduled = "n5Wv3eHgconAED2j4gcVhP"; + await createBookingScenario( + getScenarioData({ + eventTypes: [ + { + id: 1, + slotInterval: 15, + length: 15, + hosts: [ + { + userId: 101, + isFixed: false, + }, + { + userId: 102, + isFixed: false, + }, + ], + schedulingType: SchedulingType.ROUND_ROBIN, + rescheduleWithSameRoundRobinHost: true, + }, + ], + bookings: [ + { + uid: uidOfBookingToBeRescheduled, + eventTypeId: 1, + userId: 102, + status: BookingStatus.ACCEPTED, + startTime: `${plus1DateString}T05:00:00.000Z`, + endTime: `${plus1DateString}T05:15:00.000Z`, + metadata: { + videoCallUrl: "https://existing-daily-video-call-url.example.com", + }, + }, + ], + organizer: otherHost, + usersApartFromOrganizer: [hostOfOriginalBooking], + apps: [TestData.apps["google-calendar"], TestData.apps["daily-video"]], + }) + ); + + mockSuccessfulVideoMeetingCreation({ + metadataLookupKey: "dailyvideo", + }); + + mockCalendarToHaveNoBusySlots("googlecalendar", { + create: { + uid: "MOCK_ID", + }, + update: { + uid: "UPDATED_MOCK_ID", + iCalUID: "MOCKED_GOOGLE_CALENDAR_ICS_ID", + }, + }); + + const mockBookingData = getMockRequestDataForBooking({ + data: { + eventTypeId: 1, + user: otherHost.name, + rescheduleUid: uidOfBookingToBeRescheduled, + start: `${plus1DateString}T04:00:00.000Z`, + end: `${plus1DateString}T04:15:00.000Z`, + routedTeamMemberIds: [101], + responses: { + email: booker.email, + name: booker.name, + location: { optionValue: "", value: BookingLocations.CalVideo }, + }, + }, + }); + const { req } = createMockNextJsRequest({ + method: "POST", + body: mockBookingData, + }); + + const createdBooking = await handleNewBooking(req); + + const previousBooking = await prismaMock.booking.findUnique({ + where: { + uid: uidOfBookingToBeRescheduled, + }, + }); + + logger.silly({ + previousBooking, + allBookings: await prismaMock.booking.findMany(), + }); + + // Expect previous booking to be cancelled + await expectBookingToBeInDatabase({ + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + uid: uidOfBookingToBeRescheduled, + status: BookingStatus.CANCELLED, + }); + + expect(previousBooking?.status).toBe(BookingStatus.CANCELLED); + /** + * Booking Time should be new time + */ + expect(createdBooking.startTime?.toISOString()).toBe(`${plus1DateString}T04:00:00.000Z`); + expect(createdBooking.endTime?.toISOString()).toBe(`${plus1DateString}T04:15:00.000Z`); + + expect(createdBooking.userId).toBe(otherHost.id); + + await expectBookingInDBToBeRescheduledFromTo({ + from: { + uid: uidOfBookingToBeRescheduled, + }, + to: { + description: "", + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + uid: createdBooking.uid!, + eventTypeId: mockBookingData.eventTypeId, + status: BookingStatus.ACCEPTED, + location: BookingLocations.CalVideo, + responses: expect.objectContaining({ + email: booker.email, + name: booker.name, + }), + }, + }); + + expectSuccessfulRoundRobinReschedulingEmails({ + prevOrganizer: hostOfOriginalBooking, + newOrganizer: otherHost, + emails, + }); + }, + timeout + ); }); }); }); diff --git a/packages/lib/isOutOfBounds.tsx b/packages/lib/isOutOfBounds.tsx index f38b0ac42e..6f46598809 100644 --- a/packages/lib/isOutOfBounds.tsx +++ b/packages/lib/isOutOfBounds.tsx @@ -245,7 +245,7 @@ export function isTimeViolatingFutureLimit({ const dateInSystemTz = dayjs(time); if (periodLimits.endOfRollingPeriodEndDayInBookerTz) { const isAfterRollingEndDay = dateInSystemTz.isAfter(periodLimits.endOfRollingPeriodEndDayInBookerTz); - log.debug("rollingEndDayCheck", { + log.silly("rollingEndDayCheck", { formattedDate: dateInSystemTz.format(), isAfterRollingEndDay, endOfRollingPeriodEndDayInBookerTz: periodLimits.endOfRollingPeriodEndDayInBookerTz.format(), @@ -256,7 +256,7 @@ export function isTimeViolatingFutureLimit({ if (periodLimits.startOfRangeStartDayInEventTz && periodLimits.endOfRangeEndDayInEventTz) { const isBeforeRangeStart = dateInSystemTz.isBefore(periodLimits.startOfRangeStartDayInEventTz); const isAfterRangeEnd = dateInSystemTz.isAfter(periodLimits.endOfRangeEndDayInEventTz); - log.debug("rangeCheck", { + log.silly("rangeCheck", { formattedDate: dateInSystemTz.format(), isAfterRangeEnd, isBeforeRangeStart, diff --git a/packages/trpc/server/routers/viewer/slots/util.ts b/packages/trpc/server/routers/viewer/slots/util.ts index 2d5f888316..8f77c9175f 100644 --- a/packages/trpc/server/routers/viewer/slots/util.ts +++ b/packages/trpc/server/routers/viewer/slots/util.ts @@ -502,7 +502,7 @@ async function _getAvailableSlots({ input, ctx }: GetScheduleOptions): Promise>, null>; - routedHostsWithContactOwnerAndFixedHosts: { + hosts: { isFixed?: boolean; user: GetAvailabilityUser; }[]; - contactOwnerEmail?: string | null; + contactOwnerEmail: string | null; loggerWithEventDetails: Logger; startTime: ReturnType; endTime: Dayjs; bypassBusyCalendarTimes: boolean; }) => { + const routedTeamMemberIds = input.routedTeamMemberIds ?? null; + const isRouting = !!routedTeamMemberIds; + const isRerouting = input.rescheduleUid && isRouting; if ( input.rescheduleUid && eventType.rescheduleWithSameRoundRobinHost && - eventType.schedulingType === SchedulingType.ROUND_ROBIN + 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 ) { const originalRescheduledBooking = await prisma.booking.findFirst({ where: { @@ -1046,14 +1052,12 @@ const calculateHostsAndAvailabilities = async ({ userId: true, }, }); - routedHostsWithContactOwnerAndFixedHosts = routedHostsWithContactOwnerAndFixedHosts.filter( - (host) => host.user.id === originalRescheduledBooking?.userId || 0 - ); + hosts = hosts.filter((host) => host.user.id === originalRescheduledBooking?.userId || 0); } const usersWithCredentials = monitorCallbackSync(getUsersWithCredentialsConsideringContactOwner, { contactOwnerEmail, - hosts: routedHostsWithContactOwnerAndFixedHosts, + hosts, }); loggerWithEventDetails.debug("Using users", { @@ -1174,5 +1178,10 @@ const calculateHostsAndAvailabilities = async ({ eventType.schedulingType ); - return { aggregatedAvailability, allUsersAvailability, usersWithCredentials, currentSeats }; + return { + aggregatedAvailability, + allUsersAvailability, + usersWithCredentials, + currentSeats, + }; };