fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working #14788

hariombalhara · 2024-04-29T07:02:53Z

What does this PR do?

Fixes issue with refreshed access_token not being used when credential sync is enabled.
Note that the issue isn't replicable if the refresh_token is set. You have to delete the refresh_token to be able to replicate the issue(with credential sync enabled)

Type of change

Bug fix (non-breaking change which fixes an issue)

How should this be tested?

Make sure that credential sync is enabled
Connect Google Calendar
Delete keys.refresh_token from DB
expiry_date Use a past time here so that the token expires
Simply refresh apps/installed/calendar here. On main, auto-refresh won't occur but in this branch it will occur.

Mandatory Tasks

Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Tests

Working for tests in a follow-up PR. Hitting some issues with mocking. Need to figure that out.

Checklist

I haven't added tests that prove my fix is effective or that my feature works

vercel · 2024-04-29T07:02:55Z

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

Name	Status	Preview	Comments	Updated (UTC)
ai	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 29, 2024 8:24pm
platform-starter-kit	❌ Failed (Inspect)			Apr 29, 2024 8:24pm

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Apr 29, 2024 8:24pm
calcom-web-canary	⬜️ Ignored (Inspect)	Visit Preview		Apr 29, 2024 8:24pm

github-actions · 2024-04-29T07:03:07Z

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

hariombalhara · 2024-04-29T07:03:07Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hariombalhara and the rest of your teammates on Graphite

github-actions · 2024-04-29T07:15:06Z

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

deploysentinel · 2024-04-29T07:16:45Z

Current Playwright Test Results Summary

✅ 315 Passing - ⚠️ 18 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 04/29/2024 08:36:20pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 36eb5ca

Started: 04/29/2024 08:33:10pm UTC

⚠️ Flakes

📄 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 -- legacy user can add multiple organizer address Retry 1 • Initial Attempt	0% (0) 0 / 232 runs failed over last 7 days	1.72% (4) 4 / 232 runs flaked over last 7 days

📄 packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 9 Flakes

Top 1 Common Error Messages

`null`	9 Test Cases Affected

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Popup Tests should open embed iframe on click - Configured with light theme Retry 1 • Initial Attempt	15.35% (37) 37 / 241 runs failed over last 7 days	54.36% (131) 131 / 241 runs flaked over last 7 days
Popup Tests should be able to reschedule Retry 1 • Initial Attempt	-179.45% (-131) -131 / 73 runs failed over last 7 days	179.45% (131) 131 / 73 runs flaked over last 7 days
Popup Tests should open Routing Forms embed on click Retry 1 • Initial Attempt	-178.08% (-130) -130 / 73 runs failed over last 7 days	178.08% (130) 130 / 73 runs flaked over last 7 days
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe according to system theme when no theme is configured through Embed API Retry 1 • Initial Attempt	-176.71% (-129) -129 / 73 runs failed over last 7 days	176.71% (129) 129 / 73 runs flaked over last 7 days
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe(Booker Profile Page) with dark theme when configured with dark theme using Embed API Retry 1 • Initial Attempt	-176.71% (-129) -129 / 73 runs failed over last 7 days	176.71% (129) 129 / 73 runs flaked over last 7 days
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe according to system theme when configured with 'auto' theme using Embed API Retry 1 • Initial Attempt	-176.71% (-129) -129 / 73 runs failed over last 7 days	176.71% (129) 129 / 73 runs flaked over last 7 days
Popup Tests Floating Button Popup Pro User - Configured in App with default setting of system theme should open embed iframe(Event Booking Page) with dark theme when configured with dark theme using Embed API Retry 1 • Initial Attempt	-176.71% (-129) -129 / 73 runs failed over last 7 days	176.71% (129) 129 / 73 runs flaked over last 7 days
Popup Tests should open on clicking child element Retry 1 • Initial Attempt	-163.64% (-36) -36 / 22 runs failed over last 7 days	163.64% (36) 36 / 22 runs flaked over last 7 days
Popup Tests prendered embed should be loaded and apply the config given to it Retry 1 • Initial Attempt	-176.71% (-129) -129 / 73 runs failed over last 7 days	176.71% (129) 129 / 73 runs flaked 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 Test preview should return correct route Retry 1 • Initial Attempt	0.42% (1) 1 / 239 run failed over last 7 days	35.98% (86) 86 / 239 runs flaked over last 7 days

