Add settings panel #62

larkox · 2020-03-18T16:34:09Z

Summary

Created an implementation for the settings panel which use post messages with slack attachments. This setting panel should be reusable for other projects.

Ticket Link

Fixes #61

larkox · 2020-03-18T16:36:38Z

Current implementation look like this:

Also allow disabling some settings based on the status of others:

aaronrothschild · 2020-03-18T17:20:28Z

Hmmm. Is there a way to indicate via the interactive buttons what is currently set? (ie: an active border around "Yes")

@asaadmahmood any ideas on how to best articulate to the user what is currently set already?

Current implementation look like this:

I'm thinking of changing the verbiage on current status to:

"Currently, I will send you an update before updating your status" or
"Currently, I will not send you an update before updating your status"

@asaadmahmood Should we use "I" to refer to the CalendarBot itself?

Also allow disabling some settings based on the status of others:

larkox · 2020-03-18T17:25:35Z

If we were to wait for this PR we would be able to apply some color to the one selected. Not sure if it is the best way to show it.

asaadmahmood · 2020-03-19T09:37:35Z

I didn't get this, what is "I"?
"Should we use "I" to refer to the CalendarBot itself?"

For the second one, I'd suggest:
Do you want to update your status to DND in Mattermost when you are in a meeting?
Yes No

I'm guessing the user only sees this one, not again and again, because if he has selected either yes or no, that's his setting, and he can change it when he wants, rather than seeing that message popup time and time again.

So it makes sense not to focus on what the setting is right now i.e. Currently, and just ask him what he wants to do.

larkox · 2020-03-19T09:43:26Z

@asaadmahmood with "I" Aaron refers to "I" as in "I, me, myself" or "I, you, he".

The idea of this is to work as a "panel". It will be always present there until it is somehow closed (in the current implementation, it is only when we disconnect, or write /settings again). It would be a similar idea to go to the system settings, and change settings there.

asaadmahmood · 2020-03-19T09:45:22Z

Got it. Yeah I think we can use "I" to get a more personal feel.
This post wouldn't be used much. Once I press yes or no, after a while, it may even scroll above, and I may need to access my settings from elsewhere to reconfigure the status settings.

larkox · 2020-03-19T09:54:51Z

You are right. The post will be seldom reused, and we will have to write /mscalendar settings every time we want to change a setting, since the post will scroll above, but I kind of like the idea of persistent panel. That way, you also are able to better see dependency between settings.
Finally, knowing the current state is important, since you are not changing here one single setting. You are taking a look at all the settings, and changing the one you want.

aaronrothschild

LGTM

levb · 2020-03-24T05:23:06Z

server/utils/settingspanel/handler.go

+	"github.com/mattermost/mattermost-server/v5/model"
+)
+
+type sh struct {


1/5 s/sh/handler/

levb · 2020-03-24T05:25:09Z

server/utils/bot/poster.go

+	DeletePost(postID string) error
+
+	// DMUpdatePost substitute one post with another
+	DMUpdatePost(post *model.Post) error


s/DMUpdatePost/UpdatePost/ ??

levb · 2020-03-24T05:28:48Z

Nice stuff! I think it'll need to mature and get simplified a bit, but I want to use it, now!

mickmister

Great separation of concerns. I really like the generic implementation in the utils/settingspanel package. LGTM 👍

mickmister · 2020-03-24T16:24:30Z

server/store/setting_store.go

+	GetConfirmationSettingID = "get_confirmation"
+)
+
+func (s *pluginStore) SetSetting(userID, settingID string, value interface{}) error {


value interface{} Great, I see this can support strings as well 👍

codecov-io · 2020-03-26T10:35:25Z

Codecov Report

Merging #62 into master will decrease coverage by 2.35%.
The diff coverage is 0.98%.

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   26.20%   23.85%   -2.36%     
==========================================
  Files          57       60       +3     
  Lines        2015     2218     +203     
==========================================
+ Hits          528      529       +1     
- Misses       1432     1634     +202     
  Partials       55       55

Impacted Files	Coverage Δ
server/command/command.go	`33.33% <0.00%> (-0.96%)`	⬇️
server/command/settings.go	`0.00% <0.00%> (ø)`
server/mscalendar/mscalendar.go	`100.00% <ø> (ø)`
server/mscalendar/settings.go	`0.00% <0.00%> (ø)`
server/mscalendar/settings_daily_summary.go	`0.00% <0.00%> (ø)`
server/command/disconnect.go	`100.00% <100.00%> (ø)`
server/mscalendar/notification.go	`47.26% <100.00%> (ø)`

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 20e7195...207f876. Read the comment docs.

mickmister

Great job @larkox! I have a few questions and requests since last review

server/utils/settingspanel/empty_setting.go

server/store/setting_store.go

server/utils/settingspanel/handler.go

server/utils/settingspanel/read_only_setting.go

server/mscalendar/settings_daily_summary.go

server/plugin/plugin.go

mickmister · 2020-04-09T20:09:40Z

server/mscalendar/settings_daily_summary.go

+
+		actions = []*model.PostAction{&actionOptionsH, &actionOptionsM, &actionOptionsAPM}
+
+		if currentTextValue != "Not set" {


currentTextValue is initialized as Not set. but is being compared to Not set here.

server/store/setting_store.go

mickmister · 2020-04-09T20:37:19Z

server/store/setting_store.go

+	stringValue := value.(string)
+	splittedValue := strings.Split(stringValue, " ")
+	timezone := strings.Join(splittedValue[1:], " ")
+
+	found := false
 	for _, dsum := range dsumIndex {
 		if dsum.MattermostUserID == userID {
-			stringValue := value.(string)
-			if stringValue == "true" {
+			found = true
+
+			if splittedValue[0] == "true" {
 				dsum.Enable = true
 				break
 			}

-			if stringValue == "false" {
+			if splittedValue[0] == "false" {
 				dsum.Enable = false
 				break
 			}

-			dsum.PostTime = value.(string)
+			dsum.PostTime = splittedValue[0]
+			dsum.Timezone = timezone


This seems pretty hacky to me. What is the reason for needing to accept interface{} here?

Maybe make a struct to pass in here instead of doing string parsing. We can still accept interface{}, but assert it to be what we expect it to be. This way, the caller can be more specific with what it's passing.

There are many considerations here to do it this way.

First of all, this function is called (with a few steps) by the handler, which is agnostic.
The handler passes the context in case of buttons, the option in case of selects. Since this is a select in general, we should receive a string, which is the only value an option can have, and then parse it.

Regarding receiving interface instead of string, is just to make the type conversions where it makes sense, which is here. Otherwise, we would have to make the conversions in the case block on the switch on SetSetting.

We could put all the parsing in one function that returns this struct you are talking about, but I am not sure whether it would improve the readibility.

What happens when this value gets more complicated, or we have a more complicated value we have to handle similarly? Is there anyway the origin of the data can tell this function "this is the post time", or "this is the enable flag"? For example, if this store value had more than one boolean, this would not work.

We could codify the option content as a JSON string. What do you think?

mickmister · 2020-04-09T20:39:29Z

server/store/setting_store.go

 		}
 	}

-	return "Daily summary not set", nil
+	err = s.SaveDailySummaryIndex(dsumIndex)


StoreDailySummaryUserSettings is available so you don't need to access the index directly

mickmister

Thanks for addressing the feedback @larkox! LGTM after a few more questions/comments are resolved

mickmister · 2020-04-13T12:47:59Z

server/mscalendar/settings_daily_summary.go

+	currentValueMessage := "Disabled"
+
+	actions := []*model.PostAction{}
+	if !disabled {


This !disabled block is pretty long, leading up to just about the end of the function. Do you think we can instead have a short if disabled block that returns early on?

mickmister · 2020-04-13T12:49:28Z

server/mscalendar/settings_daily_summary.go

+			if dsum != nil {
+				timezone = dsum.Timezone
+			} else {
+				timezone = "UTC"


I'm not sure what benefit the user gets in setting their settings with the wrong timezone. 1/5 Instead of falling back on UTC, return an error here saying that we couldn't get their timezone from Microsoft.

mickmister · 2020-04-13T12:51:40Z

server/mscalendar/settings_daily_summary.go

+
+		actions = []*model.PostAction{&actionOptionsH, &actionOptionsM, &actionOptionsAPM}
+
+		buttonText := "Enable"


What purpose did the if currentTextValue == "Not set." block serve that is no longer necessary? Just want to understand the code.

The idea was not to show the Enable button before a Daily Summary was set, but I thought that it would make sense to allow the enable with the default value.

…taken from Microsoft

mickmister

Awesome! LGTM 👍

aaronrothschild · 2020-04-13T15:23:53Z

LGTM

mickmister · 2020-04-16T18:49:16Z

/update-branch

DHaussermann

Spoke with @mickmister We want to merge this now to be verified in master. This will aid in making other PRs more testable.

Add settings panel

a93fa42

larkox requested review from mickmister and aaronrothschild March 18, 2020 16:34

larkox added the 1: PM Review Requires review by a product manager label Mar 18, 2020

aaronrothschild previously approved these changes Mar 20, 2020

View reviewed changes

aaronrothschild removed the 1: PM Review Requires review by a product manager label Mar 20, 2020

larkox added the 2: Dev Review Requires review by a core committer label Mar 20, 2020

larkox requested a review from levb March 20, 2020 17:42

levb previously approved these changes Mar 24, 2020

View reviewed changes

Rename sh to handler and DMUpdatePost to UpdatePost

c394d60

larkox dismissed stale reviews from levb and aaronrothschild via c394d60 March 24, 2020 16:08

mickmister previously approved these changes Mar 24, 2020

View reviewed changes

larkox added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Mar 24, 2020

larkox requested a review from DHaussermann March 24, 2020 16:42

Fix compile errors and update mocks

b273b2e

larkox dismissed mickmister’s stale review via b273b2e March 24, 2020 16:44

Merge branch 'master' into GH61SettingsPanel

e4f6f3a

Fix test

9bb6502

mickmister previously approved these changes Mar 27, 2020

View reviewed changes

larkox requested a review from mickmister April 2, 2020 12:12

mickmister added the 2: Dev Review Requires review by a core committer label Apr 2, 2020

Handle case when no Daily Summary is set and handle timezones

b9ce46a

levb previously approved these changes Apr 8, 2020

View reviewed changes

mickmister suggested changes Apr 9, 2020

View reviewed changes

larkox added 4 commits April 13, 2020 11:33

Fix typo

f20ce5b

Merge branch 'master' into GH61SettingsPanel

540d91e

Fix merge errors

0af3903

Address PR feedback

ffca8e1

larkox dismissed levb’s stale review via ffca8e1 April 13, 2020 10:57

larkox requested a review from mickmister April 13, 2020 11:03

mickmister suggested changes Apr 13, 2020

View reviewed changes

Change if order and remove default behaviour when no timezone can be …

207f876

…taken from Microsoft

larkox requested a review from mickmister April 13, 2020 13:45

mickmister approved these changes Apr 13, 2020

View reviewed changes

levb approved these changes Apr 13, 2020

View reviewed changes

levb added 1: PM Review Requires review by a product manager and removed 2: Dev Review Requires review by a core committer labels Apr 13, 2020

levb added this to the 0.1.0 milestone Apr 13, 2020

aaronrothschild removed the 1: PM Review Requires review by a product manager label Apr 13, 2020

Merge branch 'master' into GH61SettingsPanel

95aa23a

larkox mentioned this pull request Apr 16, 2020

Allow user setting for receiving notifications for upcoming events #87

Closed

Merge branch 'master' into GH61SettingsPanel

9e3ca46

DHaussermann approved these changes Apr 16, 2020

View reviewed changes

DHaussermann removed the 3: QA Review Requires review by a QA tester label Apr 16, 2020

mickmister merged commit 396a032 into mattermost:master Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add settings panel #62

Add settings panel #62

larkox commented Mar 18, 2020 •

edited by hanzei

Loading

larkox commented Mar 18, 2020

aaronrothschild commented Mar 18, 2020

larkox commented Mar 18, 2020

asaadmahmood commented Mar 19, 2020

larkox commented Mar 19, 2020

asaadmahmood commented Mar 19, 2020

larkox commented Mar 19, 2020

aaronrothschild left a comment

levb Mar 24, 2020

levb Mar 24, 2020

levb commented Mar 24, 2020

mickmister left a comment

mickmister Mar 24, 2020

codecov-io commented Mar 26, 2020 •

edited

Loading

mickmister left a comment

mickmister Apr 9, 2020

mickmister Apr 9, 2020

larkox Apr 13, 2020

mickmister Apr 13, 2020

larkox Apr 13, 2020

mickmister Apr 9, 2020

mickmister left a comment

mickmister Apr 13, 2020

mickmister Apr 13, 2020

mickmister Apr 13, 2020

larkox Apr 13, 2020

mickmister left a comment

aaronrothschild commented Apr 13, 2020

mickmister commented Apr 16, 2020

DHaussermann left a comment


		actions = []*model.PostAction{&actionOptionsH, &actionOptionsM, &actionOptionsAPM}

		if currentTextValue != "Not set" {


		actions = []*model.PostAction{&actionOptionsH, &actionOptionsM, &actionOptionsAPM}

		buttonText := "Enable"

Add settings panel #62

Add settings panel #62

Conversation

larkox commented Mar 18, 2020 • edited by hanzei Loading

Summary

Ticket Link

larkox commented Mar 18, 2020

aaronrothschild commented Mar 18, 2020

larkox commented Mar 18, 2020

asaadmahmood commented Mar 19, 2020

larkox commented Mar 19, 2020

asaadmahmood commented Mar 19, 2020

larkox commented Mar 19, 2020

aaronrothschild left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levb commented Mar 24, 2020

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 26, 2020 • edited Loading

Codecov Report

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

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

mickmister left a comment

Choose a reason for hiding this comment

aaronrothschild commented Apr 13, 2020

mickmister commented Apr 16, 2020

DHaussermann left a comment

Choose a reason for hiding this comment

larkox commented Mar 18, 2020 •

edited by hanzei

Loading

codecov-io commented Mar 26, 2020 •

edited

Loading