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

fix: rescheduling round robin events (emails and calendar events) #12469

Merged
merged 13 commits into from
Nov 27, 2023

Conversation

CarinaWolli
Copy link
Member

@CarinaWolli CarinaWolli commented Nov 21, 2023

What does this PR do?

Fixes issues with rescheduling round-robin events when they are assigned to a new host.

  1. Emails
  • Old rr host will get cancelation email
  • New rr host will get scheduling emails
  • Other fixed hosts and attendees that stay the same receive rescheduling email
  1. Calendar events
  • If the organizer of the event changes: Old event gets deleted and a new event gets created with adapted event title
  • If only rr host changed but there is a fixed host as the organizer event just gets rescheduled
  1. Organizer's default location
  • If location of event type is oragnizer's default location, it will take the default location of the new rr host

Also fixes that Google Meet link wasn't saved in metadata (videoCallUrl)

Fixes #12350
Fixes #11547
Fixes #12341

Loom Video: https://www.loom.com/share/e8964cbd58d44877a138c9c0b4a9f4e0

Type of change

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

How should this be tested?

  • Create round robin event and assign user1 and user2 as round-robin hosts.
  • Set location as 'organizer's default'
  • Make user2's default location Google Meet
  • Connect calendars for both users
  • Book rr
  • Check if calendar event is correctly created
  • Reschedule booking (make sure the other user is available, so it will be assigned to a new user)
  • Check if calendar event is removed from original booking and new event is created on the new organizer's calendar
  • Check Emails:
    • previous host -> cancellation email
    • new host -> scheduling email
    • attendee -> rescheduling email

Mandatory Tasks

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

Copy link

vercel bot commented Nov 21, 2023

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 Nov 27, 2023 5:18pm
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 5:18pm
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 5:18pm
4 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 5:18pm
cal-demo ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 5:18pm
qa ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 5:18pm
ui ⬜️ Ignored (Inspect) Visit Preview Nov 27, 2023 5:18pm

Copy link
Contributor

github-actions bot commented Nov 21, 2023

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

@zomars zomars added the core area: core, team members only label Nov 21, 2023
Copy link
Contributor

github-actions bot commented Nov 21, 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! 🙌

Copy link

deploysentinel bot commented Nov 21, 2023

Current Playwright Test Results Summary

✅ 327 Passing - ⚠️ 25 Flaky

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