📄 apps/web/playwright/webhook.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
BOOKING_RESCHEDULED when rescheduling to a booking that already exists, should send a booking rescheduled event with the existant booking uid Retry 1 • Initial Attempt	10.43% (22) 22 / 211 runs failed over last 7 days	34.12% (72) 72 / 211 runs flaked over last 7 days

📄 packages/embeds/embed-core/playwright/tests/namespacing.e2e.ts • 4 Flakes

Top 1 Common Error Messages

`null`	4 Test Cases Affected

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Namespacing Inline Embed Double install Embed Snippet with inline embed using a namespace Retry 1 • Initial Attempt	0% (0) 0 / 228 runs failed over last 7 days	53.51% (122) 122 / 228 runs flaked over last 7 days
Namespacing Different namespaces can have different init configs Retry 1 • Initial Attempt	0% (0) 0 / 226 runs failed over last 7 days	53.10% (120) 120 / 226 runs flaked over last 7 days
Namespacing Inline Embed Double install Embed Snippet with inline embed without a namespace(i.e. default namespace) Retry 1 • Initial Attempt	0% (0) 0 / 228 runs failed over last 7 days	54.39% (124) 124 / 228 runs flaked over last 7 days
Namespacing Inline Embed Add inline embed using a namespace without reload Retry 1 • Initial Attempt	0.44% (1) 1 / 228 run failed over last 7 days	53.51% (122) 122 / 228 runs flaked over last 7 days

📄 apps/web/playwright/teams.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Teams - NonOrg -- future Can create a booking for Round Robin EventType Retry 2 • Retry 1 • Initial Attempt	17.60% (44) 44 / 250 runs failed over last 7 days	22.40% (56) 56 / 250 runs flaked 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 Pending payment booking should not be confirmed by default Retry 1 • Initial Attempt	1.32% (3) 3 / 227 runs failed over last 7 days	23.35% (53) 53 / 227 runs flaked over last 7 days

View Detailed Build Results

hariombalhara · 2024-04-29T09:32:58Z

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

@@ -85,13 +85,15 @@ export default class GoogleCalendarService implements Calendar {
    this.credential = credential;
    this.auth = this.initGoogleAuth(credential);
    this.log = log.getSubLogger({ prefix: [`[[lib] ${this.integrationName}`] });
-    this.credential = credential;


It was a duplicate code line(of line 85)

hariombalhara · 2024-04-29T09:33:19Z

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

  }

  private async getMyGoogleAuthSingleton() {
-    const googleCredentials = OAuth2UniversalSchema.parse(this.credential.key);
+    if (this.myGoogleAuth) {


Simplified the code.

hariombalhara · 2024-04-29T09:34:03Z

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

    const { client_id, client_secret, redirect_uris } = await getGoogleAppKeys();
-    this.myGoogleAuth = this.myGoogleAuth || new MyGoogleAuth(client_id, client_secret, redirect_uris[0]);
+    const googleCredentials = OAuth2UniversalSchema.parse(this.credential.key);
+    this.myGoogleAuth = new MyGoogleAuth(client_id, client_secret, redirect_uris[0]);
    this.myGoogleAuth.setCredentials(googleCredentials);


Now setCredentials is also called only the first time. As getMyGoogleAuthSingleton can be called multiple times, we shouldn't set it again and again.

Rest of the code should ensure that setCredentials is called when needed

hariombalhara · 2024-04-29T09:35:19Z

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

          where: {
            id: credential.id,
          },
          data: {
            key: token,
          },
        });
+
+        // Update cached credential as well
+        this.credential.key = key;


This is important line which fixes the issue.

We update the token in DB. But in case of google-calendar there is a cached value here as well. We should keep it upto-date in case someone use this value.

graphite-app · 2024-04-29T09:38:53Z

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (04/29/24)

1 reviewer was added to this PR based on Keith Williams's automation.

joeauyeung

Tested credential syncing GCal w/o refresh_token it's grabbing the new token and working as expected.

vercel bot had a problem deploying to Preview – platform-starter-kit April 29, 2024 07:02 Failure

Handle changed cred key

509a01f

keithwillcode added consumer core area: core, team members only labels Apr 29, 2024

hariombalhara force-pushed the fix-handle-changed-credential-key branch from 86386b6 to 509a01f Compare April 29, 2024 07:04

