Skip to content
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: Add consistent iCalUID #12122

Merged
merged 55 commits into from
Dec 15, 2023
Merged

feat: Add consistent iCalUID #12122

merged 55 commits into from
Dec 15, 2023

Conversation

joeauyeung
Copy link
Contributor

@joeauyeung joeauyeung commented Oct 27, 2023

What does this PR do?

This is a part of stacked PRs that aim to refactor how we handle .ics files.

This PR adds the iCalUID and the iCalSequence to the booking record. This is a part of a stack PRs that aims to unify the .ics file that we sent.

Fixes # (issue)

Requirement/Documentation

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

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

  • Set GCal as your destination calendar
  • Create a booking
    • The iCalUID should be ${uid}@cal.com in the DB and the .ics file sent
    • The iCalSequence should be set to 0
  • Reschedule the booking
    • In the new booking, the iCalUID should be the same as the original booking
    • The iCalSequence should be set to 1
  • Reschedule that booking a second time
    • The sequence should be set to 2
  • Cancel that booking
    • The sequence should be 3
  • Now set Outlook as your destination calendar and create a booking
    • The iCalUID should come from Outlook as you can not set the icalUID through the API

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

@joeauyeung joeauyeung requested a review from a team October 27, 2023 22:34
@vercel
Copy link

vercel bot commented Oct 27, 2023

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

Name Status Preview Comments Updated (UTC)
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 15, 2023 3:17pm
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 15, 2023 3:17pm
5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ai ⬜️ Ignored (Inspect) Visit Preview Dec 15, 2023 3:17pm
cal ⬜️ Ignored (Inspect) Visit Preview Dec 15, 2023 3:17pm
cal-demo ⬜️ Ignored (Inspect) Visit Preview Dec 15, 2023 3:17pm
qa ⬜️ Ignored (Inspect) Visit Preview Dec 15, 2023 3:17pm
ui ⬜️ Ignored (Inspect) Visit Preview Dec 15, 2023 3:17pm

@github-actions github-actions bot added the ❗️ migrations contains migration files label Oct 27, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

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

@zomars zomars added the core area: core, team members only label Oct 27, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

📦 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
Copy link

deploysentinel bot commented Oct 27, 2023

Current Playwright Test Results Summary

✅ 333 Passing - ⚠️ 23 Flaky

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

