Prevent destruction of the Juju GUI service

Prevent destruction of the Juju GUI service



https://code.launchpad.net/~benji/juju-gui/no-foot-shooting/+merge/143715

(do not edit description out of merge proposal)

[benji, 2013-01-17 16:13:29]

Please take a look.

[gary.poster, 2013-01-17 17:34:06]

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-co...
File app/templates/service-footer-common-controls.partial (right):

https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-co...
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.handlebar...
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#n...
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#n...
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#n...
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&c...
: "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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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&c...

[benji, 2013-01-17 19:19:50]

Thanks for the great review.  I have fixed most of the things you noted and...
started a discussion about the rest. ;)

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

https://codereview.appspot.com/7129049/diff/1/app/templates/service-footer-co...
app/templates/service-footer-common-controls.partial:1: {{#unless
serviceIsJujuGUI}}
On 2013/01/17 17:34:06, gary.poster wrote:
> My inclination is to let the GUI increase the unit count,
> [...]

That's reasonable.  Done.

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.handlebar...
app/templates/service.handlebars:25: {{#unless serviceIsJujuGUI}}
On 2013/01/17 17:34:06, gary.poster wrote:
> If you agree that we should reinstate unit count, then IIUC you would also
> remove this conditional.

Yep.  Done.

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.
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

I would be inclined to remove the code altogether.  I accidentally duplicated
this between the menu's destroy handler and the destroy button at the bottom of
the service view.  It is needed for the menu because the menu item is only
disabled in as far as its visual styling is different, the click event is still
fired.

I have removed the code but if you like the fail-safe, I will be glad to put it
back (with commentary).

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
Fixed (all of them).  Thanks.

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#n...
app/views/topology/service.js:79: // it would break the application they are
currently using.
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

Nope, this is really the code that keeps the destroy from happening.  The menu
item is only "disabled" visually.  The click event is still fired.  This is the
pre-existing pattern in the code (for the always-dead "Units" menu item.

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);
On 2013/01/17 17:34:06, gary.poster wrote:
> 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&c...
> : "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.

Oh!  Thanks.  I wasn't aware of the improved version.  I have integrated it and
its tests.

https://codereview.appspot.com/7129049/diff/1/app/views/utils.js#newcode963
app/views/utils.js:963: var charmUrl = candidate.charm ||
candidate.get('charm');
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

All the tests pass with this version, so I am inclined to avoid the "sloppier"
version.  The things passed in really should have a "charm" attribute one way or
the other.

That being said, I don't feel strongly about it, so one more nudge will get me
to use the other branch's version.

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#newco...
lib/views/stylesheet.less:1507: .warning-message {
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

I couldn't find any sections that really made semantic sense, so I put
it in .view-container .control-group to at least limit its scope.
Suggestions welcome.

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.
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

I filed bug 1100908.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newco...
test/test_service_view.js:707: // See the XXX at the top of these tests.
On 2013/01/17 17:34:06, gary.poster wrote:
> 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.

Deleted.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newco...
test/test_service_view.js:714: // See the XXX at the top of these tests.
On 2013/01/17 17:34:06, gary.poster wrote:
> 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").

Done.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newco...
test/test_service_view.js:721: // See the XXX at the top of these tests.
On 2013/01/17 17:34:06, gary.poster wrote:
> Suggest deletion as before.

Done.

https://codereview.appspot.com/7129049/diff/1/test/test_service_view.js#newco...
test/test_service_view.js:730: // See the XXX at the top of these tests.
On 2013/01/17 17:34:06, gary.poster wrote:
> Suggest improved comment as before.

Done.

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() {
On 2013/01/17 17:34:06, gary.poster wrote:
> 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?

Yeah, there is much code that could be collapsed into a simple pattern around
template rendering.  There are also many missing tests surrounding this area so
I decided not to make this branch take any longer than it already has by adding
those tests.

> Please do bring it up Friday!

I added a "Discuss" card for this and assigned it to me.

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode81
test/test_templates.js:81: assert.notEqual(html.indexOf(warningMessage), -1);
On 2013/01/17 17:34:06, gary.poster wrote:
> 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")

"assert.include" works but there is no assert.notInclude so I decided that being
symmetric was more important than using a nice spelling in one case and a
non-nice spelling in another.

> 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. :-)

Both expect(html).to.contain(...) and expect(html).to.not.contain(...) work, but
I hate that spelling with a passion.  That hatred (and/or passion) may not be
justified, so for these at least I am willing to spell them in the BDD way. 
Another option would be to have a small test helper.  Perhaps one that adds a
"notInclude" to "assert".

Thoughts?

https://codereview.appspot.com/7129049/diff/1/test/test_templates.js#newcode86
test/test_templates.js:86: assert.equal(html.indexOf(warningMessage), -1);
On 2013/01/17 17:34:06, gary.poster wrote:
> 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).

Yep, it exists.  See above.

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() {
On 2013/01/17 17:34:06, gary.poster wrote:
> xxx?

Heh.  That is some debugging cruft that got left in.  Fixed.

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: });
On 2013/01/17 17:34:06, gary.poster wrote:
> 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&c...

Done.

[gary.poster, 2013-01-17 20:02:49]

Sounds good benji.  Thanks!

Gary

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.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > 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.
> 
> I would be inclined to remove the code altogether.  I accidentally duplicated
> this between the menu's destroy handler and the destroy button at the bottom
of
> the service view.  It is needed for the menu because the menu item is only
> disabled in as far as its visual styling is different, the click event is
still
> fired.
> 
> I have removed the code but if you like the fail-safe, I will be glad to put
it
> back (with commentary).

Cool, that's fine.  I could go either way.

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#n...
app/views/topology/service.js:79: // it would break the application they are
currently using.
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > 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.
> 
> Nope, this is really the code that keeps the destroy from happening.  The menu
> item is only "disabled" visually.  The click event is still fired.  This is
the
> pre-existing pattern in the code (for the always-dead "Units" menu item.

Ah, OK, 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#newcode963
app/views/utils.js:963: var charmUrl = candidate.charm ||
candidate.get('charm');
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > 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.
> 
> All the tests pass with this version, so I am inclined to avoid the "sloppier"
> version.  The things passed in really should have a "charm" attribute one way
or
> the other.
> 
> That being said, I don't feel strongly about it, so one more nudge will get me
> to use the other branch's version.

Naah, let's stick with what you have, thanks.

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#newco...
lib/views/stylesheet.less:1507: .warning-message {
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > 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.
> 
> I couldn't find any sections that really made semantic sense, so I put
> it in .view-container .control-group to at least limit its scope.
> Suggestions welcome.

I suggest you eat chocolate ice cream.  It's really quite good.

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#newcode81
test/test_templates.js:81: assert.notEqual(html.indexOf(warningMessage), -1);
On 2013/01/17 19:19:50, benji wrote:
> On 2013/01/17 17:34:06, gary.poster wrote:
> > 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")
> 
> "assert.include" works but there is no assert.notInclude so I decided that
being
> symmetric was more important than using a nice spelling in one case and a
> non-nice spelling in another.
> 
> > 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. :-)
> 
> Both expect(html).to.contain(...) and expect(html).to.not.contain(...) work,
but
> I hate that spelling with a passion.  That hatred (and/or passion) may not be
> justified, so for these at least I am willing to spell them in the BDD way. 
> Another option would be to have a small test helper.  Perhaps one that adds a
> "notInclude" to "assert".
> 
> Thoughts?

As I said, I dislike the BDD spelling, but not as much as you do, apparently.
:-)

assert.exclude(...) might be nice.  I don't want to hold up this branch for it. 
If you like the idea, feel free to file a bug...or to act on it now if you
really, really feel like it. :-)

