Merge into trunk : bug1167295 : Code : juju-gui

Status:

Merged

Merged at revision:

533

Proposed branch:

lp://staging/~gary/juju-gui/bug1167295

Merge into:

lp://staging/juju-gui/experimental

Diff against target:

219 lines (+84/-10)

7 files modified

app/templates/unit.handlebars (+1/-1)
app/views/topology/relation.js (+4/-4)
app/views/utils.js (+35/-0)
lib/views/stylesheet.less (+1/-0)
test/test_environment_view.js (+14/-3)
test/test_topology_relation.js (+4/-2)
test/test_utils.js (+25/-0)

To merge this branch:

bzr merge lp://staging/~gary/juju-gui/bug1167295

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-04-10	Pending
Review via email: mp+158224@code.staging.launchpad.net

Description of the change

Make safe relation ids in environment view

Relation ids included spaces and other problematic chacracters in our related DOM ids. This branch cleans those up, and also does a quick CSS cleanup to make some text legible again.

https://codereview.appspot.com/8640043/

Revision history for this message

Gary Poster (gary) wrote on 2013-04-10:

#

Download full text (8.9 KiB)

Reviewers: mp+158224_code.launchpad.net,

Message:
Please take a look.

Description:
Make safe relation ids in environment view

Relation ids included spaces and other problematic chacracters in our
related DOM ids. This branch cleans those up, and also does a quick CSS
cleanup to make some text legible again.

https://code.launchpad.net/~gary/juju-gui/bug1167295/+merge/158224

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/templates/unit.handlebars
   M app/views/topology/relation.js
   M app/views/utils.js
   M lib/views/stylesheet.less
   M test/test_environment_view.js
   M test/test_topology_relation.js
   M test/test_utils.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_environment_view.js
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js 2013-04-04 14:45:42 +0000
+++ test/test_environment_view.js 2013-04-10 21:17:18 +0000
@@ -84,7 +84,7 @@
        Y = YUI(GlobalConfig).use([
          'juju-views', 'juju-tests-utils', 'juju-env',
          'node-event-simulate', 'juju-gui', 'slider',
- 'landscape', 'dump'
+ 'landscape', 'dump', 'juju-view-utils'
        ], function(Y) {
          testUtils = Y.namespace('juju-tests.utils');
          views = Y.namespace('juju.views');
@@ -170,6 +170,13 @@
                      parseInt(this.getAttribute(e), 10))
                              .should.equal(true);
                }, line);
+
+ // Verify that the node id has been munged as expected from the
+ // relation id. This is particularly important for Juju Core.
+ var node = container.one(
+ '#' + views.utils.generateSafeDOMId('relation-0000000007'));
+ assert.isNotNull(node);
+ assert.isDefined(node);
          });

      it('must be able to render subordinate and normal services',
@@ -750,7 +757,9 @@
             db: db,
             env: env}).render();

- var relation = container.one('#relation-0000000001 .rel-label'),
+ var relation = container.one(
+ '#' + views.utils.generateSafeDOMId('relation-0000000001') +
+ ' .rel-label'),
dialog_btn,
panel;

@@ -778,7 +787,9 @@
env: env}).render();

           // Get a subordinate relation.
- var relation = container.one('#relation-0000000007 .rel-label'),
+ var relation = container.one(
+ '#' + views.utils.generateSafeDOMId('relation-0000000007') +
+ ' .rel-label'),
           dialog_btn,
           panel;

Index: test/test_topology_relation.js
=== modified file 'test/test_topology_relation.js'
--- test/test_topology_relation.js 2013-01-23 21:56:36 +0000
+++ test/test_topology_relation.js 2013-04-10 21:17:18 +0000
@@ -4,7 +4,8 @@
var Y, views, view, container, topo, db;

...

Reviewers: mp+158224_code.launchpad.net,

Message:
Please take a look.

Description:
Make safe relation ids in environment view

Relation ids included spaces and other problematic chacracters in our
related DOM ids.  This branch cleans those up, and also does a quick CSS
cleanup to make some text legible again.

https://code.launchpad.net/~gary/juju-gui/bug1167295/+merge/158224

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/templates/unit.handlebars
   M app/views/topology/relation.js
   M app/views/utils.js
   M lib/views/stylesheet.less
   M test/test_environment_view.js
   M test/test_topology_relation.js
   M test/test_utils.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: jcsackett@canonical.com-20130410171438-mc1hmriqmqtdhdb9
