From 0ad5770bb9e3ef96e989b798356839eee5590962 Mon Sep 17 00:00:00 2001 From: Hariom Balhara Date: Thu, 27 Feb 2025 23:43:36 +0530 Subject: [PATCH] fix: Valid timeslot marked 'slot no longer available' (#19590) * Handle date mismatch b/w schedule key and slot * Add feature ollout support --- .env.example | 4 +- packages/features/bookings/Booker/Booker.tsx | 18 +-- ...eIsQuickAvailabilityCheckFeatureEnabled.ts | 17 +++ .../Booker/components/hooks/useSlots.ts | 7 ++ .../utils/isFeatureEnabledForVisitor.test.ts | 104 ++++++++++++++++++ .../utils/isFeatureEnabledForVisitor.ts | 26 +++++ .../Booker/utils/isTimeslotAvailable.test.ts | 68 ++++++++++++ .../Booker/utils/isTimeslotAvailable.ts | 85 +++++++++++--- packages/lib/constants.ts | 4 + packages/lib/cookie.ts | 9 ++ turbo.json | 1 + 11 files changed, 320 insertions(+), 23 deletions(-) create mode 100644 packages/features/bookings/Booker/components/hooks/useIsQuickAvailabilityCheckFeatureEnabled.ts create mode 100644 packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.test.ts create mode 100644 packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.ts create mode 100644 packages/lib/cookie.ts diff --git a/.env.example b/.env.example index c92bd759fc..89e0c3eeee 100644 --- a/.env.example +++ b/.env.example @@ -421,4 +421,6 @@ NEXT_PUBLIC_QUERY_RESERVATION_STALE_TIME_SECONDS= # Query available slots interval - Should be kept high in minutes as it could cause significant load on the system NEXT_PUBLIC_QUERY_AVAILABLE_SLOTS_INTERVAL_SECONDS= # Used to invalidate available slots when navigating to booking form -NEXT_PUBLIC_INVALIDATE_AVAILABLE_SLOTS_ON_BOOKING_FORM=0 \ No newline at end of file +NEXT_PUBLIC_INVALIDATE_AVAILABLE_SLOTS_ON_BOOKING_FORM=0 +# Used to enable quick availability checks for x% of all visitors +NEXT_PUBLIC_QUICK_AVAILABILITY_ROLLOUT=10 \ No newline at end of file diff --git a/packages/features/bookings/Booker/Booker.tsx b/packages/features/bookings/Booker/Booker.tsx index b5c4398542..e1f6f9ce43 100644 --- a/packages/features/bookings/Booker/Booker.tsx +++ b/packages/features/bookings/Booker/Booker.tsx @@ -31,6 +31,7 @@ import { OverlayCalendar } from "./components/OverlayCalendar/OverlayCalendar"; import { RedirectToInstantMeetingModal } from "./components/RedirectToInstantMeetingModal"; import { BookerSection } from "./components/Section"; import { NotFound } from "./components/Unavailable"; +import { useIsQuickAvailabilityCheckFeatureEnabled } from "./components/hooks/useIsQuickAvailabilityCheckFeatureEnabled"; import { fadeInLeft, getBookerSizeClassNames, useBookerResizeAnimation } from "./config"; import { useBookerStore } from "./store"; import type { BookerProps, WrappedBookerProps } from "./types"; @@ -129,6 +130,7 @@ const BookerComponent = ({ const animationScope = useBookerResizeAnimation(layout, bookerState); const timeslotsRef = useRef(null); + const isQuickAvailabilityCheckFeatureEnabled = useIsQuickAvailabilityCheckFeatureEnabled(); const StickyOnDesktop = isMobile ? "div" : StickyBox; const { bookerFormErrorRef, key, formEmail, bookingForm, errors: formErrors } = bookerForm; @@ -189,13 +191,15 @@ const BookerComponent = ({ return setBookerState("booking"); }, [event, selectedDate, selectedTimeslot, setBookerState, skipConfirmStep, layout, isInstantMeeting]); - const unavailableTimeSlots = allSelectedTimeslots.filter((slot) => { - return !isTimeSlotAvailable({ - scheduleData: schedule?.data ?? null, - slotToCheckInIso: slot, - quickAvailabilityChecks: slots.quickAvailabilityChecks, - }); - }); + const unavailableTimeSlots = isQuickAvailabilityCheckFeatureEnabled + ? allSelectedTimeslots.filter((slot) => { + return !isTimeSlotAvailable({ + scheduleData: schedule?.data ?? null, + slotToCheckInIso: slot, + quickAvailabilityChecks: slots.quickAvailabilityChecks, + }); + }) + : []; const slot = getQueryParam("slot"); diff --git a/packages/features/bookings/Booker/components/hooks/useIsQuickAvailabilityCheckFeatureEnabled.ts b/packages/features/bookings/Booker/components/hooks/useIsQuickAvailabilityCheckFeatureEnabled.ts new file mode 100644 index 0000000000..7f52c9cf2f --- /dev/null +++ b/packages/features/bookings/Booker/components/hooks/useIsQuickAvailabilityCheckFeatureEnabled.ts @@ -0,0 +1,17 @@ +import { useEffect, useRef } from "react"; + +import { PUBLIC_QUICK_AVAILABILITY_ROLLOUT } from "@calcom/lib/constants"; + +import { isVisitorWithinPercentage } from "../../utils/isFeatureEnabledForVisitor"; + +export const useIsQuickAvailabilityCheckFeatureEnabled = () => { + const isQuickAvailabilityCheckFeatureEnabledRef = useRef( + isVisitorWithinPercentage({ percentage: PUBLIC_QUICK_AVAILABILITY_ROLLOUT }) + ); + + useEffect(() => { + console.log("QuickAvailabilityCheck feature enabled:", isQuickAvailabilityCheckFeatureEnabledRef.current); + }, []); + + return isQuickAvailabilityCheckFeatureEnabledRef.current; +}; diff --git a/packages/features/bookings/Booker/components/hooks/useSlots.ts b/packages/features/bookings/Booker/components/hooks/useSlots.ts index d9e5726114..72bc891be2 100644 --- a/packages/features/bookings/Booker/components/hooks/useSlots.ts +++ b/packages/features/bookings/Booker/components/hooks/useSlots.ts @@ -15,6 +15,8 @@ import { useCompatSearchParams } from "@calcom/lib/hooks/useCompatSearchParams"; import { trpc } from "@calcom/trpc"; import type { TIsAvailableOutputSchema } from "@calcom/trpc/server/routers/viewer/slots/isAvailable.schema"; +import { useIsQuickAvailabilityCheckFeatureEnabled } from "./useIsQuickAvailabilityCheckFeatureEnabled"; + export type QuickAvailabilityCheck = TIsAvailableOutputSchema["slots"][number]; const useQuickAvailabilityChecks = ({ @@ -31,6 +33,11 @@ const useQuickAvailabilityChecks = ({ // Maintain a cache to ensure previous state is maintained as the request is fetched // It is important because tentatively selecting a new timeslot will cause a new request which is uncached. const cachedQuickAvailabilityChecksRef = useRef([]); + const isQuickAvailabilityCheckFeatureEnabled = useIsQuickAvailabilityCheckFeatureEnabled(); + + if (!isQuickAvailabilityCheckFeatureEnabled) { + return cachedQuickAvailabilityChecksRef.current; + } // Create array of slots with their UTC start and end dates const slotsToCheck = timeslotsAsISOString.map((slot) => { diff --git a/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.test.ts b/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.test.ts new file mode 100644 index 0000000000..1d13289c20 --- /dev/null +++ b/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.test.ts @@ -0,0 +1,104 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { getCookie } from "@calcom/lib/cookie"; + +import { isVisitorWithinPercentage } from "./isFeatureEnabledForVisitor"; + +const generateRandomUuid = () => { + return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function (c) { + const r = (Math.random() * 16) | 0; + const v = c === "x" ? r : (r & 0x3) | 0x8; + return v.toString(16); + }); +}; + +const mockGetCookie = ({ expectedUid }: { expectedUid: string }) => + //@ts-expect-error - mock implementation + getCookie.mockImplementation((name) => { + if (name === "uid") { + return expectedUid; + } + return null; + }); + +vi.mock("@calcom/lib/cookie", () => ({ + getCookie: vi.fn(), +})); + +describe("isVisitorWithinPercentage", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + // Test for deterministic behavior + it("should provide consistent results for the same visitorId", () => { + const testUuids = [ + "f956595b-0a69-4167-b9c9-a15d6ac445bb", + "550e8400-e29b-41d4-a716-446655440000", + "6ba7b810-9dad-11d1-80b4-00c04fd430c8", + "6ba7b811-9dad-11d1-80b4-00c04fd430c8", + "6ba7b812-9dad-11d1-80b4-00c04fd430c8", + ]; + + // For each UUID, check results are consistent across multiple calls + testUuids.forEach((uuid) => { + mockGetCookie({ expectedUid: uuid }); + const result1 = isVisitorWithinPercentage({ percentage: 50 }); + const result2 = isVisitorWithinPercentage({ percentage: 50 }); + const result3 = isVisitorWithinPercentage({ percentage: 50 }); + + // All results should be the same for the same UUID + expect(result1).toBe(result2); + expect(result2).toBe(result3); + }); + }); + + // Test for correct percentage distribution + it("should approximately match the configured percentage", () => { + // Generate a large number of random UUIDs to test distribution + + const sampleSize = 10000; + const testPercentages = [10, 25, 50, 75, 90]; + + testPercentages.forEach((percentage) => { + let enabledCount = 0; + + // Run test with many random UUIDs + for (let i = 0; i < sampleSize; i++) { + const uuid = generateRandomUuid(); + mockGetCookie({ expectedUid: uuid }); + if (isVisitorWithinPercentage({ percentage })) { + enabledCount++; + } + } + + // Calculate the actual percentage + const actualPercentage = (enabledCount / sampleSize) * 100; + // We allow a 3% margin of error given the sample size + expect(actualPercentage).toBeGreaterThanOrEqual(percentage - 3); + expect(actualPercentage).toBeLessThanOrEqual(percentage + 3); + }); + }); + + // Test for fallback to random behavior when no UUID is provided + it("should not enable feature when UUID is notavailable", () => { + mockGetCookie({ expectedUid: undefined }); + expect(isVisitorWithinPercentage({ percentage: 50 })).toBe(false); + }); + + it("should disable the feature when percentage is 0", () => { + const uuids = Array.from({ length: 100 }, () => generateRandomUuid()); + uuids.forEach((uuid) => { + mockGetCookie({ expectedUid: uuid }); + expect(isVisitorWithinPercentage({ percentage: 0 })).toBe(false); + }); + }); + + it("should enable the feature when percentage is 100", () => { + const uuids = Array.from({ length: 100 }, () => generateRandomUuid()); + uuids.forEach((uuid) => { + mockGetCookie({ expectedUid: uuid }); + expect(isVisitorWithinPercentage({ percentage: 100 })).toBe(true); + }); + }); +}); diff --git a/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.ts b/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.ts new file mode 100644 index 0000000000..88a04582f4 --- /dev/null +++ b/packages/features/bookings/Booker/utils/isFeatureEnabledForVisitor.ts @@ -0,0 +1,26 @@ +import { getCookie } from "@calcom/lib/cookie"; + +/** + * NOTE: Feature can be easily rolled back by setting percentage to 0 + */ +export const isVisitorWithinPercentage = ({ percentage }: { percentage: number }) => { + // E2E tests must test all features regardless of their rollout percentage + if (process.env.NEXT_PUBLIC_IS_E2E) { + return true; + } + // TODO: The cookie is currently set when a timeslot is selected but we plan to create it on visitor's visit itself + // Current purpose is to identify the visitor who reserved a timeslot but could be used for feature rollout and other UX improvements(like if duplicate booking is attempted by the same person) + const visitorId = getCookie("uid"); + if (!visitorId) { + return false; + } + + // Use the visitor UUID to deterministically generate a number between 0-100 + // We'll use the last 4 characters of the UUID as they should be sufficiently random + const lastFourChars = visitorId.slice(-4); + // Convert hex to decimal and take modulo 100 to get a number between 0-99 + const deterministicNumber = parseInt(lastFourChars, 16) % 100; + + // Enable if this number falls within the percentage + return deterministicNumber < percentage; +}; diff --git a/packages/features/bookings/Booker/utils/isTimeslotAvailable.test.ts b/packages/features/bookings/Booker/utils/isTimeslotAvailable.test.ts index 2b142b4d1d..4d68f981de 100644 --- a/packages/features/bookings/Booker/utils/isTimeslotAvailable.test.ts +++ b/packages/features/bookings/Booker/utils/isTimeslotAvailable.test.ts @@ -57,6 +57,74 @@ describe("isTimeSlotAvailable", () => { expect(result).toBe(false); }); + describe("Different day due to timezone", () => { + it("should correctly lookup on appropriate date when the day in which to show the slot is before the date in the ISO string", () => { + const slotToCheckInIso1 = "2024-02-08T10:30:00.000Z"; + const dateToShowSlot1 = "2024-02-07"; + const quickAvailabilityChecks1: QuickAvailabilityCheck[] = []; + + const result1 = isTimeSlotAvailable({ + scheduleData: { + slots: { + [dateToShowSlot1]: [{ time: slotToCheckInIso1 }], + }, + }, + slotToCheckInIso: slotToCheckInIso1, + quickAvailabilityChecks: quickAvailabilityChecks1, + }); + + expect(result1).toBe(true); + + const slotToCheckInIso2 = "2024-02-01T10:30:00.000Z"; + const dateToShowSlot2 = "2024-01-31"; + const quickAvailabilityChecks2: QuickAvailabilityCheck[] = []; + const result2 = isTimeSlotAvailable({ + scheduleData: { + slots: { + [dateToShowSlot2]: [{ time: slotToCheckInIso2 }], + }, + }, + slotToCheckInIso: slotToCheckInIso2, + quickAvailabilityChecks: quickAvailabilityChecks2, + }); + + expect(result2).toBe(true); + }); + + it("should correctly lookup on appropriate date when the day in which to show the slot is after the date in the ISO string", () => { + const slotToCheckInIso1 = "2024-02-08T10:30:00.000Z"; + const dateToShowSlot1 = "2024-02-09"; + const quickAvailabilityChecks1: QuickAvailabilityCheck[] = []; + + const result1 = isTimeSlotAvailable({ + scheduleData: { + slots: { + [dateToShowSlot1]: [{ time: slotToCheckInIso1 }], + }, + }, + slotToCheckInIso: slotToCheckInIso1, + quickAvailabilityChecks: quickAvailabilityChecks1, + }); + + expect(result1).toBe(true); + + const slotToCheckInIso2 = "2024-02-01T10:30:00.000Z"; + const dateToShowSlot2 = "2024-01-31"; + const quickAvailabilityChecks2: QuickAvailabilityCheck[] = []; + const result2 = isTimeSlotAvailable({ + scheduleData: { + slots: { + [dateToShowSlot2]: [{ time: slotToCheckInIso2 }], + }, + }, + slotToCheckInIso: slotToCheckInIso2, + quickAvailabilityChecks: quickAvailabilityChecks2, + }); + + expect(result2).toBe(true); + }); + }); + it("should return true when the slot exists in the schedule data", () => { const slotToCheckInIso = "2024-02-08T10:30:00.000Z"; const quickAvailabilityChecks: QuickAvailabilityCheck[] = []; diff --git a/packages/features/bookings/Booker/utils/isTimeslotAvailable.ts b/packages/features/bookings/Booker/utils/isTimeslotAvailable.ts index 2bc7ec9e1f..2687278792 100644 --- a/packages/features/bookings/Booker/utils/isTimeslotAvailable.ts +++ b/packages/features/bookings/Booker/utils/isTimeslotAvailable.ts @@ -1,6 +1,68 @@ +import dayjs from "@calcom/dayjs"; + import type { QuickAvailabilityCheck } from "../components/hooks/useSlots"; import { isSlotEquivalent, isValidISOFormat } from "./isSlotEquivalent"; +type Maybe = T | undefined; + +// Format is YYYY-MM-DD +type DateInBookerTimeZone = string; + +// Format is YYYY-MM-DDTHH:mm:ssZ +type SlotInIsoFormat = string; +type SlotsInIso = { time: SlotInIsoFormat }[]; +type ScheduleData = { + /** + * `slots` is a map of date in Booker's timezone to an array of time slots in ISO format + * So, `DateInBookerTimeZone` could be on a different day number than the slot date in ISO format + * For example, if Booker's timezone is UTC+5:30, and timeslot is 2025-01-02T21:00:00Z, then `DateInBookerTimeZone` could be 2025-01-03 as it is 02:30 AM next day in IST + */ + slots: Record; +}; + +function _isSlotPresent(slotsInIsoForDate: Maybe, slotToCheckInIso: SlotInIsoFormat) { + if (!slotsInIsoForDate) return false; + // Check if the exact time slot exists in the available slots + return slotsInIsoForDate.some((slot) => { + return isSlotEquivalent({ slotTimeInIso: slot.time, slotToCheckInIso }); + }); +} + +function _isSlotPresentInSchedule({ + scheduleData, + dateInGMT, + slotToCheckInIso, +}: { + scheduleData: ScheduleData; + /** + * It is just the date in format YYYY-MM-DD + */ + dateInGMT: string; + slotToCheckInIso: SlotInIsoFormat; +}) { + // Check if the slot is present under the date, previous date and next date + // Timezones can't be more than 1 day apart, so we can safely assume that previous and next day covers all the dates where a time slot can be present + // We don't want to look up for a slot over all the dates in the schedule data unnecessarily for performance reasons + const dateBefore = dayjs(dateInGMT).subtract(1, "day").format("YYYY-MM-DD"); + const dateAfter = dayjs(dateInGMT).add(1, "day").format("YYYY-MM-DD"); + + // No matter what timezone the booker is in, the slot has to be in one of these three dates + const slotsInIsoForDate = scheduleData.slots[dateInGMT] as Maybe; + const slotsInIsoForDateBefore = scheduleData.slots[dateBefore] as Maybe; + const slotsInIsoForDateAfter = scheduleData.slots[dateAfter] as Maybe; + + const matchFoundOnDate = _isSlotPresent(slotsInIsoForDate, slotToCheckInIso); + if (matchFoundOnDate) return true; + + const matchFoundOnDateBefore = _isSlotPresent(slotsInIsoForDateBefore, slotToCheckInIso); + if (matchFoundOnDateBefore) return true; + + const matchFoundOnDateAfter = _isSlotPresent(slotsInIsoForDateAfter, slotToCheckInIso); + if (matchFoundOnDateAfter) return true; + + return false; +} + /** * Checks if a given time slot is available in the schedule * It should never give false negative, false positives are fine. @@ -12,10 +74,8 @@ export const isTimeSlotAvailable = ({ slotToCheckInIso, quickAvailabilityChecks, }: { - scheduleData: { - slots: Record; - } | null; - slotToCheckInIso: string; + scheduleData: ScheduleData | null; + slotToCheckInIso: SlotInIsoFormat; quickAvailabilityChecks: QuickAvailabilityCheck[]; }) => { const isUnavailableAsPerQuickCheck = @@ -31,19 +91,14 @@ export const isTimeSlotAvailable = ({ return true; } - const dateInIso = isValidISOFormat(slotToCheckInIso) ? slotToCheckInIso.split("T")[0] : null; + const dateInGMT = isValidISOFormat(slotToCheckInIso) ? slotToCheckInIso.split("T")[0] : null; // If the date is not in ISO format, we could errorneously consider the slot unavailable, so be on the safe side and consider it available // Though this could be a false positive, it's better to consider the slot available than unavailable - if (!dateInIso) return true; + if (!dateInGMT) return true; - // Get the slots for this date - const slotsForDateInIso = scheduleData.slots[dateInIso]; - if (!slotsForDateInIso) return false; - - // Check if the exact time slot exists in the available slots - const matchFound = slotsForDateInIso.some((slot) => { - return isSlotEquivalent({ slotTimeInIso: slot.time, slotToCheckInIso }); + return _isSlotPresentInSchedule({ + scheduleData, + dateInGMT, + slotToCheckInIso, }); - - return matchFound; }; diff --git a/packages/lib/constants.ts b/packages/lib/constants.ts index 198bcb1377..484183069d 100644 --- a/packages/lib/constants.ts +++ b/packages/lib/constants.ts @@ -76,6 +76,10 @@ export const PUBLIC_QUERY_AVAILABLE_SLOTS_INTERVAL_SECONDS = export const PUBLIC_INVALIDATE_AVAILABLE_SLOTS_ON_BOOKING_FORM = process.env.NEXT_PUBLIC_INVALIDATE_AVAILABLE_SLOTS_ON_BOOKING_FORM === "1"; +// Enables a feature for x% of all visitors. Takes a number between 0 and 100. +export const PUBLIC_QUICK_AVAILABILITY_ROLLOUT = + parseInt(process.env.NEXT_PUBLIC_QUICK_AVAILABILITY_ROLLOUT ?? "", 10) || 0; + export const CURRENT_TIMEZONE = dayjs.tz.guess() !== "Etc/Unknown" ? dayjs.tz.guess() : "Europe/London"; /** @deprecated use `WEBAPP_URL` */ diff --git a/packages/lib/cookie.ts b/packages/lib/cookie.ts new file mode 100644 index 0000000000..454a0192e0 --- /dev/null +++ b/packages/lib/cookie.ts @@ -0,0 +1,9 @@ +export const getCookie = (name: string) => { + if (typeof document === "undefined") { + return null; + } + return document.cookie + .split(";") + .find((cookie) => cookie.trim().startsWith(`${name}=`)) + ?.split("=")[1]; +}; diff --git a/turbo.json b/turbo.json index 4c966a7a72..84a6e02446 100644 --- a/turbo.json +++ b/turbo.json @@ -447,6 +447,7 @@ "NEXT_PUBLIC_QUERY_RESERVATION_INTERVAL_SECONDS", "NEXT_PUBLIC_QUERY_RESERVATION_STALE_TIME_SECONDS", "NEXT_PUBLIC_INVALIDATE_AVAILABLE_SLOTS_ON_BOOKING_FORM", + "NEXT_PUBLIC_QUICK_AVAILABILITY_ROLLOUT", "NEXT_PUBLIC_HEAD_SCRIPTS", "NEXT_PUBLIC_BODY_SCRIPTS", "NEXT_PUBLIC_ORG_SELF_SERVE_ENABLED",