Skip to content

Commit 17ab088

Browse files
joeauyeungdevin-ai-integration[bot]emrysal
authored
fix: Faulty logic around shouldServeCache (calcom#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 calcom#24465 which changes undefined return to false and changes === false check to !shouldServeCache Co-Authored-By: joe@cal.com <j.auyeung419@gmail.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 <j.auyeung419@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Alex van Andel <me@alexvanandel.com>
1 parent b81a307 commit 17ab088

4 files changed

Lines changed: 229 additions & 2 deletions

File tree

‎packages/app-store/googlecalendar/lib/CalendarService.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,7 @@ export default class GoogleCalendarService implements Calendar {
466466
args: FreeBusyArgs,
467467
shouldServeCache?: boolean
468468
): Promise<calendar_v3.Schema$FreeBusyResponse> {
469-
if (shouldServeCache === false) return await this.fetchAvailability(args);
469+
if (!shouldServeCache) return await this.fetchAvailability(args);
470470
const calendarCache = await CalendarCache.init(null);
471471
const cached = await calendarCache.getCachedAvailability({
472472
credentialId: this.credential.id,

‎packages/app-store/googlecalendar/lib/__tests__/getFreeBusyResult.test.ts‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
import { expect, test, beforeEach, vi, describe } from "vitest";
2+
import "vitest-fetch-mock";
3+
4+
import CalendarService from "../CalendarService";
5+
6+
describe("GoogleCalendarService.getFreeBusyResult - shouldServeCache logic", () => {
7+
let calendarService: CalendarService;
8+
let fetchAvailabilitySpy: ReturnType<typeof vi.fn>;
9+
10+
beforeEach(() => {
11+
vi.clearAllMocks();
12+
13+
calendarService = {} as CalendarService;
14+
15+
const mockFetchAvailability = vi.fn().mockResolvedValue({
16+
calendars: {
17+
"test@example.com": {
18+
busy: [
19+
{
20+
start: "2023-12-01T20:00:00Z",
21+
end: "2023-12-01T21:00:00Z",
22+
},
23+
],
24+
},
25+
},
26+
});
27+
28+
calendarService.fetchAvailability = mockFetchAvailability;
29+
fetchAvailabilitySpy = mockFetchAvailability;
30+
31+
calendarService.getFreeBusyResult = CalendarService.prototype.getFreeBusyResult.bind(calendarService);
32+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
33+
(calendarService as any).credential = { id: 1, userId: 1 };
34+
});
35+
36+
describe("shouldServeCache parameter handling", () => {
37+
test("should call fetchAvailability immediately when shouldServeCache is explicitly false", async () => {
38+
const args = {
39+
timeMin: new Date().toISOString(),
40+
timeMax: new Date().toISOString(),
41+
items: [{ id: "test@example.com" }],
42+
};
43+
44+
const result = await calendarService.getFreeBusyResult(args, false);
45+
46+
expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args);
47+
expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1);
48+
expect(result.calendars?.["test@example.com"]?.busy).toEqual([
49+
{
50+
start: "2023-12-01T20:00:00Z",
51+
end: "2023-12-01T21:00:00Z",
52+
},
53+
]);
54+
});
55+
56+
test("should call fetchAvailability immediately when shouldServeCache is undefined (falsey)", async () => {
57+
const args = {
58+
timeMin: new Date().toISOString(),
59+
timeMax: new Date().toISOString(),
60+
items: [{ id: "test@example.com" }],
61+
};
62+
63+
const result = await calendarService.getFreeBusyResult(args, undefined);
64+
65+
expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args);
66+
expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1);
67+
expect(result.calendars?.["test@example.com"]?.busy).toEqual([
68+
{
69+
start: "2023-12-01T20:00:00Z",
70+
end: "2023-12-01T21:00:00Z",
71+
},
72+
]);
73+
});
74+
75+
test("should call fetchAvailability immediately when shouldServeCache is null (falsey)", async () => {
76+
const args = {
77+
timeMin: new Date().toISOString(),
78+
timeMax: new Date().toISOString(),
79+
items: [{ id: "test@example.com" }],
80+
};
81+
82+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
83+
const result = await calendarService.getFreeBusyResult(args, null as any);
84+
85+
expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args);
86+
expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1);
87+
expect(result.calendars?.["test@example.com"]?.busy).toEqual([
88+
{
89+
start: "2023-12-01T20:00:00Z",
90+
end: "2023-12-01T21:00:00Z",
91+
},
92+
]);
93+
});
94+
95+
test("should call fetchAvailability immediately when shouldServeCache is 0 (falsey)", async () => {
96+
const args = {
97+
timeMin: new Date().toISOString(),
98+
timeMax: new Date().toISOString(),
99+
items: [{ id: "test@example.com" }],
100+
};
101+
102+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
103+
const result = await calendarService.getFreeBusyResult(args, 0 as any);
104+
105+
expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args);
106+
expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1);
107+
expect(result.calendars?.["test@example.com"]?.busy).toEqual([
108+
{
109+
start: "2023-12-01T20:00:00Z",
110+
end: "2023-12-01T21:00:00Z",
111+
},
112+
]);
113+
});
114+
115+
test("should call fetchAvailability immediately when shouldServeCache is empty string (falsey)", async () => {
116+
const args = {
117+
timeMin: new Date().toISOString(),
118+
timeMax: new Date().toISOString(),
119+
items: [{ id: "test@example.com" }],
120+
};
121+
122+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
123+
const result = await calendarService.getFreeBusyResult(args, "" as any);
124+
125+
expect(fetchAvailabilitySpy).toHaveBeenCalledWith(args);
126+
expect(fetchAvailabilitySpy).toHaveBeenCalledTimes(1);
127+
expect(result.calendars?.["test@example.com"]?.busy).toEqual([
128+
{
129+
start: "2023-12-01T20:00:00Z",
130+
end: "2023-12-01T21:00:00Z",
131+
},
132+
]);
133+
});
134+
});
135+
});

