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

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

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ import (
)

const (
helpText = `* |/zoom start| - Start a zoom meeting`
oAuthHelpText = `* |/zoom disconnect| - Disconnect from zoom`
helpText = `* |/zoom start| - Start a zoom meeting`
oAuthHelpText = `* |/zoom disconnect| - Disconnect from zoom`
statusHelpText = `* |/zoom status yes/no| - Change automatic status change setting, yes/no`
)

func (p *Plugin) getCommand() (*model.Command, error) {
Expand Down Expand Up @@ -68,6 +69,8 @@ func (p *Plugin) executeCommand(c *plugin.Context, args *model.CommandArgs) (str
return p.runDisconnectCommand(userID)
case "help", "":
return p.runHelpCommand()
case "status":
return p.runStatusChange(userID, split[1:])
default:
return fmt.Sprintf("Unknown action %v", action), nil
}
Expand Down Expand Up @@ -131,19 +134,43 @@ func (p *Plugin) runDisconnectCommand(userID string) (string, error) {
func (p *Plugin) runHelpCommand() (string, error) {
text := "###### Mattermost Zoom Plugin - Slash Command Help\n"
text += strings.ReplaceAll(helpText, "|", "`")
text += "\n" + strings.ReplaceAll(statusHelpText, "|", "`")

if p.configuration.EnableOAuth {
text += "\n" + strings.ReplaceAll(oAuthHelpText, "|", "`")
}
return text, nil
}

func (p *Plugin) runStatusChange(userID string, args []string) (string, error) {
errMsg := "Expecting yes/no"
if len(args) < 2 {
return errMsg, errors.New("unexpected argument")
}

action := args[1]
if action != yes && action != no {
return errMsg, errors.New("unexpected argument")
}

appErr := p.API.KVSet(fmt.Sprintf("%v_%v", changeStatusKey, userID), []byte(action))
if appErr != nil {
p.API.LogDebug("Could not save status change preference ", appErr)
return "Could not save status preference", appErr
}
return "Status preference updated successfully", nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// getAutocompleteData retrieves auto-complete data for the "/zoom" command
func (p *Plugin) getAutocompleteData() *model.AutocompleteData {
zoom := model.NewAutocompleteData("zoom", "[command]", "Available commands: start, disconnect, help")
zoom := model.NewAutocompleteData("zoom", "[command]", "Available commands: start, disconnect, status help")

start := model.NewAutocompleteData("start", "", "Starts a Zoom meeting")
status := model.NewAutocompleteData("status", "", "Change automatic status change setting")
status.AddTextArgument("Allow automatic status change yes/no", "yes/no", "")

zoom.AddCommand(start)
zoom.AddCommand(status)

// no point in showing the 'disconnect' option if OAuth is not enabled
if p.configuration.EnableOAuth {
Expand Down
58 changes: 57 additions & 1 deletion server/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ import (

const (
defaultMeetingTopic = "Zoom Meeting"
postActionPath = "/action/status"
yes = "yes"
no = "no"
ContextAccept = "accept"
ContextMeetingID = "meetingId"
zoomOAuthUserStateLength = 3
)

Expand Down Expand Up @@ -53,11 +58,52 @@ func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Req
p.completeUserOAuthToZoom(w, r)
case "/deauthorization":
p.deauthorizeUser(w, r)
case postActionPath:
p.postActionConfirm(w, r)
default:
http.NotFound(w, r)
}
}

func (p *Plugin) postActionConfirm(w http.ResponseWriter, r *http.Request) {
userID := r.Header.Get("Mattermost-User-ID")
response := model.PostActionIntegrationResponse{}
request := model.PostActionIntegrationRequestFromJson(r.Body)
accepted := request.Context[ContextAccept].(bool)
meetingID := request.Context[ContextMeetingID].(float64)

post := &model.Post{}
key := fmt.Sprintf("%v_%v", changeStatusKey, userID)

message := "Ok, the status won't be updated automatically"
changeStatus := no
if accepted {
changeStatus = yes
message = "You have accepted automatic status change. Yay!"
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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


err := p.setUserStatus(userID, int(meetingID), false)
if appErr != nil {
p.API.LogDebug("failed to change user status", "error", err)
}
Comment on lines +90 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}


sa := &model.SlackAttachment{
Title: "Status Change",
Text: message,
}
model.ParseSlackAttachment(post, []*model.SlackAttachment{sa})
response.Update = post
_, err = w.Write(response.ToJson())
if err != nil {
p.API.LogDebug("failed to write response")
}
}

func (p *Plugin) connectUserToZoom(w http.ResponseWriter, r *http.Request) {
userID := r.Header.Get("Mattermost-User-ID")
if userID == "" {
Expand Down Expand Up @@ -261,12 +307,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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

if appErr = p.deleteMeetingPostID(meetingPostID); appErr != nil {
p.API.LogWarn("failed to delete db entry", "error", appErr.Error())
return
}

_, err := w.Write([]byte(post.ToJson()))
_, err = w.Write([]byte(post.ToJson()))
if err != nil {
p.API.LogWarn("failed to write response", "error", err.Error())
}
Expand Down Expand Up @@ -307,6 +358,11 @@ func (p *Plugin) postMeeting(creator *model.User, meetingID int, channelID strin
return appErr
}

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

if appErr = p.storeMeetingPostID(meetingID, createdPost.Id); appErr != nil {
p.API.LogDebug("failed to store post id", "err", appErr)
}
Expand Down
118 changes: 118 additions & 0 deletions server/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@ import (
)

const (
oldStatusKey = "old_status_"
changeStatusKey = "zoom_status_change"

botUserName = "zoom"
botDisplayName = "Zoom"
botDescription = "Created by the Zoom plugin."

zoomProviderName = "Zoom"
pluginURLPath = "/plugins/" + botUserName
Copy link
Contributor

Choose a reason for hiding this comment

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

)

type Plugin struct {
Expand Down Expand Up @@ -185,3 +189,117 @@ func (p *Plugin) sendDirectMessage(userID string, message string) error {
_, err = p.API.CreatePost(post)
return err
}

func (p *Plugin) setUserStatus(userID string, meetingID int, meetingEnd bool) error {
storedStatusPref, appErr := p.API.KVGet(fmt.Sprintf("%v_%v", changeStatusKey, userID))
if appErr != nil {
p.API.LogDebug("Could not get stored status preference from KV ", appErr)
}
Comment on lines +216 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we should.


if storedStatusPref == nil {
err := p.sendStatusChangeAttachment(userID, p.botUserID, meetingID)
if err != nil {
p.API.LogDebug("could not send status change attachment ", "error", err)
}
}
Comment on lines +220 to +225
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we supposed to return in this if block?

Copy link
Author

Choose a reason for hiding this comment

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

We can return here but the function would anyway return in the next statement. I will add return here to be clear.


if string(storedStatusPref) != yes {
return nil
}

statusKey := fmt.Sprintf("%v%v", oldStatusKey, meetingID)
if meetingEnd {
statusVal, err := p.API.KVGet(statusKey)
if err != nil {
p.API.LogDebug("Could not get old status from KVStore", "err", appErr.Error())
return err
}
Comment on lines +233 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}


newStatus := string(statusVal)
if newStatus == "" {
newStatus = model.STATUS_ONLINE
}

_, appErr = p.API.UpdateUserStatus(userID, newStatus)
if appErr != nil {
p.API.LogDebug("Could not get update status", "err", appErr.Error())
return appErr
}

return nil
}

currentStatus, appErr := p.API.GetUserStatus(userID)
if appErr != nil {
p.API.LogDebug("Failed to update user status", "err", appErr)
return appErr
}

oldStatus := ""
if currentStatus.Manual {
oldStatus = currentStatus.Status
}

appErr = p.API.KVSetWithExpiry(statusKey, []byte(oldStatus), meetingPostIDTTL)
if appErr != nil {
p.API.LogDebug("failed to store old status", "err", appErr)
return appErr
}

_, appErr = p.API.UpdateUserStatus(userID, model.STATUS_DND)
if appErr != nil {
p.API.LogDebug("Failed to update user status", "err", appErr)
return appErr
Comment on lines +272 to +273
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

return nil
}

func (p *Plugin) sendStatusChangeAttachment(userID, botUserID string, meetingID int) error {
url := pluginURLPath + postActionPath
actionYes := &model.PostAction{
Name: "Yes",
Integration: &model.PostActionIntegration{
URL: url,
Context: map[string]interface{}{
ContextAccept: true,
ContextMeetingID: meetingID,
},
},
}

actionNo := &model.PostAction{
Name: "No",
Integration: &model.PostActionIntegration{
URL: url,
Context: map[string]interface{}{
ContextAccept: false,
ContextMeetingID: meetingID,
},
},
}

sa := &model.SlackAttachment{
Title: "Status change",
Text: "Allow Zoom plugin to automatically change status",
Actions: []*model.PostAction{actionYes, actionNo},
}

attachmentPost := model.Post{}
model.ParseSlackAttachment(&attachmentPost, []*model.SlackAttachment{sa})
directChannel, appErr := p.API.GetDirectChannel(userID, botUserID)
larkox marked this conversation as resolved.
Show resolved Hide resolved
if appErr != nil {
p.API.LogDebug("Create Attachment: ", appErr)
return appErr
Comment on lines +313 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
attachmentPost.ChannelId = directChannel.Id
attachmentPost.UserId = botUserID

_, appErr = p.API.CreatePost(&attachmentPost)
if appErr != nil {
p.API.LogDebug("Create Attachment: ", appErr)
return appErr
Comment on lines +321 to +322
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

return nil
}
7 changes: 7 additions & 0 deletions server/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,23 +88,30 @@ func TestPlugin(t *testing.T) {
}, nil)

api.On("GetChannelMember", "thechannelid", "theuserid").Return(&model.ChannelMember{}, nil)
api.On("GetUserStatus", "theuserid").Return(&model.Status{}, nil)

api.On("GetPost", "thepostid").Return(&model.Post{Props: map[string]interface{}{}}, nil)
api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil)
api.On("UpdatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{}, nil)
api.On("GetPostsSince", "thechannelid", mock.AnythingOfType("int64")).Return(&model.PostList{}, nil)

api.On("KVSetWithExpiry", "old_status_123", mock.AnythingOfType("[]uint8"), mock.AnythingOfType("int64")).Return(nil)
api.On("KVSetWithExpiry", fmt.Sprintf("%v%v", postMeetingKey, 234), mock.AnythingOfType("[]uint8"), mock.AnythingOfType("int64")).Return(nil)
api.On("KVSetWithExpiry", fmt.Sprintf("%v%v", postMeetingKey, 123), mock.AnythingOfType("[]uint8"), mock.AnythingOfType("int64")).Return(nil)

api.On("KVGet", fmt.Sprintf("%v%v", postMeetingKey, 234)).Return([]byte("thepostid"), nil)
api.On("KVGet", fmt.Sprintf("%v%v", postMeetingKey, 123)).Return([]byte("thepostid"), nil)
api.On("KVGet", "zoom_status_change_theuserid").Return([]byte("yes"), nil)
api.On("KVGet", "zoom_status_change_").Return([]byte("yes"), nil)
api.On("KVGet", "old_status_0").Return([]byte("online"), nil)

api.On("KVDelete", fmt.Sprintf("%v%v", postMeetingKey, 234)).Return(nil)

api.On("LogWarn", mock.AnythingOfType("string")).Return()
api.On("LogDebug", mock.AnythingOfType("string"), mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return()

api.On("UpdateUserStatus", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(&model.Status{}, nil)

path, err := filepath.Abs("..")
require.Nil(t, err)
api.On("GetBundlePath").Return(path, nil)
Expand Down