(Last updated on 11/27/2023 05:20:55pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 6fcff69

Started: 11/27/2023 05:12:01pm UTC

⚠️ Flakes

📄   apps/web/playwright/booking-seats.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Booking with Seats Attendees can cancel a seated event time slot
Retry 1Initial Attempt
0% (0) 0 / 270 runs
failed over last 7 days
1.85% (5) 5 / 270 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 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (ar) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (zh) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (zh-CN) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (zh-TW) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (pt) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (pt-br) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.51% (-98) -98 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
unauthorized user sees correct translations (es-419) should use correct translations and html attributes
Retry 2Retry 1Initial Attempt
-35.87% (-99) -99 / 276 runs
failed over last 7 days
35.87% (99) 99 / 276 runs
flaked over last 7 days
authorized user sees correct translations (de) should return correct translations and html attributes
Retry 2Retry 1Initial Attempt
-34.31% (-94) -94 / 274 runs
failed over last 7 days
36.13% (99) 99 / 274 runs
flaked over last 7 days
authorized user sees correct translations (pt-br) should return correct translations and html attributes
Retry 2Retry 1Initial Attempt
-34.57% (-93) -93 / 269 runs
failed over last 7 days
36.80% (99) 99 / 269 runs
flaked over last 7 days
authorized user sees correct translations (ar) should return correct translations and html attributes
Retry 2Retry 1Initial Attempt
-36.88% (-97) -97 / 263 runs
failed over last 7 days
37.26% (98) 98 / 263 runs
flaked over last 7 days
authorized user sees changed translations (de->ar) should return correct translations and html attributes
Retry 2Retry 1Initial Attempt
-13.79% (-36) -36 / 261 runs
failed over last 7 days
27.59% (72) 72 / 261 runs
flaked over last 7 days
authorized user sees changed translations (de->pt-BR) [locale1] should return correct translations and html attributes
Retry 2Retry 1Initial Attempt
-14.22% (-32) -32 / 225 runs
failed over last 7 days
24.89% (56) 56 / 225 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
1.76% (5) 5 / 284 runs
failed over last 7 days
5.28% (15) 15 / 284 runs
flaked over last 7 days

📄   apps/web/playwright/teams.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
Teams - NonOrg Non admin team members cannot create team in org
Retry 1Initial Attempt
3.40% (10) 10 / 294 runs
failed over last 7 days
29.93% (88) 88 / 294 runs
flaked over last 7 days
Teams - Org Can create teams via Wizard
Retry 1Initial Attempt
7.12% (21) 21 / 295 runs
failed over last 7 days
9.83% (29) 29 / 295 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.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
Popup Tests should be able to reschedule
Retry 2Retry 1Initial Attempt
10.85% (32) 32 / 295 runs
failed over last 7 days
85.42% (252) 252 / 295 runs
flaked over last 7 days
Popup Tests should open embed iframe on click - Configured with light theme
Retry 1Initial Attempt
0.68% (2) 2 / 295 runs
failed over last 7 days
54.24% (160) 160 / 295 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
1.35% (4) 4 / 297 runs
failed over last 7 days
68.69% (204) 204 / 297 runs
flaked over last 7 days
Popup Tests should open Routing Forms embed on click
Retry 1Initial Attempt
2.03% (6) 6 / 296 runs
failed over last 7 days
30.07% (89) 89 / 296 runs
flaked over last 7 days

📄   apps/web/playwright/settings/upload-avatar.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
UploadAvatar can upload an image
Retry 2Retry 1Initial Attempt
0% (0) 0 / 272 runs
failed over last 7 days
8.09% (22) 22 / 272 runs
flaked over last 7 days

📄   apps/web/playwright/embed-code-generator.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
Embed Code Generator Tests Event Types Page open Embed Dialog and choose element-click for First Event Type
Retry 1Initial Attempt
1.02% (3) 3 / 295 runs
failed over last 7 days
3.05% (9) 9 / 295 runs
flaked over last 7 days
Embed Code Generator Tests Event Types Page open Embed Dialog and choose floating-popup for First Event Type
Retry 1Initial Attempt
1.02% (3) 3 / 294 runs
failed over last 7 days
3.40% (10) 10 / 294 runs
flaked over last 7 days

📄   apps/web/playwright/managed-event-types.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Managed Event Types tests Can create managed event type
Retry 1Initial Attempt
8.87% (26) 26 / 293 runs
failed over last 7 days
8.53% (25) 25 / 293 runs
flaked over last 7 days

View Detailed Build Results


@CarinaWolli CarinaWolli added 🐛 bug Something isn't working High priority Created by Linear-GitHub Sync labels Nov 22, 2023
log.debug("RescheduleOrganizerChanged: Deleting Event and Meeting for previous booking");
await this.deleteEventsAndMeetings({ booking, event });
// New event is created in handleNewBooking
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

else part is normal rescheduled logic, nothing changed here

evt.location = updatedEvent.url;
if (changedOrganizer) {
log.debug("RescheduleOrganizerChanged: Deleting Event and Meeting for previous booking");
await this.deleteEventsAndMeetings({ booking, event });
Copy link
Member Author

Choose a reason for hiding this comment

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

When the organizer changes:

  • delete event
  • create event with new organizer

Copy link
Contributor

Choose a reason for hiding this comment

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

Once this is merged, we should consider the effect on #12122. Since a completely new calendar event is being created.


const updatedEvt = {
...evt,
destinationCalendar: originalRescheduledBooking?.destinationCalendar
Copy link
Member Author

Choose a reason for hiding this comment

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

If organizer has changed, the destinationCalendar of evt would be the new organizer otherwise

@@ -48,619 +49,441 @@ describe("handleNewBooking", () => {
setupAndTeardown();

describe("Reschedule", () => {
test(
`should rechedule an existing booking successfully with Cal Video(Daily Video)
describe("User event-type", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing changed for User event-type tests

);
});
});
describe("Team event-type", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Added new tests here

@@ -2373,7 +2581,7 @@ async function handler(

const metadata = videoCallUrl
? {
videoCallUrl: getVideoCallUrlFromCalEvent(evt),
videoCallUrl: getVideoCallUrlFromCalEvent(evt) || videoCallUrl,
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sure that Google Meet links is saved in metadata

expectBookingRequestedEmails({
booker,
organizer,
expectSuccessfulRoudRobinReschedulingEmails({
Copy link
Member Author

Choose a reason for hiding this comment

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

Expects that roundRobinHost1 receives cancelation email and roundRobinHost2 to receive scheduling email

expectSuccessfulBookingRescheduledEmails({
booker,
organizer,
expectSuccessfulRoudRobinReschedulingEmails({
Copy link
Member Author

Choose a reason for hiding this comment

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

Expects that round roundRobinHost1 recieves rescheduling email

Copy link
Contributor

@joeauyeung joeauyeung left a comment

Choose a reason for hiding this comment

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

Changes are looking good.

I just had a question about saving the Google Meet links in the metadata. I thought we were trying to move away from relying on metadata. Should the new Meet link be saved as a bookingReference?

evt.location = updatedEvent.url;
if (changedOrganizer) {
log.debug("RescheduleOrganizerChanged: Deleting Event and Meeting for previous booking");
await this.deleteEventsAndMeetings({ booking, event });
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this is merged, we should consider the effect on #12122. Since a completely new calendar event is being created.

if (updatedEvent) {
evt.videoCallData = updatedEvent;
evt.location = updatedEvent.url;
if (changedOrganizer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how I feel about calling the event manager reschedule and the create methods back to back. How do we feel about calling only one event manager method and handling the deletion and creation of the calendar events?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. IMO reschedule should handle event recreation. But could be done in a follow up

Copy link
Member

@zomars zomars left a comment

Choose a reason for hiding this comment

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

LGTM

@CarinaWolli
Copy link
Member Author

Agreed. IMO reschedule should handle event recreation. But could be done in a follow up

Totally agree with that, I will do a follow-up.

I just had a question about saving the Google Meet links in the metadata. I thought we were trying to move away from relying on metadata. Should the new Meet link be saved as a bookingReference?

@joeauyeung We should have it in both booking reference and metadata. I know we are trying to move away from metadata but right now we are still using it for getting the videocall url in webhooks, workflows, etc.
But I do see that the bookingReference isn't saved correctly when booking a round-robin event, I will revisit that as well in the follow-up

@CarinaWolli CarinaWolli merged commit 230b82d into main Nov 27, 2023
36 checks passed
@CarinaWolli CarinaWolli deleted the fix/rr-rescheduling branch November 27, 2023 18:09
zomars pushed a commit that referenced this pull request Nov 28, 2023
## What does this PR do?

Follow up for #12469

Event creation is now already handled in the reschedule function of EventManager. 

## Type of change

- [x] Chore (refactoring code, technical debt, workflow improvements)

## How should this be tested?

- Same as in #12469
jakazzy pushed a commit to jakazzy/cal.com that referenced this pull request Dec 5, 2023
…lcom#12469)

* send the correct booking email for round robin rescheduling

* fixing originalBookingMemberEmails

* fix calendar event (needs some code refactoring)

* refactor rescheduling code

* code clean up

* add comment

* fix event name if host changes

* add tests for rr rescheduling emails

* fix videoCallUrl of google meet

* code clean up

* fix destinationCalendar for new booking

---------

Co-authored-by: CarinaWolli <wollencarina@gmail.com>
jakazzy pushed a commit to jakazzy/cal.com that referenced this pull request Dec 5, 2023
…m#12574)

## What does this PR do?

Follow up for calcom#12469

Event creation is now already handled in the reschedule function of EventManager. 

## Type of change

- [x] Chore (refactoring code, technical debt, workflow improvements)

## How should this be tested?

- Same as in calcom#12469
hbjORbj pushed a commit to codemod-com/cal.com-demo that referenced this pull request Dec 21, 2023
…lcom#12469)

* send the correct booking email for round robin rescheduling

* fixing originalBookingMemberEmails

* fix calendar event (needs some code refactoring)

* refactor rescheduling code

* code clean up

* add comment

* fix event name if host changes

* add tests for rr rescheduling emails

* fix videoCallUrl of google meet

* code clean up

* fix destinationCalendar for new booking

---------

Co-authored-by: CarinaWolli <wollencarina@gmail.com>
hbjORbj pushed a commit to codemod-com/cal.com-demo that referenced this pull request Dec 21, 2023
…m#12574)

## What does this PR do?

Follow up for calcom#12469

Event creation is now already handled in the reschedule function of EventManager. 

## Type of change

- [x] Chore (refactoring code, technical debt, workflow improvements)

## How should this be tested?

- Same as in calcom#12469
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working core area: core, team members only High priority Created by Linear-GitHub Sync
Projects
None yet
3 participants