Add user login support.

Add user login support.

Future work: we need to implement a real UI for this.  At the moment this
branch uses prompt() calls to generate modal dialogs.  Also, this will
probably need slight tweaking once the backend actually requires
authorization.

https://code.launchpad.net/~benji/juju-gui/login/+merge/141133

(do not edit description out of merge proposal)

[benji, 2012-12-21 20:53:42]

Please take a look.

[hazmat, 2012-12-21 21:06:18]

needs work

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

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode544
app/app.js:544: if (!view.waiting && !view.userIsAuthenticated) {
this content belongs in the app, not the view. the app needs to re-login on a
socket reconnect.

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode547
app/app.js:547: next();
this is an either or proposition, not an and. we either display the login or the
app, not overlay the login over the app, which will be broken without a working
ws.

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

https://codereview.appspot.com/7007047/diff/1/app/store/env.js#newcode77
app/store/env.js:77: this.set('serverReady', true);
instead of introducing another value just move connected down here.

[bac, 2013-01-02 14:45:03]

This looks ok to me, given the changes Kapil requests.  I'll look again after
those changes are made.

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

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode530
app/app.js:530: * @param {Object} res ???
Why the duplicate 'res' and the ???

https://codereview.appspot.com/7007047/diff/1/app/views/login.js
File app/views/login.js (right):

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3
app/views/login.js:3: var no_login_prompts = false;
Why not camelCase?

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3
app/views/login.js:3: var no_login_prompts = false;
Why not camelCase?

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode27
app/views/login.js:27: * React to the results of sennding a login message to the
server.
typo: sending

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js
File test/test_login_view.js (right):

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js#newcode147
test/test_login_view.js:147: // If there are know credentials that are not known
to be bad (they are
s/know/no/

[benji, 2013-01-02 20:43:28]

Please take a look.

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

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode530
app/app.js:530: * @param {Object} res ???
On 2013/01/02 14:45:03, bac wrote:
> Why the duplicate 'res' and the ???

Done.

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode544
app/app.js:544: if (!view.waiting && !view.userIsAuthenticated) {
On 2012/12/21 21:06:18, hazmat wrote:
> this content belongs in the app, not the view. the app needs to re-login on a
> socket reconnect.

It appears to me that a socket reconnect is a "reboot" situation for the app. 
That is, since we can not retry the operation that was ongoing when the
reconnect happened, we need to restart the app from scratch in order to retain
consistency.  Am I missing a way to retry the last operation after a reconnect?

https://codereview.appspot.com/7007047/diff/1/app/app.js#newcode547
app/app.js:547: next();
On 2012/12/21 21:06:18, hazmat wrote:
> this is an either or proposition, not an and. we either display the login or
the
> app, not overlay the login over the app, which will be broken without a
working
> ws.

That was my intent, but without a backend that actually requires logins I
couldn't be sure it is working correctly so I settled on this compromise until
then.

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

https://codereview.appspot.com/7007047/diff/1/app/store/env.js#newcode77
app/store/env.js:77: this.set('serverReady', true);
On 2012/12/21 21:06:18, hazmat wrote:
> instead of introducing another value just move connected down here.

Done.

https://codereview.appspot.com/7007047/diff/1/app/views/login.js
File app/views/login.js (right):

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3
app/views/login.js:3: var no_login_prompts = false;
On 2013/01/02 14:45:03, bac wrote:
> Why not camelCase?

Done.

https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode27
app/views/login.js:27: * React to the results of sennding a login message to the
server.
On 2013/01/02 14:45:03, bac wrote:
> typo: sending

Done.

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js
File test/test_login_view.js (right):

https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js#newcode147
test/test_login_view.js:147: // If there are know credentials that are not known
to be bad (they are
On 2013/01/02 14:45:03, bac wrote:
> s/know/no/

Done.

[benji, 2013-01-02 20:44:45]

I have addressed all the comments (some with further questions) and pushed the
changes.

[hazmat, 2013-01-02 21:09:52]

On Wed, Jan 2, 2013 at 2:43 PM, <benji.york@gmail.com> wrote:

> Please take a look.
>
>
>
>
https://codereview.appspot.**com/7007047/diff/1/app/app.js<https://codereview...
> File app/app.js (right):
>
>
https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode530<https...
> app/app.js:530: * @param {Object} res ???
> On 2013/01/02 14:45:03, bac wrote:
>
>> Why the duplicate 'res' and the ???
>>
>
> Done.
>
>
https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode544<https...
> app/app.js:544: if (!view.waiting && !view.userIsAuthenticated) {
> On 2012/12/21 21:06:18, hazmat wrote:
>
>> this content belongs in the app, not the view. the app needs to
>>
> re-login on a
>
>> socket reconnect.
>>
>
> It appears to me that a socket reconnect is a "reboot" situation for the
> app.
>

i think we have a misunderstanding here, afaics whether or not its a
'reboot' situation has nothing to do with the app's responsibility to hold
on to the credential data or authenticating on the websocket.

 That is, since we can not retry the operation that was ongoing
> when the reconnect happened, we need to restart the app from scratch in
> order to retain consistency.  Am I missing a way to retry the last
> operation after a reconnect?
>
>
we can and do track in flight rpc operations, and wait till success ack
before modifying local state. yes, the local state for delta sync data is
effectively reset on reconnect already but that has nothing to do with
storing credentials for authenticating the user again. what's the
concern/usage with retrying operations?



>
https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode547<https...
> app/app.js:547: next();
> On 2012/12/21 21:06:18, hazmat wrote:
>
>> this is an either or proposition, not an and. we either display the
>>
> login or the
>
>> app, not overlay the login over the app, which will be broken without
>>
> a working
>
>> ws.
>>
>
> That was my intent, but without a backend that actually requires logins
> I couldn't be sure it is working correctly so I settled on this
> compromise until then.
>
>
the backend requiring the login is irrelevant to the logic that needs to be
done here. the front end app should never toss an error on an
uauthenticated connection. the app knows the connection status and whether
auth has been performed on it, it knows if credentials have been provided
it. If they haven't it displays the login form. If they have it logins the
user in. the user should never see a not authenticated error or the login
form multiple times in a single session (which is  incidentally another
reason why the login view shouldn't be persistent).




>
https://codereview.appspot.**com/7007047/diff/1/app/store/**env.js<https://co...
> File app/store/env.js (right):
>


> https://codereview.appspot.**com/7007047/diff/1/app/store/**
>
env.js#newcode77<https://codereview.appspot.com/7007047/diff/1/app/store/env.js#newcode77>
> app/store/env.js:77: this.set('serverReady', true);
> On 2012/12/21 21:06:18, hazmat wrote:
>
>> instead of introducing another value just move connected down here.
>>
>
> Done.
>
>
>
https://codereview.appspot.**com/7007047/diff/1/app/views/**login.js<https://...
> File app/views/login.js (right):
>
> https://codereview.appspot.**com/7007047/diff/1/app/views/**
>
login.js#newcode3<https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode3>
> app/views/login.js:3: var no_login_prompts = false;
> On 2013/01/02 14:45:03, bac wrote:
>
>> Why not camelCase?
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7007047/diff/1/app/views/**
>
login.js#newcode27<https://codereview.appspot.com/7007047/diff/1/app/views/login.js#newcode27>
> app/views/login.js:27: * React to the results of sennding a login
> message to the server.
> On 2013/01/02 14:45:03, bac wrote:
>
>> typo: sending
>>
>
> Done.
>
>
>
https://codereview.appspot.**com/7007047/diff/1/test/test_**login_view.js<htt...
> File test/test_login_view.js (right):
>
> https://codereview.appspot.**com/7007047/diff/1/test/test_**
>
login_view.js#newcode147<https://codereview.appspot.com/7007047/diff/1/test/test_login_view.js#newcode147>
> test/test_login_view.js:147: // If there are know credentials that are
> not known to be bad (they are
> On 2013/01/02 14:45:03, bac wrote:
>
>> s/know/no/
>>
>
> Done.
>
>
https://codereview.appspot.**com/7007047/<https://codereview.appspot.com/7007...
>

[benji, 2013-01-02 21:25:49]

Please take a look.

[benji, 2013-01-07 20:14:39]

Please take a look.

[gary.poster, 2013-01-07 20:59:59]

If you want me to look at this again I would be happy to, but otherwise land
with changes.

Thank you

Gary

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js
File app/store/env.js (right):

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode87
app/store/env.js:87: this.set('defaultSeries', msg.default_series);
The above two lines are supposed to be added to handleLoginEvent, and removed
from here entirely later. Unfortunately, the current rapi implementation does
not support this.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode113
app/store/env.js:113: }
The implementation does not do what was specced, but we should look forward to
it anyway, I think.

else {
  if (evt.data.provider_type) {
    this.set('providerType', msg.data.provider_type);
  }
  if (evt.data.default_series) {
    this.set('defaultSeries', msg.data.default_series);
  }
}

In the future we should be able to remove the conditional parts of that, and
rely on the two values existing.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode223
app/store/env.js:223: window.setTimeout(Y.bind(this.login, this), 500);
We talked about this and you liked the idea of connecting this to an event
instead (event fired by login attempt).

https://codereview.appspot.com/7007047/diff/15001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/7007047/diff/15001/test/index.html#newcode37
test/index.html:37: // *after* the test runner is invoked.  In which case the
test runner
"In this case,"

[gary.poster, 2013-01-07 21:02:04]

Please make a high priority card for the UI implementation, btw

[benji, 2013-01-07 21:39:26]

Please take a look.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js
File app/store/env.js (right):

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode113
app/store/env.js:113: }
On 2013/01/07 20:59:59, gary.poster wrote:
> The implementation does not do what was specced, but we should look forward to
> it anyway, I think.

I would prefer we make the change when the back end actually behaves
in this way.

https://codereview.appspot.com/7007047/diff/15001/app/store/env.js#newcode223
app/store/env.js:223: window.setTimeout(Y.bind(this.login, this), 500);
On 2013/01/07 20:59:59, gary.poster wrote:
> We talked about this and you liked the idea of connecting this to an event
> instead (event fired by login attempt).

I was able to refactor this to remove the setTimeout and without the need for an
event, just a function call.

https://codereview.appspot.com/7007047/diff/15001/test/index.html
File test/index.html (right):

https://codereview.appspot.com/7007047/diff/15001/test/index.html#newcode37
test/index.html:37: // *after* the test runner is invoked.  In which case the
test runner
On 2013/01/07 20:59:59, gary.poster wrote:
> "In this case,"

Done.

[benji, 2013-01-07 21:39:36]

Thanks for the review.  I have addressed each comment and created a "make better
UI" card.

[benji, 2013-01-08 13:47:36]

*** Submitted:

Add user login support.

Future work: we need to implement a real UI for this.  At the moment this
branch uses prompt() calls to generate modal dialogs.  Also, this will
probably need slight tweaking once the backend actually requires
authorization.

R=hazmat, bac, gary.poster
CC=
https://codereview.appspot.com/7007047