(Last updated on 12/15/2023 03:23:58pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 9be58d4

Started: 12/15/2023 03:14:28pm 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 user -- legacy Different Locations Tests Can remove location from multiple locations that are saved
Retry 1Initial Attempt
0% (0) 0 / 245 runs
failed over last 7 days
1.63% (4) 4 / 245 runs
flaked over last 7 days

📄   apps/web/playwright/locale.e2e.ts • 13 Flakes

Top 1 Common Error Messages

null

13 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
unauthorized user sees correct translations (de) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (ar) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (zh) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (zh-CN) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (zh-TW) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (pt) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (pt-br) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
unauthorized user sees correct translations (es-419) should use correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
authorized user sees correct translations (de) should return correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
authorized user sees correct translations (pt-br) should return correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
authorized user sees correct translations (ar) should return correct translations and html attributes
Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
13.79% (36) 36 / 261 runs
flaked over last 7 days
authorized user sees changed translations (de->ar) should return correct translations and html attributes
Retry 1Initial Attempt
-2.30% (-6) -6 / 261 runs
failed over last 7 days
11.49% (30) 30 / 261 runs
flaked over last 7 days
authorized user sees changed translations (de->pt-BR) [locale1] should return correct translations and html attributes
Retry 1Initial Attempt
-8.86% (-21) -21 / 237 runs
failed over last 7 days
11.81% (28) 28 / 237 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Reschedule Tests Should do a reschedule from user owner
Retry 1Initial Attempt
0% (0) 0 / 261 runs
failed over last 7 days
10.73% (28) 28 / 261 runs
flaked over last 7 days

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

Top 1 Common Error Messages

null

3 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 1Initial Attempt
0.75% (2) 2 / 266 runs
failed over last 7 days
60.53% (161) 161 / 266 runs
flaked over last 7 days
Popup Tests should open Routing Forms embed on click
Retry 1Initial Attempt
0.38% (1) 1 / 265 run
failed over last 7 days
48.30% (128) 128 / 265 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 1Initial Attempt
0% (0) 0 / 265 runs
failed over last 7 days
81.89% (217) 217 / 265 runs
flaked 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 login flow user & logout using dashboard
Retry 1Initial Attempt
0.77% (2) 2 / 260 runs
failed over last 7 days
25.38% (66) 66 / 260 runs
flaked over last 7 days

📄   apps/web/playwright/team/team-invitation.e2e.ts • 2 Flakes

Top 1 Common Error Messages

null

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Team Invitation (non verified)
Retry 1Initial Attempt
4.56% (12) 12 / 263 runs
failed over last 7 days
92.02% (242) 242 / 263 runs
flaked over last 7 days
Team Invitation (verified)
Retry 1Initial Attempt
0.38% (1) 1 / 263 run
failed over last 7 days
93.92% (247) 247 / 263 runs
flaked over last 7 days

📄   apps/web/playwright/organization/organization-creation.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Organization should be able to create an organization and complete onboarding
Retry 1Initial Attempt
22.22% (60) 60 / 270 runs
failed over last 7 days
45.56% (123) 123 / 270 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
FORM_SUBMITTED on submitting user form, triggers user webhook
Retry 1Initial Attempt
0.38% (1) 1 / 264 run
failed over last 7 days
6.06% (16) 16 / 264 runs
flaked over last 7 days

View Detailed Build Results


Copy link
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. But we should also add unit tests to this to confirm the logic for calculating the UID and sequence.


*/
-- AlterTable
ALTER TABLE "Booking" ADD COLUMN "iCalSequence" INTEGER NOT NULL DEFAULT 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These adds are out of order compared to the code. Intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify - The order specified throughout the code is iCalUID and then iCalSequence. See prisma schema.

This alter table statement has them in the reverse order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was generated using prisma migrate. I can fix this and moving forward ensure that the adds line up with the schema.

packages/features/bookings/lib/handleNewBooking.ts Outdated Show resolved Hide resolved
packages/features/bookings/lib/handleNewBooking.ts Outdated Show resolved Hide resolved
*/
-- AlterTable
ALTER TABLE "Booking" ADD COLUMN "iCalSequence" INTEGER NOT NULL DEFAULT 0,
ADD COLUMN "iCalUID" TEXT NOT NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a TEXT column and not VARCHAR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was generated using prisma migrate. I can change this and moving forward I'll change the column type to VARCHAR when appropriate.

@@ -417,6 +417,8 @@ model Booking {
/// @zod.custom(imports.bookingMetadataSchema)
metadata Json?
isRecorded Boolean @default(false)
iCalUID String @db.VarChar()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithwillcode added @db.VarChar() to the schema maps the column to VARCHAR since Prisma will default map the column to TEXT for Postgres.

https://www.prisma.io/docs/reference/api-reference/prisma-schema-reference#default-type-mappings

@@ -42,6 +43,8 @@ export default class AttendeeWasRequestedToRescheduleEmail extends OrganizerSche
// @OVERRIDE
protected getiCalEventAsString(): string | undefined {
const icsEvent = createEvent({
uid: this.calEvent.iCalUID || this.calEvent.uid!,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I'm tending to start telling people to use nullish coalescing, eg:

Suggested change
uid: this.calEvent.iCalUID || this.calEvent.uid!,
uid: this.calEvent.iCalUID ?? this.calEvent.uid!,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we have no guarantee of this.calEvent.uid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting. I've seen this in our codebase before. Looking at the schema, uid is required when saving bookings. Although the CalendarEvent type we often use in the code base sets uid as possibly undefined.

@@ -42,6 +43,8 @@ export default class AttendeeWasRequestedToRescheduleEmail extends OrganizerSche
// @OVERRIDE
protected getiCalEventAsString(): string | undefined {
const icsEvent = createEvent({
uid: this.calEvent.iCalUID || this.calEvent.uid!,
sequence: 100,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question-non-blocking: What's sequence 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when calendars parse .ics files they look at the sequence number to determine the most up-to-date information. Sequences start at 0. When requesting to reschedule, we cancel the calendar event. As a fallback, I wanted the sequence to be so high that it would likely be the most up-to-date .ics file.

@keithwillcode keithwillcode added Medium priority Created by Linear-GitHub Sync and removed High priority Created by Linear-GitHub Sync labels Dec 15, 2023
Copy link
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked with @joeauyeung and this has been tested thoroughly.

@keithwillcode keithwillcode merged commit 9dfa596 into main Dec 15, 2023
40 checks passed
@keithwillcode keithwillcode deleted the write-icaluid-to-booking branch December 15, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin ❗️ changes requested Waiting for the author to implement fixes/suggestions core area: core, team members only ❗️ .env changes contains changes to env variables Medium priority Created by Linear-GitHub Sync ❗️ migrations contains migration files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants