GitHub Issue #170 - Update status when starting/ending a zoom meeting #171

cvhariharan · 2020-10-04T11:57:26Z

Summary

This PR adds ability to the mattermost-zoom-plugin to automatically update the status of the user starting/ending a zoom meeting. When the meeting is started by a user, their status is set to DND and when they end the meeting, the status is set to ONLINE.

Ticket Link

Fixes #170

mattermod · 2020-10-04T11:57:30Z

Hello @cvhariharan,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

jasonblais · 2020-10-04T13:53:46Z

@cvhariharan thanks for the contribution! Are you open to help review the build failure and let us know if you have any questions on how to resolve?

codecov · 2020-10-04T14:26:42Z

Codecov Report

Merging #171 (52060a2) into master (45747e2) will decrease coverage by 1.08%.
The diff coverage is 14.72%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   20.85%   19.76%   -1.09%     
==========================================
  Files           7        7              
  Lines         729      855     +126     
==========================================
+ Hits          152      169      +17     
- Misses        536      633      +97     
- Partials       41       53      +12

Impacted Files	Coverage Δ
server/http.go	`24.15% <8.57%> (-1.78%)`	⬇️
server/plugin.go	`20.12% <16.00%> (-3.48%)`	⬇️
server/command.go	`14.50% <21.05%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45747e2...52060a2. Read the comment docs.

jasonblais · 2020-10-04T14:32:33Z

Thanks @cvhariharan 🚀

larkox · 2020-10-05T09:30:45Z

@cvhariharan Thank you for the contribution. The current implementation seems nice, but I am not 100% sure about the use, because of edge cases. For example:

If I have manually set my status as away or offline, and I create a meeting, I will be set to DnD, and when the meeting finish, I will be set as Online. Is this expected?
If the webhooks are not set up, I will always get put to DnD, but never back to Online.

@aaronrothschild any thoughts on this? It is a similar problem as the one we had on Microsoft Calendar. Also, should we add a setting to set this on or off?

cvhariharan · 2020-10-05T13:58:08Z

Hi @larkox, to solve the first issue, could we do something like only update the status to dnd iff the status is online already or maybe save the old status before changing to dnd and reverting back when the meeting ends?

For the second issue, I do agree that the status won't go back to online if the webhooks are not setup unless we have some background job that checks the meeting status.

mickmister · 2020-10-05T16:39:42Z

to solve the first issue, could we do something like only update the status to dnd iff the status is online already or maybe save the old status before changing to dnd and reverting back when the meeting ends?

@cvhariharan To handle the "which status should we set the user back to" issue, we used the status.Manual property to determine whether the user's current status was manually set: https://github.com/mattermost/mattermost-plugin-mscalendar/blob/c751170a9ab789526782266d76ce970fad220a83/server/mscalendar/availability.go#L231

If the most recent status was manually set, meaning status.Manual is currently true:

Store the status in the kv store to be used later
When it comes time to set their status back, set it to the stored status

If the most recent status was not manually set:

Store a blank status in the kv store, signaling that it was not manually set
When it comes time to set their status back, set it to Online. This allows the system to go back into its own cycle of automating the user's status.

For the second issue, I do agree that the status won't go back to online if the webhooks are not setup unless we have some background job that checks the meeting status.

I think this can be put into a follow up ticket

aaronrothschild · 2020-10-05T22:53:50Z

I think we should ask the user what they would like us to do the first time we are about to set their status to Away/Dnd.

Frankly, you should probably just copy the behavior from the MS Calendar plugin that asks the user about their preference. Not sure how much scope that might add to this ticket (or not).

For the second issue, I do agree that the status won't go back to online if the webhooks are not setup unless we have some background job that checks the meeting status.

I think this can be put into a follow up ticket

Agreed, hopefully most should have webhooks setup. The important aspect is that we tell the user that we've switched their status (add some text to the Zoom message saying it started the meeting AND "We've set your Mattermost status to "Away/Dnd""

cvhariharan · 2020-10-06T03:32:07Z

From what I understand, for the first meeting that the user creates with this feature on, the plugin asks their preference on allowing automatic status updates. Their preference is persisted for future use.

I will go through the MS Calendar plugin and try to see how I can replicate the behavior here.
Is there any code sample or documentation that I can refer to understand how prompts in plugins work?

larkox · 2020-10-06T08:43:09Z

@cvhariharan What you want to use for prompting the user are Slack Attachments. You could try to use the experimental flow (or wizard, I plan to change the name eventually), but probably it is an overkill.

You should also add a slash command to change this setting. Something simple as /zoom settings updateStatus on.

Finally, for the status change, you may want to check this PR: mattermost/mattermost-plugin-mscalendar#140

cvhariharan · 2020-10-06T09:05:47Z

Thanks @larkox, I was beginning to experiment with slack attachments. The PR is also very helpful. I will see what I can do.

…o change status change preference

cvhariharan · 2020-10-08T16:26:01Z

Hello, from the previous discussions I have made the following changes

When the user starts their first zoom call, they get a direct message from zoom bot asking if they want to allow automatic status change.
Added a /zoom status command which takes yes/no as an argument which can be used to change the status change preference

Let me know if something should be changed. 😄

larkox

In general it looks good. Just some minor improvements.

server/command.go

server/http.go

server/plugin.go

larkox · 2020-10-13T12:04:52Z

@cvhariharan Could you also post screenshots with all possible outcomes so @aaronrothschild can check from a UX perspective the messages and look of the feature?

aaronrothschild · 2020-10-14T12:56:31Z

Thanks for your work @cvhariharan , looking forward to checking this out!

aaronrothschild · 2020-10-27T14:59:23Z

@cvhariharan Thanks for the screenshots!

@abhijit-singh @levb or @hanzei thoughts on the "settings" as part of the /zoom slash command?

Perhaps we should add a /zoom settings status off which is a similar pattern to the /jitsi settings options approach?

levb · 2020-10-28T13:09:00Z

0/5 the idea is awesome, the UX needs some work - sorry late to the discussion. Either way, we can ship with the feature off by default while we work out the kinks of managing the setting.

mattermod · 2020-11-03T01:01:19Z

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!

/cc @jasonblais @jfrerich @emilyacook

hanzei · 2020-11-03T09:33:25Z

/zoom settings status off seems like a good idea. We can disable my default.

larkox

From my side it looks good. Thanks for the contribution!

hanzei · 2020-11-03T10:31:11Z

@cvhariharan Heads up that there is a merge conflict to resolve

cvhariharan · 2020-11-03T17:27:12Z

The merge conflicts are now resolved @hanzei.

mickmister

Awesome work @cvhariharan! I just have some requests regarding error handling

mickmister · 2020-11-10T07:59:50Z

server/command.go

@@ -19,6 +19,8 @@ const oAuthHelpText = `* |/zoom connect| - Connect to zoom

 const alreadyConnectedString = "Already connected"

