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: Seat Attendee Rescheduling Logic #14784

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: Seat Attendee Rescheduling Logic #14784

wants to merge 7 commits into from

Conversation

joeauyeung
Copy link
Contributor

@joeauyeung joeauyeung commented Apr 28, 2024

What does this PR do?

This PR aims to fix bugs around when attendees reschedule a seated event to different time slots. We do this by:

  • Ensure the correct iCalUID is passed for the original rescheduled booking and the new time slot booking if applicable
  • Only calling the necessary rescheduling logic from the EventManager
  • Moving generating iCalUIDs from uuidV5 to uuidV4 to avoid collisions

Fixes # (issue)

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?

  • Book a seated event
  • Book a second attendee in that slot
    • The calendar event should have both attendees
  • Reschedule the second attendee to a time slot with no attendees
    • Should be done in an incognito window
    • There should be two calendar events with the correct attendees
  • Book a third attendee in the first time slot
  • Reschedule the third attendee to the time slot with the second attendee
    • The calendar events should have the correct number of attendees

Mandatory Tasks

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

Checklist

  • I haven't checked if new and existing unit tests pass locally with my changes

Copy link

vercel bot commented Apr 28, 2024

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 May 8, 2024 7:26pm
platform-starter-kit ❌ Failed (Inspect) May 8, 2024 7:26pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview May 8, 2024 7:26pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview May 8, 2024 7:26pm

Copy link
Contributor

github-actions bot commented Apr 28, 2024

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

Copy link
Contributor

github-actions bot commented Apr 28, 2024

📦 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 Apr 28, 2024

Current Playwright Test Results Summary

✅ 319 Passing - ❌ 1 Failing - ⚠️ 27 Flaky

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

