Skip to content

fix: Duplicate Calendar Invites on rescheduling an accepted booking that requires confirmation#11827

Merged
CarinaWolli merged 3 commits intomainfrom
fix/duplicate-calendar-events-for-rescheduled-booking
Oct 12, 2023
Merged

fix: Duplicate Calendar Invites on rescheduling an accepted booking that requires confirmation#11827
CarinaWolli merged 3 commits intomainfrom
fix/duplicate-calendar-events-for-rescheduled-booking

Conversation

@hariombalhara
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara commented Oct 11, 2023

What does this PR do?

Fixes #8444

Reason behind the issue was simply that the code always assumed that rescheduling would move the calendar invite to new time but in case of a booking that requires confirmation it is not true as the booking goes to PENDING state on reschedule.

Added a test as well for the flow
Before Fix - Loom
After Fix - Loom

Type of change

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

How should this be tested?

  • Turn on event approval in settings
  • Do a booking as booker
  • Accept the booking as organizer
  • Reschedule the booking as booker. At this point notice that the calendar event didn't get cancelled, it just moved to new time without confirmation from organizer.
  • Confirm the new booking. Now, you would see two calendar invites.

Mandatory Tasks

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

@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 11, 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 Oct 12, 2023 5:31am
api ❌ Failed (Inspect) Oct 12, 2023 5:31am
cal-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 5:31am
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 5:31am
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 5:31am
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Oct 12, 2023 5:31am
qa ⬜️ Ignored (Inspect) Visit Preview Oct 12, 2023 5:31am

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 11, 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 Oct 11, 2023
@hariombalhara hariombalhara changed the title Delete calendar events and meetings for previous booking in case of b… fix: Duplicate Calendar Invites on rescheduling an accepted booking that requires confirmation Oct 11, 2023
};
};

export const deleteEvent = async (
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was unused method, started using it and fixed it.

videoCredential = this.videoCredentials
// Whenever a new video connection is added, latest credentials are added with the highest ID.
// Because you can't rely on having them in the highest first order here, ensure this by sorting in DESC order
.sort((a, b) => {
Copy link
Copy Markdown
Member Author

@hariombalhara hariombalhara Oct 11, 2023

Choose a reason for hiding this comment

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

Sort does the sorting in place, so it modifies this.videoCredentials itself. Moved this sorting to constructor to make the intention more clear and so that others don't need to do it again.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 11, 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
Copy Markdown

deploysentinel Bot commented Oct 11, 2023

Current Playwright Test Results Summary

✅ 147 Passing - ⚠️ 3 Flaky

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

(Last updated on 10/12/2023 05:33:33am UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 4a361d0

Started: 10/12/2023 05:31:05am UTC

⚠️ Flakes

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
2FA Tests should allow a user to enable 2FA and login using 2FA
Retry 1Initial Attempt
1.37% (4) 4 / 291 runs
failed over last 7 days
36.08% (105) 105 / 291 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.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
Popup Tests should be able to reschedule
Retry 1Initial Attempt
5.25% (16) 16 / 305 runs
failed over last 7 days
94.10% (287) 287 / 305 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
2.29% (7) 7 / 306 runs
failed over last 7 days
18.95% (58) 58 / 306 runs
flaked over last 7 days

View Detailed Build Results


@hariombalhara hariombalhara force-pushed the fix/duplicate-calendar-events-for-rescheduled-booking branch from 46581f0 to 3ec69ad Compare October 11, 2023 12:47
reference.type.includes("_calendar")
);
// There was a case that booking didn't had any reference and we don't want to throw error on function
if (bookingCalendarReference) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Extracted out in a variable.

Comment on lines +370 to +389
// If the reschedule doesn't require confirmation, we can "update" the events and meetings to new time.
const isDedicated = evt.location ? isDedicatedIntegration(evt.location) : null;
// If and only if event type is a dedicated meeting, update the dedicated video meeting.
if (isDedicated) {
const result = await this.updateVideoEvent(evt, booking);
const [updatedEvent] = Array.isArray(result.updatedEvent)
? result.updatedEvent
: [result.updatedEvent];

if (updatedEvent) {
evt.videoCallData = updatedEvent;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved existing code into this else condition

},
};
}
if (booking.attendees) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Support creating fake attendees as well.

});
},
deleteEvent: async (...rest: any[]) => {
log.silly("mockCalendar.deleteEvent", JSON.stringify({ rest }));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Verify deleteEvent calls

});
},
deleteMeeting: async (...rest: any[]) => {
log.silly("MockVideoApiAdapter.deleteMeeting", JSON.stringify(rest));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Allow verifying deleteMeeting calls

Comment on lines +101 to +106
// Whenever a new video connection is added, latest credentials are added with the highest ID.
// Because you can't rely on having them in the highest first order here, ensure this by sorting in DESC order
// We also don't have updatedAt or createdAt dates on credentials so this is the best we can do
.sort((a, b) => {
return b.id - a.id;
});
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved here from other place.

Comment thread packages/features/bookings/lib/handleNewBooking.ts Outdated
Comment thread packages/features/bookings/lib/handleNewBooking.ts
Comment thread packages/features/bookings/lib/handleNewBooking.ts Outdated
@hariombalhara hariombalhara force-pushed the fix/duplicate-calendar-events-for-rescheduled-booking branch from 73db1d5 to 4a361d0 Compare October 12, 2023 05:23
);

// eslint-disable-next-line playwright/no-skipped-test
test.skip(
Copy link
Copy Markdown
Member Author

@hariombalhara hariombalhara Oct 12, 2023

Choose a reason for hiding this comment

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

Wrote the following 2 tests but they were failing as there is bug in identifying properly when an organizer reschedules the booking. Fill fix it in the followup PR as that's taking time.

Copy link
Copy Markdown
Member

@CarinaWolli CarinaWolli left a comment

Choose a reason for hiding this comment

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

Tested and works. Code looks good as well 🙏

@CarinaWolli CarinaWolli merged commit db059d8 into main Oct 12, 2023
@CarinaWolli CarinaWolli deleted the fix/duplicate-calendar-events-for-rescheduled-booking branch October 12, 2023 12:29
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 core area: core, team members only High priority Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-1694] Duplicate events when using the event approval feature.

3 participants