+const statusHelpText = `* |/zoom status yes/no| - Change automatic status change setting, yes/no`


Can we describe what this means a bit here? Maybe something like "With this activated, your status will automatically change based on if you are in a meeting"

mickmister · 2020-11-10T08:00:07Z

server/command.go

+		p.API.LogDebug("Could not save status change preference ", appErr)
+		return "Could not save status preference", appErr
+	}
+	return "Status preference updated successfully", nil


Can this message convey the fact of "Your status will be updated" or "Your status will not be updated"?

mickmister · 2020-11-10T08:04:18Z

server/http.go

+	appErr := p.API.KVSet(key, []byte(changeStatus))
+	if appErr != nil {
+		p.API.LogDebug("failed to set status change preference", "error", appErr.Error())
+	}


I think we should return here instead of proceeding after the if statement. Maybe have this function return an error, and call this function with a smaller function that handles the error. Any error that occurs in this function should cause the function to return early.

@mickmister Here if we don't return early, the next call is to setUserStatus which would automatically send another slack attachment for user confirmation on automatic status change. Won't that be more convenient?

Also, if I understand this correctly, you are suggesting we remove the KVSet and setUserStatus snippets and put it in a separate function and return early if that function throws any error?

mickmister · 2020-11-10T08:05:41Z

server/http.go

+	err := p.setUserStatus(userID, int(meetingID), false)
+	if appErr != nil {
+		p.API.LogDebug("failed to change user status", "error", err)
+	}


Suggested change