vercel bot had a problem deploying to Preview – platform-starter-kit April 29, 2024 07:04 Failure

vercel bot deployed to Preview – ai April 29, 2024 07:08 View deployment

vercel bot deployed to Preview – api April 29, 2024 07:16 View deployment

vercel bot had a problem deploying to Preview – dev April 29, 2024 07:24 Failure

hariombalhara mentioned this pull request Apr 29, 2024

test: Google CalendarService test #14793

Merged

1 task

hariombalhara commented Apr 29, 2024

View reviewed changes

hariombalhara changed the title ~~Handle changed cred key~~ fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working Apr 29, 2024

hariombalhara marked this pull request as ready for review April 29, 2024 09:36

graphite-app bot requested a review from a team April 29, 2024 09:36

vercel bot had a problem deploying to Preview – platform-starter-kit April 29, 2024 09:36 Failure

dosubot bot added the 🐛 bug Something isn't working label Apr 29, 2024

vercel bot deployed to Preview – ai April 29, 2024 09:39 View deployment

vercel bot deployed to Preview – api April 29, 2024 09:43 View deployment

vercel bot deployed to Preview – dev April 29, 2024 09:56 View deployment

hariombalhara force-pushed the fix-handle-changed-credential-key branch from 0c69f7a to 509a01f Compare April 29, 2024 13:06

joeauyeung approved these changes Apr 29, 2024

View reviewed changes

Merge branch 'main' into fix-handle-changed-credential-key

36eb5ca

vercel bot had a problem deploying to Preview – platform-starter-kit April 29, 2024 20:22 Failure

joeauyeung enabled auto-merge (squash) April 29, 2024 20:24

vercel bot deployed to Preview – ai April 29, 2024 20:24 View deployment

vercel bot deployed to Preview – api April 29, 2024 20:28 View deployment

vercel bot had a problem deploying to Preview – dev April 29, 2024 20:33 Failure

joeauyeung merged commit 900e90c into main Apr 29, 2024
37 of 39 checks passed

joeauyeung deleted the fix-handle-changed-credential-key branch April 29, 2024 20:37

hariombalhara added this to the v4.1 milestone May 3, 2024

hariombalhara self-assigned this May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working #14788

fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working #14788

hariombalhara commented Apr 29, 2024 •

edited

vercel bot commented Apr 29, 2024 •

edited

github-actions bot commented Apr 29, 2024 •

edited

hariombalhara commented Apr 29, 2024 •

edited

github-actions bot commented Apr 29, 2024 •

edited

deploysentinel bot commented Apr 29, 2024 •

edited

Test Case Results

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

hariombalhara Apr 29, 2024 •

edited

hariombalhara Apr 29, 2024

hariombalhara Apr 29, 2024 •

edited

hariombalhara Apr 29, 2024

graphite-app bot commented Apr 29, 2024

joeauyeung left a comment

fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working #14788

fix: With Credential Sync enabled, fix expired google calendar token handling - Refresh wasn't working #14788

Conversation

hariombalhara commented Apr 29, 2024 • edited

What does this PR do?

Type of change

How should this be tested?

Mandatory Tasks

Tests

Checklist

vercel bot commented Apr 29, 2024 • edited

github-actions bot commented Apr 29, 2024 • edited

hariombalhara commented Apr 29, 2024 • edited

github-actions bot commented Apr 29, 2024 • edited

📦 Next.js Bundle Analysis for @calcom/web

deploysentinel bot commented Apr 29, 2024 • edited

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

hariombalhara Apr 29, 2024 • edited

Choose a reason for hiding this comment

hariombalhara Apr 29, 2024

Choose a reason for hiding this comment

hariombalhara Apr 29, 2024 • edited

Choose a reason for hiding this comment

hariombalhara Apr 29, 2024

Choose a reason for hiding this comment

graphite-app bot commented Apr 29, 2024

Graphite Automations

joeauyeung left a comment

Choose a reason for hiding this comment

hariombalhara commented Apr 29, 2024 •

edited

vercel bot commented Apr 29, 2024 •

edited

github-actions bot commented Apr 29, 2024 •

edited

hariombalhara commented Apr 29, 2024 •

edited

github-actions bot commented Apr 29, 2024 •

edited

deploysentinel bot commented Apr 29, 2024 •

edited

hariombalhara Apr 29, 2024 •

edited

hariombalhara Apr 29, 2024 •

edited