(Last updated on 05/08/2024 07:39:06pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: f381d13

Started: 05/08/2024 07:35:19pm UTC

❌ Failures

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Update Profile Can resend verification link if the secondary email is unverified
Retry 2Retry 1Initial Attempt
Error: expect(received).toEqual(expected) // deep equality...
expect(received).toEqual(expected) // deep equality

Expected: "http://localhost:3000/api/auth/verify-email?token=undefined"
Received: "http://localhost:3000/api/auth/verify-email?token=add661bb5e800bf126d7cab432de6e71f428401e39ac6def9aff0a088af0f186"
9.54% (37) 37 / 388 runs
failed over last 7 days
23.97% (93) 93 / 388 runs
flaked over last 7 days

⚠️ Flakes

📄   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 1Initial Attempt
1.84% (6) 6 / 326 runs
failed over last 7 days
9.20% (30) 30 / 326 runs
flaked over last 7 days

📄   packages/app-store/routing-forms/playwright/tests/basic.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
Routing Forms Seeded Routing Form Router URL should work
Retry 1Initial Attempt
0% (0) 0 / 352 runs
failed over last 7 days
11.65% (41) 41 / 352 runs
flaked over last 7 days
Routing Forms Seeded Routing Form Test preview should return correct route
Retry 1Initial Attempt
0.28% (1) 1 / 352 run
failed over last 7 days
37.50% (132) 132 / 352 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Settings/admin tests -- future should render /settings/admin page
Retry 1Initial Attempt
0% (0) 0 / 387 runs
failed over last 7 days
7.75% (30) 30 / 387 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Organization Email not matching orgAutoAcceptEmail nonexisting user invited to a Team inside organization
Retry 1Initial Attempt
0.58% (2) 2 / 342 runs
failed over last 7 days
8.77% (30) 30 / 342 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 1Initial Attempt
4.82% (17) 17 / 353 runs
failed over last 7 days
53.82% (190) 190 / 353 runs
flaked over last 7 days
Popup Tests should be able to reschedule
Retry 1Initial Attempt
-130.14% (-190) -190 / 146 runs
failed over last 7 days
130.14% (190) 190 / 146 runs
flaked over last 7 days
Popup Tests should open Routing Forms embed on click
Retry 1Initial Attempt
-131.03% (-190) -190 / 145 runs
failed over last 7 days
131.03% (190) 190 / 145 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
-128.28% (-186) -186 / 145 runs
failed over last 7 days
128.28% (186) 186 / 145 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 1Initial Attempt
-128.28% (-186) -186 / 145 runs
failed over last 7 days
128.28% (186) 186 / 145 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 1Initial Attempt
-128.28% (-186) -186 / 145 runs
failed over last 7 days
128.28% (186) 186 / 145 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 1Initial Attempt
-128.28% (-186) -186 / 145 runs
failed over last 7 days
128.28% (186) 186 / 145 runs
flaked over last 7 days
Popup Tests prendered embed should be loaded and apply the config given to it
Retry 1Initial Attempt
-128.28% (-186) -186 / 145 runs
failed over last 7 days
128.28% (186) 186 / 145 runs
flaked over last 7 days
Popup Tests should open on clicking child element
Retry 1Initial Attempt
-125.52% (-182) -182 / 145 runs
failed over last 7 days
125.52% (182) 182 / 145 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 -- future user can add multiple organizer address
Retry 1Initial Attempt
1.10% (4) 4 / 363 runs
failed over last 7 days
26.17% (95) 95 / 363 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 Add inline embed using a namespace without reload
Retry 1Initial Attempt
0.56% (2) 2 / 355 runs
failed over last 7 days
51.83% (184) 184 / 355 runs
flaked over last 7 days
Namespacing Inline Embed Double install Embed Snippet with inline embed using a namespace
Retry 1Initial Attempt
1.13% (4) 4 / 355 runs
failed over last 7 days
52.39% (186) 186 / 355 runs
flaked over last 7 days
Namespacing Inline Embed Double install Embed Snippet with inline embed without a namespace(i.e. default namespace)
Retry 1Initial Attempt
0% (0) 0 / 355 runs
failed over last 7 days
52.96% (188) 188 / 355 runs
flaked over last 7 days
Namespacing Different namespaces can have different init configs
Retry 1Initial Attempt
0% (0) 0 / 355 runs
failed over last 7 days
52.39% (186) 186 / 355 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 Team Onboarding Invite Members
Retry 1Initial Attempt
4.37% (16) 16 / 366 runs
failed over last 7 days
23.50% (86) 86 / 366 runs
flaked over last 7 days

📄   apps/web/playwright/profile.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
Update Profile Can update a users email (verification enabled)
Retry 1Initial Attempt
25% (97) 97 / 388 runs
failed over last 7 days
33.51% (130) 130 / 388 runs
flaked over last 7 days
Update Profile Can delete the newly added secondary email
Retry 1Initial Attempt
0% (0) 0 / 388 runs
failed over last 7 days
0.52% (2) 2 / 388 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Unpublished Organization user event-type
Retry 1Initial Attempt
0% (0) 0 / 352 runs
failed over last 7 days
0.28% (1) 1 / 352 run
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 -- future login flow user & logout using dashboard
Retry 2Retry 1Initial Attempt
4.44% (16) 16 / 360 runs
failed over last 7 days
32.22% (116) 116 / 360 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 1Initial Attempt
0.82% (3) 3 / 367 runs
failed over last 7 days
10.35% (38) 38 / 367 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Bookings Team Event Can create a booking for Round Robin EventType
Retry 1Initial Attempt
9.19% (33) 33 / 359 runs
failed over last 7 days
28.41% (102) 102 / 359 runs
flaked over last 7 days

📄   packages/embeds/embed-react/playwright/tests/basic.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
React Embed Element Click Popup should verify that the iframe got created with correct URL - namespaced
Retry 1Initial Attempt
18.38% (68) 68 / 370 runs
failed over last 7 days
38.38% (142) 142 / 370 runs
flaked over last 7 days

View Detailed Build Results


@@ -1,5 +1,5 @@
import short from "short-uuid";
import { v5 as uuidv5 } from "uuid";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting iCalUID collisions. Looking into this we should be using uuidv4 for truly random uids

https://generate-uuid.com/which-uuid-version-should-you-use#:~:text=You%20should%20generally%20be%20using,the%20inputs%20name%20and%20namespace%20

@@ -15,11 +15,13 @@ const getICalUID = ({
uid,
event,
defaultToEventUid,
attendeeId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the attendeeId to the id to avoid further collisions

} else {
// Rescheduling logic for the original seated event was handled in handleSeats
// We want to use new booking logic for the new time slot
originalRescheduledBooking = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If originalRescheduledbooking was defined then we would have hit the rescheduling logic. That's already handled in handleSeats

Comment on lines +26 to +42
// Update the original calendar event by removing the attendee that is rescheduling
if (originalBookingEvt && originalRescheduledBooking) {
// Event would probably be deleted so we first check than instead of updating references
const filteredAttendees = originalRescheduledBooking?.attendees.filter((attendee) => {
return attendee.email !== bookerEmail;
});
const deletedReference = await lastAttendeeDeleteBooking(
originalRescheduledBooking,
filteredAttendees,
originalBookingEvt
);

if (!deletedReference) {
await eventManager.updateCalendarAttendees(originalBookingEvt, originalRescheduledBooking);
}
}

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 block removes the rescheduling attendee from the calendar event. This should have been happening for every rescheduled booking instead of if there was no newTimeSlotBooking

Comment on lines +80 to +88
// Add the new attendees to the new time slot booking attendees
for (const attendee of newTimeSlotBooking.attendees) {
const language = await getTranslation(attendee.locale ?? "en", "common");
evt.attendees.push({
email: attendee.email,
name: attendee.name,
language,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The evt that was being passed from handleNewBooking only contained the rescheduling attendee. Let's add all attendees from the newTimeSlotBooking before updating the event.

Comment on lines +42 to +49
iCalUID: true,
userId: true,
attendees: {
include: {
bookingSeat: true,
},
},
references: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the iCalUID and references of the newTimeSlotBooking so that the proper 3rd party data is being updated.

@@ -76,17 +77,19 @@ const attendeeRescheduleSeatedBooking = async (
]);
}

const copyEvent = cloneDeep(evt);

const updateManager = await eventManager.reschedule(copyEvent, rescheduleUid, newTimeSlotBooking.id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were calling the whole reschedule logic of the EventManager class but we only need to update calendar attendees.

evt.iCalUID = Array.isArray(calendarResult?.updatedEvent)
? calendarResult?.updatedEvent[0]?.iCalUID
: calendarResult?.updatedEvent?.iCalUID || undefined;
await eventManager.updateCalendarAttendees(copyEvent, newTimeSlotBooking);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only update the new time slot calendar event with the rescheduled attendee. Emails are still being sent to the organizer and rescheduled attendee.

@joeauyeung joeauyeung requested a review from emrysal April 28, 2024 17:10
@joeauyeung joeauyeung added the seats area: seats, guest meetings, multiple people label Apr 28, 2024
@joeauyeung joeauyeung marked this pull request as ready for review April 28, 2024 17:11
@graphite-app graphite-app bot requested a review from a team April 28, 2024 17:11
@dosubot dosubot bot added bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working labels Apr 28, 2024
Copy link

graphite-app bot commented Apr 28, 2024

Graphite Automations

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

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

@PeerRich
Copy link
Member

hell yeah! this will make many people incluiding me very happy

Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

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

@joeauyeung

Maybe something wrong with my setup. I tried enabling "Offer seats" and in the video you can see that it displayed 2 are available for slot 9:45 am but after one attendee has booked then that slot is no longer displayed.

Screen.Recording.2024-04-29.at.4.36.29.PM.mov

@joeauyeung
Copy link
Contributor Author

@joeauyeung

Maybe something wrong with my setup. I tried enabling "Offer seats" and in the video you can see that it displayed 2 are available for slot 9:45 am but after one attendee has booked then that slot is no longer displayed.

Screen.Recording.2024-04-29.at.4.36.29.PM.mov

Interesting, I dug into it, and the collective hosts other than the organizer count towards the attendee count. That's why the slot is disappearing for you. I'll create a ticket for this.

@keithwillcode keithwillcode added this to the v4.1 milestone May 8, 2024
Copy link
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working consumer core area: core, team members only seats area: seats, guest meetings, multiple people Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants