Remove the serviceInspector feature flag.

Remove the serviceInspector feature flag.

- Note: that there's still some instance of serviceInspector, but these are not
feature flag instances, just variable names.
- Note: This does not remove the old service views from the system. Some of
that is needed for notifications and will be a follow up once we've dealt with
them.
- Some tests broke during these changes and required some additional cleanup.
These are noted in the reviewer inline comments.

https://code.launchpad.net/~rharding/juju-gui/remove-inspector-flag/+merge/186315

(do not edit description out of merge proposal)

[rharding, 2013-09-19 14:44:31]

Please take a look.

[rharding, 2013-09-19 15:06:28]

Reviewer comments added below.

https://codereview.appspot.com/13775044/diff/3001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/13775044/diff/3001/app/app.js#newcode1489
app/app.js:1489: 'juju-view-environment',
this was missing from the deps and caused an error in tests once some extra
cleanup was done to them.

https://codereview.appspot.com/13775044/diff/3001/app/templates/overview.hand...
File app/templates/overview.handlebars (left):

https://codereview.appspot.com/13775044/diff/3001/app/templates/overview.hand...
app/templates/overview.handlebars:17: <li class="view-service">View</li>
these two links are no longer part of the menu, but done via the inspector which
pops up whenever this menu shows up.

https://codereview.appspot.com/13775044/diff/3001/app/views/inspector.js
File app/views/inspector.js (left):

https://codereview.appspot.com/13775044/diff/3001/app/views/inspector.js#oldc...
app/views/inspector.js:315: link: getModelURL(service),
this link isn't valid any longer so just removed. We don't want the notification
to link to the service views.

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service.js
File app/views/topology/service.js (left):

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:373: click: 'destroyServiceClick'
these menus are gone

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:827: context.destroyServiceConfirm(box);
and the callbacks for them are no longer required.

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:1417: }
this was about changing the menu for the inspector service block. So it's
removed entirely since that destroy is gone.

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service.js
File app/views/topology/service.js (right):

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:1399: createServiceInspector(service);
this is what caused tests to have issues. The service inspector requires a
div#content to exist and many of the tests didn't create that.

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js#newc...
test/test_endpoints.js:346: destroyMe.push(env);
the env wasn't getting destroyed. This led to the additional dep in the app.js

https://codereview.appspot.com/13775044/diff/3001/test/test_environment_view.js
File test/test_environment_view.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_environment_view....
test/test_environment_view.js:130: container =
testUtils.makeContainer('content');
and the #content needed for the inspector to work since it loads on all
showServiceMenu calls now.

https://codereview.appspot.com/13775044/diff/3001/test/test_service_module.js
File test/test_service_module.js (left):

https://codereview.appspot.com/13775044/diff/3001/test/test_service_module.js...
test/test_service_module.js:245: it('hides the service menu when the View entry
is clicked', function() {
menu tests no longer required.

https://codereview.appspot.com/13775044/diff/3001/test/test_service_module.js
File test/test_service_module.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_service_module.js...
test/test_service_module.js:123: beforeEach(function() {
extra bootstrap needed to get the inspector a real charm to deal with since it's
loaded with the service menu.

https://codereview.appspot.com/13775044/diff/3001/test/test_service_module.js...
test/test_service_module.js:157: fakeStore.destroy();
and a lot of missing cleanup.

https://codereview.appspot.com/13775044/diff/3001/test/test_service_view.js
File test/test_service_view.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_service_view.js#n...
test/test_service_view.js:166: // we clean out the last of the old service view
code.
tried to explain here. I feel better skipping than removing as they help point
the way to code to delete once we can get rid of the service views.

[jeff.pihach, 2013-09-19 15:36:06]

LGTM QA OK
Thanks for this!

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js#newc...
test/test_endpoints.js:346: destroyMe.push(env);
Nice find

[gary.poster, 2013-09-19 15:51:28]

Thank you very much for doing this.  It is exciting to see this process started.

For better and worse, I think there are a couple of additional things that need
to be removed, per my comments, which will have additional fallout.  I'd like to
re-review after those changes, and will postpone the second qa you requested
till after that happens.

https://codereview.appspot.com/13775044/diff/3001/app/templates/overview.hand...
File app/templates/overview.handlebars (left):

https://codereview.appspot.com/13775044/diff/3001/app/templates/overview.hand...
app/templates/overview.handlebars:20: <li class="landscape-reboot"><a
href="#">Machine Restart Needed</a></li>
Seems like we could remove these now too, but we can do that later.

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service.js
File app/views/topology/service.js (left):

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:584: if (box.pending &&
!window.flags.serviceInspector) {
Wait a sec, this says to only do the block if serviceInspector is turned *off*. 
Shouldn't we delete this whole block?

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:1379: if (landscape && !flags.serviceInspector) {
No, look, this says *not* serviceInspector.  We can delete this block and the
related landscape code that I mentioned before and also related handlers.

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/13775044/diff/3001/test/test_endpoints.js#newc...
test/test_endpoints.js:346: destroyMe.push(env);
+1

[rharding, 2013-09-19 17:10:57]

Please take a look.

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service.js
File app/views/topology/service.js (left):

https://codereview.appspot.com/13775044/diff/3001/app/views/topology/service....
app/views/topology/service.js:584: if (box.pending &&
!window.flags.serviceInspector) {
On 2013/09/19 15:51:29, gary.poster wrote:
> Wait a sec, this says to only do the block if serviceInspector is turned
*off*. 
> Shouldn't we delete this whole block?

Yes, misread this one.

[gary.poster, 2013-09-19 17:47:15]

Code LGTM, thank you.  Will QA now.

[gary.poster, 2013-09-19 17:55:21]

On 2013/09/19 17:47:15, gary.poster wrote:
> Code LGTM, thank you.  Will QA now.

QA good.  Thanks again!

[rharding, 2013-09-19 18:12:18]

*** Submitted:

Remove the serviceInspector feature flag.

- Note: that there's still some instance of serviceInspector, but these are not
feature flag instances, just variable names.
- Note: This does not remove the old service views from the system. Some of
that is needed for notifications and will be a follow up once we've dealt with
them.
- Some tests broke during these changes and required some additional cleanup.
These are noted in the reviewer inline comments.

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