-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
feat: Populate gCal calendar cache via webhooks #11928
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
3297976
to
6490d57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated procedures to new boilerplate syntax
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 New Page AddedThe following page was added to the bundle from the code in this PR:
|
Current Playwright Test Results Summary✅ 447 Passing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 02/14/2024 11:21:12pm UTC) Run DetailsRunning Workflow PR Update on Github Actions Commit: d77b02f Started: 02/14/2024 11:12:29pm UTC
|
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Booking With Multiple Email Question and Each Other Question Booking With Multiple Email Question and checkbox group Question Multiple Email required and checkbox group required
Retry 1 • Initial Attempt |
0.40% (1)1 / 253 runfailed over last 7 days |
4.74% (12)12 / 253 runsflaked over last 7 days |
📄 apps/web/playwright/booking/selectQuestion.e2e.ts • 3 Flakes
Top 1 Common Error Messages
|
3 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Booking With Phone Question and Each Other Question Booking With Select Question and checkbox group Question Select and checkbox group not required
Retry 1 • Initial Attempt |
0% (0)0 / 256 runsfailed over last 7 days |
5.47% (14)14 / 256 runsflaked over last 7 days |
Booking With Phone Question and Each Other Question Booking With Select Question and Radio group Question Select required and Radio group required
Retry 1 • Initial Attempt |
0% (0)0 / 256 runsfailed over last 7 days |
3.91% (10)10 / 256 runsflaked over last 7 days |
Booking With Phone Question and Each Other Question Booking With Select Question and Short text question Select and Short text not required
Retry 1 • Initial Attempt |
0% (0)0 / 256 runsfailed over last 7 days |
4.30% (11)11 / 256 runsflaked over last 7 days |
📄 apps/web/playwright/booking/responsiveBooking.e2e.ts • 2 Flakes
Top 1 Common Error Messages
|
2 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Booking page with no questions Booking page with 1920x1080 resolution
Retry 1 • Initial Attempt |
0% (0)0 / 256 runsfailed over last 7 days |
8.20% (21)21 / 256 runsflaked over last 7 days |
Booking page with no questions Booking page with 640x480 resolution
Retry 1 • Initial Attempt |
1.56% (4)4 / 256 runsfailed over last 7 days |
6.64% (17)17 / 256 runsflaked over last 7 days |
📄 apps/web/playwright/login.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
user can login & logout succesfully -- future login flow user & logout using dashboard
Retry 1 • Initial Attempt |
0.78% (2)2 / 256 runsfailed over last 7 days |
45.31% (116)116 / 256 runsflaked over last 7 days |
📄 apps/web/playwright/integrations-stripe.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Stripe integration Paid booking should be able to be rescheduled
Retry 1 • Initial Attempt |
0.76% (2)2 / 262 runsfailed over last 7 days |
3.82% (10)10 / 262 runsflaked over last 7 days |
📄 apps/web/playwright/booking/addressQuestione2e/addressQuestion.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Booking With Address Question and Each Other Question Booking With Address Question and select Question Address required and select required
Retry 1 • Initial Attempt |
0% (0)0 / 261 runsfailed over last 7 days |
4.21% (11)11 / 261 runsflaked over last 7 days |
📄 apps/web/playwright/event-types.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Event Types tests -- future user Different Locations Tests can add Attendee Phone Number location and book with it
Retry 1 • Initial Attempt |
0.74% (2)2 / 271 runsfailed over last 7 days |
11.44% (31)31 / 271 runsflaked over last 7 days |
📄 apps/web/playwright/profile.e2e.ts • 2 Flakes
Top 1 Common Error Messages
|
2 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Update Profile Cannot update a users email when existing user has same email (verification enabled)
Retry 1 • Initial Attempt |
0.42% (1)1 / 239 runfailed over last 7 days |
50.63% (121)121 / 239 runsflaked over last 7 days |
Update Profile Can update a users email (verification enabled)
Retry 1 • Initial Attempt |
11.93% (29)29 / 243 runsfailed over last 7 days |
52.67% (128)128 / 243 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/preview.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Preview Preview - embed-core should load
Retry 1 • Initial Attempt |
0% (0)0 / 273 runsfailed over last 7 days |
35.53% (97)97 / 273 runsflaked over last 7 days |
📄 packages/app-store/routing-forms/playwright/tests/basic.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Routing Forms Seeded Routing Form Router URL should work
Retry 1 • Initial Attempt |
0% (0)0 / 272 runsfailed over last 7 days |
14.34% (39)39 / 272 runsflaked over last 7 days |
📄 packages/embeds/embed-core/playwright/tests/inline.e2e.ts • 1 Flake
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Inline Iframe Inline Iframe - Configured with Dark Theme
Retry 1 • Initial Attempt |
0.73% (2)2 / 274 runsfailed over last 7 days |
42.34% (116)116 / 274 runsflaked over last 7 days |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for review. Happy anniversary!
* This runs each minute and we need fresh data each time | ||
* @see https://nextjs.org/docs/app/building-your-application/caching#opting-out-2 | ||
**/ | ||
export const revalidate = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So cron is always fresh
() => "" | ||
); | ||
|
||
const Page = () => <h1>Calendar cache index</h1>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a placeholder page where we might control cache entries or remove them if necessary.
@@ -20,7 +23,7 @@ try { | |||
"*/5 * * * * *", | |||
async function () { | |||
await Promise.allSettled([ | |||
fetchCron("/tasks/cron"), | |||
fetchCron("/calendar-cache/cron"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing locally
const user = req.userWithCredentials; | ||
const { integration, externalId, credentialId } = selectedCalendarSelectSchema.parse(req.query); | ||
const calendarCacheRepository = await CalendarCache.initFromCredentialId(credentialId); | ||
await calendarCacheRepository.unwatchCalendar({ calendarId: externalId }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even tho we already handle subscriptions in the cron, we make sure we purge cache when removing a selected calendar.
@@ -8,6 +8,10 @@ | |||
"path": "/api/tasks/cron", | |||
"schedule": "* * * * *" | |||
}, | |||
{ | |||
"path": "/api/calendar-cache/cron", | |||
"schedule": "* * * * *" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running each minute
|
||
import type * as OAuthManager from "../../_utils/oauth/OAuthManager"; | ||
|
||
vi.mock("../../_utils/oauth/OAuthManager", () => oAuthManagerMock); | ||
|
||
beforeEach(() => { | ||
mockReset(oAuthManagerMock); | ||
mockClear(oAuthManagerMock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hariombalhara this was causing errors. IDK if this can have unintended behaviors. Left more context on Campsite.
const passedSelectedCalendars = selectedCalendars.filter((sc) => sc.integration === type); | ||
const passedSelectedCalendars = selectedCalendars | ||
.filter((sc) => sc.integration === type) | ||
// Needed to ensure cache keys are consistent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, to increase chance of cache hits.
handleCalendarsToUnwatch(), | ||
]); | ||
|
||
// TODO: Credentials can be installed on a whole team, check for selected calendars on the team |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @joeauyeung you might have some ideas here?
// new URLSearchParams does not accept numbers | ||
credentialId: String(credentialId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use coerce on server side so we're good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful query to have. Also we could use it in as typedRaw query once we upgrade prisma
|
||
export default defaultResponder(async (req: NextApiRequest, res: NextApiResponse) => { | ||
await authMiddleware(req); | ||
return defaultHandler({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice changes in this file 🙏
const isCalendarCacheEnabledGlobally = await featureRepo.checkIfFeatureIsEnabledGlobally( | ||
"calendar-cache" | ||
); | ||
if (isCalendarCacheEnabledGlobally) return new CalendarCacheRepository(calendar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great example of pulling the "if" statements as high up in the app as possible and letting IoC take over.
@keithwillcode feedback addressed. 🙏 |
); | ||
|
||
if (!selectedCalendar) { | ||
throw new HttpError({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird to throw an exception for a 200. Maybe in a follow-up we can refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware. But defaultResponder just handles the proper response and code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @zomars.
As discussed. This will be launched dark and tested from there.
yeah lets test with i.cal.com org for now |
This reverts commit d294a74.
)" (…" This reverts commit 6272c88.
fixes #11524
What does this PR do?
Calendar Cache and Cron Jobs Enhancements:
apps/web/app/api/calendar-cache/cron/route.ts
: Exported theGET
handler and set cache revalidation to zero to ensure fresh data each minute.apps/web/cron-tester.ts
: Added dotenv configuration for environment variables and updated cron job endpoints to use the correct API key parameter. [1] [2]apps/web/vercel.json
: Added a new cron job entry for the calendar cache API.API Handler Refactoring:
apps/web/pages/api/availability/calendar.ts
: Refactored the API handler by splitting it into separate GET, POST, and DELETE handlers, and introduced shared authentication middleware. [1] [2] [3] [4]packages/app-store/googlecalendar/api/webhook.ts
: Added a new webhook handler to validate requests and update calendar availability cache.Authentication and Security Improvements:
apps/web/pages/api/cron/calendar-cache-cleanup.ts
: Added API key validation to ensure only authorized requests can access the cron job endpoint.Calendar Cache and Google Calendar Integration:
packages/app-store/googlecalendar/lib/CalendarService.ts
: Integrated theCalendarCache
class for handling calendar cache operations and removed outdated cache handling functions. [1] [2] [3]packages/app-store/googlecalendar/lib/CalendarService.test.ts
: Updated tests to cover new calendar cache functionalities, including cache hits, misses, and calendar watch/unwatch operations. [1] [2] [3] [4] [5]These changes aim to improve the efficiency, security, and reliability of calendar-related features in the application.
TODO (Follow up maybe)
Type of change
How should this be tested?
.env
likeGOOGLE_WEBHOOK_URL="https://xxxxxxxxx.tunnelmole.net"
yarn dev:cron
inside apps/web directory.TeamFeature
entry with the properteamId
and afeatureId
namedcalendar-cache
Mandatory Tasks