Remove DB.units list

Remove DB.units list

This branch is a monster. Apologies in advance.

This removes the db.units list and makes the GUI only use service.get('units')
for service specific unit lists. Outside of the simulator we only ever look 
at units in the context of a service and keeping two copies of things was
silly. This works better with the databinding code and is a pattern we intend 
to move forward with (wrt to relations I expect we'll get rid of the global
list as well).

There was however a fair bit of code that touched units which lead to a number
of minor changes. Then I realized I was fixing some code in deprecated views
so I went through and did a first pass at removing dead code related to the
old views.

Also there are changes to the test server to run on a random port which should
allow safely running make check/lbox w/o getting the test server from another
branch.

https://code.launchpad.net/~bcsaller/juju-gui/destroyService-error/+merge/187944

(do not edit description out of merge proposal)

[bcsaller, 2013-09-26 22:24:00]

Please take a look.

[benjamin.saller, 2013-09-26 22:39:46]

Now with review notes

https://codereview.appspot.com/13997043/diff/1/app/app.js
File app/app.js (left):

https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode550
app/app.js:550: 
We always had one of these around, but no more.

https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode737
app/app.js:737: 
Prune unused routes.

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js
File app/models/handlers.js (right):

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#newcode277
app/models/handlers.js:277: 
resolveModelByName does the right thing, we can't use db['units'] anymore

https://codereview.appspot.com/13997043/diff/1/app/models/models.js
File app/models/models.js (left):

https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode571
app/models/models.js:571: 
Started off just removing this... and it steamrolled.

https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode992
app/models/models.js:992: 
Our current scaling expectations don't require this comment.

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

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:471: 
get on modellist returns the array of values for all children by default.

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

https://codereview.appspot.com/13997043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:155: 
Couldn't walk the global units list here so I reordered things a little.

https://codereview.appspot.com/13997043/diff/1/test-server.js
File test-server.js (right):

https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48
test-server.js:48: 
Bind on 0 requests an unallocated random port... Oh UNIX :)

https://codereview.appspot.com/13997043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21
test/test_model.js:21: charm.get('revision').should.equal(0);
Bad diff, I put the whole thing in a describe so I could run them all at once,
but that hides the db.units changes internally, sorry.

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

https://codereview.appspot.com/13997043/diff/1/test/utils.js#newcode140
test/utils.js:140: 
This shouldn't have made it in, but is still technically a fix... I guess.

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

https://codereview.appspot.com/13997043/diff/1/undocumented#oldcode1
undocumented:1: 
from the pruning

[jeff.pihach, 2013-09-27 14:30:59]

LGTM with trivial
Thanks for putting in the time to do all of this cleanup!

I'll QA shortly and respond.

https://codereview.appspot.com/13997043/diff/1/app/app.js
File app/app.js (left):

https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode550
app/app.js:550: 
haha good find

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js
File app/models/handlers.js (left):

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#oldcode124
app/models/handlers.js:124: 
I didn't see this removed from models.js and I'm pretty sure it's not used
anywhere else.

https://codereview.appspot.com/13997043/diff/1/app/models/models.js
File app/models/models.js (left):

https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode571
app/models/models.js:571: 
lol

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

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:265: 
Should this var be renamed?

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:471: 
Interesting, maybe a inline comment to this effect

https://codereview.appspot.com/13997043/diff/1/test-server.js
File test-server.js (right):

https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48
test-server.js:48: 
Maybe an inline comment to this effect.

https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:122: 
The relations are never verified outside of the deepEquals

https://codereview.appspot.com/13997043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21
test/test_model.js:21: charm.get('revision').should.equal(0);
Then I guess I'll just have to take your word on it :)

[gary.poster, 2013-09-27 14:55:53]

LGTM with largely irrelevant comments.  This is great, thank you!  Exciting to
see old code start to be pruned (even though that would have been better as a
separate branch or two!).  Assuming lint is fixed and qa goes well with Jeff,
land it!

https://codereview.appspot.com/13997043/diff/1/app/models/models.js
File app/models/models.js (left):

