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

Webex integration #7651

Merged
merged 57 commits into from
May 11, 2023
Merged

Webex integration #7651

merged 57 commits into from
May 11, 2023

Conversation

aar2dee2
Copy link
Contributor

@aar2dee2 aar2dee2 commented Mar 10, 2023

What does this PR do?

Fixes #2744
/claim #2744

Environment: Staging(main branch) / Production

Type of change

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

How should this be tested?

  • Test A
  • Test B

Checklist

  • I haven't checked if my changes generate no new warnings
  • 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

@aar2dee2 aar2dee2 requested a review from zomars as a code owner March 10, 2023 07:01
@aar2dee2 aar2dee2 requested review from a team March 10, 2023 07:01
@vercel
Copy link

vercel bot commented Mar 10, 2023

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

Name Status Preview Comments Updated (UTC)
cal ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 4:16am
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2023 4:16am

@vercel
Copy link

vercel bot commented Mar 10, 2023

@aar2dee2 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the ❗️ .env changes contains changes to env variables label Mar 10, 2023
@aar2dee2
Copy link
Contributor Author

Hi,

I'm using the Zoom integration as a reference for building the Webex integration.
The Oauth is working:

Screen.Recording.2023-03-09.at.5.24.17.PM.mov

1/ I'm trying to test creating a meeting, but there's a Prisma error in the event types table.
Screenshot 2023-03-09 at 6 22 05 PM

How should I fix this?

2/ I'm working on functionality for

  • listing meetings
  • creating a meeting
  • updating a meeting
  • deleting a meeting

Are any other functions required from the integration?

3/ Other TODOs

- Screenshots
- Tests
-What else should be there?

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 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! 🙌

@sean-brydon
Copy link
Member

Hi,

I'm using the Zoom integration as a reference for building the Webex integration. The Oauth is working:

1/ I'm trying to test creating a meeting, but there's a Prisma error in the event types table. Screenshot 2023-03-09 at 6 22 05 PM

How should I fix this?

2/ I'm working on functionality for

  • listing meetings
  • creating a meeting
  • updating a meeting
  • deleting a meeting

Are any other functions required from the integration?

3/ Other TODOs

- Screenshots
- Tests
-What else should be there?

Can you try running yarn clean && yarn dx in project root - looks like you might be missing a migration

@hariombalhara hariombalhara dismissed their stale review May 10, 2023 12:38

Issue fixed now

6. Give your app a name.
7. Upload an icon or choose one of the default icons.
8. Give your app a short description.
9. Set the Redirect URI as `<Cal.com URL>/api/integrations/webex/callback` replacing Cal.com URL with the URI at which your application runs. Also add the `<Cal.com URL>/api/auth/callback/webex` in Redirect URIs to allow authentication.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
9. Set the Redirect URI as `<Cal.com URL>/api/integrations/webex/callback` replacing Cal.com URL with the URI at which your application runs. Also add the `<Cal.com URL>/api/auth/callback/webex` in Redirect URIs to allow authentication.
9. Set the Redirect URI as `<Cal.com URL>/api/integrations/webex/callback` replacing Cal.com URL with the URI at which your application runs.

/auth/callback is not needed

Copy link
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

LGTM now.

Tests:

  • Verified that the meeting is successfully created in webex after following the steps.
  • Meeting time in Webex is correct

Copy link
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Yet to verify a change in the core booking flow

yarn.lock Outdated Show resolved Hide resolved
const videoCredential = bookingToDelete.user.credentials.find(
(credential) => credential.id === credentialId
);
const { credentialId, uid, type } = bookingVideoReference;
Copy link
Member

Choose a reason for hiding this comment

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

I reverted this entire file and was still able to cancel the webex meeting by cancelling the booking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hariombalhara

the cancel booking behavior is not consistent for me. I reverted the file to the original code and am running into errors again while canceling the booking. The videoAdapter is not picked correctly from credentials. This results in the meeting getting deleted on cal, but not on Webex. I added logs in a couple of places to show this.

Screenshot 2023-05-11 at 9 36 28 AM Screenshot 2023-05-11 at 9 36 16 AM

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @aar2dee2. Looks like some weird problem. But as I see it, it has nothing to do with your app. Let's keep it reverted for this PR.
I would let @alannnc or @joeauyeung take care of it as they have more knowledge about this flow and this should be a problem with all video apps including Zoom in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay. thanks!

any other recommended changes?

Copy link
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

LGTM now !! It is in a mergeable state. I see that console.logs are still there instead of logger but that can be a followup PR.

Regarding that change in handleCancelBooking, that's a core change which shoould be tackled separately.

@aar2dee2
Copy link
Contributor Author

thank you!!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 11, 2023
@hariombalhara hariombalhara added this pull request to the merge queue May 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 11, 2023
@hariombalhara hariombalhara added this pull request to the merge queue May 11, 2023
Merged via the queue into calcom:main with commit e2cfd0d May 11, 2023
@aar2dee2 aar2dee2 deleted the webex-app branch May 11, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗️ .env changes contains changes to env variables
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebEx App
5 participants