Merge lp:~gary/juju-gui/nameCheck into lp:juju-gui/experimental

[Gary Poster]

Fix ghost inspector initial name validation

QA: deploy mediawiki with the default name. Make another ghost, and the inspector should correctly show that the default name is invalid.

https://codereview.appspot.com/22500043/

[Gary Poster (gary)]

Reviewers: mp+194234_code.launchpad.net,

Message:
Please take a look.

Description:
Fix ghost inspector initial name validation

QA: deploy mediawiki with the default name. Make another ghost, and the
inspector should correctly show that the default name is invalid.

Thank you.

https://code.launchpad.net/~gary/juju-gui/nameCheck/+merge/194234

(do not edit description out of merge proposal)

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

Affected files (+60, -16 lines):
   A [revision details]
   M app/views/viewlets/inspector-header.js
   M test/test_ghost_inspector.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test/test_ghost_inspector.js
=== modified file 'test/test_ghost_inspector.js'
--- test/test_ghost_inspector.js 2013-11-05 18:10:05 +0000
+++ test/test_ghost_inspector.js 2013-11-06 21:04:34 +0000
@@ -47,6 +47,7 @@
      conn = new utils.SocketStub();
      db = new models.Database();
      env = juju.newEnvironment({conn: conn});
+ env.connect();
    });

    afterEach(function(done) {
@@ -93,6 +94,52 @@
      return view.createServiceInspector(service, {databinding: {interval:
0}});
    };

+ describe('charm name validity', function() {
+ it('shows when a charm name is invalid initially', function() {
+ db.services.add({id: 'mediawiki', charm: 'cs:precise/mediawiki'});
+ inspector = setUpInspector();
+ var model = inspector.model;
+ var serviceNameInput = Y.one('input[name=service-name]');
+ assert.equal(model.get('displayName'), '(mediawiki)');
+ assert.isTrue(serviceNameInput.hasClass('invalid'));
+ assert.isFalse(serviceNameInput.hasClass('valid'));
+ });
+
+ it('shows when a charm name is valid initially', function() {
+ db.services.add({id: 'mediawiki42', charm: 'cs:precise/mediawiki'});
+ inspector = setUpInspector();
+ var model = inspector.model;
+ var serviceNameInput = Y.one('input[name=service-name]');
+ assert.equal(model.get('displayName'), '(mediawiki)');
+ assert.isFalse(serviceNameInput.hasClass('invalid'));
+ assert.isTrue(serviceNameInput.hasClass('valid'));
+ });
+
+ it('shows when a charm name becomes invalid', function() {
+ db.services.add({id: 'mediawiki42', charm: 'cs:precise/mediawiki'});
+ inspector = setUpInspector();
+ var serviceNameInput = Y.one('input[name=service-name]');
+ // This is usually fired by an event. The event simulation is
broken as
+ // of this writing, and we can do more of a unit test this way.
+ inspector.updateGhostName(
+ {newVal: 'mediawiki42', currentTarget: serviceNameInput});
+ assert.isTrue(serviceNameInput.hasClass('invalid'));
+ assert.isFalse(serviceNameInput.hasClass('valid'));
+ });
+
+ it('shows when a charm name becomes valid', function() {
+ db.services.add({id: 'mediawiki', charm: 'cs:precise/mediawiki'});
+ in...

[Jeff Pihach (hatch)]

LGTM with a small comment request.
QA OK
Thanks for this fix!

https://codereview.appspot.com/22500043/diff/1/app/views/viewlets/inspector-header.js
File app/views/viewlets/inspector-header.js (right):

https://codereview.appspot.com/22500043/diff/1/app/views/viewlets/inspector-header.js#newcode59
app/views/viewlets/inspector-header.js:59: var name =
pojoModel.displayName.match(/^\(([^)]*)\)$/)[1];
I'd love it if there was a quick comment describing what this regex
looks for to make the code easier to scan.

https://codereview.appspot.com/22500043/diff/1/test/test_ghost_inspector.js
File test/test_ghost_inspector.js (right):

https://codereview.appspot.com/22500043/diff/1/test/test_ghost_inspector.js#newcode50
test/test_ghost_inspector.js:50: env.connect();
Ahh the shibboleth of writing environment tests.....you may proceed.

https://codereview.appspot.com/22500043/

[lp:~gary/juju-gui/nameCheck]

1184. By Gary Poster on 2013-11-06

merge trunk

1185. By Gary Poster on 2013-11-06

add explanatory comments per review

[Gary Poster (gary)]

*** Submitted:

Fix ghost inspector initial name validation

QA: deploy mediawiki with the default name. Make another ghost, and the
inspector should correctly show that the default name is invalid.

R=jeff.pihach
CC=
https://codereview.appspot.com/22500043

https://codereview.appspot.com/22500043/

[Gary Poster (gary)]

On 2013/11/06 21:20:50, jeff.pihach wrote:
...

https://codereview.appspot.com/22500043/diff/1/app/views/viewlets/inspector-header.js#newcode59
> app/views/viewlets/inspector-header.js:59: var name =
> pojoModel.displayName.match(/^\(([^)]*)\)$/)[1];
> I'd love it if there was a quick comment describing what this regex
looks for to
> make the code easier to scan.

You want comments? I'm your man! Done. :-)

Thanks for the review!

https://codereview.appspot.com/22500043/

=== modified file 'app/views/ghost-inspector.js'
--- app/views/ghost-inspector.js	2013-10-15 19:33:04 +0000
+++ app/views/ghost-inspector.js	2013-11-06 21:41:10 +0000
@@ -170,6 +170,11 @@
       @param {Y.EventFacade} e event object from valuechange.
     */
     updateGhostName: function(e) {
+      // This code has a fragile dependency: the inspector-header.js render
+      // method expects these parentheses around the model displayName.  If you
+      // change this format, or this code, make sure you look at that method
+      // too.  Hopefully the associated tests will catch it as well.
+      // Also see resetCanvas, below.
       var name = '(' + e.newVal + ')';
       this.model.set('displayName', name);
       this.serviceNameInputStatus(
@@ -184,6 +189,11 @@
       @method resetCanvas
     */
     resetCanvas: function() {
+      // This code has a fragile dependency: the inspector-header.js render
+      // method expects these parentheses around the model displayName.  If you
+      // change this format, or this code, make sure you look at that method
+      // too.  Hopefully the associated tests will catch it as well.
+      // Also see updateGhostName, above.
       this.model.set('displayName', '(' + this.model.get('packageName') + ')');
       this.viewletManager.destroy();
     },

=== modified file 'app/views/viewlets/inspector-header.js'
--- app/views/viewlets/inspector-header.js	2013-09-30 15:38:34 +0000
+++ app/views/viewlets/inspector-header.js	2013-11-06 21:41:10 +0000
@@ -48,25 +48,22 @@
     'render': function(model, viewContainerAttrs) {
       this.container = Y.Node.create(this.templateWrapper);
       var pojoModel = model.getAttrs();
-      if (model instanceof models.Charm) {
-        pojoModel.ghost = true;
-        pojoModel.charmUrl = pojoModel.id;
-      } else if (model instanceof models.Service) {
-        pojoModel.charmUrl = pojoModel.charm;
-      } else {
-        throw 'Programmer error: unknown model type';
-      }
+      pojoModel.charmUrl = pojoModel.charm;
       // Manually add the icon url for the charm since we don't have access to
       // the browser handlebars helper at this location.
       pojoModel.icon = viewContainerAttrs.store.iconpath(pojoModel.charmUrl);
-
-      // Check if there is already a service using the default name to
-      // trigger the name ux.
-      if (utils.checkForExistingService(pojoModel.name,
-          viewContainerAttrs.db)) {
-        pojoModel.invalidName = 'invalid';
-      } else {
-        pojoModel.invalidName = 'valid';
+      if (pojoModel.pending) {
+        // Check if there is already a service using the default name to
+        // trigger the name ux.
+        // This regex simply removes the outer parentheses from the
+        // displayName that is set in the ghost-inspector.js updateGhostName
+        // method.  If the regex doesn't match, blow up.  It should match.
+        var name = pojoModel.displayName.match(/^\(([^)]*)\)$/)[1];
+        if (utils.checkForExistingService(name, viewContainerAttrs.db)) {
+          pojoModel.invalidName = 'invalid';
+        } else {
+          pojoModel.invalidName = 'valid';
+        }
       }
 
       this.container.setHTML(this.template(pojoModel));

=== modified file 'test/test_ghost_inspector.js'
--- test/test_ghost_inspector.js	2013-11-05 18:10:05 +0000
+++ test/test_ghost_inspector.js	2013-11-06 21:41:10 +0000
@@ -47,6 +47,7 @@
     conn = new utils.SocketStub();
     db = new models.Database();
     env = juju.newEnvironment({conn: conn});
+    env.connect();
   });
 
   afterEach(function(done) {
@@ -93,6 +94,52 @@
     return view.createServiceInspector(service, {databinding: {interval: 0}});
   };
 
+  describe('charm name validity', function() {
+    it('shows when a charm name is invalid initially', function() {
+      db.services.add({id: 'mediawiki', charm: 'cs:precise/mediawiki'});
+      inspector = setUpInspector();
+      var model = inspector.model;
+      var serviceNameInput = Y.one('input[name=service-name]');
+      assert.equal(model.get('displayName'), '(mediawiki)');
+      assert.isTrue(serviceNameInput.hasClass('invalid'));
+      assert.isFalse(serviceNameInput.hasClass('valid'));
+    });
+
+    it('shows when a charm name is valid initially', function() {
+      db.services.add({id: 'mediawiki42', charm: 'cs:precise/mediawiki'});
+      inspector = setUpInspector();
+      var model = inspector.model;
+      var serviceNameInput = Y.one('input[name=service-name]');
+      assert.equal(model.get('displayName'), '(mediawiki)');
+      assert.isFalse(serviceNameInput.hasClass('invalid'));
+      assert.isTrue(serviceNameInput.hasClass('valid'));
+    });
+
+    it('shows when a charm name becomes invalid', function() {
+      db.services.add({id: 'mediawiki42', charm: 'cs:precise/mediawiki'});
+      inspector = setUpInspector();
+      var serviceNameInput = Y.one('input[name=service-name]');
+      // This is usually fired by an event.  The event simulation is broken as
+      // of this writing, and we can do more of a unit test this way.
+      inspector.updateGhostName(
+          {newVal: 'mediawiki42', currentTarget: serviceNameInput});
+      assert.isTrue(serviceNameInput.hasClass('invalid'));
+      assert.isFalse(serviceNameInput.hasClass('valid'));
+    });
+
+    it('shows when a charm name becomes valid', function() {
+      db.services.add({id: 'mediawiki', charm: 'cs:precise/mediawiki'});
+      inspector = setUpInspector();
+      var serviceNameInput = Y.one('input[name=service-name]');
+      // This is usually fired by an event.  The event simulation is broken as
+      // of this writing, and we can do more of a unit test this way.
+      inspector.updateGhostName(
+          {newVal: 'mediawiki42', currentTarget: serviceNameInput});
+      assert.isFalse(serviceNameInput.hasClass('invalid'));
+      assert.isTrue(serviceNameInput.hasClass('valid'));
+    });
+  });
+
   it('updates the service name in the topology when changed in the inspector',
       function(done) {
         // XXX (Jeff) YUI's simulate can't properly simulate focus or blur in