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

raghavaggarwal2308 · 2023-11-23T12:20:03Z

Summary

If a user is using a Personal Meeting ID (PMI) for meeting creation and his/her PMI setting is disabled in the Zoom settings, the meeting was getting created with an invalid meeting ID. We updated the code to send an ephemeral message to the user to enable PMI in the Zoom settings.
Fixes the issue with the /zoom settings command not working in the threads view.
Fixes the issue with slack attachment to ask for PMI setting not getting posted in the threads view.

Screenshot

Ephemeral message:
Settings command:
Slack attachment to ask for PMI:

What to test?

An ephemeral message is received if the user is using PMI and his PMI setting is disabled on Zoom.

Steps to reproduce:

Connect your Zoom account.
Run the /zoom settings command and set the "Use your Personal Meeting ID" setting to "Yes".
Make sure that "Enable Personal Meeting ID" is "false" on the Zoom settings page.
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

…her PMI setting is disable on Zoom.

codecov · 2023-11-23T12:23:14Z

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 ⚠️
server/command.go	0.00%	9 Missing ⚠️

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

1. Setting command not working in threads view 2. Slack attachment to ask for PMI setting not getting posted in threads view.

mickmister · 2023-11-28T01:44:10Z

@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

Nice work on this @raghavaggarwal2308 👍 I have a few comments and requested a review from UX on the new functionality

server/command.go

server/http.go

… into MM-199

raghavaggarwal2308 · 2023-11-28T09:33:23Z

@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 · 2023-11-28T16:11:34Z

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

raghavaggarwal2308 · 2023-11-29T07:19:41Z

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

mickmister · 2023-11-29T19:21:27Z

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

"Okay, I'll create with a unique id instead"
or "Okay I changed my settings in Zoom, now I want to create with my PMI with one click"

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 · 2023-11-30T07:22:22Z

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 · 2023-12-04T15:08:54Z

@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 · 2023-12-04T17:55:33Z

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

asaadmahmood · 2023-12-04T17:58:20Z

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

raghavaggarwal2308 · 2023-12-05T10:13:24Z

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

asaadmahmood · 2023-12-05T11:05:06Z

@raghavaggarwal2308 Looks good to me

mickmister

LGTM, just one comment about sending errors back to the client

mickmister · 2023-12-06T06:14:59Z

server/http.go