[benji, 2013-01-17 20:29:26]

Thanks for the follow-up.  I think this branch is good to go now.  I will rustle
up a second reviewer.

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#newco...
lib/views/stylesheet.less:1507: .warning-message {
On 2013/01/17 20:02:50, gary.poster wrote:
> On 2013/01/17 19:19:50, benji wrote:
> > On 2013/01/17 17:34:06, gary.poster wrote:
> > > 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.
> > 
> > I couldn't find any sections that really made semantic sense, so I put
> > it in .view-container .control-group to at least limit its scope.
> > Suggestions welcome.
> 
> I suggest you eat chocolate ice cream.  It's really quite good.

Done.

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#newcode81
test/test_templates.js:81: assert.notEqual(html.indexOf(warningMessage), -1);
On 2013/01/17 20:02:50, gary.poster wrote:
> On 2013/01/17 19:19:50, benji wrote:
> > On 2013/01/17 17:34:06, gary.poster wrote:
> > > 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")
> > 
> > "assert.include" works but there is no assert.notInclude so I decided that
> being
> > symmetric was more important than using a nice spelling in one case and a
> > non-nice spelling in another.
> > 
> > > 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. :-)
> > 
> > Both expect(html).to.contain(...) and expect(html).to.not.contain(...) work,
> but
> > I hate that spelling with a passion.  That hatred (and/or passion) may not
be
> > justified, so for these at least I am willing to spell them in the BDD way. 
> > Another option would be to have a small test helper.  Perhaps one that adds
a
> > "notInclude" to "assert".
> > 
> > Thoughts?
> 
> As I said, I dislike the BDD spelling, but not as much as you do, apparently.
> :-)
> 
> assert.exclude(...) might be nice.  I don't want to hold up this branch for
it. 
> If you like the idea, feel free to file a bug...or to act on it now if you
> really, really feel like it. :-)

