← Back to team overview

yellow team mailing list archive

Re: Add user login support. (issue 7007047)

 

On Wed, Jan 2, 2013 at 2:43 PM, <benji.york@xxxxxxxxx> 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/>
>

-- 
https://code.launchpad.net/~benji/juju-gui/login/+merge/141133
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~benji/juju-gui/login into lp:juju-gui.


References