New plugin API "RestoreToPreviousCustomStatus" added #20851

M-ZubairAhmed · 2022-08-19T06:07:39Z

Summary

The objective of this new plugin API is for the plugins/products to set custom status for a temporary duration of time. For example: When zoom call is started, the user who joined/started the call will have their custom status changed to "In Zoom meeting". When the call is ended their status is reverted back to whatever they had set previously.

This is achieved by a new plugin API "RestoreToPreviousCustomStatus", which plugins can use to restore the previous stored user custom status. The way it does is by adding a new key to user custom status preference category called "setByProject". When a product/plugin sets the custom status by calling "UpdateUserCustomStatus" they need to pass in value for "setByProject" as true, then the "SetCustomStatus" function does not stored the custom status in the recent custom status list.

Ticket Link

n/a

Release Note

RestoreToPreviousCustomStatus method added in plugins api

mm-cloud-bot · 2022-08-19T06:07:41Z

@M-ZubairAhmed: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

mattermod · 2022-08-19T06:16:05Z

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

Update plugin_api_test.go

isacikgoz

Hey @M-ZubairAhmed did my first pass, I have some points that needs to be addressed.

isacikgoz · 2022-08-22T05:40:32Z

app/plugin_api.go

+	if cs == nil || !cs.SetByProduct {
+		return model.NewAppError("RestoreToPreviousCustomStatus", "plugin.api.restore_to_previous_custom_status", nil, "", http.StatusBadRequest)
+	}


Is this a real error case? Or, we are just ignoring the restoration process? If the latter, I don't think it's necessary to return error.

its the latter case. Changed it to return nil

isacikgoz · 2022-08-22T05:41:26Z

app/plugin_api.go

+	if err != nil {
+		return model.NewAppError("RestoreToPreviousCustomStatus", "plugin.api.restore_to_previous_custom_status", nil, "failed to get recent statuses to reset", http.StatusInternalServerError)
+	}


We are losing the error detail here, I'd suggest using Wrap(error) method for the model.AppError.

Added the method to the AppError

isacikgoz · 2022-08-22T05:41:42Z

app/plugin_api.go

