Login workflow #44

larkox · 2020-03-03T19:56:15Z

Summary

Added a first approach to the login workflow defined here. The current solution is far from optimal, but allows us to get a better understanding of the different moving parts of the solution.

I will keep working to improve the different parts, both in how to present the data, and the underlying architecture. Here is the current look of a full login process.

mickmister

@larkox Great job with the Welcomer patterns introduced.

My main blocker here is getting the mscalendar-specific Welcomer code out of the utils package. No issues found otherwise 👍

mickmister · 2020-03-04T05:01:46Z

server/command/connect.go

-		c.Config.PluginURL)
+	out := "The bot just sent you a message with the instructions to connect."
+	if err := c.MSCalendar.Welcome(c.Args.UserId); err != nil {
+		out = "There has been a  problem while trying to connect."


Looks like two spaces got in there somehow

Suggested change

out = "There has been a problem while trying to connect."

out = "There has been a problem while trying to connect."

mickmister · 2020-03-04T05:03:49Z

server/command/connect.go

-		config.ApplicationName,
-		c.Config.PluginURL)
+	out := "The bot just sent you a message with the instructions to connect."
+	if err := c.MSCalendar.Welcome(c.Args.UserId); err != nil {


We're moving away from the if statements in this format, favoring:

err := c.MSCalendar.Welcome(c.Args.UserId) if err != nil { out = "There has been a problem while trying to connect." }

mickmister · 2020-03-04T05:28:04Z

server/utils/bot/welcomer.go

+	Welcome(userID string) error
+	AfterSuccessfullyConnect(userID, userLogin string) error
+	AfterUpdateStatus(userID string, status bool) error
+	AfterSetConfirmations(userID string, set bool) error


The utils package is more-or-less supposed to be plugin-agnostic. Maybe this code should go into the mscalendar package since the sign-up flow is specific to this plugin. Then we can access the typed kvstore in the store package, to store the WelcomePostID.

Edit: Or it can go in a new package that has knowledge of the mscalendar package.

…mscalendar and improve postID handling on the welcomer

… to get the status updated

larkox · 2020-03-11T09:06:58Z

Here I leave some screenshots of the current behaviour:
Ephemeral:

Bot messages step 1:

Bot messages step 2:

Bot messages step 3:

Bot messages step 4:

codecov-io · 2020-03-11T09:15:22Z

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #44   +/-   ##
=======================================
  Coverage   21.62%   21.62%           
=======================================
  Files          64       64           
  Lines        2418     2418           
=======================================
  Hits          523      523           
  Misses       1836     1836           
  Partials       59       59

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 5a5c110...5a5c110. Read the comment docs.

levb

Left initial feedback; can use a clarification on the stored states/transitions.

Makefile

server/mscalendar/calendar.go

levb · 2020-03-11T17:33:57Z

server/mscalendar/mscalendar.go

@@ -18,6 +18,7 @@ type MSCalendar interface {
 	EventResponder
 	Subscriptions
 	Users
+	Welcome(userID string) error


Why not Welcomer?

(1/5) MSCalendar is not a welcomer, the bot in the dependencies is. But MSCalendar must know how to greet a new user, to be used on the commands, so that is why we expose that function on the interface.

Dunno, I'd advocate composing the entire interface in. What's the harm? Polluting MSCalendar namespace?

levb · 2020-03-11T17:43:11Z

server/mscalendar/welcomer.go

+	AfterDisconnect(userID string) error
+}
+
+type MSCalendarBot struct {


suggest s/MSCalendarBot/welcomer/ or... what I think is really the idea here was to have a mscalendar.Bot (implemented as type bot struct) that implements all messaging, not just Welcome.

levb · 2020-03-11T17:45:12Z

server/mscalendar/welcomer.go

+}
+
+func (bot *MSCalendarBot) AfterSuccessfullyConnect(userID, userLogin string) error {
+	user, err := bot.Store.LoadUser(userID)


in solar lottery I chose to pass in *User, etc and then Expand - most of the time the caller should have the expanded entity values already, if not, create an ID-only entity, and it gets expanded to format the message. Ping me if you want an example.

I understand the benefit and how to do it, but I think with the new shape of the program this is not necessary. Could you confirm?

server/store/welcome_store.go

larkox · 2020-03-12T16:51:51Z

@levb After our conversation yesterday, I could not stop thinking about making the flow more detached from the welcome logic, so I made a refactoring in this line. Now we can add any steps we want as long as they are Yes/No questions.

With the current implementation, we declare our flow with its steps (an implementation of Flow interface) and our flowStore (an implementation of FlowStore). With that, Welcomer still handles the connection, but all the flow of Yes/No questions is handled by the Flow.

mickmister

Awesome work @larkox! Just some suggestions and questions.

mickmister · 2020-03-13T13:52:20Z

server/command/connect_test.go

 			},
-			expectedOutput: "[Click here to link your Microsoft Calendar account.](http://localhost/oauth2/connect)",
-			expectedError:  "",
+			expectedOutput: `Welcome to the Microsoft Calendar Bot.


I'm not sure what this should be worded as, but I'm thinking that we should be welcoming them to the plugin or the functionality, instead of the bot.

We should review the consolidated messages with @aaronrothschild and @asaadmahmood - let's focus on making sure they're all "in one place"?

mickmister · 2020-03-13T13:53:41Z

server/mscalendar/oauth2_test.go

@@ -1,4 +1,4 @@
-package mscalendar
+package mscalendar_test


Maybe a confused question, but if it's a separate package, wouldn't ./test be a better place?

This is a workaround that Go has for cycles in the tests. Conceptually, it is still a test of mscalendar, but due to the import cycles, it cannot be part of mscalendar package. With this naming, the compiler consider this a separate package, which solves the import cycles.

^^ seems fishy to me, we should be well-clear of cyclical dependencies. I'm ok with the PR as is, but would like to understand this better, offline?

Solved by creating the mock of the welcomer in a different package.

server/mscalendar/welcomer.go

server/utils/bot/bot.go

server/utils/flow/handler.go

mickmister · 2020-03-13T14:50:06Z

server/utils/bot/flower.go

+}
+
+func (bot *bot) setFlowStep(userID string, step int) error {
+	// TODO: Use KVStore for HA


I assume this will be covered in this PR? I'd be fine with deferring to a ticket if you want to get this merged sooner.

My plan is to have this covered in this PR, but I wanted to confirm first that the new shape of the flow was interesting. I will take a look to have this done tomorrow.

levb · 2020-03-15T19:29:35Z

server/command/connect_test.go

 			},
-			expectedOutput: "[Click here to link your Microsoft Calendar account.](http://localhost/oauth2/connect)",
-			expectedError:  "",
+			expectedOutput: `Welcome to the Microsoft Calendar Bot.


We should review the consolidated messages with @aaronrothschild and @asaadmahmood - let's focus on making sure they're all "in one place"?

levb · 2020-03-15T19:31:20Z

server/mscalendar/mscalendar.go

@@ -18,6 +18,7 @@ type MSCalendar interface {
 	EventResponder
 	Subscriptions
 	Users
+	Welcome(userID string) error


Dunno, I'd advocate composing the entire interface in. What's the harm? Polluting MSCalendar namespace?

levb · 2020-03-15T19:32:55Z

server/mscalendar/oauth2_test.go

@@ -1,4 +1,4 @@
-package mscalendar
+package mscalendar_test


Maybe a confused question, but if it's a separate package, wouldn't ./test be a better place?

levb · 2020-03-15T19:41:16Z

server/utils/flow/flow.go

+	}
+}
+
+func (s *step) PostSA(flowHandler string, i int) *model.SlackAttachment {


I know it's verbose, but 2/5 SlackAttachment please... (grep-ability)

mickmister · 2020-04-20T00:54:14Z

server/utils/settingspanel/handler.go

-	sh.panel.Set(mattermostUserID, idString, value)
+	err := sh.panel.Set(mattermostUserID, idString, value)
+	if err != nil {
+		utils.SlackAttachmentError(w, "Error: cannot set the property, "+err.Error())


Do these errors get shown in the UI? It seems a bit low-level

Yes, these would get shown in the UI. My idea is the error to be meaningful to debug, but we could move the internals of the error to a log in the server. Nevertheless, that would imply passing a logger to the handler, which will make things a bit dirtier.

DHaussermann · 2020-04-22T16:44:21Z

@larkox I have done a test run through this now. Just a couple points.

It seems like the login workflow may not properly store the response to confirm login status change? Maybe this is just because the related PR is not in this branch?
Using mscalendar connect creates the new post that is part of the login workflow as well as the previous one. So the user ends up with 2. Can you get rid of the old one?
UX point - Tell me HOW to change the settings here (using the slash command)

larkox · 2020-04-22T16:49:32Z

That must be a bug. I will look into it ASAP.
It is a complicated decision. Slash commands must return something. And we want to have this connect message on the bot. At some point I thought about a "The MSCalendar bot just sent you the information to connect", but that adds another step. Also, the normal flow would be to call the command in a different channel than the bots direct message, so in general, you will not see the two messages together. @aaronrothschild Any thoughts on this?
My bad. This was changed at some point (since we didn't have a settings panel at the moment), and have to be changed again as you said.

aaronrothschild · 2020-04-22T16:53:18Z

2. It is a complicated decision. Slash commands must return something. And we want to have this connect message on the bot. At some point I thought about a "The MSCalendar bot just sent you the information to connect", but that adds another step. Also, the normal flow would be to call the command in a different channel than the bots direct message, so in general, you will not see the two messages together. @aaronrothschild Any thoughts on this?

@larkox sorry, I'm not following. Where does the double message show up?

larkox · 2020-04-22T17:00:15Z

@aaronrothschild
After /mscalendar connect you will receive an answer to the slash command (and ephemeral message) on whichever channel you are. You will also receive a Direct Message from the bot. Both messages have almost the same information, with the only difference being that the bot one will change when you successfully connect.
If you were already on the Direct Message of the bot (something that normal users will not do in general), you will see both messages together, as the image Dylan shared.
We could remove the message from the bot and just leave the command response (the ephemeral message) provide the connection link, and when the connection is finished, then send the welcome message to the user.
Or we could give as a command response a message like "The MSCalendar bot just sent you the information on how to connect your account."

larkox · 2020-04-22T17:13:13Z

@DHaussermann
Just heads up that the latest commit should fix 1 and 3.

larkox · 2020-04-23T10:24:32Z

@aaronrothschild I just discovered that slash commands can redirect the user to a different URL. What do you think about /mscalendar connect redirects to the bot direct message channel? Also, my assumption about the slash commands not being able to return nothing may be wrong. The final experience would be:

I type /mscalendar connect and hit enter. The window changes automatically to the MSCalendar bot direct message. There I find the instructions to sign in.

…refactoring

larkox · 2020-04-23T17:08:29Z

@DHaussermann Not sure what is happening with the CI, but this latest commit should fix also 2, so feel free to give it another round whenever you have the time.

DHaussermann · 2020-04-24T22:53:03Z

@larkox Thanks for the fixes! 1., 2. and 3. are resolved.

This may be ready to merge

Tested answering questions in every applicable combination of yes/no.
Data saved is accurate
Confirmed 3 issues above are resolved

Just a couple points:

Intermittently I see the issue where answering the last question about subscription throws an error. This may be cause by data created by builds from other branches. Uninstalling the plugin and re-installing solves the issue even though it's the same build that was already running.

Please advise if this needs to be investigated in this PR or we're okay creating a separate issue if we see it in master.
I think we should implement this suggestion to redirect the user to where the post will be Login workflow #44 (comment)

larkox · 2020-04-27T07:52:55Z

@DHaussermann

It is weird. I think that this PR is becoming bit enough as it is, so I would say we merge this, and investigate the issue as a separate ticket.
This should be already implemented. Can you not see this behaviour in the latest build?

mickmister · 2020-04-27T10:08:04Z

I've tested this as well with no issues found. Though I agree with Dylan's comment in our conversation on MM to make it so the user can change their mind within thr settings panel about whether or not they should receive event invitations.

It is weird. I think that this PR is becoming bit enough as it is, so I would say we merge this, and investigate the issue as a separate ticket.

Dylan and I have talked about stale data from different PRs affecting each other. I think it's important to document the behaviors so as to be on the lookout for them when testing master. This can be done with a follow up ticket. No reason to increase scope, thoughts @larkox?

Things like connecting to two different Microsoft accounts with one MM account (disconnecting in between them) may cause these sorts of things too. Things that we wouldn't directly design for. I'm not saying that caused this issue, just unexpected user actions like that may get the account in a weird state.

This should be already implemented. Can you not see this behavior in the latest build?

I can confirm this is implemented and working correctly. In fact, when running on localhost, it redirects me to my ngrok instance's bot DM channel. This initially caught me off guard, but it ensures I am logged into the necessary domain before connecting.

LGTM 👍

@aaronrothschild We will need your blessing here

DHaussermann

Tested and passed.

I will create an issue with as much detail as possible if I see this occur on master.
Yes sorry for being unclear on this I also see the bot DM redirect happening as expected.

Thanks for all the effort on this @larkox and thanks to @mickmister for helping with some peer testing.
@aaronrothschild I will created a follow-up issue to add the subscription question to the settings panel. (#103)
LGTM!

aaronrothschild · 2020-04-28T16:55:03Z

Sweet!

mickmister · 2020-04-28T18:03:38Z

/update-branch

First draft of login workflow

1dd41f7

larkox requested a review from mickmister March 3, 2020 19:56

mickmister added the Work In Progress Not yet ready for review label Mar 4, 2020

mickmister suggested changes Mar 4, 2020

View reviewed changes

larkox added 2 commits March 4, 2020 08:11

Fix typo, remove "if assign; condition {" instance, move Welcomer to …

459af20

…mscalendar and improve postID handling on the welcomer

Do not ask if the user want to get a confirmation if he does not want…

1e9f6cc

… to get the status updated

larkox requested a review from mickmister March 4, 2020 13:19

Welcome flow with slack attachments

4cfbfb8

Update mocks

bb1e037

larkox requested review from levb and aaronrothschild March 11, 2020 09:14

larkox added 2 commits March 11, 2020 05:17

Fix lint

6514ad2

Fix tests

9cbccd2

levb suggested changes Mar 11, 2020

View reviewed changes

Refactor welcome flow to use a flow abstraction

29c866d

Update mocks

cccc8b3

larkox requested a review from levb March 12, 2020 17:24

mickmister suggested changes Mar 13, 2020

View reviewed changes

levb suggested changes Mar 15, 2020

View reviewed changes

mickmister assigned larkox Mar 16, 2020

larkox added 6 commits March 17, 2020 10:58

Make mscalendar a Welcomer

1239a98

Make mscalendar bot as main bot

54e615f

Use mscalendar Bot as an interface

da99b3f

Make repeated connect messages come from the same variable

4c45950

Fix tests and remove cycles in oauth test

5e9e08b

Add missing error handling

4e4f9c0

mickmister approved these changes Apr 16, 2020

View reviewed changes

larkox added 2 commits April 17, 2020 09:15

Merge branch 'master' into LoginWorkflow

cdcdf02

Improve error handling in settings panel

17f0a8d

mickmister approved these changes Apr 20, 2020

View reviewed changes

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

Merge branch 'master' into LoginWorkflow

dc69f61

Add instructions for changing settings and correctly map settings

87ea5ac

larkox added 2 commits April 23, 2020 13:26

Merge branch 'master' into LoginWorkflow

85031c9

Redirect when using connect or settings to the bot message and minor …

5a5c110

…refactoring

Fix test

6aea390

DHaussermann approved these changes Apr 27, 2020

View reviewed changes

DHaussermann added the QA Review Done PR has been approved by QA label Apr 27, 2020

aaronrothschild approved these changes Apr 28, 2020

View reviewed changes

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

mickmister added the 4: Reviews Complete All reviewers have approved the pull request label Apr 28, 2020

Merge branch 'master' into LoginWorkflow

25b7b4a

mickmister merged commit 4fdc115 into mattermost:master Apr 28, 2020

	out = "There has been a problem while trying to connect."
	out = "There has been a problem while trying to connect."

Login workflow #44

Login workflow #44

Conversation

larkox commented Mar 3, 2020

Summary

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

mickmister Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

larkox commented Mar 11, 2020 • edited Loading

codecov-io commented Mar 11, 2020 • edited Loading

Codecov Report

levb 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

larkox commented Mar 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHaussermann commented Apr 22, 2020

larkox commented Apr 22, 2020

aaronrothschild commented Apr 22, 2020

larkox commented Apr 22, 2020

larkox commented Apr 22, 2020

larkox commented Apr 23, 2020

larkox commented Apr 23, 2020

DHaussermann commented Apr 24, 2020

larkox commented Apr 27, 2020

mickmister commented Apr 27, 2020 • edited Loading

DHaussermann left a comment • edited Loading

Choose a reason for hiding this comment

aaronrothschild commented Apr 28, 2020

mickmister commented Apr 28, 2020

mickmister Mar 4, 2020 •

edited

Loading

larkox commented Mar 11, 2020 •

edited

Loading

codecov-io commented Mar 11, 2020 •

edited

Loading

mickmister commented Apr 27, 2020 •

edited

Loading

DHaussermann left a comment •

edited

Loading