mattermost / mattermost-plugin-zoom

Zoom plugin for Mattermost :electric_plug:
Apache License 2.0
107 stars 68 forks source link

[GH-199] Added a ephemeral message if the user is using PMI and his/her PMI setting is disable on Zoom. #324

Closed raghavaggarwal2308 closed 8 months ago

raghavaggarwal2308 commented 9 months ago

Summary

Screenshot

  1. Ephemeral message: image
  2. Settings command: image
  3. Slack attachment to ask for PMI: image

What to test?

Steps to reproduce:
  1. Connect your Zoom account.
  2. Run the /zoom settings command and set the "Use your Personal Meeting ID" setting to "Yes".
  3. Make sure that "Enable Personal Meeting ID" is "false" on the Zoom settings page.
  4. Run the /zoom start command OR click on the Zoom icon in the app bar.
Environment:

MM version: v9.2.2 Node version: 14.18.0 Go version: 1.20.11

Ticket Link

Fixes #199

codecov[bot] commented 9 months ago

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (8f844a9) 18.86% compared to head (31298da) 18.42%.

Files Patch % Lines
server/http.go 0.00% 42 Missing :warning:
server/command.go 0.00% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #324 +/- ## ========================================== - Coverage 18.86% 18.42% -0.44% ========================================== Files 9 9 Lines 1479 1514 +35 ========================================== Hits 279 279 - Misses 1149 1184 +35 Partials 51 51 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mickmister commented 9 months ago

@raghavaggarwal2308 Under what circumstances does the ephemeral message in the PR description come up? When the user tries to start a meeting specifically with their PMI?

raghavaggarwal2308 commented 9 months ago

@raghavaggarwal2308 Under what circumstances does the ephemeral message in the PR description come up? When the user tries to start a meeting specifically with their PMI?

@mickmister Yes, PMI should be disabled on the Zoom side and enabled on MM.

mickmister commented 9 months ago

@raghavaggarwal2308 I mean, does this error still show up if the user just wants to create a meeting with a unique meeting id?

raghavaggarwal2308 commented 9 months ago

@raghavaggarwal2308 I mean, does this error still show up if the user just wants to create a meeting with a unique meeting id?

@mickmister No, only when the user is trying to create a meeting with PMI and it's disabled on Zoom. I am attaching screenshots to clarify it more. In both screenshots, PMI is disabled on the Zoom side.

image image

mickmister commented 9 months ago

@raghavaggarwal2308 One thing comes to mind that maybe we should keep the two options showing if we reach that error, so the user can recorrect their action as:

Or we can let them know before that step and say something like "We've disabled the PMI button because you don't have zoom configured correctly"

I'll defer this UX decision to @asaadmahmood or @matthewbirtch

asaadmahmood commented 9 months ago

I think if the user tries to create the meeting without the PMI configured, we just automatically create the meeting without asking the user, but let him know that his PMI is not configured in zoom, so he can't create a meeting using it.

And once he has configured his PMI, we can ask him again when he tries to create a new zoom meeting.

@raghavaggarwal2308 @mickmister

raghavaggarwal2308 commented 9 months ago

@mickmister Made the changes suggested by @asaadmahmood. Now we are creating a meeting with unique meeting ID if PMI is disabled on Zoom side and enabled on MM.

mickmister commented 9 months ago

@asaadmahmood Are you able to take this PR for a spin to see the flow in action?

asaadmahmood commented 9 months ago

@raghavaggarwal2308 Is it possible to create a video recording of the flow and send it here?

raghavaggarwal2308 commented 9 months ago

@asaadmahmood @mickmister Here is a recording with all the different flows for creating a meeting: Demo video

asaadmahmood commented 9 months ago

@raghavaggarwal2308 Looks good to me