Add sequence diagram to show overall initial flow #1253

srvance · 2017-03-08T20:52:54Z

I added a sequence diagram to the ESA Torii GitHub guide showing the overall flow. I'd love a review. I can easily make derived versions with more detail or showing other scenarios, but I wanted to start here.

This is meant to be high level, so there are some details that are left out. Please let me know if you feel I skipped any important ones or included any that confuse for the target level.

If I pay, I could generate a higher resolution PNG or an SVG that we could possibly change the styling on. Let me know if that's desirable. I can also export the raw text description if you'd find that useful.

… of the various pieces.

marcoow · 2017-03-09T19:40:37Z

Ideally we would use something like GraphViz or a similar tool (I'm not sure GraphViz actually supports sequence diagrams) that would allow us to have source files for the diagrams in the repo vs. the generated SVGs/PNGs. That would also make changes easier as we could change individual parts of the diagram without having to replace the whole generated file (and we could comment on individual changed lines).

marcoow · 2017-03-09T19:45:15Z

https://github.com/knsv/mermaid looks like something that could work very well I guess?

In general this is a great addition 🎉 - I think we'll just have to find a good way for keeping source files in the repo and have a good way of regenerating the diagram reliably.

srvance · 2017-03-09T20:24:05Z

That package is inspired by the tool I used to generate this, so the raw source file for the diagram will work unchanged! There's even an ember-mermaid addon! What's the best way to embed this?

srvance · 2017-03-09T20:35:40Z

So ember-mermaid doesn't work in current Ember, but I can fix it if we want to use it.

marcoow · 2017-03-10T15:45:46Z

I don't think we need ember-mermaid as we don't need to render the diagram on the fly in an Ember app. I guess adding mermaid as a devDependency (+ a script in package.json for rebuilding the diagram) should be fine.

srvance · 2017-03-10T15:58:41Z

I added the source for the sequence diagram. We can use mermaid as a devDependency. Should I trigger the SVG generation from the ember-cli-build.js? For 'production' only? Perhaps with a separate entry in package.json "scripts", as well?

marcoow · 2017-03-10T16:00:06Z

I think building from ember-cli-build.js isn't necessary as we'll be including both the source as well as the built artifact in the repo anyway. A script in package.json should be sufficient. Maybe also one for a mermaid build server that rebuilds the diagram continuously when the source file changes as well (if mermaid supports that)?

srvance · 2017-03-13T01:34:36Z

How's this? You need to do npm run-script diagrams since it's a non-standard command. It regenerates both the PNG and the SVG from the source.

Mermaid doesn't have a continuous mode that I could find. The CLI wrapper already existed.

marcoow

This is pretty cool!

Also the fact that we now have the source file for the diagram in the repo allows commenting on individual things in the diagram which is pretty awesome I think.

I left a few comments regarding the content of the diagram.

Do you think we need both the SVG and the PNG or would the PNG alone be sufficient? I guess the PNG is certainly valuable for seeing the effects of the changes to the diagram's source on PR pages but maybe we can use it in the guide as well?

marcoow · 2017-03-14T15:45:26Z

package.json

@@ -19,7 +19,8 @@
    "test": "ember try:each",
    "test:node": "mocha node-tests node-tests/blueprints",
    "test:fastboot": "mocha fastboot-tests",
-    "lint": "eslint app addon blueprints config server test-support tests *.js"
+    "lint": "eslint app addon blueprints config server test-support tests *.js",
+    "diagrams": "node node_modules/mermaid/bin/mermaid.js --svg --png -o guides/assets guides/assets/esa-initial-flow.txt"


I think

"diagrams": "mermaid --svg --png -o guides/assets guides/assets/esa-initial-flow.txt"

should work?

Done. Works well. Nice to know.

marcoow · 2017-03-14T15:46:58Z

guides/assets/esa-initial-flow.txt

+    participant GitHub
+    participant Token Exchanger
+    User->>App: Use https://git-tool.com
+    App->>Application Route: transitionTo(application)


This transitionTo doesn't actually happen. Instead this is the application route's beforeModel.

Agreed. This assumes the Application Route uses the ApplicationRouteMixin and is a deliberate simplification. Is there a better way to indicate the use of the mixin?

Do you think

App->>Application Route: beforeModel Application Route->>Application Route: transitionTo(application)

would be more accurate?

I think App->>Application Route: beforeModel is good? I guess the ApplicationRouteMixin is already covered in the text anyway?

I was trying to maintain some brevity in the actor labels, but in the process turned the mixins into implicit actors and the routes into unnamed entities that are almost pieces of data. Should I just be more verbose in the actor names, e.g. Index Route (AuthenticatedRouteMixin)?

I think the points you're raising highlight that the diagram doesn't really stand on its own with the simplifications I made. It either requires embedding in an explanatory context or more elaboration in the diagram itself. What do you think?

Should I just be more verbose in the actor names, e.g. Index Route (AuthenticatedRouteMixin)?

I think that sounds like a good idea.

I think the points you're raising highlight that the diagram doesn't really stand on its own with the simplifications I made. It either requires embedding in an explanatory context or more elaboration in the diagram itself. What do you think?

I think we'll probably never be able to make the diagram stand on its own as it just includes so much information that it will always need some context I guess. I don't think that's a problem though as long as it works well in combination with the text? My main point was really only that a few things in there are slightly different from what actually happens in reality (e.g. there is no transitionTo when the user boots the app on the index route) and thus might be misleading.

marcoow · 2017-03-14T15:47:58Z

guides/assets/esa-initial-flow.txt

+    User->>App: Use https://git-tool.com
+    App->>Application Route: transitionTo(application)
+    Application Route->>Application Route: subscribeToSessionEvents()
+    App->>Authenticated Route: transitionTo(index)


This transitionTo also doesn't really happen, instead the app boots up in that route.

Similar. The actor name is meant to imply the use of the corresponding mixin.

Yes, but what happens is actually that the index route's beforeModel method is invoked.

marcoow · 2017-03-14T15:49:07Z

guides/assets/esa-initial-flow.txt

+    Unauthenticated Route->>Unauthenticated Route: beforeModel()
+    Unauthenticated Route->>Session Service: isAuthenticated?
+    Session Service-->>Unauthenticated Route: false
+    Unauthenticated Route->>Unauthenticated Route: render(loginTemplate)


I'd remove the loginTemplate here as that seems to indicate this is something that is configurable inside of ESA which isn't really the case. Instead, the route simply renders its own template.

What if this transition went back to User instead? It definitely isn't ESA, but it ties the Ember behavior to what the user sees in a more concrete way, although with a little hand-waving.

Pushed some proposed changes.

marcoow · 2017-03-14T15:49:35Z

guides/assets/esa-initial-flow.txt

+    Session Service-->>Unauthenticated Route: false
+    Unauthenticated Route->>Unauthenticated Route: render(loginTemplate)
+    User->>App: Login
+    App->>Unauthenticated Route: login


Maybe say click "Login" or so here?

In the User->>App transition? Good idea.

Pushed changes.

marcoow · 2017-03-14T15:51:02Z

guides/assets/esa-initial-flow.txt

+    App->>Unauthenticated Route: login
+    Unauthenticated Route->>Unauthenticated Route: controller.login()
+    Unauthenticated Route->>Session Service: authenticate('authenticator:torii', 'github')
+    Session Service->>Torii Provider: authenticate()


Should there be 2 separate items for the Torii authenticator and the Torii provider that this uses under the hood?

I debated how much detail to go into for the Torii Provider. Do you think some internal details of the interaction with the torii API would be appropriate?

srvance · 2017-03-16T15:30:48Z

I think this is more accurate and addresses your concerns.

marcoow

Looks great!

I left a few comments on the diagram.

I'd also remove the SVG and always use the PNG maybe as we don't really need both formats.

marcoow · 2017-03-16T16:11:13Z

guides/assets/esa-initial-flow.txt

+    Login Route (UnauthenticatedRouteMixin)->>Login Route (UnauthenticatedRouteMixin): beforeModel()
+    Login Route (UnauthenticatedRouteMixin)->>Session Service: isAuthenticated?
+    Session Service-->>Login Route (UnauthenticatedRouteMixin): false
+    Login Route (UnauthenticatedRouteMixin)-->>App: render(loginTemplate)


I think I'd remove this as it might cause confusion. This is something that happens internally in Ember and that is also in no way overridden or hooked into by ESA so I don't think this needs to be shown at all.

When I took it out, it left a gap in the flow of control that seemed strange to me. As you'll see in the next commit, I left it in but gave it informal wording rather than making it look like a function. The reference to loginTemplate isn't really necessary with the actor relabeling.

marcoow · 2017-03-16T16:13:00Z

guides/assets/esa-initial-flow.txt

+    App-->>User: Login screen
+    User->>App: Click "Login"
+    App->>Login Route (UnauthenticatedRouteMixin): login
+    Login Route (UnauthenticatedRouteMixin)->>Login Route (UnauthenticatedRouteMixin): controller.login()


This isn't really what actually happens - the route calling the login method on the controller. Instead it might make sense to add the Login controller to the diagram and replace this and line 24 above with App->>Login Controller: login action?

Done. This was a brevity/accuracy trade off. The larger message I'm hearing is that you prefer accuracy over brevity, which makes sense in this context. Brevity made more sense in the presentation I originally developed this for.

marcoow · 2017-03-16T16:13:26Z

guides/assets/esa-initial-flow.txt

+    App->>Login Route (UnauthenticatedRouteMixin): login
+    Login Route (UnauthenticatedRouteMixin)->>Login Route (UnauthenticatedRouteMixin): controller.login()
+    Login Route (UnauthenticatedRouteMixin)->>Session Service: authenticate('authenticator:torii', 'github')
+    Session Service->>Torii Provider: authenticate()


This would be open().

You're right. This is accurate if it's the ToriiAuthenticator but then omits some torii interactions. I've differentiated between ToriiAuthenticator and Torii Provider now.

marcoow · 2017-03-16T16:14:48Z

guides/assets/esa-initial-flow.txt

+    Token Exchanger-->>Torii Provider: access_token
+    Torii Provider-->>Session Service: access_token
+    Session Service-->>Login Route (UnauthenticatedRouteMixin): isAuthenticated=true
+    Login Route (UnauthenticatedRouteMixin)-->>Index Route (AuthenticatedRouteMixin): transitionTo(routeIfAlreadyAuthenticated)


In this and the above line we could replace Login Route (UnauthenticatedRouteMixin) with Login Controller if we added that to the diagram as well.

I reworked this considerably to be more accurate, I think.

srvance · 2017-03-16T19:29:40Z

Especially with the additional actors, I find the PNG too small unmagnified and fuzzy magnified. I prefer the scalable representation, which, along with the size, is why I linked it from the guide instead of embedding it. Of course, the SVG has the problems that not all browsers handle it and markdown can't embed it. Let me know if you still prefer the PNG after looking at the latest revision.

marcoow · 2017-03-17T08:04:42Z

@srvance: can't we make the PNG bigger though?

marcoow

Only one last comment - sorry for the back and forth…

Also I think if we can make the PNG much larger I think we can drop the SVG?

marcoow · 2017-03-17T08:06:40Z

guides/assets/esa-initial-flow.txt

+    App-->>User: Login screen
+    User->>App: Click "Login"
+    App->>Login Route (UnauthenticatedRouteMixin): login
+    Login Route (UnauthenticatedRouteMixin)->>Login Controller: login action


I think line 26 and 27 could actually be combined into one:

App->>Login Controller: login action

The route isn't actually involved in this at all.

Good catch.

…tor text doesn't overflow the boxes. Remove SVG generation.

srvance · 2017-03-17T16:24:02Z

It took a combination of CSS styling, a config file, and adjusting the generation command, but I think we have a result we can both approve.

Thank you for all the great feedback! I think we have a result that really supplements the documentation nicely and that we can use to create additional diagrams.

marcoow · 2017-03-17T17:22:33Z

Do you think the extra configs for the graph are necessary? I feel the more we add the more we have to maintain in the long run. If we can simply make the graph larger but use the standard layout we'd save ourselves a bunch of LOC that then don't become part of the codebase?

srvance · 2017-03-17T21:04:32Z

The CSS is the only way to control the font, which gets otherwise resized when the diagram width is changed. The config is the only way to control the actor box size, which ensures the actor text doesn't overlap when the size increases and that the text fits within the box. Unfortunately, it doesn't accept incremental CSS and configs. If you supply them, it omits its own. I'm not thrilled with the number of pieces, either.

marcoow · 2017-03-20T12:12:23Z

@srvance: this config seems to be sufficient actually:

{
  "width": 275,
  "height": 65,
  "boxMargin": 10,
  "boxTextMargin": 5
}

I'm not sure we need to control the font at all actually. I produced this with changing the diagrams command to

"diagrams": "mermaid --sequenceConfig guides/assets/esa-initial-flow.config.json --width 2400 -o guides/assets guides/assets/esa-initial-flow.txt"

srvance · 2017-03-20T14:20:41Z

I investigated the CSS before finding the config settings and didn't go back to re-evaluate the styling. Thanks for digging into this. It seems the larger rendering clears up some of the font legibility issues I was trying to solve with the styling.

I didn't use any, obviously, but we can also add notes if there are areas that could use more explanation. I forgot about that when we were discussing some of the representations earlier.

marcoow

Yay 🎉

This is certainly a great addition to the guide!

Add sequence diagram to show overall initial flow and the interaction…

1b67a6a

… of the various pieces.

marcoow added the documentation label Mar 9, 2017

Add source for sequence diagram

6012f68

Add the script to generate the flow diagram from the source.

8589ee8

marcoow reviewed Mar 14, 2017

View reviewed changes

srvance added 3 commits March 14, 2017 21:04

Simplify the "diagrams" script invocation.

1d3d114

Improve communication about the user login interaction.

42e8d62

Better represent the initial route visitation.

466835c

marcoow reviewed Mar 16, 2017

View reviewed changes

Improve the accuracy of the flow.

f7e1e3b

marcoow reviewed Mar 17, 2017

View reviewed changes

srvance added 3 commits March 17, 2017 09:55

Simplify login action transitions.

b7bcab6

Fix typo.

c0b19c7

Adjust styling so that the PNG is clear when magnified and so that ac…

7399327

…tor text doesn't overflow the boxes. Remove SVG generation.

Minimize the amount of configuration necessary for a legible diagram.

836d727

marcoow approved these changes Mar 20, 2017

View reviewed changes

marcoow merged commit b57dd72 into mainmatter:master Mar 20, 2017

marcoow deleted the add-initial-flow-diagram branch March 20, 2017 14:52

Add sequence diagram to show overall initial flow #1253

Add sequence diagram to show overall initial flow #1253

Conversation

srvance commented Mar 8, 2017

marcoow commented Mar 9, 2017 • edited Loading

marcoow commented Mar 9, 2017

srvance commented Mar 9, 2017

srvance commented Mar 9, 2017

marcoow commented Mar 10, 2017

srvance commented Mar 10, 2017

marcoow commented Mar 10, 2017 • edited Loading

srvance commented Mar 13, 2017

marcoow 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srvance commented Mar 16, 2017

marcoow 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

srvance commented Mar 16, 2017

marcoow commented Mar 17, 2017

marcoow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srvance commented Mar 17, 2017

marcoow commented Mar 17, 2017 • edited Loading

srvance commented Mar 17, 2017

marcoow commented Mar 20, 2017

srvance commented Mar 20, 2017

marcoow left a comment

Choose a reason for hiding this comment

marcoow commented Mar 9, 2017 •

edited

Loading

marcoow commented Mar 10, 2017 •

edited

Loading

marcoow commented Mar 17, 2017 •

edited

Loading