Add expose/unexpose to the inspector.

Add expose/unexpose to the inspector.



https://code.launchpad.net/~bac/juju-gui/inspector-expose/+merge/173268

(do not edit description out of merge proposal)

[bac, 2013-07-05 19:22:20]

Please take a look.

[jeff.pihach, 2013-07-05 19:39:13]

Looking good - I'd like to see some cleanup on the tests but thanks for writing
them :)

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:9: <span></span>
Should these not have sane defaults?

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:14: <hr style="border: dashed;
border-width: 1px 0 0 0">
Is it intended that this will be removed?

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

https://codereview.appspot.com/10964044/diff/1/app/views/environment.js#newco...
app/views/environment.js:127: // previous check would've caused a return.
I think you are right here - this might have been an artifact of removing the
multiple inspector capability - some more testing is necessary and then it could
be removed.....not sure if you want to do in this branch or not.

https://codereview.appspot.com/10964044/diff/1/app/views/environment.js#newco...
app/views/environment.js:174: env: this.topo.get('env'),
heh looks like all of our active branches add env to view-container.

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

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1095
app/views/service.js:1095: service.set('exposed', exposed);
How long does it take to expose a service? Maybe this should be done in the env
callbacks.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1268
app/views/service.js:1268: /**
These really shouldn't need to be documented as they are callbacks for another
class. If you would like to remove the comment you can quote update.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1405
app/views/service.js:1405: this.inspector.unexposeService =
exposeButtonMixin.unexposeService;
These should not be required as they should be properly bound in the above
forEach loop - plz remove.

If it breaks then let me know and we can work out a proper solution

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:57: container =
Y['juju-tests'].utils.makeContainer('container');
just utils.makeContainer()

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:69: afterEach(function() {
I am pretty sure we also want to destroy the environment and null out the socket
stub

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:103: assert.isFalse(service.get('exposed'));
This isn't really testing that a service inspector was created.

[benji, 2013-07-05 19:40:02]

Looks good.  I had a few small suggestions for your consideration.

LGTM-ly y'rs,
Benji

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:9: <span></span>
These two lines could use some comments explaining how they work.

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:14: <hr style="border: dashed;
border-width: 1px 0 0 0">
I don't feel this way, but the project as a whole has stayed away form inline
style.

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

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode273
app/views/service.js:273: this.get('model'),
How does this variation strike you?

(svcInspector ? this.inspector : this).get('model')

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode274
app/views/service.js:274: env = svcInspector ? this.inspector.get('env') :
this.get('env');
How does this strike you?

var svcInspector = window.flags && window.flags.serviceInspector,
    dataSource = (svcInspector ? this.inspector : this),
    service = dataSource.get('model'),
    env = dataSource.get('env');

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1082
app/views/service.js:1082: @param {Y.EventFacade} e An event object.
An "@return {undefined} Nothing." would be a nice addition.

https://codereview.appspot.com/10964044/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/10964044/diff/1/test/index.html#newcode81
test/index.html:81: <script src="test_inspector_settings.js"></script>
Some people (other than me) would prefer this list alphabetized.

[bac, 2013-07-05 20:57:07]

Please take a look.

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
File app/templates/service-configuration.handlebars (right):

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:9: <span></span>
Probably.

https://codereview.appspot.com/10964044/diff/1/app/templates/service-configur...
app/templates/service-configuration.handlebars:14: <hr style="border: dashed;
border-width: 1px 0 0 0">
Yes, it should be removed.  I was experimenting to restore the dotted line you
removed!  :)

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

https://codereview.appspot.com/10964044/diff/1/app/views/environment.js#newco...
app/views/environment.js:127: // previous check would've caused a return.
I'll remove it and run the tests.  I feel confident b/c just looking at the code
it is obvious it can never be triggered.

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

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode273
app/views/service.js:273: this.get('model'),
It makes me twitch a little.  Mind if I don't do it?

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode274
app/views/service.js:274: env = svcInspector ? this.inspector.get('env') :
this.get('env');
Better!

This logic is short-lived so optimizing doesn't make the most sense.  But I like
this change.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1082
app/views/service.js:1082: @param {Y.EventFacade} e An event object.
On 2013/07/05 19:40:02, benji wrote:
> An "@return {undefined} Nothing." would be a nice addition.

Done.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1095
app/views/service.js:1095: service.set('exposed', exposed);
It is async so I don't think it is an issue being here.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1268
app/views/service.js:1268: /**
Righto.

https://codereview.appspot.com/10964044/diff/1/app/views/service.js#newcode1405
app/views/service.js:1405: this.inspector.unexposeService =
exposeButtonMixin.unexposeService;
They do break when removed.

https://codereview.appspot.com/10964044/diff/1/test/index.html
File test/index.html (right):

https://codereview.appspot.com/10964044/diff/1/test/index.html#newcode81
test/index.html:81: <script src="test_inspector_settings.js"></script>
On 2013/07/05 19:40:02, benji wrote:
> Some people (other than me) would prefer this list alphabetized.

Done.

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.js
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:57: container =
Y['juju-tests'].utils.makeContainer('container');
On 2013/07/05 19:39:13, jeff.pihach wrote:
> just utils.makeContainer()

Done.

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:69: afterEach(function() {
On 2013/07/05 19:39:13, jeff.pihach wrote:
> I am pretty sure we also want to destroy the environment and null out the
socket
> stub

Done.

https://codereview.appspot.com/10964044/diff/1/test/test_inspector_settings.j...
test/test_inspector_settings.js:103: assert.isFalse(service.get('exposed'));
No, it was useful when developing the test but no longer serves a purpose. 
Removed.

[jeff.pihach, 2013-07-05 21:03:46]

LGTM great work, thanks for the updates.

very small request in the tests before landing please :)

https://codereview.appspot.com/10964044/diff/11001/test/test_inspector_settin...
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/10964044/diff/11001/test/test_inspector_settin...
test/test_inspector_settings.js:77: env.destroy();
because destroy is async I would probably add a done() inside of a callback

env.after('destroy', function() { done(); });
env.destroy();

https://codereview.appspot.com/10964044/diff/11001/test/test_inspector_settin...
test/test_inspector_settings.js:82: delete window.flags;
nice catch!

[bac, 2013-07-05 21:21:49]

*** Submitted:

Add expose/unexpose to the inspector.

R=jeff.pihach, benji
CC=
https://codereview.appspot.com/10964044

https://codereview.appspot.com/10964044/diff/11001/test/test_inspector_settin...
File test/test_inspector_settings.js (right):

https://codereview.appspot.com/10964044/diff/11001/test/test_inspector_settin...
test/test_inspector_settings.js:77: env.destroy();
On 2013/07/05 21:03:46, jeff.pihach wrote:
> because destroy is async I would probably add a done() inside of a callback
> 
> env.after('destroy', function() { done(); });
> env.destroy();

Done.