New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allows forcing/skipping calendar cache serving #18224

Merged

emrysal merged 7 commits into main from feat-calendar-cache-settings

Dec 17, 2024

Member

zomars commented Dec 17, 2024 •

edited

Loading

Folow up to #11928

What does this PR do?

This pull request introduces several enhancements and refactors to the calendar service, focusing on caching and availability fetching mechanisms. The key changes include the introduction of a new shouldServeCache parameter, refactoring of cache-related functions, and updates to various calendar service methods to incorporate these changes.

Enhancements to caching and availability fetching:

packages/app-store/googlecalendar/lib/CalendarService.ts: Introduced the shouldServeCache parameter in multiple methods, refactored getCacheOrFetchAvailability to use a new helper method getFreeBusyResult, and updated cache handling in unwatchCalendar. [1] [2] [3] [4] [5]
packages/core/CalendarManager.ts: Removed the getCachedResults function and added the shouldServeCache parameter to getBusyCalendarTimes and related functions to improve cache handling. [1] [2] [3]
packages/core/getBusyTimes.ts: Added the shouldServeCache parameter to the getBusyTimes function to control cache usage. [1] [2] [3]
packages/core/getCalendarsEvents.ts: Updated the getCalendarsEvents function to include the shouldServeCache parameter for better cache management. [1] [2]
packages/features/calendar-cache/lib/datesForCache.ts: Added new utility functions getTimeMin and getTimeMax to handle date range expansions for caching purposes.

Fixes #XXXX (GitHub issue number)
Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Mandatory Tasks (DO NOT REMOVE)

I have self-reviewed the code (A decent size PR without self-review might be rejected).
I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

Scenario 1

Have a team with a TeamFeature entry for both "calendar-cache" and "calendar-cache-serve"
Test out a booking normally
Test out a booking with ?cal.cache=false parameter. It should skip cache altogether.

Scenario 2

Have a team with a TeamFeature entry for both "calendar-cache"only.
Test out a booking normally
Test out a booking with ?cal.cache=true parameter. It should force cache serving.

Checklist

I haven't read the contributing guide
My code doesn't follow the style guidelines of this project
I haven't commented my code, particularly in hard-to-understand areas
I haven't checked if my changes generate no new warnings


          feat: allows forcing/skipping calendar cache serving

704ae48

Signed-off-by: Omar López <zomars@me.com>

zomars added the ready-for-e2e label

zomars requested a review from a team as a code owner

December 17, 2024 04:53

github-actions bot added the ❗️ migrations label

keithwillcode added core foundation labels

dosubot bot added app-store ✨ feature labels

vercel bot deployed to Preview – api

December 17, 2024 05:01

View deployment


          Update features.repository.ts

07ae45a

vercel bot deployed to Preview – dev

December 17, 2024 05:10

View deployment


          Added to HNB

653f536

vercel bot commented Dec 17, 2024 •

edited

Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Dec 17, 2024 4:04pm
calcom-web-canary	⬜️ Ignored (Inspect)	Visit Preview		Dec 17, 2024 4:04pm

vercel bot deployed to Preview – api

December 17, 2024 06:47

View deployment

vercel bot deployed to Preview – dev

December 17, 2024 06:54

View deployment

Contributor

github-actions bot commented Dec 17, 2024 •

edited

Loading

E2E results are ready!

Workflow #41661.1 latest results

zomars commented

View reviewed changes

Member Author

zomars left a comment

Ready for review

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

Comment on lines +514 to +517

+                async getFreeBusyResult(
+                  args: FreeBusyArgs,
+                  shouldServeCache?: boolean
+                ): Promise<calendar_v3.Schema$FreeBusyResponse> {

Member Author

zomars Dec 17, 2024

Extracted to take advantage of early returns

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

+                  args: FreeBusyArgs,
+                  shouldServeCache?: boolean
+                ): Promise<calendar_v3.Schema$FreeBusyResponse> {
+                  if (shouldServeCache === false) return await this.fetchAvailability(args);

Member Author

zomars Dec 17, 2024

If the parameter is explicitly set to false we skip cache altogether

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

                 async getAvailability(
                   dateFrom: string,
                   dateTo: string,
-                  selectedCalendars: IntegrationCalendar[]
+                  selectedCalendars: IntegrationCalendar[],
+                  shouldServeCache?: boolean

Member Author

zomars Dec 17, 2024

I'm not a fan to drilling but I couldn't find a cleaner way to pass this setting around.

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

-                      externalId: calendarId,
-                    },
-                  });
+                  const sc = await SelectedCalendarRepository.findByExternalId(credentialId, calendarId);

