GH-154: Created the settings command to change the preferences for meeting creation #284

Nityanand13 · 2022-11-14T14:50:29Z

Summary

This PR support /zoom settings command, so user can choose their preferences for creating a meeting. For now, the user will get an ephemeral post on running this command with 3 buttons Yes, No, and Ask.

Yes : Allow the user to use his personal meeting id to start a meeting.
No: Allow the user to use a unique meeting id to start a meeting.
Ask: Ask the user whether he wants to use his personal meeting id or a unique meeting id to start a meeting.

Ticket Link

Issue #154

…olve merge conflicts (#1) * add "/zoom setting usePMI" command * update from 'master' branch * save usePMI as user preference * fix error handling in /zoom usePMI setting command * update from upstream * create meeting without PMI * finish feature * fix lint * tidy go module * update tests, optimize code * tidy the go module * [MI-1996]: Resolved the bug in use PMI feature of zoom plugin 1. Resolved the problem of 500 internal server error which we were getting on clicking create new meeting when our use PMI was set to ask 2. Added c.apiURL in URL 3. Added auto suggestion for use PMI * [MI-1996]: Resolved merge conflicts * [MI-1996]: Resolved the error of make: *** No rule to make target 'webapp/node_modules'. Stop * [MI-1996]: Removed error of check-style * [MI-1996]: Resolved the error faced in golangci-lint * [MI-1996]: Review fixes done 1. Added constants at various places * [MI-1996]:Review fixes done 1. Added EOF 2. Added Proper comments * [MI-1996]:Resolved the panic that we were getting on running slash command of zoom plugin * [MI-1996]: Work in Progress * [MI-1996]:Added properly with and without PMI feature of zoom plugin * [MI-1996]: Improved code quality * [MI-1996]: Fixed CI errors * [MI-1996]:Review fixes done 1.Improved code quality 2.Added various constants * [MI-1996]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-1996]:Review fixes done 1. Improved code quality 2. Improved code readability 3. Resolved gramatical mistakes * [MI-1996]: Review fixes done 1. Changed few names * [MI-1996]: Review fixes done 1. Improved code readability 2. Removed indirect dependencies 3. Changed few names * [MI-1996]: Review fixes done 1. Improved code quality 2. Returned json instead of text * Fixed CI errors * [MI-1996]:Review fixes done 1. Improved code quality Co-authored-by: lantrungseo <[email protected]>

* add "/zoom setting usePMI" command * update from 'master' branch * save usePMI as user preference * fix error handling in /zoom usePMI setting command * update from upstream * create meeting without PMI * finish feature * fix lint * tidy go module * update tests, optimize code * tidy the go module * [MI-1996]: Resolved the bug in use PMI feature of zoom plugin 1. Resolved the problem of 500 internal server error which we were getting on clicking create new meeting when our use PMI was set to ask 2. Added c.apiURL in URL 3. Added auto suggestion for use PMI * [MI-1996]: Resolved merge conflicts * [MI-1996]: Resolved the error of make: *** No rule to make target 'webapp/node_modules'. Stop * [MI-1996]: Removed error of check-style * [MI-1996]: Resolved the error faced in golangci-lint * [MI-1996]: Review fixes done 1. Added constants at various places * [MI-1996]:Review fixes done 1. Added EOF 2. Added Proper comments * [MI-1996]:Resolved the panic that we were getting on running slash command of zoom plugin * [MI-1996]: Work in Progress * [MI-1996]:Added properly with and without PMI feature of zoom plugin * [MI-1996]: Improved code quality * [MI-1996]: Fixed CI errors * [MI-1996]:Review fixes done 1.Improved code quality 2.Added various constants * [MI-1996]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-1996]:Review fixes done 1. Improved code quality 2. Improved code readability 3. Resolved gramatical mistakes * [MI-1996]: Review fixes done 1. Changed few names * [MI-1996]: Review fixes done 1. Improved code readability 2. Removed indirect dependencies 3. Changed few names * [MI-1996]: Review fixes done 1. Improved code quality 2. Returned json instead of text * Fixed CI errors * [MI-1996]:Review fixes done 1. Improved code quality * [MI-1996]: Improved code quality Co-authored-by: lantrungseo <[email protected]>

