Store credentials in sessionStorage

Store credentials in sessionStorage

This branch moves credentials into sessionStorage in preparation for landscape work; it also adds a logout functionality.  To test, comment out the password line in app/config-debug.js and run the application.  You can login; refresh and not need to login again; logout and be presented with a login screen; refresh and still have to log in again.

https://code.launchpad.net/~makyo/juju-gui/session-storage-1123693/+merge/149669

(do not edit description out of merge proposal)

[matthew.scott, 2013-02-20 19:31:11]

Please take a look.

[gary.poster, 2013-02-20 21:55:41]

Land with changes.  Very nice Matt, thank you.

I want to qa it, but if someone else does for you, I'm not blocking.

Gary

https://codereview.appspot.com/7395043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode305
app/app.js:305: // arrive with the log), and redispatch.
Maybe, "(a delta will arrive after successfully authenticating)"

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode714
app/app.js:714: this.show_login();
What do you think about resetting the URL as well?

Oh!  this.loggingOut will do that.  Maybe add a comment to that effect?  Do what
you will.

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode767
app/app.js:767: Y.Lang.isValue(credentials.password) &&
Since you do this twice, you could make an "are_available" property on the
credentials object, so this could be

if (Y.Lang.isObject(credentials) && credentials.are_available &&
!this.env.userIsAuthenticated) {...

Whatever you want to call it.

Just an idea.  Take it if you like it.

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js#newcode80
app/store/env/python.js:80: this.setCredentials({});
You use null in the base, and {} here.  Consistency would be trivial but nice.

[jeff.pihach, 2013-02-20 22:15:58]

If I click 'login' without a password in the password field the field gets
disabled and nothing happens.

Any further refreshes to the page sit on the "connecting" screen.

[jeff.pihach, 2013-02-20 22:58:57]

https://codereview.appspot.com/7395043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode758
app/app.js:758: if (Y.Lang.isObject(credentials) &&
I'd try to combine these two if checks into one block.

This should work because javascript short circuits in if statements, so if any
of these is invalid it shows the log in screen. 

if (credentials && credentials.user && credentials.password &&
!this.env.userIsAuthenticated) {
  this.env.login();
} else {
  this.show_login();
}

https://codereview.appspot.com/7395043/diff/1/app/store/env/base.js
File app/store/env/base.js (right):

https://codereview.appspot.com/7395043/diff/1/app/store/env/base.js#newcode53
app/store/env/base.js:53: if (Y.Lang.isValue(this.get('user'))) {
You can almost always substitute isValue with a simple truthy/falsy check as
that's pretty much all it's doing under the hood.

http://yuilibrary.com/yui/docs/api/files/yui_js_yui-lang.js.html#l197

Less fn calls will make the application faster, even trivial ones.

if (this.get('user')) {}

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js#newcode80
app/store/env/python.js:80: this.setCredentials({});
On 2013/02/20 21:55:41, gary.poster wrote:
> You use null in the base, and {} here.  Consistency would be trivial but nice.

I'd agree, {} is truthy but null is falsy

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js#newcode254
app/store/env/python.js:254: credentials.user && credentials.password) {
if empty credentials is always falsy when empty (see above) then you can ditch
this isObject check

[gary.poster, 2013-02-21 14:49:42]

One other thing.  I think you will need something lines 363-392 (right side) of
https://codereview.appspot.com/7372044/diff/1/app/app.js in order to have your
logout redirect to "/" work the way you want in a namespace-URL world.  Could
you add those (or similar) changes to your branch with some tests?

I'll remove that code from my in-progress branch so your version wins.

Thanks!

Gary

[matthew.scott, 2013-02-21 18:02:13]

https://codereview.appspot.com/7395043/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode305
app/app.js:305: // arrive with the log), and redispatch.
On 2013/02/20 21:55:41, gary.poster wrote:
> Maybe, "(a delta will arrive after successfully authenticating)"

Done.

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode714
app/app.js:714: this.show_login();
On 2013/02/20 21:55:41, gary.poster wrote:
> What do you think about resetting the URL as well?
> 
> Oh!  this.loggingOut will do that.  Maybe add a comment to that effect?  Do
what
> you will.

Done.

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode758
app/app.js:758: if (Y.Lang.isObject(credentials) &&
On 2013/02/20 22:58:57, jeff.pihach wrote:
> I'd try to combine these two if checks into one block.
> 
> This should work because javascript short circuits in if statements, so if any
> of these is invalid it shows the log in screen. 
> 
> if (credentials && credentials.user && credentials.password &&
> !this.env.userIsAuthenticated) {
>   this.env.login();
> } else {
>   this.show_login();
> }

I restructured this some, but the logic changed slightly with another fix, so
it's not exactly this.

https://codereview.appspot.com/7395043/diff/1/app/app.js#newcode767
app/app.js:767: Y.Lang.isValue(credentials.password) &&
On 2013/02/20 21:55:41, gary.poster wrote:
> Since you do this twice, you could make an "are_available" property on the
> credentials object, so this could be
> 
> if (Y.Lang.isObject(credentials) && credentials.are_available &&
> !this.env.userIsAuthenticated) {...
> 
> Whatever you want to call it.
> 
> Just an idea.  Take it if you like it.

Done.

https://codereview.appspot.com/7395043/diff/1/app/store/env/base.js
File app/store/env/base.js (right):

https://codereview.appspot.com/7395043/diff/1/app/store/env/base.js#newcode53
app/store/env/base.js:53: if (Y.Lang.isValue(this.get('user'))) {
On 2013/02/20 22:58:57, jeff.pihach wrote:
> You can almost always substitute isValue with a simple truthy/falsy check as
> that's pretty much all it's doing under the hood.
> 
> http://yuilibrary.com/yui/docs/api/files/yui_js_yui-lang.js.html#l197
> 
> Less fn calls will make the application faster, even trivial ones.
> 
> if (this.get('user')) {}

We discussed this in channel and created a card for weekly.  Going to leave it
for now; if we decide to switch, we can make a slack clean-up branch.

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js
File app/store/env/python.js (right):

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js#newcode80
app/store/env/python.js:80: this.setCredentials({});
On 2013/02/20 22:58:57, jeff.pihach wrote:
> On 2013/02/20 21:55:41, gary.poster wrote:
> > You use null in the base, and {} here.  Consistency would be trivial but
nice.
> 
> I'd agree, {} is truthy but null is falsy

Done.

https://codereview.appspot.com/7395043/diff/1/app/store/env/python.js#newcode254
app/store/env/python.js:254: credentials.user && credentials.password) {
On 2013/02/20 22:58:57, jeff.pihach wrote:
> if empty credentials is always falsy when empty (see above) then you can ditch
> this isObject check

Done.

[matthew.scott, 2013-02-21 18:11:33]

Please take a look.

[gary.poster, 2013-02-21 18:35:00]

LGTM with changes.  Very nice.  Works great.  Thanks!

Gary

https://codereview.appspot.com/7395043/diff/11001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/7395043/diff/11001/app/app.js#newcode352
app/app.js:352: * of preserving existing namespaces in the url.
Before you land, could you merge in trunk and then expand the comment to explain
what overrideAllNamespaces does?

https://codereview.appspot.com/7395043/diff/11001/app/app.js#newcode707
app/app.js:707: passwordField.focus();
Nice usability improvement.  Thank you.

https://codereview.appspot.com/7395043/diff/11001/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/7395043/diff/11001/lib/views/stylesheet.less#n...
lib/views/stylesheet.less:178: color: @navbar-color;
I think we ought to remove the underline-on-hover also.

I also wonder if the new vertical dividing line ought to be the same length as
the charm one, but I'm happy to leave that to UX review.

[jeff.pihach, 2013-02-21 18:50:58]

Looks good thanks, Land as is.

[matthew.scott, 2013-02-21 19:10:39]

*** Submitted:

Store credentials in sessionStorage

This branch moves credentials into sessionStorage in preparation for landscape
work; it also adds a logout functionality.  To test, comment out the password
line in app/config-debug.js and run the application.  You can login; refresh and
not need to login again; logout and be presented with a login screen; refresh
and still have to log in again.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/7395043