From 17ab088af837679bf73965fd00add64c38c1158a Mon Sep 17 00:00:00 2001 From: Joe Au-Yeung <65426560+joeauyeung@users.noreply.github.com> Date: Wed, 15 Oct 2025 07:20:25 -0400 Subject: [PATCH] fix: Faulty logic around `shouldServeCache` (#24465) * Should return `false` if no `teamId` is passed to `getShouldServeCache` * Change to falsey check * test: Add comprehensive tests for shouldServeCache fix - Add tests for CacheService.getShouldServeCache to verify it returns false when no teamId is provided (instead of undefined) - Add tests for GoogleCalendarService.getFreeBusyResult to verify the falsey check properly handles undefined, null, 0, empty string, and false values - All tests verify the fix in PR #24465 which changes undefined return to false and changes === false check to !shouldServeCache Co-Authored-By: joe@cal.com * fix: Resolve type errors in test files - Fix private credential property access by using type assertion - Add missing IFeaturesRepository methods to mock Co-Authored-By: joe@cal.com --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Alex van Andel --- .../googlecalendar/lib/CalendarService.ts | 2 +- .../lib/__tests__/getFreeBusyResult.test.ts | 135 ++++++++++++++++++ .../lib/getShouldServeCache.test.ts | 92 ++++++++++++ .../calendar-cache/lib/getShouldServeCache.ts | 2 +- 4 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts create mode 100644 packages/features/calendar-cache/lib/getShouldServeCache.test.ts diff --git a/packages/app-store/googlecalendar/lib/CalendarService.ts b/packages/app-store/googlecalendar/lib/CalendarService.ts index ef3e9c57e2..13c331ecf1 100644 --- a/packages/app-store/googlecalendar/lib/CalendarService.ts +++ b/packages/app-store/googlecalendar/lib/CalendarService.ts @@ -466,7 +466,7 @@ export default class GoogleCalendarService implements Calendar { args: FreeBusyArgs, shouldServeCache?: boolean ): Promise { - if (shouldServeCache === false) return await this.fetchAvailability(args); + if (!shouldServeCache) return await this.fetchAvailability(args); const calendarCache = await CalendarCache.init(null); const cached = await calendarCache.getCachedAvailability({ credentialId: this.credential.id, diff --git a/packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts b/packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts new file mode 100644 index 0000000000..64dfea74c9 --- /dev/null +++ b/packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts @@ -0,0 +1,135 @@ +import { expect, test, beforeEach, vi, describe } from "vitest"; +import "vitest-fetch-mock"; + +import CalendarService from "../CalendarService"; + +describe("GoogleCalendarService.getFreeBusyResult - shouldServeCache logic", () => { + let calendarService: CalendarService; + let fetchAvailabilitySpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + + calendarService = {} as CalendarService; + + const mockFetchAvailability = vi.fn().mockResolvedValue({ + calendars: { + "test@example.com": { + busy: [ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ], + }, + }, + }); + + calendarService.fetchAvailability = mockFetchAvailability; + fetchAvailabilitySpy = mockFetchAvailability; + + calendarService.getFreeBusyResult = CalendarService.prototype.getFreeBusyResult.bind(calendarService); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (calendarService as any).credential = { id: 1, userId: 1 }; + }); + + describe("shouldServeCache parameter handling", () => { + test("should call fetchAvailability immediately when shouldServeCache is explicitly false", async () => { + const args = { + timeMin: new Date().toISOString(), + timeMax: new Date().toISOString(), + items: [{ id: "test@example.com" }], + }; + + const result = await calendarService.getFreeBusyResult(args, false); + + expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); + expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); + expect(result.calendars?.["test@example.com"]?.busy).toEqual([ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ]); + }); + + test("should call fetchAvailability immediately when shouldServeCache is undefined (falsey)", async () => { + const args = { + timeMin: new Date().toISOString(), + timeMax: new Date().toISOString(), + items: [{ id: "test@example.com" }], + }; + + const result = await calendarService.getFreeBusyResult(args, undefined); + + expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); + expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); + expect(result.calendars?.["test@example.com"]?.busy).toEqual([ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ]); + }); + + test("should call fetchAvailability immediately when shouldServeCache is null (falsey)", async () => { + const args = { + timeMin: new Date().toISOString(), + timeMax: new Date().toISOString(), + items: [{ id: "test@example.com" }], + }; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await calendarService.getFreeBusyResult(args, null as any); + + expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); + expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); + expect(result.calendars?.["test@example.com"]?.busy).toEqual([ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ]); + }); + + test("should call fetchAvailability immediately when shouldServeCache is 0 (falsey)", async () => { + const args = { + timeMin: new Date().toISOString(), + timeMax: new Date().toISOString(), + items: [{ id: "test@example.com" }], + }; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await calendarService.getFreeBusyResult(args, 0 as any); + + expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); + expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); + expect(result.calendars?.["test@example.com"]?.busy).toEqual([ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ]); + }); + + test("should call fetchAvailability immediately when shouldServeCache is empty string (falsey)", async () => { + const args = { + timeMin: new Date().toISOString(), + timeMax: new Date().toISOString(), + items: [{ id: "test@example.com" }], + }; + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await calendarService.getFreeBusyResult(args, "" as any); + + expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args); + expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1); + expect(result.calendars?.["test@example.com"]?.busy).toEqual([ + { + start: "2023-12-01T20:00:00Z", + end: "2023-12-01T21:00:00Z", + }, + ]); + }); + }); +}); diff --git a/packages/features/calendar-cache/lib/getShouldServeCache.test.ts b/packages/features/calendar-cache/lib/getShouldServeCache.test.ts new file mode 100644 index 0000000000..4d2d844f17 --- /dev/null +++ b/packages/features/calendar-cache/lib/getShouldServeCache.test.ts @@ -0,0 +1,92 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import type { IFeaturesRepository } from "@calcom/features/flags/features.repository.interface"; + +import { CacheService } from "./getShouldServeCache"; + +describe("CacheService.getShouldServeCache", () => { + const mockFeaturesRepository: IFeaturesRepository = { + checkIfTeamHasFeature: vi.fn(), + checkIfFeatureIsEnabledGlobally: vi.fn(), + checkIfUserHasFeature: vi.fn(), + }; + + const cacheService = new CacheService({ featuresRepository: mockFeaturesRepository }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe("when shouldServeCache is explicitly set to boolean", () => { + it("should return true when shouldServeCache is true", async () => { + const result = await cacheService.getShouldServeCache(true, 123); + expect(result).toBe(true); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + + it("should return false when shouldServeCache is false", async () => { + const result = await cacheService.getShouldServeCache(false, 123); + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + }); + + describe("when shouldServeCache is undefined", () => { + it("should return false when no teamId is provided", async () => { + const result = await cacheService.getShouldServeCache(undefined, undefined); + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + + it("should return false when teamId is null", async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const result = await cacheService.getShouldServeCache(undefined, null as any); + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + + it("should return false when teamId is 0", async () => { + const result = await cacheService.getShouldServeCache(undefined, 0); + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + + it("should check feature repository when teamId is provided and return true if feature is enabled", async () => { + vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true); + + const result = await cacheService.getShouldServeCache(undefined, 123); + + expect(result).toBe(true); + expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(123, "calendar-cache-serve"); + }); + + it("should check feature repository when teamId is provided and return false if feature is disabled", async () => { + vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(false); + + const result = await cacheService.getShouldServeCache(undefined, 456); + + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(456, "calendar-cache-serve"); + }); + }); + + describe("edge cases", () => { + it("should prioritize explicit shouldServeCache over teamId check", async () => { + vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true); + + const result = await cacheService.getShouldServeCache(false, 123); + + expect(result).toBe(false); + expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled(); + }); + + it("should handle positive teamId correctly", async () => { + vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true); + + const result = await cacheService.getShouldServeCache(undefined, 999); + + expect(result).toBe(true); + expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(999, "calendar-cache-serve"); + }); + }); +}); diff --git a/packages/features/calendar-cache/lib/getShouldServeCache.ts b/packages/features/calendar-cache/lib/getShouldServeCache.ts index d445d5e039..73d4e86687 100644 --- a/packages/features/calendar-cache/lib/getShouldServeCache.ts +++ b/packages/features/calendar-cache/lib/getShouldServeCache.ts @@ -9,7 +9,7 @@ export class CacheService { async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) { if (typeof shouldServeCache === "boolean") return shouldServeCache; - if (!teamId) return undefined; + if (!teamId) return false; return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, "calendar-cache-serve"); } }