Wire up the first bits of the megawatcher.

Wire up the first bits of the megawatcher.

There is still much left to do, but what is here works and is reasonably
well tested.

https://code.launchpad.net/~benji/juju-gui/megawatcher-propose/+merge/155471

(do not edit description out of merge proposal)

[benji, 2013-03-26 12:15:13]

Please take a look.

[jeff.pihach, 2013-03-26 14:09:21]

LGTM thanks! 'land with changes' I would like to see the two tests mentioned
below switched to use done() and then just take a look at the one which isn't
async but possibly should be?

Thanks again!

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

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode77
app/store/env/go.js:77: instance_id: entityInfo.InstanceId // XXX and more stuff
Can this be camelCased?

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode58
test/test_env_go.js:58: assert.isTrue(loginFired);
Last night I did some reading on the best approach to this and I think that
using the done() method is the best way. This way could potentially have a race
condition causing the test to fail.

see: http://visionmedia.github.com/mocha/#asynchronous-code

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode72
test/test_env_go.js:72: assert.isTrue(loginFired);
see done() above

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode85
test/test_env_go.js:85: conn.msg({RequestId: 1, Response: {}});
conn.msg() is async above but not here?

https://codereview.appspot.com/7783050/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/7783050/diff/1/test/utils.js#newcode30
test/utils.js:30: if (!back) {
just FYI - another commonly used convention is
back = back || 1;

[benji, 2013-03-26 14:25:55]

Thanks for the review.  I have addressed each point, including one request for
more information.

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

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode77
app/store/env/go.js:77: instance_id: entityInfo.InstanceId // XXX and more stuff
On 2013/03/26 14:09:21, jeff.pihach wrote:
> Can this be camelCased?

This is a reflection of the non-dromedarian nature of the (Python) Juju
server-side.  The names of the fields coming from the server are spelled this
way and (at the moment) we are converting from the Go format to the Python
format.  All this will change soonish to a more regularized approach.

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode58
test/test_env_go.js:58: assert.isTrue(loginFired);
On 2013/03/26 14:09:21, jeff.pihach wrote:
> Last night I did some reading on the best approach to this and I think that
> using the done() method is the best way. This way could potentially have a
race
> condition causing the test to fail.
> 
> see: http://visionmedia.github.com/mocha/#asynchronous-code

I do not see how the original code avoided any races.  done was called
unconditionally in the main line execution.  It was doing nothing to gate the
end of the test.  I could see putting a call to done in the login handler on
line 51, but my understanding of events is that they are executed immediately
upon event firing, so there is no real asynchronicity here.

Perhaps we should put a done call in the handler just in case the handlers ever
start being called asynchronously, but that feels a bit superstitious.

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode85
test/test_env_go.js:85: conn.msg({RequestId: 1, Response: {}});
On 2013/03/26 14:09:21, jeff.pihach wrote:
> conn.msg() is async above but not here?

Heck if I know. ;)

https://codereview.appspot.com/7783050/diff/1/test/utils.js
File test/utils.js (right):

https://codereview.appspot.com/7783050/diff/1/test/utils.js#newcode30
test/utils.js:30: if (!back) {
On 2013/03/26 14:09:21, jeff.pihach wrote:
> just FYI - another commonly used convention is
> back = back || 1;

I prefer spelling it out.

[frankban, 2013-03-26 15:30:35]

This branch looks good, a solid base for the megawatcher stuff.
LGTM with some questions and comments below.
Thank you.

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

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode98
app/store/env/go.js:98: XXX FAKE FAKE FAKE, does not do anything.
Can't we just remove this method?

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode148
app/store/env/go.js:148: if (!op.Params) {
Nice, thanks.

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode196
app/store/env/go.js:196: deltas.push([kind, operation, converter(entityInfo)]);
Very nice and simple.

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode219
app/store/env/go.js:219: this.fire('_rpc_response', data);
Just curiosity: why an event is preferred here over directly calling
_handleRpcResponse?

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode82
test/test_env_go.js:82: it('calls environmentInfo and watchAll ofter login',
function() {
typo: after

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode678
test/test_env_go.js:678: assert.isTrue('Id' in msg.Params);
Here we could also test that Params.Id is effectively the answer...

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode715
test/test_env_go.js:715: Y = YUI(GlobalConfig).use(['juju-env',
'juju-tests-utils'], function(Y) {
It seems 'juju-tests-utils' is not really required by this test case. Ditto for
all the converter tests below.

https://codereview.appspot.com/7783050/diff/1/undocumented
File undocumented (left):

https://codereview.appspot.com/7783050/diff/1/undocumented#oldcode15
undocumented:15: app/store/env/python.js:371 "get_endpoints"
Thank you.

[benji, 2013-03-26 16:10:58]

Good stuff.  Everything has been addressed.

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

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode98
app/store/env/go.js:98: XXX FAKE FAKE FAKE, does not do anything.
On 2013/03/26 15:30:35, frankban wrote:
> Can't we just remove this method?

It is expected to exist by other code.  If it doesn't exist the app will break. 
I could have made the call conditional but since we do in fact want this to be
fleshed out in the future, a placeholder seemed appropriate.

https://codereview.appspot.com/7783050/diff/1/app/store/env/go.js#newcode219
app/store/env/go.js:219: this.fire('_rpc_response', data);
On 2013/03/26 15:30:35, frankban wrote:
> Just curiosity: why an event is preferred here over directly calling
> _handleRpcResponse?

The tests were simplified quite a bit by decoupling the two.

The original code had the body of _handleRpcResponse inline here, as you would
expect.

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode678
test/test_env_go.js:678: assert.isTrue('Id' in msg.Params);
On 2013/03/26 15:30:35, frankban wrote:
> Here we could also test that Params.Id is effectively the answer...

Good call.  Done.

https://codereview.appspot.com/7783050/diff/1/test/test_env_go.js#newcode715
test/test_env_go.js:715: Y = YUI(GlobalConfig).use(['juju-env',
'juju-tests-utils'], function(Y) {
On 2013/03/26 15:30:35, frankban wrote:
> It seems 'juju-tests-utils' is not really required by this test case. Ditto
for
> all the converter tests below.

Nice catch.  Fixed.

[benji, 2013-03-26 17:11:09]

*** Submitted:

Wire up the first bits of the megawatcher.

There is still much left to do, but what is here works and is reasonably
well tested.

R=jeff.pihach, frankban
CC=
https://codereview.appspot.com/7783050