Add units, unit controls to inspector

Add units, unit controls to inspector

This adds the unit-count control and an initial pass at the unit list to the service inspector.

https://code.launchpad.net/~makyo/juju-gui/unit-controls/+merge/172407

(do not edit description out of merge proposal)

[matthew.scott, 2013-07-01 20:13:16]

Please take a look.

[gary.poster, 2013-07-02 13:04:29]

Hi Matt.  This seems to be breaking the ghost inspector.  When I merge your
branch and try to add a service (using the Add button) then I don't see the
ghost inspector.  The console log reports "Uncaught RangeError: Maximum call
stack size exceeded" from calling a Y.mix.  I don't see why on a quick scan,
though as I'm sure you know, this usually indicates an object reference cycle. 
I did not try to look harder at the branch to actually inspect the new
functionality, though in passing I did wonder if there would be an easier way to
handle the inspector flag in the new code--easier to discard the old code.  It
won't be too bad even the way you have it, so <shrug>.

Thanks

Gary

[matthew.scott, 2013-07-03 21:07:55]

Please take a look.

[bac, 2013-07-03 22:02:32]

LGTM.  A test for the inspector parts would be nice.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:1383: [c.ghostInspector, c.serviceInspector,
manageUnitsMixin]
Could you add a comment about why manageUnitsMixin is here since it isn't an
inspector.

[jeff.pihach, 2013-07-04 15:23:09]

This is looking good thanks, there are a few changes I'd like and I'm really
interested in service.js:1411, let me know if removing those breaks the script. 

Thanks.

https://codereview.appspot.com/10763045/diff/5001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/models/models.js#newcod...
app/models/models.js:313: if (data instanceof String) {
You should use `typeof` here as instanceof has some undesired issues wrt
literals.

typeof data === 'string'

https://codereview.appspot.com/10763045/diff/5001/app/templates/serviceOvervi...
File app/templates/serviceOverview.handlebars (right):

https://codereview.appspot.com/10763045/diff/5001/app/templates/serviceOvervi...
app/templates/serviceOverview.handlebars:19: <input type="text"
id="num-service-units" data-bind="unit_count" />
This id will cause issues if we ever switch to multiple inspectors, can you
change it to a class please.

https://codereview.appspot.com/10763045/diff/5001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/views/environment.js#ne...
app/views/environment.js:185: '#num-service-units': {
see comment in template about this id

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcode45
app/views/service.js:45: events: {
can you make a comment here mentioning that this will need to be removed once
the feature flag is removed.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcode53
app/views/service.js:53: * XXX Makyo - all instances of testing this.inspector
will go away once
instead of using this.inspector check - should we not use the flag check?
Because it will need to be changed once the flag goes away it's easy to grep
for.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:115: var service = this.model ? this.model :
this.get('model');
var service = this.model || this.get('model')

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:1411: this.inspector.resetUnits =
manageUnitsMixin.resetUnits;
These should not be required as the events should call the prototype objects in
the proper context. If this isn't the case then lets discuss alternate
approaches.

[matthew.scott, 2013-07-05 16:24:56]

Responding to comments only with this next propose; will be working on adding
tests meanwhile.

https://codereview.appspot.com/10763045/diff/5001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/models/models.js#newcod...
app/models/models.js:313: if (data instanceof String) {
On 2013/07/04 15:23:09, jeff.pihach wrote:
> You should use `typeof` here as instanceof has some undesired issues wrt
> literals.
> 
> typeof data === 'string'

Done.

https://codereview.appspot.com/10763045/diff/5001/app/templates/serviceOvervi...
File app/templates/serviceOverview.handlebars (right):

https://codereview.appspot.com/10763045/diff/5001/app/templates/serviceOvervi...
app/templates/serviceOverview.handlebars:19: <input type="text"
id="num-service-units" data-bind="unit_count" />
On 2013/07/04 15:23:09, jeff.pihach wrote:
> This id will cause issues if we ever switch to multiple inspectors, can you
> change it to a class please.

Done.

https://codereview.appspot.com/10763045/diff/5001/app/views/environment.js
File app/views/environment.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/views/environment.js#ne...
app/views/environment.js:185: '#num-service-units': {
On 2013/07/04 15:23:09, jeff.pihach wrote:
> see comment in template about this id

Done.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js
File app/views/service.js (right):

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcode53
app/views/service.js:53: * XXX Makyo - all instances of testing this.inspector
will go away once
On 2013/07/04 15:23:09, jeff.pihach wrote:
> instead of using this.inspector check - should we not use the flag check?
> Because it will need to be changed once the flag goes away it's easy to grep
> for.

D'oh, yeah, of course.  Done.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:115: var service = this.model ? this.model :
this.get('model');
On 2013/07/04 15:23:09, jeff.pihach wrote:
> var service = this.model || this.get('model')

Not a huge fan, but done.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:1383: [c.ghostInspector, c.serviceInspector,
manageUnitsMixin]
On 2013/07/03 22:02:32, bac wrote:
> Could you add a comment about why manageUnitsMixin is here since it isn't an
> inspector.

Done.

https://codereview.appspot.com/10763045/diff/5001/app/views/service.js#newcod...
app/views/service.js:1411: this.inspector.resetUnits =
manageUnitsMixin.resetUnits;
On 2013/07/04 15:23:09, jeff.pihach wrote:
> These should not be required as the events should call the prototype objects
in
> the proper context. If this isn't the case then lets discuss alternate
> approaches.

Yep, leftover from early explorations.  Good catch.  Done.

[matthew.scott, 2013-07-05 16:29:16]

Please take a look.

[jeff.pihach, 2013-07-05 16:32:43]

LGTM Thanks a lot for those updates!

[matthew.scott, 2013-07-08 15:07:28]

Please take a look.

[jeff.pihach, 2013-07-08 15:17:03]

LGTM Thanks for the tests - one small q which we can discuss

https://codereview.appspot.com/10763045/diff/27001/test/test_inspector_overvi...
File test/test_inspector_overview.js (right):

https://codereview.appspot.com/10763045/diff/27001/test/test_inspector_overvi...
test/test_inspector_overview.js:24: var charmConfig =
I'm wondering if we shouldn't use the utils method to pull in real charm data
instead of mocking it.

[matthew.scott, 2013-07-08 15:52:45]

*** Submitted:

Add units, unit controls to inspector

This adds the unit-count control and an initial pass at the unit list to the
service inspector.

R=gary.poster, bac, jeff.pihach
CC=
https://codereview.appspot.com/10763045