+			meetingID, createMeetingErr = p.createMeetingWithoutPMI(user, zoomUser, req.ChannelID, topic)
+			if createMeetingErr != nil {
+				p.API.LogWarn("failed to create the meeting", "Error", createMeetingErr.Error())
+				http.Error(w, createMeetingErr.Error(), http.StatusInternalServerError)


Does this call to http.Error make this error show in the client? Maybe we want something more user-friendly if so. Wondering about the other calls to http.Error as well

@mickmister Yes, the error is shown to the client. Do you want us to show the user a generic message whenever we get an API error?

It depends how low-level or cryptic these errors are. I trust your judgment on this

@mickmister Updated, displaying a generic error to the user now.

…ng meeting

…m into MM-199

mickmister · 2023-12-08T08:16:25Z

webapp/src/actions/index.js

-            let m = error.message;
-            if (error.message && error.message[0] === '{') {
-                const e = JSON.parse(error.message);
-
-                // Error is from Zoom API
-                if (e && e.message) {
-                    m = '\nZoom error: ' + e.message;
-                }
-            }


Hmm this error may have some useful context for why the meeting couldn't start, like something going on with the user's zoom account. I'm thinking this can stay

@mickmister Reverted the changes to display generic error message

It's mainly when we enter the "Error is from Zoom API" block that I'm thinking we should show the provided error. If the others from our server are more cryptic/low-level, then let's only show the error from error.message if the error came from Zoom, meaning we are in the "Error is from Zoom API" block here

@mickmister Just to clarify, if we are in the "Error is from Zoom API" block, then we show the error from the API and show a generic error otherwise. Right?

@raghavaggarwal2308 Yes that sounds right to me

@mickmister Updated

arush-vashishtha

Tested the PR for the ephemeral message generated and the meeting created when the user has disabled the personal meeting on their zoom account and has selected yes in the message generated for /zoom settings.
The PR is working Fine. LGTM

Note: The message displayed in the screenshot for the ephemeral message is updated according to the discussion in the PR and is verified.
cc @AayushChaudhary0001

[MI-3763] Added a emhemeral message if the user is using PMI and his/…

20fb84d

…her PMI setting is disable on Zoom.

raghavaggarwal2308 requested a review from mickmister as a code owner November 23, 2023 12:20

raghavaggarwal2308 added 3 commits November 23, 2023 19:58

[MI-3763] Fix the issue for PMI setting "Ask"

6fff252

[MI-3763] Fixed a minor issue

b32765b

[MI-3763] Fix issues:

dc8c0d8

1. Setting command not working in threads view 2. Slack attachment to ask for PMI setting not getting posted in threads view.

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester 1: UX Review Requires review by a UX Designer labels Nov 28, 2023

mickmister requested a review from asaadmahmood November 28, 2023 01:44

mickmister reviewed Nov 28, 2023

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

server/http.go Outdated Show resolved Hide resolved

mickmister requested a review from AayushChaudhary0001 November 28, 2023 01:54

Merge branch 'master' of github.com:mattermost/mattermost-plugin-zoom…

a4411cf

… into MM-199

[MI-3763] Review fix

aef9479

raghavaggarwal2308 requested a review from mickmister November 28, 2023 12:20

mickmister approved these changes Nov 28, 2023

View reviewed changes

mickmister removed the 2: Dev Review Requires review by a core committer label Nov 28, 2023

mickmister self-requested a review November 29, 2023 19:21

avas27JTG approved these changes Dec 1, 2023

View reviewed changes

[MI-3763] Create a meeting with unique ID if PMI is disabled on zoom

b325fc6

Merge branch 'master' into MM-199

56c0575

asaadmahmood approved these changes Dec 5, 2023

View reviewed changes

mickmister approved these changes Dec 6, 2023

View reviewed changes

mickmister removed the 1: UX Review Requires review by a UX Designer label Dec 6, 2023

raghavaggarwal2308 added 2 commits December 7, 2023 16:34

[MM-4] Review fixes: Send generic error in case of error while creati…

8285087

…ng meeting

Merge branch 'MM-199' of github.com:Brightscout/mattermost-plugin-zoo…

51d0260

…m into MM-199

mickmister reviewed Dec 8, 2023

View reviewed changes

raghavaggarwal2308 added 5 commits December 13, 2023 16:34

[MM-4] Review fix

7abbc3e

Merge branch 'master' into MM-199

4e45cd1

Merge branch 'master' into MM-199

8d5efc2

[MM-4] Review fixes

b0b4f3b

Merge branch 'master' into MM-199

7bc8155

mickmister approved these changes Dec 21, 2023

View reviewed changes

Merge branch 'master' into MM-199

5d65870

arush-vashishtha approved these changes Dec 29, 2023

View reviewed changes

Merge branch 'master' into MM-199

31298da

avas27JTG merged commit e22524d into mattermost:master Jan 2, 2024
11 checks passed

avas27JTG deleted the MM-199 branch January 2, 2024 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raghavaggarwal2308 commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

mickmister commented Nov 28, 2023

mickmister left a comment

raghavaggarwal2308 commented Nov 28, 2023

mickmister commented Nov 28, 2023

raghavaggarwal2308 commented Nov 29, 2023

mickmister commented Nov 29, 2023 •

edited

Loading

asaadmahmood commented Nov 30, 2023

raghavaggarwal2308 commented Dec 4, 2023

mickmister commented Dec 4, 2023

asaadmahmood commented Dec 4, 2023

raghavaggarwal2308 commented Dec 5, 2023

asaadmahmood commented Dec 5, 2023

mickmister left a comment

mickmister Dec 6, 2023

raghavaggarwal2308 Dec 6, 2023

mickmister Dec 6, 2023

raghavaggarwal2308 Dec 7, 2023

mickmister Dec 8, 2023

raghavaggarwal2308 Dec 13, 2023

mickmister Dec 14, 2023

raghavaggarwal2308 Dec 15, 2023

mickmister Dec 18, 2023

raghavaggarwal2308 Dec 19, 2023

arush-vashishtha left a comment •

edited

Loading

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

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

Conversation

raghavaggarwal2308 commented Nov 23, 2023 • edited Loading

Summary

Screenshot

What to test?

Steps to reproduce:

Environment:

Ticket Link

codecov bot commented Nov 23, 2023 • edited Loading

Codecov Report

mickmister commented Nov 28, 2023

mickmister left a comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Nov 28, 2023

mickmister commented Nov 28, 2023

raghavaggarwal2308 commented Nov 29, 2023

mickmister commented Nov 29, 2023 • edited Loading

asaadmahmood commented Nov 30, 2023

raghavaggarwal2308 commented Dec 4, 2023

mickmister commented Dec 4, 2023

asaadmahmood commented Dec 4, 2023

raghavaggarwal2308 commented Dec 5, 2023

asaadmahmood commented Dec 5, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arush-vashishtha left a comment • edited Loading

Choose a reason for hiding this comment

raghavaggarwal2308 commented Nov 23, 2023 •

edited

Loading

codecov bot commented Nov 23, 2023 •

edited

Loading

mickmister commented Nov 29, 2023 •

edited

Loading

arush-vashishtha left a comment •

edited

Loading