err := p.setUserStatus(userID, int(meetingID), false)

if appErr != nil {

p.API.LogDebug("failed to change user status", "error", err)

}

err := p.setUserStatus(userID, int(meetingID), false)

if err != nil {

p.API.LogDebug("failed to change user status", "error", err)

}

mickmister · 2020-11-10T08:07:50Z

server/http.go

@@ -283,12 +329,17 @@ func (p *Plugin) handleMeetingEnded(w http.ResponseWriter, r *http.Request, webh
 		return
 	}

+	err := p.setUserStatus(post.UserId, int(meetingID), true)
+	if err != nil {
+		p.API.LogDebug("failed to change user status", "error", err)


Can we return the error, and handle it on the caller's side? I think we will need to make a separate small function to do so.

mickmister · 2020-11-10T08:14:04Z

server/plugin.go

+		statusVal, err := p.API.KVGet(statusKey)
+		if err != nil {
+			p.API.LogDebug("Could not get old status from KVStore", "err", appErr.Error())
+			return err
+		}


Suggested change

statusVal, err := p.API.KVGet(statusKey)

if err != nil {

p.API.LogDebug("Could not get old status from KVStore", "err", appErr.Error())

return err

}

statusVal, appErr := p.API.KVGet(statusKey)

if appErr != nil {

p.API.LogDebug("Could not get old status from KVStore", "err", appErr.Error())

return appErr

}

mickmister · 2020-11-10T08:29:00Z

server/plugin.go

+		p.API.LogDebug("Create Attachment: ", appErr)
+		return appErr


Suggested change

p.API.LogDebug("Create Attachment: ", appErr)

return appErr

return errors.Wrap(appErr, "Failed to create status change attachment post")

mickmister · 2020-11-10T08:29:28Z

server/plugin.go

+		p.API.LogDebug("Create Attachment: ", appErr)
+		return appErr


Suggested change

p.API.LogDebug("Create Attachment: ", appErr)

return appErr

return errors.Wrap(appErr, "Failed to get bot DM channel")

mickmister · 2020-11-10T08:30:28Z

server/plugin.go

+		p.API.LogDebug("Failed to update user status", "err", appErr)
+		return appErr


Can we use errors.Wrap to decorate errors with relevant error messages in this function and have the parent log, instead of logging here?

mickmister · 2020-11-10T08:30:49Z

server/plugin.go

+	if appErr != nil {
+		p.API.LogDebug("Could not get stored status preference from KV ", appErr)
+	}


Should we return here?

Yes, we should.

cvhariharan · 2020-11-12T13:39:18Z

@mickmister Hi, sorry I have been caught up with work, I will take a look into this over the weekend.

mickmister · 2020-11-12T18:09:37Z

@mickmister Hi, sorry I have been caught up with work, I will take a look into this over the weekend.

No problem, thanks @cvhariharan 🙂

mattermod · 2020-11-23T01:01:24Z

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!

/cc @jasonblais @jfrerich @emilyacook

jfrerich · 2020-11-23T18:39:55Z

Hi @cvhariharan! Have you had a chance to address the PR feedback? Do you need any assistance? Thanks!

cvhariharan · 2020-11-24T01:58:12Z

Hi @jfrerich, I haven't been able to look into this yet. I have been caught up with work for sometime. I will get back to this as soon as possible.

lindy65 · 2021-05-26T09:00:50Z

Hi @cvhariharan - I'm helping to review stale PRs and will be closing this one for now as it's been standing for 6 months. Please feel free to re-open it if you're able to prioritize the work :)

I've re-added the "Up for Grabs" label on #170

//cc @larkox @mickmister

Update status when starting/ending a zoom meeting. Fixes mattermost#170

1c65449

cvhariharan requested a review from larkox as a code owner October 4, 2020 11:57

mattermod added the Hacktoberfest label Oct 4, 2020

jasonblais requested a review from mickmister October 4, 2020 13:53

jasonblais added the 2: Dev Review Requires review by a core committer label Oct 4, 2020

Add mock implementation for UpdateUserStatus

3d3971f

Merge branch 'master' into master

58c00b7

jasonblais added 3: QA Review Requires review by a QA tester hacktoberfest-accepted labels Oct 6, 2020

cvhariharan added 3 commits October 7, 2020 22:29

Merge branch 'master' into master

b69fba9

Ask user confirmation before first status change, add slash command t…

95008bd

…o change status change preference

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

fedc699

cvhariharan added 2 commits October 8, 2020 22:07

Fix linting issues

c92363d

Merge branch 'master' into master

ff80b69

larkox requested changes Oct 13, 2020

View reviewed changes

server/command.go Outdated Show resolved Hide resolved

server/http.go Outdated Show resolved Hide resolved

server/http.go Outdated Show resolved Hide resolved

server/plugin.go Show resolved Hide resolved

server/plugin.go Outdated Show resolved Hide resolved

Use ephemeral post

da42848

cvhariharan added 2 commits October 28, 2020 11:01

Merge branch 'master' into master

c3b3ac5

Fix merge conflicts

ec0a039

mattermod added the Lifecycle/1:stale label Nov 3, 2020

larkox approved these changes Nov 3, 2020

View reviewed changes

hanzei added the Awaiting Submitter Action Blocked on the author label Nov 3, 2020

hanzei added this to the v2.0.0 milestone Nov 3, 2020

hanzei removed the Lifecycle/1:stale label Nov 3, 2020

cvhariharan added 2 commits November 3, 2020 22:38

Merge branch 'master' into master

077e049

Fix merge conflicts

70102a6

hanzei removed the Awaiting Submitter Action Blocked on the author label Nov 3, 2020

mickmister suggested changes Nov 10, 2020

View reviewed changes

mattermod added the Lifecycle/1:stale label Nov 23, 2020

Merge branch 'master' into master

52060a2

lindy65 closed this May 26, 2021

hanzei added Lifecycle/3:orphaned and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Lifecycle/1:stale labels May 28, 2021

		@@ -19,6 +19,8 @@ const oAuthHelpText = `* \|/zoom connect\| - Connect to zoom

		const alreadyConnectedString = "Already connected"

		const statusHelpText = `* \|/zoom status yes/no\| - Change automatic status change setting, yes/no`

	p.API.LogDebug("Create Attachment: ", appErr)
	return appErr
	return errors.Wrap(appErr, "Failed to create status change attachment post")

	p.API.LogDebug("Create Attachment: ", appErr)
	return appErr
	return errors.Wrap(appErr, "Failed to get bot DM channel")

		p.API.LogDebug("Failed to update user status", "err", appErr)
		return appErr

GitHub Issue #170 - Update status when starting/ending a zoom meeting #171

GitHub Issue #170 - Update status when starting/ending a zoom meeting #171

Conversation

cvhariharan commented Oct 4, 2020

Summary

Ticket Link

mattermod commented Oct 4, 2020

jasonblais commented Oct 4, 2020

codecov bot commented Oct 4, 2020 • edited Loading

Codecov Report

jasonblais commented Oct 4, 2020

larkox commented Oct 5, 2020

cvhariharan commented Oct 5, 2020

mickmister commented Oct 5, 2020 • edited Loading

aaronrothschild commented Oct 5, 2020

cvhariharan commented Oct 6, 2020 • edited Loading

larkox commented Oct 6, 2020

cvhariharan commented Oct 6, 2020

cvhariharan commented Oct 8, 2020 • edited Loading

larkox left a comment

Choose a reason for hiding this comment

larkox commented Oct 13, 2020

aaronrothschild commented Oct 14, 2020

aaronrothschild commented Oct 27, 2020

levb commented Oct 28, 2020

mattermod commented Nov 3, 2020

hanzei commented Nov 3, 2020

larkox left a comment

Choose a reason for hiding this comment

hanzei commented Nov 3, 2020

cvhariharan commented Nov 3, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvhariharan commented Nov 12, 2020

mickmister commented Nov 12, 2020

mattermod commented Nov 23, 2020

jfrerich commented Nov 23, 2020

cvhariharan commented Nov 24, 2020

lindy65 commented May 26, 2021

codecov bot commented Oct 4, 2020 •

edited

Loading

mickmister commented Oct 5, 2020 •

edited

Loading

cvhariharan commented Oct 6, 2020 •

edited

Loading

cvhariharan commented Oct 8, 2020 •

edited

Loading