fix: N Calendar Invites For Collective Events #14610

joeauyeung · 2024-04-16T13:31:14Z

What does this PR do?

This PR aims to prevent attendees receiving multiple calendar invites by only creating one calendar event.

Fixes #14580 Fixes CAL-3412 Fixes CAL-3468

https://www.loom.com/share/89fecb968c9a4511869bfa807c273ca4

Requirement/Documentation

If there is a requirement document, please, share it here.
If there is a UI/UX design document, please, share it here.

Type of change

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

How should this be tested?

Create a collective team event with each team member having Outlook as a destination calendar
Create a booking under the collective event
The attendee should receive one email from Outlook and all team members should be invited to the event

Mandatory Tasks

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

Checklist

I haven't added tests that prove my fix is effective or that my feature works
I haven't checked if new and existing unit tests pass locally with my changes

linear · 2024-04-16T13:31:17Z

CAL-3412 a collective event of N people sends N invites

github-actions · 2024-04-16T13:31:26Z

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

graphite-app · 2024-04-16T13:35:12Z

Graphite Automations

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

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

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

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

linear · 2024-04-16T13:42:24Z

CAL-3468 Outlook sending multiple invites when booking a team event (from each member of the event)

github-actions · 2024-04-16T13:42:33Z

📦 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-16T13:43:30Z

Current Playwright Test Results Summary

✅ 222 Passing - ⚠️ 8 Flaky

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