https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode992
app/models/models.js:992: 
On 2013/09/26 22:39:47, benjamin.saller wrote:
> Our current scaling expectations don't require this comment.

+1.

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

https://codereview.appspot.com/13997043/diff/1/app/models/models.js#newcode1028
app/models/models.js:1028: 
else { return; } or whatever locally correct "missing" behavior is?  It probably
does this already, but it might be nice to be explicit.

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

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:671: 
what "above"?  Confused.

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:1228: 
This little six-line snippet is roughly or duped in earlier file, IIRC.  Maybe
indicates good opportunity to share code, maybe not.  Just noting if you hadn't
considered.

https://codereview.appspot.com/13997043/diff/1/app/views/charm-panel.js
File app/views/charm-panel.js (left):

https://codereview.appspot.com/13997043/diff/1/app/views/charm-panel.js#oldcode2
app/views/charm-panel.js:2: 
I love the smell of napalmed code in the morning.

https://codereview.appspot.com/13997043/diff/1/test-server.js
File test-server.js (right):

https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48
test-server.js:48: 
On 2013/09/27 14:30:59, jeff.pihach wrote:
> Maybe an inline comment to this effect.

heh, +1

https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode49
test-server.js:49: 
cool, thanks for doing this.

https://codereview.appspot.com/13997043/diff/1/test/index.html
File test/index.html (left):

https://codereview.appspot.com/13997043/diff/1/test/index.html#oldcode118
test/index.html:118: 
I like this fix. ;-)

https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:122: 
On 2013/09/27 14:30:59, jeff.pihach wrote:
> The relations are never verified outside of the deepEquals

Maybe you mean "initialize units and relations," Ben?

https://codereview.appspot.com/13997043/diff/1/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21
test/test_model.js:21: charm.get('revision').should.equal(0);
On 2013/09/27 14:30:59, jeff.pihach wrote:
> Then I guess I'll just have to take your word on it :)

Heh, yeah. :-)

https://codereview.appspot.com/13997043/diff/1/test/test_sandbox_go.js
File test/test_sandbox_go.js (right):

https://codereview.appspot.com/13997043/diff/1/test/test_sandbox_go.js#newcod...
test/test_sandbox_go.js:384: 
huh, that looks like it shouldn't have worked before

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

https://codereview.appspot.com/13997043/diff/1/test/utils.js#newcode140
test/utils.js:140: 
On 2013/09/26 22:39:47, benjamin.saller wrote:
> This shouldn't have made it in, but is still technically a fix... I guess. 

heh

[benjamin.saller, 2013-09-27 17:12:42]

Thanks for the reviews, cleaning up a little after the trunk merge, but should
land soon.

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js
File app/models/handlers.js (left):

https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#oldcode124
app/models/handlers.js:124: 
On 2013/09/27 14:30:59, jeff.pihach wrote:
> I didn't see this removed from models.js and I'm pretty sure it's not used
> anywhere else.

It is still used here, but with a little refactoring wouldn't be.

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

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:265: 
On 2013/09/27 14:30:59, jeff.pihach wrote:
> Should this var be renamed?

unitsInError now

https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n...
app/store/env/fakebackend.js:471: 
On 2013/09/27 14:30:59, jeff.pihach wrote:
> Interesting, maybe a inline comment to this effect

Done.

[bcsaller, 2013-09-27 17:20:53]

*** Submitted:

Remove DB.units list

This branch is a monster. Apologies in advance.

This removes the db.units list and makes the GUI only use service.get('units')
for service specific unit lists. Outside of the simulator we only ever look 
at units in the context of a service and keeping two copies of things was
silly. This works better with the databinding code and is a pattern we intend 
to move forward with (wrt to relations I expect we'll get rid of the global
list as well).

There was however a fair bit of code that touched units which lead to a number
of minor changes. Then I realized I was fixing some code in deprecated views
so I went through and did a first pass at removing dead code related to the
old views.

Also there are changes to the test server to run on a random port which should
allow safely running make check/lbox w/o getting the test server from another
branch.

R=benjamin.saller, jeff.pihach, gary.poster
CC=
https://codereview.appspot.com/13997043