+New revision: gary.poster@canonical.com-20130410211718-qz53c2ialq20tq5j

Index: test/test_environment_view.js
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2013-04-04 14:45:42 +0000
+++ test/test_environment_view.js	2013-04-10 21:17:18 +0000
@@ -84,7 +84,7 @@
        Y = YUI(GlobalConfig).use([
          'juju-views', 'juju-tests-utils', 'juju-env',
          'node-event-simulate', 'juju-gui', 'slider',
-        'landscape', 'dump'
+        'landscape', 'dump', 'juju-view-utils'
        ], function(Y) {
          testUtils = Y.namespace('juju-tests.utils');
          views = Y.namespace('juju.views');
@@ -170,6 +170,13 @@
                      parseInt(this.getAttribute(e), 10))
                              .should.equal(true);
                }, line);
+
+          // Verify that the node id has been munged as expected from the
+          // relation id. This is particularly important for Juju Core.
+          var node = container.one(
+              '#' + views.utils.generateSafeDOMId('relation-0000000007'));
+          assert.isNotNull(node);
+          assert.isDefined(node);
          });

it('must be able to render subordinate and normal services',
@@ -750,7 +757,9 @@
             db: db,
             env: env}).render();

-         var relation = container.one('#relation-0000000001 .rel-label'),
+         var relation = container.one(
+              '#' + views.utils.generateSafeDOMId('relation-0000000001') +
+              ' .rel-label'),
           dialog_btn,
           panel;

@@ -778,7 +787,9 @@
             env: env}).render();

// Get a subordinate relation.
-         var relation = container.one('#relation-0000000007 .rel-label'),
+         var relation = container.one(
+              '#' + views.utils.generateSafeDOMId('relation-0000000007') +
+              ' .rel-label'),
           dialog_btn,
           panel;

Index: test/test_topology_relation.js
=== modified file 'test/test_topology_relation.js'
--- test/test_topology_relation.js	2013-01-23 21:56:36 +0000
+++ test/test_topology_relation.js	2013-04-10 21:17:18 +0000
@@ -4,7 +4,8 @@
    var Y, views, view, container, topo, db;

before(function(done) {
-    Y =  
YUI(GlobalConfig).use(['juju-topology', 'node', 'node-event-simulate'],
+    Y = YUI(GlobalConfig).use(
+         
['juju-topology', 'node', 'node-event-simulate', 'juju-view-utils'],
          function(Y) {
            views = Y.namespace('juju.views');
            done();
@@ -87,7 +88,8 @@
        endpoints: [null, null]
      };
      view.removeRelation.call(fauxView, relation, fauxView, undefined);
-    assert.equal(requestedSelector, '#' + relationId);
+    assert.equal(
+        requestedSelector, '#' +  
views.utils.generateSafeDOMId(relationId));
    });

});

Index: app/templates/unit.handlebars
=== modified file 'app/templates/unit.handlebars'
--- app/templates/unit.handlebars	2013-04-10 09:48:16 +0000
+++ app/templates/unit.handlebars	2013-04-10 20:37:33 +0000
@@ -55,7 +55,7 @@
  </div>

