Code review comment for lp://staging/~gary/juju-gui/bug1167295

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;

    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;