(Last updated on 04/16/2024 07:07:23pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: e19d594

Started: 04/16/2024 07:03:28pm UTC

⚠️ Flakes

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

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Login and logout tests -- legacy Login flow validations -- future Should warn when user does not exist Retry 1 • Initial Attempt	0% (0) 0 / 284 runs failed over last 7 days	1.41% (4) 4 / 284 runs flaked 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 -- legacy user Different Locations Tests Can remove location from multiple locations that are saved Retry 1 • Initial Attempt	0% (0) 0 / 291 runs failed over last 7 days	19.24% (56) 56 / 291 runs flaked over last 7 days

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

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Insights should be able to go to insights as admins Retry 1 • Initial Attempt	0.34% (1) 1 / 291 run failed over last 7 days	6.87% (20) 20 / 291 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.72% (5) 5 / 291 runs failed over last 7 days	20.96% (61) 61 / 291 runs flaked over last 7 days

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

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Update Profile Can verify the newly added secondary email Retry 1 • Initial Attempt	0% (0) 0 / 296 runs failed over last 7 days	7.77% (23) 23 / 296 runs flaked over last 7 days

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

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Signup Flow Test Email verification sent if enabled Retry 1 • Initial Attempt	0% (0) 0 / 308 runs failed over last 7 days	2.27% (7) 7 / 308 runs flaked over last 7 days

📄 apps/web/playwright/change-username.e2e.ts • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Change username on settings User can change username Retry 1 • Initial Attempt	1.01% (3) 3 / 298 runs failed over last 7 days	2.68% (8) 8 / 298 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 1 • Initial Attempt	0.65% (2) 2 / 308 runs failed over last 7 days	1.62% (5) 5 / 308 runs flaked over last 7 days

View Detailed Build Results

joeauyeung · 2024-04-16T13:37:45Z

packages/app-store/_utils/calendars/processExternalId.ts

For Outlook since the calendarId is for internal purposes we can only invite the primary email to calendar events. https://www.loom.com/share/60ac4ad245414714979dc2e11d8d347a?sid=6b089642-a97d-40cd-a015-37fc97a3e8d9

joeauyeung · 2024-04-16T13:38:30Z

packages/core/EventManager.ts

+        if (!credentialId) {
+          // Keep this logic here for other events that need to fallback to other calendars
+          if (eventCreationAttempts === 0) {
+            const destinationCalendarCredentials = this.calendarCredentials.filter(
+              (c) => c.type === destinationCalendar.integration
+            );
+            // It might not be the first connected calendar as it seems that the order is not guaranteed to be ascending of credentialId.
+            const firstCalendarCredential = destinationCalendarCredentials[0] as
+              | (typeof destinationCalendarCredentials)[number]
+              | undefined;
+
+            if (!firstCalendarCredential) {
+              log.warn(
+                "No other credentials found of the same type as the destination calendar. Falling back to first connected calendar"
+              );
+              await fallbackToFirstConnectedCalendar();
+            } else {
+              log.warn(
+                "No credentialId found for destination calendar, falling back to first found calendar of same type as destination calendar",
+                safeStringify({
+                  destination: getPiiFreeDestinationCalendar(destinationCalendar),
+                  firstConnectedCalendar: getPiiFreeCredential(firstCalendarCredential),
+                })
+              );
+              createdEvents.push(await createEvent(firstCalendarCredential, event));
+              eventCreated = true;
+              break;


Not a huge fan of this approach but keeping this in here to maintain what we currently have.

vercel · 2024-04-16T15:13:20Z

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

Name	Status	Preview	Comments	Updated (UTC)
platform-starter-kit	❌ Failed (Inspect)			Apr 16, 2024 6:52pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
ai	⬜️ Ignored (Inspect)	Visit Preview	Apr 16, 2024 6:52pm
cal	⬜️ Ignored (Inspect)	Visit Preview	Apr 16, 2024 6:52pm
calcom-web-canary	⬜️ Ignored (Inspect)	Visit Preview	Apr 16, 2024 6:52pm

…thub.com/calcom/cal.com into fix-calendar-events-on-collective-events

keithwillcode

@joeauyeung I just tested creating an OAuthClient on the preview development and I can't click the Submit button at all

keithwillcode · 2024-04-16T18:15:34Z

@joeauyeung I just tested creating an OAuthClient on the preview development and I can't click the Submit button at all

Ok. Tested again. Not sure what was going on but now I can submit successfully.

keithwillcode

It's concerning to me that all this logic is changing and no automated tests are updated/added. We need to follow up ASAP with a PR adding unit tests to verify all of this logic.

keithwillcode · 2024-04-16T18:16:57Z

packages/app-store/_utils/calendars/processExternalId.ts

+ * When inviting attendees to a calendar event, sometimes the external ID is only used for internal purposes
+ * Need to process the correct external ID for the calendar service
+ */
+const processExternalId = (destinationCalendar: DestinationCalendar) => {


Do you think this function has the potential to grow in the number of "if" statements moving forward? i.e. We'll have to have different logic for a large number of different calendar apps. If so, we should move it off to its own file per calendar integration.

Since this is urgent to get fixed, that could be a follow-up refactor.

joeauyeung · 2024-04-16T18:30:20Z

It's concerning to me that all this logic is changing and no automated tests are updated/added. We need to follow up ASAP with a PR adding unit tests to verify all of this logic.

@keithwillcode there are 0 tests for the EventManager class so I'm writing tests out but I'll follow up in another PR so we're not waiting for this one.

Regarding processExternalId AFAIK it's just Outlook that treats calendarIds like this. I see what you're getting at though. If there are other calendars that are added that have their own custom logic we should refactor processing the externalIds under their respective app package.

…th-clients/create.tsx

packages/core/EventManager.ts

Co-authored-by: Omar López <zomars@me.com>

zomars · 2024-04-16T18:57:28Z

packages/core/EventManager.ts

@@ -535,78 +535,105 @@ export default class EventManager {
    };

    if (event.destinationCalendar && event.destinationCalendar.length > 0) {
-      // Since GCal pushes events to multiple calendars we only want to create one event per booking
-      let gCalAdded = false;
-      const destinationCalendars: DestinationCalendar[] = event.destinationCalendar.reduce(


Is the reduce refactor necessary to fix the bug? Or was it done for getting two birds with one stone?

If it's the latter then I'm afraid it is making the PR harder to review since it's not clear "where" is the bug that we're fixing.

Also as @keithwillcode mentioned we don't have EventManager tests to ship this PR with 100% confidence. RN if I approve this I'm not 100% confident this will not break other event type configurations.

My suggestion would be:

Make a PR with "only" the changes needed to fix the current bug. Then other PR for adding tests. Then other PR to do chores and refactoring.

The refactor is necessary since ideally only one calendar event is created for the booking. When inviting team members it should add events to their default destination calendars by inviting those calendars to the event.

Since this is affecting collective events, a PR with "only" the fix would be to remove pushing multiple destination calendars when the event is a collective one. That would be the safest bet. Looking through the CalendarService files of GCal and Outlook we already add team members as the attendees. Although for Outlook we still need to process the externalId properly so all team members are added to the event.

Yeah I think we should focus the rest of the day on getting tests wired up here and we ship this tomorrow.

This is the type of change that is high risk of making things worse than the original bug we are trying to fix.

The tests should written against main and not against this code first to verify all existing logic stays the same. Once verified, we then add tests to verify this change in logic

zomars · 2024-04-19T22:28:34Z

Closing in favor of #14617

joeauyeung added 2 commits April 16, 2024 08:43

Only create one calendar event for collective events

edaa7ab

Handle processing calendar externalId

fef1557

graphite-app bot requested a review from a team April 16, 2024 13:31

github-actions bot added consumer emails area: emails, cancellation email, reschedule email, inbox, spam folder, not getting email teams area: teams, round robin, collective, managed event-types Urgent Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Apr 16, 2024

dosubot bot added the bookings area: bookings, availability, timezones, double booking label Apr 16, 2024

keithwillcode added the core area: core, team members only label Apr 16, 2024

joeauyeung added the high-risk Requires approval by Foundation team label Apr 16, 2024

graphite-app bot requested a review from a team April 16, 2024 13:33

joeauyeung commented Apr 16, 2024

View reviewed changes

Merge branch 'main' into fix-calendar-events-on-collective-events

4028437

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 15:13 Failure

vercel bot had a problem deploying to Preview – api April 16, 2024 15:16 Failure

joeauyeung added 2 commits April 16, 2024 11:22

Type fix

9500e90

Merge branch 'fix-calendar-events-on-collective-events' of https://gi…

60f6f50

…thub.com/calcom/cal.com into fix-calendar-events-on-collective-events

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 15:23 Failure

vercel bot deployed to Preview – api April 16, 2024 15:30 View deployment

Push attempted cals to array

18d2b33

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 15:37 Failure

vercel bot deployed to Preview – api April 16, 2024 15:43 View deployment

vercel bot deployed to Preview – dev April 16, 2024 16:12 View deployment

Fix lint error

8221b54

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 16:23 Failure

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

vercel bot deployed to Preview – dev April 16, 2024 16:42 View deployment

keithwillcode requested changes Apr 16, 2024

View reviewed changes

Merge branch 'main' into fix-calendar-events-on-collective-events

0930794

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 17:45 Failure

vercel bot deployed to Preview – api April 16, 2024 17:51 View deployment

vercel bot deployed to Preview – dev April 16, 2024 18:04 View deployment

keithwillcode reviewed Apr 16, 2024

View reviewed changes

Merge branch 'main' into fix-calendar-events-on-collective-events

790f8d1

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 18:45 Failure

Discard changes to apps/web/pages/settings/organizations/platform/oau…

6756992

…th-clients/create.tsx

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 18:46 Failure

zomars reviewed Apr 16, 2024

View reviewed changes

packages/core/EventManager.ts Outdated Show resolved Hide resolved

Change typing of attemptedDestinationCalendarIds

e19d594

Co-authored-by: Omar López <zomars@me.com>

joeauyeung requested a review from zomars April 16, 2024 18:51

vercel bot had a problem deploying to Preview – platform-starter-kit April 16, 2024 18:51 Failure

zomars requested changes Apr 16, 2024

View reviewed changes

vercel bot deployed to Preview – api April 16, 2024 18:58 View deployment

vercel bot deployed to Preview – dev April 16, 2024 19:24 View deployment

joeauyeung mentioned this pull request Apr 16, 2024

fix: Minimum fix for N Calendar Invites #14617

Merged

1 task

PeerRich added the High priority Created by Linear-GitHub Sync label Apr 19, 2024

zomars closed this Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: N Calendar Invites For Collective Events #14610

fix: N Calendar Invites For Collective Events #14610

joeauyeung commented Apr 16, 2024 •

edited

linear bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 •

edited

graphite-app bot commented Apr 16, 2024

linear bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 •

edited

deploysentinel bot commented Apr 16, 2024 •

edited

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

joeauyeung Apr 16, 2024

joeauyeung Apr 16, 2024

vercel bot commented Apr 16, 2024 •

edited

keithwillcode left a comment •

edited

keithwillcode commented Apr 16, 2024

keithwillcode left a comment

keithwillcode Apr 16, 2024

joeauyeung commented Apr 16, 2024 •

edited

zomars Apr 16, 2024

joeauyeung Apr 16, 2024

keithwillcode Apr 16, 2024 •

edited

keithwillcode Apr 16, 2024

zomars commented Apr 19, 2024

fix: N Calendar Invites For Collective Events #14610

fix: N Calendar Invites For Collective Events #14610

Conversation

joeauyeung commented Apr 16, 2024 • edited

What does this PR do?

Requirement/Documentation

Type of change

How should this be tested?

Mandatory Tasks

Checklist

linear bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 • edited

graphite-app bot commented Apr 16, 2024

Graphite Automations

linear bot commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 • edited

📦 Next.js Bundle Analysis for @calcom/web

deploysentinel bot commented Apr 16, 2024 • edited

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

Test Case Results

joeauyeung Apr 16, 2024

Choose a reason for hiding this comment

joeauyeung Apr 16, 2024

Choose a reason for hiding this comment

vercel bot commented Apr 16, 2024 • edited

keithwillcode left a comment • edited

Choose a reason for hiding this comment

keithwillcode commented Apr 16, 2024

keithwillcode left a comment

Choose a reason for hiding this comment

keithwillcode Apr 16, 2024

Choose a reason for hiding this comment

joeauyeung commented Apr 16, 2024 • edited

zomars Apr 16, 2024

Choose a reason for hiding this comment

joeauyeung Apr 16, 2024

Choose a reason for hiding this comment

keithwillcode Apr 16, 2024 • edited

Choose a reason for hiding this comment

keithwillcode Apr 16, 2024

Choose a reason for hiding this comment

zomars commented Apr 19, 2024

joeauyeung commented Apr 16, 2024 •

edited

github-actions bot commented Apr 16, 2024 •

edited

github-actions bot commented Apr 16, 2024 •

edited

deploysentinel bot commented Apr 16, 2024 •

edited

vercel bot commented Apr 16, 2024 •

edited

keithwillcode left a comment •

edited

joeauyeung commented Apr 16, 2024 •

edited

keithwillcode Apr 16, 2024 •

edited