Merge lp://qastaging/~hatch/juju-gui/service-name-1252578 into lp://qastaging/juju-gui/experimental

Reviewers: mp+197543_code.launchpad.net,

Message:
Please take a look.

Description:
Validates the service names on user entry

The service name input field in the ghost inspector now validates
against a similar regex that is used in juju-core to avoid failures
trying to deploy services.

To QA:
Add a service to the canvas
Try to deploy the service using the values provided in the included
tests

https://code.launchpad.net/~hatch/juju-gui/service-name-1252578/+merge/197543

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/36500044/

Affected files (+106, -8 lines):
   A [revision details]
   M app/views/ghost-inspector.js
   M app/views/utils.js
   M test/test_ghost_inspector.js
   M test/test_utils.js

Please take a look.

https://codereview.appspot.com/36500044/

LGTM with a few nitpicks.

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode106
app/views/ghost-inspector.js:106: message: 'Service name is invalid.',
can we make this more conversational english like the old message? "The
requested service name is invalid" or the like?

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode109
app/views/ghost-inspector.js:109: return false;
rather than return here, when we return nothing lower below, just stick
the other stuff in an else clause? Then we can make the above if
non-negative. Just "If validServiceName, do this, else, error"

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode182
app/views/ghost-inspector.js:182: var valid =
utils.validateServiceName(name, db);
is this fails like the if block above then is there anything to do vs
just continue on?

A function called validateServiceName I'd expect to only return a bool
and using the return value seems fishy.

Edit: ok, I think naming this is_valid would read nicer.

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

https://codereview.appspot.com/36500044/diff/20001/app/views/utils.js#newcode858
app/views/utils.js:858: return false;
ok, it does return true/false.

https://codereview.appspot.com/36500044/diff/20001/test/test_ghost_inspector.js
File test/test_ghost_inspector.js (right):

https://codereview.appspot.com/36500044/diff/20001/test/test_ghost_inspector.js#newcode147
test/test_ghost_inspector.js:147: serviceNameInput =
Y.one('input[name=service-name]');
do we have to do a Y.one? Isn't there a container we can get at?

https://codereview.appspot.com/36500044/diff/20001/test/test_ghost_inspector.js#newcode149
test/test_ghost_inspector.js:149: // of this writing, and we can do more
of a unit test this way.
:(

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js
File test/test_utils.js (right):

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js#newcode1446
test/test_utils.js:1446: YUI(GlobalConfig).use('juju-view-utils',
function(Y) {
Why not mirror the setup in the previous tests using Y?

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js#newcode1466
test/test_utils.js:1466: // result should be false because we are
simulating finding a service
Capital R

https://codereview.appspot.com/36500044/

Thanks for the review - changes made and landing

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode106
app/views/ghost-inspector.js:106: message: 'Service name is invalid.',
On 2013/12/03 17:06:27, rharding wrote:
> can we make this more conversational english like the old message?
"The
> requested service name is invalid" or the like?

Done.

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode109
app/views/ghost-inspector.js:109: return false;
I didn't really want to refactor the method. The only reason I'm
returning false here and not undefined is to make it easier to test.

https://codereview.appspot.com/36500044/diff/20001/app/views/ghost-inspector.js#newcode182
app/views/ghost-inspector.js:182: var valid =
utils.validateServiceName(name, db);
On 2013/12/03 17:06:27, rharding wrote:
> is this fails like the if block above then is there anything to do vs
just
> continue on?

> A function called validateServiceName I'd expect to only return a bool
and using
> the return value seems fishy.

> Edit: ok, I think naming this is_valid would read nicer.

Done.

https://codereview.appspot.com/36500044/diff/20001/test/test_ghost_inspector.js
File test/test_ghost_inspector.js (right):

https://codereview.appspot.com/36500044/diff/20001/test/test_ghost_inspector.js#newcode147
test/test_ghost_inspector.js:147: serviceNameInput =
Y.one('input[name=service-name]');
copied this from the previous tests.

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js
File test/test_utils.js (right):

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js#newcode1446
test/test_utils.js:1446: YUI(GlobalConfig).use('juju-view-utils',
function(Y) {
No reason, just wrote it.

https://codereview.appspot.com/36500044/diff/20001/test/test_utils.js#newcode1466
test/test_utils.js:1466: // result should be false because we are
simulating finding a service
On 2013/12/03 17:06:27, rharding wrote:
> Capital R

Done.

https://codereview.appspot.com/36500044/

*** Submitted:

Validates the service names on user entry

The service name input field in the ghost inspector now validates
against a similar regex that is used in juju-core to avoid failures
trying to deploy services.

R=rharding
CC=
https://codereview.appspot.com/36500044

https://codereview.appspot.com/36500044/