Good stuff, Benji, thank you.

Land with changes.  What I'm bringing is usually trivial, and should be
fairly easy to address, I hope, even when I think it is important.

Gary


https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-common-controls.partial
File app/templates/service-footer-common-controls.partial (right):

https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-common-controls.partial#newcode1
app/templates/service-footer-common-controls.partial:1: {{#unless
serviceIsJujuGUI}}
My inclination is to let the GUI increase the unit count, but not be
exposed--IOW, move serviceIsJujuGUI into only be around
"control-expose," below.  Given the basic approach that I think we have
now of "innocent until proven guilty" for disabling GUI service
functionality, do you have a concrete reason to exclude unit count--a
reason to say that this is a problem?  Note that I am pretty sure that
unit count may not be reduced beneath one, thanks to a pre-existing
constraint in the GUI.

https://codereview.appspot.com/7129049/diff/1/app/templates/service.handlebars
File app/templates/service.handlebars (right):

https://codereview.appspot.com/7129049/diff/1/app/templates/service.handlebars#newcode25
app/templates/service.handlebars:25: {{#unless serviceIsJujuGUI}}
If you agree that we should reinstate unit count, then IIUC you would
also remove this conditional.

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

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode170
app/views/service.js:170: // it would break the application they are
currently using.
Suggest you add another comment similar to the following:

// Note that the destroy button is not shown for the Juju GUI, so this
is a fail-safe.

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode381
app/views/service.js:381: * Aside from a nice seperation of concerns,
this method also
typo: separation

https://codereview.appspot.com/7129049/diff/1/app/views/service.js#newcode382
app/views/service.js:382: * facilitates testing.
Interesting approach.  I like it so far.

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

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode79
app/views/topology/service.js:79: // it would break the application they
are currently using.
I'm not sure yet from the code, but I'm hoping that this is again a
fail-safe because the "destroy" option is not given to the user, in
which case a comment to that effect would again be my preference.

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode740
app/views/topology/service.js:740: var menu =
view.get('container').one('#service-menu'),
nicer variable name, thank you :-)

https://codereview.appspot.com/7129049/diff/1/app/views/topology/service.js#newcode755
app/views/topology/service.js:755:
menu.one('.destroy-service').addClass('disabled');
Per my previous comment, yep, I see you disable it visually as well.
Cool.

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

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js#newcode950
app/views/utils.js:950: return
(/^(?:[^:]+:|[^\/]*\/)juju-gui-\d+$/).test(charmUrl);
This is the old regex.  It has problems with unofficial charms.  I
suggest using the regex from
https://codereview.appspot.com/7069068/diff/1/app/views/utils.js (and
the associated tests in
https://codereview.appspot.com/7069068/diff/1/test/test_utils.js?context=10&column_width=80
: "should recognize unofficial charm store URLs" and "should ignore
owners of unofficial charm store URLs".  Even if you want to reimplement
yourself for some reason, this is an important case to cover that
affects us today.

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js#newcode963
app/views/utils.js:963: var charmUrl = candidate.charm ||
candidate.get('charm');
In the previous branch, Nicola and I both found this to be too fragile.
See the version in
https://codereview.appspot.com/7069068/diff/1/app/views/utils.js .  You
don't need to use that approach, and *maybe* it is not necessary now for
some reason, but it was necessary in the previous branch for tests to
pass.

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less
File lib/views/stylesheet.less (right):

https://codereview.appspot.com/7129049/diff/1/lib/views/stylesheet.less#newcode1507
lib/views/stylesheet.less:1507: .warning-message {
This is the kind of thing I wish we didn't do--but I did immediately
above, with the "error" class!  In some perfect world, our app would
have enough logical consistency to its construction (and in our
understanding of it) that we could do one of these sorts of things once.

All that said, unless you are sure that this is some new generic CSS
functionality that you are providing, or the warning message is really
rendered in the body of the page rather than within a tighter conceptual
region as defined by a CSS class, or the warning message is used in two
or more very different contexts, I suggest that you consider whether
there is a pertinent section in the LESS to place this, so someone
looking at the general functionality in the LESS file can also see this
part of it.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js
File test/test_service_view.js (right):

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newcode7
test/test_service_view.js:7: // for one or the other.
I agree that there is a messiness here.  If you want to call it out as
needing a fix, please make a bug for it.

To state the obvious, the lie could be addressed by changing the
describe line to say "juju service views" (plural).  I expect the more
interesting part to you is how we can divide things up to be more
maintainable.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newcode707
test/test_service_view.js:707: // See the XXX at the top of these tests.
I don't understand connection.  You are using the normal service view,
yeah?  We would keep this one in the existing test suite, and only move
out relation views.  Suggest deleting comment.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newcode714
test/test_service_view.js:714: // See the XXX at the top of these tests.
This comment makes sense to me.  I suggest being more
specific (e.g., "This would ideally be in its own suite: see XXX at top
of file").

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newcode721
test/test_service_view.js:721: // See the XXX at the top of these tests.
Suggest deletion as before.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newcode730
test/test_service_view.js:730: // See the XXX at the top of these tests.
Suggest improved comment as before.

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js
File test/test_templates.js (right):

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode5
test/test_templates.js:5: describe('Template:
service-footer-destroy-service.partial', function() {
Interesting approach--testing the template and the data gathering and
ignoring the actual rendering hookup itself.  I like it, though I must
admit that I miss the fact that this keeps us from testing that the
rendering actually uses the right template.  Perhaps if we used the same
render method everywhere, and simply tested whether the class specified
the expected template was in the attribute that the standard render
method used?  Please do bring it up Friday!

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode81
test/test_templates.js:81: assert.notEqual(html.indexOf(warningMessage),
-1);
I get confused about this but I think we are using mocha plus chai.  If
so, maybe this works?

assert.include(html, warningMessage, "html contains warning")

alternate spelling, which would work nicely with my next comment, would
be this.

expect(html).to.contain(warningMessage)

Would be nice if at least one of those worked. :-)

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode86
test/test_templates.js:86: assert.equal(html.indexOf(warningMessage),
-1);
Could be

expect(html).to.not.contain(warningMessage)

I don't think there is an assert spelling for this.

I usually am not a big fan of the "expect" syntax, but in this case I
think it is more readable than the -1 thing (which I have used myself).

https://codereview.appspot.com/7129049/diff/1/test/test_topology.js
File test/test_topology.js (right):

https://codereview.appspot.com/7129049/diff/1/test/test_topology.js#newcode123
test/test_topology.js:123: describe('service menu xxx', function() {
xxx?

https://codereview.appspot.com/7129049/diff/1/test/test_utils.js
File test/test_utils.js (right):

https://codereview.appspot.com/7129049/diff/1/test/test_utils.js#newcode396
test/test_utils.js:396: });
As noted before, I have more tests that I suggest you include from lines
428-434 of
https://codereview.appspot.com/7069068/diff/1/test/test_utils.js?context=10&column_width=80

https://codereview.appspot.com/7129049/