Skip to content

fix: round robin re assignment google meet bug#17378

Merged
joeauyeung merged 14 commits into
mainfrom
fix/reassignment-google-meet
Oct 29, 2024
Merged

fix: round robin re assignment google meet bug#17378
joeauyeung merged 14 commits into
mainfrom
fix/reassignment-google-meet

Conversation

@Udit-takkar
Copy link
Copy Markdown
Contributor

@Udit-takkar Udit-takkar commented Oct 28, 2024

What does this PR do?

BEFORE:- Adds cal video

Screenshot 2024-10-29 at 1 08 22 AM

Screenshot 2024-10-29 at 12 44 32 AM

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • N/A I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • mentioned in the issue description

Check all email's body

@linear
Copy link
Copy Markdown

linear Bot commented Oct 28, 2024

@github-actions github-actions Bot added calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar emails area: emails, cancellation email, reschedule email, inbox, spam folder, not getting email enterprise area: enterprise, audit log, organisation, SAML, SSO Urgent Created by Linear-GitHub Sync 🐛 bug Something isn't working labels Oct 28, 2024
@dosubot dosubot Bot added the teams area: teams, round robin, collective, managed event-types label Oct 28, 2024
@keithwillcode keithwillcode added the core area: core, team members only label Oct 28, 2024
@Udit-takkar Udit-takkar added the High priority Created by Linear-GitHub Sync label Oct 28, 2024
@Udit-takkar Udit-takkar added this to the v4.7 milestone Oct 28, 2024
@vercel
Copy link
Copy Markdown

vercel Bot commented Oct 28, 2024

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

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Oct 29, 2024 5:28pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Oct 29, 2024 5:28pm

@graphite-app graphite-app Bot requested a review from a team October 28, 2024 19:17
Copy link
Copy Markdown
Contributor Author

@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.

Self review done

const responses = responseSafeParse.success ? responseSafeParse.data : undefined;

let bookingLocation = booking.location;
if (eventType.locations.includes({ type: OrganizerDefaultConferencingAppType })) {
Copy link
Copy Markdown
Contributor Author

@Udit-takkar Udit-takkar Oct 28, 2024

Choose a reason for hiding this comment

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

includes compare object by reference and not by value so this won't ever be true even when it contains an object { type: OrganizerDefaultConferencingAppType }

Comment on lines +286 to +295
if (results.length) {
// Handle Google Meet results
// We use the original booking location since the evt location changes to daily
if (bookingLocation === MeetLocationType) {
const googleMeetResult = {
appName: GoogleMeetMetadata.name,
type: "conferencing",
uid: results[0].uid,
originalEvent: results[0].originalEvent,
};
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using the same code from handleNewBooking

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we reuse this code instead of copy/pasting?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is important logic that ideally lives in 1 place

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

handleNewBooking is also doing a lot of things during rescheduling. It's mutating a lot of variables which could be difficult to track.

Comment on lines +363 to +367
prisma.booking.update({
where: { id: bookingId },
data: { metadata },
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

update the new metadata to make sure new google meet url is saved

@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Oct 28, 2024

Graphite Automations

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

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 28, 2024

E2E results are ready!

Copy link
Copy Markdown
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.

I'm reading through the code here and in handleNewBooking and the problem is that we're applying additional logic to the return array of referencesToCreate from the EventManager. Could we instead move this logic inside of the EventManager that way we don't have multiple instances of this logic?

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.

  • I still got the wrong conferencing app when the two host's had different default apps

  • When I try to reassign my booking (both google meet default conferencing app), I am getting an error:
    Screenshot 2024-10-29 at 3 12 03 PM

Comment thread packages/features/ee/round-robin/roundRobinManualReassignment.ts
@Udit-takkar
Copy link
Copy Markdown
Contributor Author

@CarinaWolli everything should be fixed now. i was passing wrong metadata while updating booking

Copy link
Copy Markdown
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.

@Udit-takkar @CarinaWolli I tested this again and it's working as expected. @CarinaWolli has pointed out this doesn't work if the two users have different conferencing apps but for now this fixes the issue when two users have Google Meet set as their default conferencing app.

@joeauyeung joeauyeung merged commit 04a49e7 into main Oct 29, 2024
@joeauyeung joeauyeung deleted the fix/reassignment-google-meet branch October 29, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 bug Something isn't working calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar core area: core, team members only emails area: emails, cancellation email, reschedule email, inbox, spam folder, not getting email enterprise area: enterprise, audit log, organisation, SAML, SSO High priority Created by Linear-GitHub Sync ready-for-e2e teams area: teams, round robin, collective, managed event-types Urgent Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-4636] Booking manual re assignment with google meet

4 participants