+
+	if len(rcs) == 0 {
+		err := api.app.RemoveCustomStatus(api.ctx, userID)
+		if err != nil {


Again, the error is lost.

handled with wrap

isacikgoz · 2022-08-22T05:41:52Z

app/plugin_api.go

+	}
+
+	cserr := api.app.SetCustomStatus(api.ctx, userID, rcs[0])
+	if cserr != nil {


Same as above.

handled with wrap

isacikgoz · 2022-08-22T05:42:41Z

app/status.go

+func (a *App) GetRecentCustomStatuses(userID string) ([]*model.CustomStatus, error) {
+	pref, err := a.GetPreferenceByCategoryAndNameForUser(userID, model.PreferenceCategoryCustomStatus, model.PreferenceNameRecentCustomStatuses)
+
+	var existingRCS []*model.CustomStatus


Why we are ignoring the error here? I mean the err variable from the L:460

Handled and returned the err

agarciamontoro · 2022-08-26T19:12:51Z

I've been postponing this until today, but I had an unexpected super long meeting, so I didn't have time to go through it. I'll be off next week, back on Tuesday September 6, so if you want to change reviewers, please feel free to do so.

M-ZubairAhmed · 2022-08-30T10:29:16Z

/update-branch

isacikgoz

Looks good, we should also run a e2e test for these to be sure. Just a single thing that needs to be changed. Otherwise looks good.

isacikgoz · 2022-09-02T14:36:21Z

app/status.go

+	if err != nil {
+		return nil, err
+	}


We need to remove this, it's already handled in L:461

mickmister

Awesome work @M-ZubairAhmed! This greatly improves the power of this feature.

The PR mostly looks good 👍 I mainly have some requests in regards to the unit test added here

app/plugin_api_test.go

app/status.go

mickmister · 2022-09-02T14:37:23Z

plugin/api.go

+	// Minimum server version: 7.2
+	RestoreToPreviousCustomStatus(userID string) *model.AppError


Note that this will need to be updated if this PR is not merged with 7.2 (I think that's obvious though)

app/plugin_api.go

mickmister · 2022-09-02T22:41:36Z

app/plugin_api.go

+		return nil
+	}
+
+	cserr := api.app.SetCustomStatus(api.ctx, userID, rcs[0])


Let's say I have recent custom statuses, but my last choice as a user was to remove my custom status. With the logic here, will this function set my status back to my last recent one, rather than setting it back to my proper previous state of "no custom status"?

Yes, https://github.com/mattermost/mattermost-server/blob/6a5a50c67490d0173b0f005d4683222a289acdb9/app/plugin_api.go#L388-L392

I read this code you're linking to, but I figured in my scenario described above, len(rcs) would not be zero. Under what conditions does GetRecentCustomStatuses return an empty slice?

If a user clears their status, wouldn't their "recent custom statuses" preference still contain values, thus making it so len is greater than zero?

Do you mind writing a unit test for this block? The steps I'm thinking:

User sets custom status

User clears custom status

Plugin sets custom status

Call restore status

Assert that user has no status, rather than the initial one they had on

@mickmister yes the more i think about it , you are right even if user clears custom status the recent ones are still there. No restoring but actually take up the first of them and set it up. What do you think we can do here?

gentle ping on this one @mickmister

@M-ZubairAhmed I'm thinking the recent statuses field should resemble "the most recent statuses set by the user", and shouldn't be something where we remove from that list. I don't think it should be used as a state variable for logic like this snippet we're commenting on.

Maybe we can add two fields to custom status:

One that means "last status user manually chose", and clear this value when they clear their status

One that means "last status set by a product", and clear this value when:

Product wants to clear it

and when User sets or clears their status manually

The product status field would always take precedence for what is shown in the UI, but it always gets cleared whenever the user sets or clears their custom status. What do you think @M-ZubairAhmed?

M-ZubairAhmed · 2022-10-14T12:02:07Z

/update-branch

…termost-server into user-custom-status-plugin

mickmister

Just one request to make sure the case of #20851 (comment) is unit tested. Otherwise LGTM 👍

javaguirre

Just a couple of details to discuss

javaguirre · 2022-10-17T08:39:45Z

model/custom_status.go

+	Text         string    `json:"text"`
+	Duration     string    `json:"duration"`
+	ExpiresAt    time.Time `json:"expires_at"`
+	SetByProduct bool      `json:"set_by_product,omitempty"`


Not sure if SetByProduct is descriptive, maybe focusing more on the behavior rather than in "who" set it? like IsSetAutomatically or something like this? also changing the name to reference it's a boolean (that's why I added Is at the beginning).

javaguirre · 2022-10-17T08:42:18Z

model/custom_status.go

-	ExpiresAt time.Time `json:"expires_at"`
+	Emoji        string    `json:"emoji"`
+	Text         string    `json:"text"`
+	Duration     string    `json:"duration"`


Why is Duration a string? wouldn't it be better a Duration from time?

isacikgoz · 2022-11-08T15:08:44Z

Hey @M-ZubairAhmed it seems like there is an extra file included as output to the PR. Was it by mistake?

M-ZubairAhmed · 2022-11-09T08:05:24Z

Hey @M-ZubairAhmed it seems like there is an extra file included as output to the PR. Was it by mistake?

Yes, i removed this.

isacikgoz · 2022-11-22T17:09:03Z

/update-branch

M-ZubairAhmed · 2023-02-04T21:33:52Z

Closing for now, will get back to this when time permits again

mickmister · 2023-10-20T17:58:19Z

@M-ZubairAhmed Do you have interest in continuing this PR? Asking to know if we should reassign this to implement mattermost/mattermost-plugin-mscalendar#228

mm-cloud-bot added the do-not-merge/release-note-label-needed label Aug 19, 2022

implementation added

312c3b4

M-ZubairAhmed force-pushed the user-custom-status-plugin branch from f492b42 to 312c3b4 Compare August 19, 2022 06:13

M-ZubairAhmed changed the title ~~implementation added~~ New method "RestoreToPreviousCustomStatus" added in plugins api Aug 19, 2022

M-ZubairAhmed requested review from agarciamontoro and isacikgoz August 19, 2022 06:15

M-ZubairAhmed added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Aug 19, 2022

mm-cloud-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed labels Aug 19, 2022

generated files

460a19f

M-ZubairAhmed marked this pull request as ready for review August 19, 2022 06:26

Update en.json

f40c5ad

M-ZubairAhmed mentioned this pull request Aug 19, 2022

Hackathon : Auto change user's custom status on Zoom call mattermost/mattermost-plugin-zoom#267

Closed

test added

d4f2dc6

Update plugin_api_test.go

M-ZubairAhmed force-pushed the user-custom-status-plugin branch from 49e2d05 to d4f2dc6 Compare August 19, 2022 07:51

improvement grammar

bf9c7ea

M-ZubairAhmed changed the title ~~New method "RestoreToPreviousCustomStatus" added in plugins api~~ New plugin API "RestoreToPreviousCustomStatus" added Aug 19, 2022

isacikgoz reviewed Aug 22, 2022

View reviewed changes

M-ZubairAhmed requested a review from isacikgoz August 26, 2022 08:44

review comments 1

f1f93dc

M-ZubairAhmed requested review from BenCookie95 and removed request for agarciamontoro August 30, 2022 07:41

Merge branch 'master' into user-custom-status-plugin

96e7804

M-ZubairAhmed requested review from levb and removed request for BenCookie95 September 2, 2022 12:36

M-ZubairAhmed requested a review from catalintomai September 2, 2022 12:36

isacikgoz reviewed Sep 2, 2022

View reviewed changes

mickmister suggested changes Sep 2, 2022

View reviewed changes

mickmister reviewed Sep 2, 2022

View reviewed changes

rev #20851 (comment)

0e7f66e

mattermod and others added 3 commits October 14, 2022 15:02

Merge branch 'master' into user-custom-status-plugin

c338657

Merge branch 'user-custom-status-plugin' of github.com:mattermost/mat…

6a5a50c

…termost-server into user-custom-status-plugin

Update opentracing_layer.go

7004fc5

catalintomai requested review from javaguirre and removed request for catalintomai October 14, 2022 15:57

M-ZubairAhmed added 2 commits October 15, 2022 00:58

Update plugin_api_test.go

c76f202

move to app

1bdd80a

M-ZubairAhmed requested review from mickmister and isacikgoz October 14, 2022 19:48

mickmister reviewed Oct 14, 2022

View reviewed changes

javaguirre reviewed Oct 17, 2022

View reviewed changes

Delete output

84fd002

Merge branch 'master' into user-custom-status-plugin

ba809fc

M-ZubairAhmed closed this Feb 4, 2023

mickmister mentioned this pull request May 31, 2023

Use custom status for "In a meeting" user state mattermost/mattermost-plugin-mscalendar#228

Open

		// Minimum server version: 7.2
		RestoreToPreviousCustomStatus(userID string) *model.AppError

New plugin API "RestoreToPreviousCustomStatus" added #20851

New plugin API "RestoreToPreviousCustomStatus" added #20851

Conversation

M-ZubairAhmed commented Aug 19, 2022 • edited Loading

Summary

Ticket Link

Release Note

mm-cloud-bot commented Aug 19, 2022

mattermod commented Aug 19, 2022

isacikgoz 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

agarciamontoro commented Aug 26, 2022

M-ZubairAhmed commented Aug 30, 2022

isacikgoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

M-ZubairAhmed Oct 14, 2022 • edited Loading

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

M-ZubairAhmed commented Oct 14, 2022

mickmister left a comment

Choose a reason for hiding this comment

javaguirre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isacikgoz commented Nov 8, 2022

M-ZubairAhmed commented Nov 9, 2022

isacikgoz commented Nov 22, 2022

M-ZubairAhmed commented Feb 4, 2023

mickmister commented Oct 20, 2023

M-ZubairAhmed commented Aug 19, 2022 •

edited

Loading

M-ZubairAhmed Oct 14, 2022 •

edited

Loading