Validates the service names on user entry

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.

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

(do not edit description out of merge proposal)

[jeff.pihach, 2013-12-03 15:12:31]

Please take a look.

[jeff.pihach, 2013-12-03 16:09:30]

Please take a look.

[rharding, 2013-12-03 17:06:26]

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....
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....
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....
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....
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....
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#newcode...
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#newcode...
test/test_utils.js:1466: // result should be false because we are simulating
finding a service
Capital R

[jeff.pihach, 2013-12-03 17:16:31]

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....
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....
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....
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....
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#newcode...
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#newcode...
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.

[jeff.pihach, 2013-12-03 17:23:15]

*** 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