codecov · 2022-11-15T15:09:21Z

Codecov Report

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

Comparison is base (ecd317a) 23.51% compared to head (dd77223) 19.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
- Coverage   23.51%   19.92%   -3.60%     
==========================================
  Files           9        9              
  Lines        1212     1511     +299     
==========================================
+ Hits          285      301      +16     
- Misses        874     1155     +281     
- Partials       53       55       +2

Files	Coverage Δ
server/store.go	`11.36% <100.00%> (ø)`
server/telemetry.go	`33.33% <100.00%> (+10.25%)`	⬆️
server/command.go	`13.55% <9.61%> (-0.51%)`	⬇️
server/http.go	`13.26% <6.66%> (-6.74%)`	⬇️

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

mattermod · 2022-11-26T01:02:05Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mickmister

Thanks for this improvement @Nityanand13 👍 I have some requests and some questions on the code. Please let me know what you think 👍

plugin.json

server/plugin.go

mickmister · 2022-12-01T04:39:19Z

server/plugin_test.go

+			api.On("GetPreferencesForUser", mock.AnythingOfType("string")).Return([]model.Preference{
+				{
+					UserId:   "test-userid",
+					Category: zoomPreferenceCategory,
+					Name:     zoomPMISettingName,
+					Value:    trueString,
+				},
+			}, nil)


Plugins don't usually use the Preferences API to store plugin-level preferences. It's not necessarily a problem, but I want to be consistent with other plugins. I'm really not sure if this is an issue though, @hanzei do you have thoughts on this?

webapp/src/actions/index.js

mickmister · 2022-12-01T05:20:33Z

server/http.go

 	}
+	http.Error(w, err.Error(), http.StatusInternalServerError)


Why do we assume err is not nil here?

It will never be nil here, as we are already returning earlier if this err is nil.

The code was a bit different when I originally commented on this. The check for the error was inside of a nested if statement, which seemed decoupled from this line.

The control flow is not easy to follow in this function. I think part of the reason for that is we are using several variable names for errors. Are we able to use just one?

Also can we make it so the above block returns when this is an error? Then we can avoid the large if block. I think that would make this easier to read.

server/http.go

hanzei · 2022-12-02T15:03:57Z

Removing myself as a reviewer until the conversation with @mickmister is wrapped up

larkox

A few minor questions apart of the changes already proposed.

server/command.go

1. Improved code quality 2. Changed the names of few constants

mattermod · 2022-12-23T01:02:32Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

* add "/zoom setting usePMI" command * update from 'master' branch * save usePMI as user preference * fix error handling in /zoom usePMI setting command * update from upstream * create meeting without PMI * finish feature * fix lint * tidy go module * update tests, optimize code * tidy the go module * [MI-1996]: Resolved the bug in use PMI feature of zoom plugin 1. Resolved the problem of 500 internal server error which we were getting on clicking create new meeting when our use PMI was set to ask 2. Added c.apiURL in URL 3. Added auto suggestion for use PMI * [MI-1996]: Resolved merge conflicts * [MI-1996]: Resolved the error of make: *** No rule to make target 'webapp/node_modules'. Stop * [MI-1996]: Removed error of check-style * [MI-1996]: Resolved the error faced in golangci-lint * [MI-1996]: Review fixes done 1. Added constants at various places * [MI-1996]:Review fixes done 1. Added EOF 2. Added Proper comments * [MI-1996]:Resolved the panic that we were getting on running slash command of zoom plugin * [MI-1996]: Work in Progress * [MI-1996]:Added properly with and without PMI feature of zoom plugin * [MI-1996]: Improved code quality * [MI-1996]: Fixed CI errors * [MI-1996]:Review fixes done 1.Improved code quality 2.Added various constants * [MI-1996]: Review fixes done 1. Improved code quality 2. Changed few names * [MI-1996]:Review fixes done 1. Improved code quality 2. Improved code readability 3. Resolved gramatical mistakes * [MI-1996]: Review fixes done 1. Changed few names * [MI-1996]: Review fixes done 1. Improved code readability 2. Removed indirect dependencies 3. Changed few names * [MI-1996]: Review fixes done 1. Improved code quality 2. Returned json instead of text * Fixed CI errors * [MI-1996]:Review fixes done 1. Improved code quality * [MI-1996]: Improved code quality * [MI-1996]:Done the review fixes of a PR mattermost#284 1. Improved code quality 2. Changed the names of few constants * [MI-1996]: Review fixes done 1. Changed few names * [MI-1996]: Review fixes done 1. Modified error * [MI-1996]: Review fixes done 1. Changed default topic to topic * [MI-1996]: Review fixes done 1. Improved code quality Co-authored-by: lantrungseo <[email protected]>