{{#unless relations}}
-This unit has no relations.
+<div class="yui3-u">This unit has no relations.</div>
  {{else}}
  <div id="relations" class="yui3-g">
    <div class="yui3-u-1">

Index: app/views/utils.js
=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-04-10 09:40:06 +0000
+++ app/views/utils.js	2013-04-10 21:17:18 +0000
@@ -12,6 +12,41 @@
    var views = Y.namespace('juju.views'),
        utils = Y.namespace('juju.views.utils');

+  /*jshint bitwise: false*/
+  /**
+    Create a hash of a string. From stackoverflow: http://goo.gl/PEOgF
+
+    @method generateHash
+    @param {String} value The string to hash.
+    @return {Integer} The hash of the string.
+   */
+  var generateHash = function(value) {
+    return value.split('').reduce(
+        function(hash, character) {
+          hash = ((hash << 5) - hash) + character.charCodeAt(0);
+          return hash & hash;
+        },
+        0
+    );
+  };
+  /*jshint bitwise: true*/
+  utils.generateHash = generateHash;
+
+  /**
+    Create a stable, safe DOM id given an arbitrary string.
+    See details and discussion in
+    https://bugs.launchpad.net/juju-gui/+bug/1167295
+
+    @method generateSafeDOMId
+    @param {String} value The string to hash.
+    @return {String} The calculated DOM id.
+   */
+  var generateSafeDOMId = function(value) {
+    return (
+        value.replace(/\W/g, '_') + '-' + generateHash(value));
+  };
+  utils.generateSafeDOMId = generateSafeDOMId;
+
    var timestrings = {
      prefixAgo: null,
      prefixFromNow: null,

Index: app/views/topology/relation.js
=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js	2013-04-10 16:38:13 +0000
+++ app/views/topology/relation.js	2013-04-10 21:17:18 +0000
@@ -164,7 +164,7 @@
          return relation.source.id === service.id ||
              relation.target.id === service.id;
        }), function(relation) {
-        var rel_group = d3.select('#' + relation.id);
+        var rel_group = d3.select('#' +  
utils.generateSafeDOMId(relation.id));
          var connectors = relation.source
                    .getConnectorPair(relation.target);
          var s = connectors[0];
@@ -199,7 +199,7 @@

enter.insert('g', 'g.service')
                .attr('id', function(d) {
-            return d.id;
+            return utils.generateSafeDOMId(d.id);
            })
                .attr('class', function(d) {
                  // Mark the rel-group as a subordinate relation if need be.
@@ -461,7 +461,7 @@
        // At this time, relations may have been redrawn, so here we have to
        // retrieve the relation DOM element again.
        var relationElement = view.get('container')
-        .one('#' + relation.relation_id);
+        .one('#' + utils.generateSafeDOMId(relation.relation_id));
        utils.addSVGClass(relationElement, 'to-remove pending-relation');
        env.remove_relation(relation.endpoints[0], relation.endpoints[1],
            Y.bind(this._removeRelationCallback, this, view,
@@ -747,7 +747,7 @@
        // Remove our pending relation from the DB, error or no.
        db.relations.remove(
            db.relations.getById(relation_id));
-      vis.select('#' + relation_id).remove();
+      vis.select('#' + utils.generateSafeDOMId(relation_id)).remove();
        if (ev.err) {
          db.notifications.add(
              new models.Notification({

Index: lib/views/stylesheet.less
=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less	2013-04-09 14:18:49 +0000
+++ lib/views/stylesheet.less	2013-04-10 20:37:33 +0000
@@ -872,6 +872,7 @@
      background-clip: padding-box;
      outline: none;
      font-weight: normal;
+    letter-spacing: normal;
      .yui3-widget-bd {
          margin-bottom: 1em;
      }

Index: test/test_utils.js
=== modified file 'test/test_utils.js'
--- test/test_utils.js	2013-04-10 09:54:21 +0000
+++ test/test_utils.js	2013-04-10 21:17:18 +0000
@@ -12,6 +12,31 @@
            });
      });

+    it('can generate a hash', function() {
+      // We aren't testing the algorithm here, just basic hash  
characteristics.
+      // It's a number.
+      assert.strictEqual(views.utils.generateHash(''), 0);
+      assert.isNumber(views.utils.generateHash('kumquat'));
+      assert.isNumber(views.utils.generateHash('qumquat'));
+      // It's stable.
+      assert.strictEqual(
+          views.utils.generateHash('kumquat'),
+          views.utils.generateHash('kumquat'));
+      // Different values hash differently.
+      assert.notEqual(
+          views.utils.generateHash('kumquat'),
+          views.utils.generateHash('qumquat'));
+    });
+
+    it('can generate safe relation ids', function() {
+      var relationId;
+      relationId = 'foo:Bar relation-00000006!@#';
+      assert.strictEqual(
+          views.utils.generateSafeDOMId(relationId),
+          'foo_Bar_relation_00000006___-' +
+          views.utils.generateHash(relationId));
+    });
+
      it('should create a confirmation panel',
         function() {
            var confirmed = false;

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-04-10:

#

LGTM - nice clean approach!

https://codereview.appspot.com/8640043/

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-04-10:

#

LGTM - thanks!

https://codereview.appspot.com/8640043/

Revision history for this message

Gary Poster (gary) wrote on 2013-04-10:

#

*** Submitted:

Make safe relation ids in environment view

Relation ids included spaces and other problematic chacracters in our
related DOM ids. This branch cleans those up, and also does a quick CSS
cleanup to make some text legible again.

R=jeff.pihach, matthew.scott
CC=
https://codereview.appspot.com/8640043

https://codereview.appspot.com/8640043/

juju-gui

Merge lp://staging/~gary/juju-gui/bug1167295 into lp://staging/juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers