Code review - Issue 7007047: Add user login support.https://codereview.appspot.com/2013-01-08T13:47:36+00:00rietveld
Message from unknown
2012-12-21T20:53:37+00:00benjiurn:md5:1503504cff1c6c1e742af6b116c1a5d1
Message from benji.york@gmail.com
2012-12-21T20:53:42+00:00benjiurn:md5:32bb312439986f93f97d367806941ad4
Please take a look.
Message from kapilt@gmail.com
2012-12-21T21:06:18+00:00hazmaturn:md5:7f2e5f9150c36e4fddbd595e14be6216
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.
Message from bac@canonical.com
2013-01-02T14:45:03+00:00bacurn:md5:32110a8d1c3047b47fd6b0a766d7e28d
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/
Message from unknown
2013-01-02T20:43:25+00:00benjiurn:md5:4eb19c881bccc6c06d7228395f1828d8
Message from benji.york@gmail.com
2013-01-02T20:43:28+00:00benjiurn:md5:78893cdaad18e77ecbfc45b40d4ffde8
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.
Message from benji.york@gmail.com
2013-01-02T20:44:45+00:00benjiurn:md5:6e4188caf39c46a8358743196031e1f1
I have addressed all the comments (some with further questions) and pushed the changes.
Message from kapilt@gmail.com
2013-01-02T21:09:52+00:00hazmaturn:md5:56b455f0baec7e128920f80092587af4
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.appspot.com/7007047/diff/1/app/app.js>
> File app/app.js (right):
>
> https://codereview.appspot.**com/7007047/diff/1/app/app.js#**newcode530<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<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.
>
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://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.
>
>
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://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<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://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<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<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<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/7007047/>
>
Message from unknown
2013-01-02T21:25:41+00:00benjiurn:md5:7a1c7d782cfea3a0657fa6feb9367285
Message from benji.york@gmail.com
2013-01-02T21:25:49+00:00benjiurn:md5:f0c1c3ae9f1fbeb13bfef45c4ecf0d11
Please take a look.
Message from unknown
2013-01-07T20:14:35+00:00benjiurn:md5:e26a74842f1f3ea4be15bd77a36e3bc2
Message from benji.york@gmail.com
2013-01-07T20:14:39+00:00benjiurn:md5:25b32a7af305cf9bd4c86663b8df6b0b
Please take a look.
Message from gary.poster@canonical.com
2013-01-07T20:59:59+00:00gary.posterurn:md5:daf20458103b6ce4d58ce3db31cf6e71
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,"
Message from gary.poster@canonical.com
2013-01-07T21:02:04+00:00gary.posterurn:md5:074a4fa382f82719ce8baca8a4c288eb
Please make a high priority card for the UI implementation, btw
Message from unknown
2013-01-07T21:39:23+00:00benjiurn:md5:83f83d08b5afc868c23bfe3263152d87
Message from benji.york@gmail.com
2013-01-07T21:39:26+00:00benjiurn:md5:a42934c9e52cf58f4b7d61c695ec4eef
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.
Message from benji.york@gmail.com
2013-01-07T21:39:36+00:00benjiurn:md5:3e229de0032e635e9cb10d9a26079e35
Thanks for the review. I have addressed each comment and created a "make better UI" card.
Message from unknown
2013-01-08T13:45:51+00:00benjiurn:md5:0091736894a45473cdb714bcec76448c
Message from benji.york@gmail.com
2013-01-08T13:47:36+00:00benjiurn:md5:068a37a279d5ffabb9b0c22026627194
*** 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