1.Improved code readability

mickmister

@Nityanand13 Thanks for addressing the feedback 👍 I have a few more questions and requests

server/http.go

mickmister · 2023-01-03T22:44:54Z

server/http.go

 	}
+	http.Error(w, err.Error(), http.StatusInternalServerError)


The code was a bit different when I originally commented on this. The check for the error was inside of a nested if statement, which seemed decoupled from this line.

The control flow is not easy to follow in this function. I think part of the reason for that is we are using several variable names for errors. Are we able to use just one?

Also can we make it so the above block returns when this is an error? Then we can avoid the large if block. I think that would make this easier to read.

mickmister · 2023-01-03T22:50:53Z

webapp/src/client/client.js

+            return response;
+        }
+    } catch (err) {
+        return {err};


Do we ever check this err value?

Now I have removed this try catch as we are checking the error here: https://github.com/Brightscout/mattermost-plugin-zoom/blob/2537671471cc8720efdf4cb1b16cf1c6ab2247e7/webapp/src/actions/index.js#L24

server/http.go

* [MI-2571]: Review fixes done 1. Improved code quality and readability * [MI-2571]: Review fixes done 1.Improved code quality * [MI-2571]: Review fixes done 1. Changed status code * [MI-2571]: Review fixes done 1. Improved code quality 2. Changed few names of variables and functions * [MI-2571]: Review fixes done 1. Improved code quality

mickmister · 2023-01-24T00:09:40Z

@Nityanand13 Is this ready for re-review?

Nityanand13 · 2023-01-24T13:19:18Z

@Nityanand13 Is this ready for re-review?

yes @mickmister

hanzei · 2023-10-10T07:32:08Z

@Nityanand13 Can you please merge master into your branch?

…m into mattermostGH-154-PMI-setting-command

AayushChaudhary0001 · 2023-10-25T11:37:37Z

I have tested this PR, but it was found that the response of the question asked in the custom post can be changed at any time, even after the meeting is started.
Also, since there can be multiple customs posts, so the latest generated post may not consist the latest response as it can be changed in the earlier one's.

For example, in the screenshot below, the latest custom post generated has the response as "Ask", whereas the post generated earlier has the updated response as "Yes". So according to the latest post, the Zoom bot should ask the user which mail ID to use, but instead it started the meeting as the latest response stored was "Yes".

Also, this being a ephermal post, the response can not be updated on refreshing the page.

Kindly look into this if we can handle this case also. @mickmister @Kshitij-Katiyar @avas27JTG

In addition, I am also experiencing some UI issues regarding the icons on starting a meet, which needs to be fixed.

mickmister · 2023-10-25T14:34:58Z

@asaadmahmood Would you happen to know why the camera icon is so big the screenshot above? I know you were working with the buttons here recently and I think it's unlikely that it affected that but just wojndering if you have any ideas

…user has selected an option for PMI setting.

avas27JTG · 2023-10-26T10:28:45Z

@asaadmahmood Would you happen to know why the camera icon is so big the screenshot above? I know you were working with the buttons here recently and I think it's unlikely that it affected that but just wojndering if you have any ideas

@mickmister @asaadmahmood I have fixed it, the issue was due to the change in SVG width and button styling in this PR

avas27JTG · 2023-10-26T10:29:51Z

I have tested this PR, but it was found that the response of the question asked in the custom post can be changed at any time, even after the meeting is started. Also, since there can be multiple customs posts, so the latest generated post may not consist the latest response as it can be changed in the earlier one's.

For example, in the screenshot below, the latest custom post generated has the response as "Ask", whereas the post generated earlier has the updated response as "Yes". So according to the latest post, the Zoom bot should ask the user which mail ID to use, but instead it started the meeting as the latest response stored was "Yes".

Also, this being a ephermal post, the response can not be updated on refreshing the page.

Kindly look into this if we can handle this case also. @mickmister @Kshitij-Katiyar @avas27JTG

In addition, I am also experiencing some UI issues regarding the icons on starting a meet, which needs to be fixed.

@AayushChaudhary0001 both the PMI setting's post issue and zoom video icon have been fixed now.

asaadmahmood · 2023-10-26T10:45:36Z

@avas27JTG Can you make sure the zoom button still appears like this?

avas27JTG · 2023-10-26T10:55:00Z

@asaadmahmood

asaadmahmood · 2023-10-26T11:05:02Z

Cool, thanks!

AayushChaudhary0001

Tested and approved
Working fine for the edge case, which was figured out earlier also, LGTM!

Nityanand13 and others added 2 commits November 14, 2022 18:06

Nityanand13 requested a review from larkox as a code owner November 14, 2022 14:50

Nityanand13 changed the title ~~Gh 154 pmi setting command~~ GH-154: Updated the settings command to change your preferences to start a meeting Nov 14, 2022

Nityanand13 changed the title ~~GH-154: Updated the settings command to change your preferences to start a meeting~~ GH-154: Created the settings command to change the preferences for meeting creation Nov 14, 2022

larkox requested a review from mickmister November 15, 2022 13:24

Nityanand13 force-pushed the GH-154-PMI-setting-command branch from 4c4a8c8 to 0aadfd9 Compare November 15, 2022 15:23

mattermod added the Lifecycle/1:stale label Nov 26, 2022

mickmister suggested changes Dec 1, 2022

View reviewed changes

mickmister requested a review from hanzei December 1, 2022 05:36

hanzei removed their request for review December 2, 2022 15:03

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Lifecycle/1:stale labels Dec 2, 2022

hanzei linked an issue Dec 2, 2022 that may be closed by this pull request

Decide whether to use PMI or not on instant meetings #154

Closed

larkox requested changes Dec 12, 2022

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

server/command.go Outdated Show resolved Hide resolved

Nityanand13 added a commit to Brightscout/mattermost-plugin-zoom that referenced this pull request Dec 12, 2022

[MI-1996]:Done the review fixes of a PR mattermost#284

ec6305b

1. Improved code quality 2. Changed the names of few constants

Nityanand13 mentioned this pull request Dec 12, 2022

Done the Review fixes of a PR #284 Brightscout/mattermost-plugin-zoom#3

Merged

mattermod added the Lifecycle/1:stale label Dec 23, 2022

Nityanand13 and others added 2 commits December 27, 2022 12:53

Review fixes done

f6c1879

1.Improved code readability

mickmister reviewed Jan 3, 2023

View reviewed changes

Nityanand13 mentioned this pull request Jan 5, 2023

[MI-2571]: Done the review fixes of PR: #284 Brightscout/mattermost-plugin-zoom#4

Merged

hanzei added the Awaiting Submitter Action Blocked on the author label Oct 16, 2023

hanzei removed the request for review from DHaussermann October 16, 2023 20:25

avas27JTG added 2 commits October 23, 2023 17:51

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

fbb4f88

…m into mattermostGH-154-PMI-setting-command

[MI-3656]: Fixed test case.

bb54ab8

avas27JTG removed the Awaiting Submitter Action Blocked on the author label Oct 23, 2023

avas27JTG requested a review from AayushChaudhary0001 October 23, 2023 12:43

Merge branch 'master' into mattermostGH-154-PMI-setting-command

589fccf

avas27JTG added 2 commits October 26, 2023 15:13

[mattermostGH-154]: Added logic to remove remove action buttons once …

953a0b0

…user has selected an option for PMI setting.

[mattermostGH-154]: Fixed zoom video icon.

9816390

[mattermostGH-154]: Added original zoom video button styles.

dd77223

AayushChaudhary0001 approved these changes Oct 27, 2023

View reviewed changes

avas27JTG merged commit 598725e into mattermost:master Oct 27, 2023
9 checks passed

avas27JTG deleted the GH-154-PMI-setting-command branch October 27, 2023 12:42

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Oct 27, 2023

hanzei added this to the v2.0.0 milestone Oct 27, 2023

This was referenced Oct 31, 2023

[MI-3689]: Test cases of Zoom PR #284 and #287 Brightscout/mattermost-test-management#9

Merged

Added test cases of Zoom settings slash command and edit meeting message. mattermost/mattermost-test-management#125

Open

This was referenced Nov 14, 2023

Bump version v2.0.0 (closed, renamed to v1.7.0) #321

Closed

Bump version v1.7.0. #322

Merged

raghavaggarwal2308 mentioned this pull request Jan 19, 2024

Enhance the code to use KV for storing user's meeting ID preference #340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-154: Created the settings command to change the preferences for meeting creation #284

GH-154: Created the settings command to change the preferences for meeting creation #284

Nityanand13 commented Nov 14, 2022 •

edited

Loading

codecov bot commented Nov 15, 2022 •

edited

Loading

mattermod commented Nov 26, 2022

mickmister left a comment

mickmister Dec 1, 2022

mickmister Dec 1, 2022

Nityanand13 Dec 27, 2022 •

edited

Loading

mickmister Jan 3, 2023

hanzei commented Dec 2, 2022

larkox left a comment

mattermod commented Dec 23, 2022

mickmister left a comment

mickmister Jan 3, 2023

mickmister Jan 3, 2023

Nityanand13 Jan 17, 2023

mickmister commented Jan 24, 2023

Nityanand13 commented Jan 24, 2023

hanzei commented Oct 10, 2023

AayushChaudhary0001 commented Oct 25, 2023 •

edited

Loading

mickmister commented Oct 25, 2023

avas27JTG commented Oct 26, 2023 •

edited

Loading

avas27JTG commented Oct 26, 2023

asaadmahmood commented Oct 26, 2023

avas27JTG commented Oct 26, 2023

asaadmahmood commented Oct 26, 2023

AayushChaudhary0001 left a comment

GH-154: Created the settings command to change the preferences for meeting creation #284

GH-154: Created the settings command to change the preferences for meeting creation #284

Conversation

Nityanand13 commented Nov 14, 2022 • edited Loading

Summary

Ticket Link

codecov bot commented Nov 15, 2022 • edited Loading

Codecov Report

mattermod commented Nov 26, 2022

mickmister left a comment

Choose a reason for hiding this comment

mickmister Dec 1, 2022

Choose a reason for hiding this comment

mickmister Dec 1, 2022

Choose a reason for hiding this comment

Nityanand13 Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

mickmister Jan 3, 2023

Choose a reason for hiding this comment

hanzei commented Dec 2, 2022

larkox left a comment

Choose a reason for hiding this comment

mattermod commented Dec 23, 2022

mickmister left a comment

Choose a reason for hiding this comment

mickmister Jan 3, 2023

Choose a reason for hiding this comment

mickmister Jan 3, 2023

Choose a reason for hiding this comment

Nityanand13 Jan 17, 2023

Choose a reason for hiding this comment

mickmister commented Jan 24, 2023

Nityanand13 commented Jan 24, 2023

hanzei commented Oct 10, 2023

AayushChaudhary0001 commented Oct 25, 2023 • edited Loading

mickmister commented Oct 25, 2023

avas27JTG commented Oct 26, 2023 • edited Loading

avas27JTG commented Oct 26, 2023

asaadmahmood commented Oct 26, 2023

avas27JTG commented Oct 26, 2023

asaadmahmood commented Oct 26, 2023

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Nityanand13 commented Nov 14, 2022 •

edited

Loading

codecov bot commented Nov 15, 2022 •

edited

Loading

Nityanand13 Dec 27, 2022 •

edited

Loading

AayushChaudhary0001 commented Oct 25, 2023 •

edited

Loading

avas27JTG commented Oct 26, 2023 •

edited

Loading