‎packages/features/calendar-cache/lib/getShouldServeCache.test.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
3+
import type { IFeaturesRepository } from "@calcom/features/flags/features.repository.interface";
4+
5+
import { CacheService } from "./getShouldServeCache";
6+
7+
describe("CacheService.getShouldServeCache", () => {
8+
const mockFeaturesRepository: IFeaturesRepository = {
9+
checkIfTeamHasFeature: vi.fn(),
10+
checkIfFeatureIsEnabledGlobally: vi.fn(),
11+
checkIfUserHasFeature: vi.fn(),
12+
};
13+
14+
const cacheService = new CacheService({ featuresRepository: mockFeaturesRepository });
15+
16+
afterEach(() => {
17+
vi.clearAllMocks();
18+
});
19+
20+
describe("when shouldServeCache is explicitly set to boolean", () => {
21+
it("should return true when shouldServeCache is true", async () => {
22+
const result = await cacheService.getShouldServeCache(true, 123);
23+
expect(result).toBe(true);
24+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
25+
});
26+
27+
it("should return false when shouldServeCache is false", async () => {
28+
const result = await cacheService.getShouldServeCache(false, 123);
29+
expect(result).toBe(false);
30+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
31+
});
32+
});
33+
34+
describe("when shouldServeCache is undefined", () => {
35+
it("should return false when no teamId is provided", async () => {
36+
const result = await cacheService.getShouldServeCache(undefined, undefined);
37+
expect(result).toBe(false);
38+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
39+
});
40+
41+
it("should return false when teamId is null", async () => {
42+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
43+
const result = await cacheService.getShouldServeCache(undefined, null as any);
44+
expect(result).toBe(false);
45+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
46+
});
47+
48+
it("should return false when teamId is 0", async () => {
49+
const result = await cacheService.getShouldServeCache(undefined, 0);
50+
expect(result).toBe(false);
51+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
52+
});
53+
54+
it("should check feature repository when teamId is provided and return true if feature is enabled", async () => {
55+
vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true);
56+
57+
const result = await cacheService.getShouldServeCache(undefined, 123);
58+
59+
expect(result).toBe(true);
60+
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(123, "calendar-cache-serve");
61+
});
62+
63+
it("should check feature repository when teamId is provided and return false if feature is disabled", async () => {
64+
vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(false);
65+
66+
const result = await cacheService.getShouldServeCache(undefined, 456);
67+
68+
expect(result).toBe(false);
69+
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(456, "calendar-cache-serve");
70+
});
71+
});
72+
73+
describe("edge cases", () => {
74+
it("should prioritize explicit shouldServeCache over teamId check", async () => {
75+
vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true);
76+
77+
const result = await cacheService.getShouldServeCache(false, 123);
78+
79+
expect(result).toBe(false);
80+
expect(mockFeaturesRepository.checkIfTeamHasFeature).not.toHaveBeenCalled();
81+
});
82+
83+
it("should handle positive teamId correctly", async () => {
84+
vi.mocked(mockFeaturesRepository.checkIfTeamHasFeature).mockResolvedValue(true);
85+
86+
const result = await cacheService.getShouldServeCache(undefined, 999);
87+
88+
expect(result).toBe(true);
89+
expect(mockFeaturesRepository.checkIfTeamHasFeature).toHaveBeenCalledWith(999, "calendar-cache-serve");
90+
});
91+
});
92+
});

‎packages/features/calendar-cache/lib/getShouldServeCache.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export class CacheService {
99

1010
async getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) {
1111
if (typeof shouldServeCache === "boolean") return shouldServeCache;
12-
if (!teamId) return undefined;
12+
if (!teamId) return false;
1313
return await this.dependencies.featuresRepository.checkIfTeamHasFeature(teamId, "calendar-cache-serve");
1414
}
1515
}

0 commit comments

Comments
 (0)