Member Author

zomars Dec 17, 2024

Moved to repo

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

Comment on lines +711 to +716

+                  // Populate the cache back for the remaining calendars, if any
+                  const remainingCalendars =
+                    sc?.credential?.selectedCalendars.filter((sc) => sc.externalId !== calendarId) || [];
+                  if (remainingCalendars.length > 0) {
+                    await this.fetchAvailabilityAndSetCache(remainingCalendars);
+                  }

Member Author

zomars Dec 17, 2024

This fixes a concern that @hariombalhara exposed

packages/core/CalendarManager.ts

@@ @@ -136,51 +135,6 @@ const cleanIntegrationKeys = ( @@
                 return rest;
               };
-              // here I will fetch the page json file.
-              export const getCachedResults = async (

Member Author

zomars Dec 17, 2024

Dead code

packages/features/flags/config.ts

@@ @@ -4,6 +4,7 @@ @@
                **/
               export type AppFlags = {
                 "calendar-cache": boolean;
+                "calendar-cache-serve": boolean;

Member Author

zomars Dec 17, 2024

Whether to serve the cache by default or not. Override-able via URL params. @keithwillcode

packages/features/schedules/lib/use-schedule/useSchedule.ts Outdated

@@ @@ -48,6 +48,7 @@ export const useSchedule = ({ @@
                 const searchParams = useSearchParams();
                 const routedTeamMemberIds = searchParams ? getRoutedTeamMemberIdsFromSearchParams(searchParams) : null;
                 const skipContactOwner = searchParams ? searchParams.get("cal.skipContactOwner") === "true" : false;
+                const shouldServeCache = searchParams ? searchParams.get("cal.cache") === "true" : undefined;

Member Author

zomars Dec 17, 2024

Undefined will fallback to the feature flag for this specific team/org

packages/trpc/server/routers/viewer/slots/util.ts

               import getSlots from "@calcom/lib/slots";
               import prisma, { availabilityUserSelect } from "@calcom/prisma";
               import { PeriodType, Prisma } from "@calcom/prisma/client";
-              import { SchedulingType } from "@calcom/prisma/enums";
-              import { BookingStatus } from "@calcom/prisma/enums";
+              import { BookingStatus, SchedulingType } from "@calcom/prisma/enums";

Member Author

zomars Dec 17, 2024

Sorted imports

packages/trpc/server/routers/viewer/slots/util.ts Outdated

Comment on lines 454 to 458

+                const shouldServeCache =
+                  /** If we set the parameter use that, else check if the team has the feature enabled */
+                  typeof _shouldServeCache === "boolean" || !teamId
+                    ? _shouldServeCache
+                    : await featureRepo.checkIfTeamHasFeature(teamId, "calendar-cache-serve");

Member Author

zomars Dec 17, 2024

Here we decide if the we should serve cache or not


          type fixes

80e144f

zomars added the high-risk label

vercel bot deployed to Preview – api

December 17, 2024 07:16

View deployment

vercel bot deployed to Preview – dev

December 17, 2024 07:23

View deployment

keithwillcode reviewed

View reviewed changes

packages/core/getCalendarsEvents.ts

@@ @@ -11,7 +11,8 @@ const getCalendarsEvents = async ( @@
                 withCredentials: CredentialPayload[],
                 dateFrom: string,
                 dateTo: string,
-                selectedCalendars: SelectedCalendar[]
+                selectedCalendars: SelectedCalendar[],
+                shouldServeCache?: boolean

Contributor

keithwillcode Dec 17, 2024

Why is this nullable? Seems like we should either just pass true or false.

Member Author

zomars Dec 17, 2024

#18224 (comment)

Member Author

zomars Dec 17, 2024

Undefined will fallback to the DB default. If we force a boolean value the fallback will never execute.

packages/core/getBusyTimes.ts

@@ @@ -44,6 +44,7 @@ export async function getBusyTimes(params: { @@
                     })[]
                   | null;
                 bypassBusyCalendarTimes: boolean;
+                shouldServeCache?: boolean;

Contributor

keithwillcode Dec 17, 2024

Why is this nullable? Seems like we should either just pass true or false.

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

+              export async function getShouldServeCache(shouldServeCache?: boolean | undefined, teamId?: number) {
+                if (typeof shouldServeCache === "boolean") return shouldServeCache;
+                if (!teamId) return undefined;

Contributor

keithwillcode Dec 17, 2024

Why not false here?

Member Author

zomars Dec 17, 2024

undefined has meaning in this feature.

Member Author

zomars Dec 17, 2024

Undefined means there's no preference. So the regular default behavior will apply.

packages/prisma/migrations/20241216000000_add_calendar_cache_serve/migration.sql Outdated Show resolved Hide resolved

packages/prisma/zod-utils.ts Outdated Show resolved Hide resolved

zomars and others added 2 commits

December 17, 2024 08:17


          Update packages/prisma/migrations/20241216000000_add_calendar_cache_s…

d273512

…erve/migration.sql

Co-authored-by: Keith Williams <keithwillcode@gmail.com>


          Update packages/prisma/zod-utils.ts

3427d8b

Co-authored-by: Keith Williams <keithwillcode@gmail.com>

vercel bot deployed to Preview – api

December 17, 2024 15:32

View deployment

vercel bot deployed to Preview – dev

December 17, 2024 15:44

View deployment

tarunkumarmetaforms approved these changes

View reviewed changes


          Update selectedCalendar.ts

20f1a40

zomars requested review from keithwillcode and hariombalhara

December 17, 2024 16:04

vercel bot deployed to Preview – api

December 17, 2024 16:12

View deployment

emrysal reviewed

View reviewed changes

packages/features/flags/features.repository.ts

                     throw err;
                   }
                 }
+                async checkIfTeamHasFeature(teamId: number, featureId: keyof AppFlags) {
+                  try {
+                    const teamFeature = await db.teamFeatures.findUnique({

Contributor

emrysal Dec 17, 2024

Did you mean?

Suggested change

      
                  const teamFeature = await db.teamFeatures.findUnique({
          
                  const teamFeature = await db.teamFeatures.findUniqueOrThrow({

Member Author

zomars Dec 17, 2024

Not throwing since we're expecting a boolean return value

vercel bot deployed to Preview – dev

December 17, 2024 16:17

View deployment

zomars requested a review from emrysal

December 17, 2024 16:43

emrysal approved these changes

View reviewed changes

Contributor

emrysal left a comment

Happy with this, let's get it merged.

emrysal merged commit 2eb385a into main

40 checks passed

emrysal deleted the feat-calendar-cache-settings branch

December 17, 2024 16:54

This was referenced Dec 17, 2024

fix: calendar cache date ranges #18237

Merged

fix: single rejection caused cache cron to fail #18239

Merged

MuhammadAimanSulaiman pushed a commit to hit-pay/cal.com that referenced this pull request


          feat: allows forcing/skipping calendar cache serving (calcom#18224)

6cfc1e2

* feat: allows forcing/skipping calendar cache serving

Signed-off-by: Omar López <zomars@me.com>

* Update features.repository.ts

* Added to HNB

* type fixes

* Update packages/prisma/migrations/20241216000000_add_calendar_cache_serve/migration.sql

Co-authored-by: Keith Williams <keithwillcode@gmail.com>

* Update packages/prisma/zod-utils.ts

Co-authored-by: Keith Williams <keithwillcode@gmail.com>

* Update selectedCalendar.ts

---------

Signed-off-by: Omar López <zomars@me.com>
Co-authored-by: Keith Williams <keithwillcode@gmail.com>

MuhammadAimanSulaiman pushed a commit to hit-pay/cal.com that referenced this pull request


          feat: allows forcing/skipping calendar cache serving (calcom#18224)

599a695

* feat: allows forcing/skipping calendar cache serving

Signed-off-by: Omar López <zomars@me.com>

* Update features.repository.ts

* Added to HNB

* type fixes

* Update packages/prisma/migrations/20241216000000_add_calendar_cache_serve/migration.sql

Co-authored-by: Keith Williams <keithwillcode@gmail.com>

* Update packages/prisma/zod-utils.ts

Co-authored-by: Keith Williams <keithwillcode@gmail.com>

* Update selectedCalendar.ts

---------

Signed-off-by: Omar López <zomars@me.com>
Co-authored-by: Keith Williams <keithwillcode@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app-store core ✨ feature foundation high-risk ❗️ migrations ready-for-e2e