I switched all of the tests using this pattern to use ".match" instead since
there is a corresponding ".notMatch".  They look pretty decent, I'd say:

    assert.notMatch(html, /Destroy/);
    assert.match(html, /Destroy/);

[benji, 2013-01-17 20:32:10]

Please take a look.

[bcsaller, 2013-01-17 22:26:57]

LGTM, a minor suggestion or two included.

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

https://codereview.appspot.com/7129049/diff/15001/app/views/service.js#newcod...
app/views/service.js:566: *
Agreed, each of these changes wrt gatherRenderData is nice.

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

https://codereview.appspot.com/7129049/diff/15001/app/views/topology/service....
app/views/topology/service.js:769:
menu.one('.destroy-service').addClass('disabled');
Does this actually work? .destroy-service still has the click handler bound even
if the class greys out the link or something, no?

Might we remove the link from the menu all together?

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

https://codereview.appspot.com/7129049/diff/15001/test/test_topology.js#newco...
test/test_topology.js:118:
topo.events.ServiceModule.scene['.destroy-service'].click.callback(
a simulate('click') test might be better here if you leave this in the menu.

[benji, 2013-01-18 13:33:47]

Thanks for the review.

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

https://codereview.appspot.com/7129049/diff/15001/app/views/topology/service....
app/views/topology/service.js:769:
menu.one('.destroy-service').addClass('disabled');
On 2013/01/17 22:26:58, bcsaller wrote:
> Does this actually work? .destroy-service still has the click handler bound
even
> if the class greys out the link or something, no?

Right, this is just visual "disable" the click handler has the logic to do the
"real" disabling.

> Might we remove the link from the menu all together?

We have the perma-disabled "Units" menu entry now, so I followed that pattern. 
I'm cool either way, but I am inclined toward the status quo.

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

https://codereview.appspot.com/7129049/diff/15001/test/test_topology.js#newco...
test/test_topology.js:118:
topo.events.ServiceModule.scene['.destroy-service'].click.callback(
On 2013/01/17 22:26:58, bcsaller wrote:
> a simulate('click') test might be better here if you leave this in the menu. 

My intent here is a function call, not an interaction with a UI, so I don't see
a benefit to simulating the click.

[benji, 2013-01-18 14:17:17]

*** Submitted:

Prevent destruction of the Juju GUI service

R=gary.poster, bcsaller
CC=
https://codereview.appspot.com/7129049