Merge lp:~benji/juju-gui/bug-1103477 into lp:juju-gui/experimental

[Benji York]

Add a test for the fix for bug 1103204

This branch also includes some refactoring of where relation names are
calculated in an effort to DRY it up as well as make testing a tad easier.

https://codereview.appspot.com/7206047/

[Madison Scott-Clary (makyo)]

Land as is after landing parent branch, looks good to me with the change
involved in the parent.

https://codereview.appspot.com/7206047/

[Gary Poster (gary)]

Land with changes.

Extremely nice branch, Benji. Thank you.

Gary

https://codereview.appspot.com/7206047/diff/1/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/7206047/diff/1/app/views/topology/relation.js#newcode834
app/views/topology/relation.js:834: relationClick: function(d, self) {
I expected you to change this "d" argument to "relation" like your other
nice changes. Would be nice. :-)

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

https://codereview.appspot.com/7206047/diff/1/app/views/utils.js#newcode827
app/views/utils.js:827: * Decorate a relation with some related/derrived
data.
typo: derived

https://codereview.appspot.com/7206047/

[lp:~benji/juju-gui/bug-1103477]

346. By Benji York on 2013-01-24

merge from trunk

347. By Benji York on 2013-01-24

give a parameter a better name

=== modified file 'app/store/env.js'
--- app/store/env.js	2013-01-24 14:14:45 +0000
+++ app/store/env.js	2013-01-24 19:23:21 +0000
@@ -13,6 +13,9 @@
     Environment.superclass.constructor.apply(this, arguments);
   }
 
+  var endpointToName = function(endpoint) {
+    return endpoint[0] + ':' + endpoint[1].name;
+  };
 
   Environment.NAME = 'env';
   Environment.ATTRS = {
@@ -204,19 +207,19 @@
      * Add a relation between two services.
      *
      * @method add_relation
-     * @param {String} endpoint_a A string `service:interface` representing
+     * @param {Object} endpointA An array of [service, interface] representing
          the first endpoint to connect.
-     * @param {String} endpoint_b A string `service:interface` representing
+     * @param {Object} endpointB An array of [service, interface] representing
          the second endpoint to connect.
      * @param {Function} callback A callable that must be called once the
          operation is performed.
      * @return {undefined} Sends a message to the server only.
      */
-    add_relation: function(endpoint_a, endpoint_b, callback) {
+    add_relation: function(endpointA, endpointB, callback) {
       this._send_rpc({
         'op': 'add_relation',
-        'endpoint_a': endpoint_a,
-        'endpoint_b': endpoint_b}, callback, true);
+        'endpoint_a': endpointToName(endpointA),
+        'endpoint_b': endpointToName(endpointB)}, callback, true);
     },
 
     get_charm: function(charm_url, callback) {
@@ -311,19 +314,19 @@
      * Remove a relation between two services.
      *
      * @method remove_relation
-     * @param {String} endpoint_a A string `service:interface` representing
+     * @param {Object} endpointA An array of [service, interface] representing
          the first endpoint to disconnect.
-     * @param {String} endpoint_b A string `service:interface` representing
+     * @param {Object} endpointB An array of [service, interface] representing
          the second endpoint to disconnect.
      * @param {Function} callback A callable that must be called once the
          operation is performed.
      * @return {undefined} Sends a message to the server only.
      */
-    remove_relation: function(endpoint_a, endpoint_b, callback) {
+    remove_relation: function(endpointA, endpointB, callback) {
       this._send_rpc({
         'op': 'remove_relation',
-        'endpoint_a': endpoint_a,
-        'endpoint_b': endpoint_b}, callback, true);
+        'endpoint_a': endpointToName(endpointA),
+        'endpoint_b': endpointToName(endpointB)}, callback, true);
     },
 
     /**

=== modified file 'app/views/service.js'
--- app/views/service.js	2013-01-24 14:14:45 +0000
+++ app/views/service.js	2013-01-24 19:23:21 +0000
@@ -445,14 +445,14 @@
               service = this.get('model'),
               relation = db.relations.getById(rel_id),
               endpoints = relation.get('endpoints'),
-              endpoint_a = endpoints[0][0] + ':' + endpoints[0][1].name,
+              endpoint_a = endpoints[0],
               endpoint_b;
 
           if (endpoints.length === 1) {
             // For a peer relationship, both endpoints are the same.
             endpoint_b = endpoint_a;
           } else {
-            endpoint_b = endpoints[1][0] + ':' + endpoints[1][1].name;
+            endpoint_b = endpoints[1];
           }
 
           ev.target.set('disabled', true);

=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js	2013-01-24 19:02:40 +0000
+++ app/views/topology/relation.js	2013-01-24 19:23:21 +0000
@@ -445,19 +445,16 @@
         self.addRelation(); // Will clear the state.
       }
     },
-    removeRelation: function(d, view, confirmButton) {
+    removeRelation: function(relation, view, confirmButton) {
       var env = this.get('component').get('env');
-      var endpoints = d.endpoints;
-      var relationId = d.relation_id;
       // 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('#' + relationId);
+      var relationElement = view.get('container')
+        .one('#' + relation.relation_id);
       utils.addSVGClass(relationElement, 'to-remove pending-relation');
-      env.remove_relation(
-          endpoints[0][0] + ':' + endpoints[0][1].name,
-          endpoints[1][0] + ':' + endpoints[1][1].name,
+      env.remove_relation(relation.endpoints[0], relation.endpoints[1],
           Y.bind(this._removeRelationCallback, this, view,
-          relationElement, relationId, confirmButton));
+          relationElement, relation.relation_id, confirmButton));
     },
 
     _removeRelationCallback: function(view,
@@ -722,9 +719,7 @@
 
       // Fire event to add relation in juju.
       // This needs to specify interface in the future.
-      env.add_relation(
-          endpoints[0][0] + ':' + endpoints[0][1].name,
-          endpoints[1][0] + ':' + endpoints[1][1].name,
+      env.add_relation(endpoints[0], endpoints[1],
           Y.bind(this._addRelationCallback, this, view, relation_id)
       );
       view.set('currentServiceClickAction', 'hideServiceMenu');
@@ -838,8 +833,8 @@
           'active');
     },
 
-    relationClick: function(d, self) {
-      if (d.isSubordinate) {
+    relationClick: function(relation self) {
+      if (relation.isSubordinate) {
         var subRelDialog = views.createModalPanel(
             'You may not remove a subordinate relation.',
             '#rmsubrelation-modal-panel');
@@ -861,7 +856,7 @@
         subRelDialog.get('boundingBox').all('.yui3-button')
                 .removeClass('yui3-button');
       } else {
-        self.removeRelationConfirm(d, this, self);
+        self.removeRelationConfirm(relation, this, self);
       }
     }
 

=== modified file 'test/test_application_notifications.js'
--- test/test_application_notifications.js	2013-01-17 16:19:17 +0000
+++ test/test_application_notifications.js	2013-01-24 19:23:21 +0000
@@ -2,7 +2,7 @@
 
 describe('juju application notifications', function() {
   var _setTimeout, _viewsHighlightRow, db, ERR_EV, juju, models, NO_OP,
-      viewContainer, views, Y;
+      viewContainer, views, Y, willError;
 
   before(function(done) {
     Y = YUI(GlobalConfig).use(['node',
@@ -22,6 +22,18 @@
     ERR_EV = {
       err: true
     };
+
+    /**
+     * A function that accepts a callback as its last (rightmost) argument and
+     * calls that callback with a synthetic error event.
+     */
+    willError = function() {
+      // The last argument is the callback.  We do not care what the
+      // other arguments are or if they change in the future.
+      var callback = arguments[arguments.length - 1];
+      callback(ERR_EV);
+    };
+
     NO_OP = function() {};
   });
 
@@ -91,12 +103,8 @@
          },
          db: db,
          env: {
-           add_unit: function(serviceId, delta, callback) {
-             callback(ERR_EV);
-           },
-           remove_units: function(param, callback) {
-             callback(ERR_EV);
-           }
+           add_unit: willError,
+           remove_units: willError
          },
          model: {
            getAttrs: NO_OP,
@@ -140,9 +148,8 @@
              { err: true,
                unit_names: ['aaa']});
            },
-           resolved: function(unit_name, relation_name, retry, callback) {
-             callback(ERR_EV);
-           }},
+           resolved: willError
+         },
          unit: {},
          querystring: {}
        });
@@ -257,10 +264,7 @@
                }},
               getModelURL: NO_OP,
               db: db,
-              env:
-                  { set_constraints: function(id, values, callback) {
-                    callback(ERR_EV);
-                  }},
+              env: {set_constraints: willError},
               container: viewContainer}).render();
 
         view.updateConstraints();
@@ -274,15 +278,9 @@
         var view = new views.service_config(
        { db: db,
          env: {
-           set_config: function(id, newValues, callback) {
-             callback(ERR_EV);
-           },
-           expose: function(id, callback) {
-             callback(ERR_EV);
-           },
-           unexpose: function(id, callback) {
-             callback({err: true, service_name: '1234'});
-           }
+           set_config: willError,
+           expose: willError,
+           unexpose: willError
          },
          getModelURL: NO_OP,
          model: {
@@ -329,11 +327,7 @@
 
         var view = new views.service_relations(
        { db: db,
-         env: {
-           remove_relation: function(id, newValues, callback) {
-             callback(ERR_EV);
-           }
-         },
+         env: {remove_relation: willError},
          getModelURL: NO_OP,
          container: viewContainer});
 
@@ -388,11 +382,7 @@
            return {get: NO_OP};
          }
        },
-       env = {
-         deploy: function(charmUrl, serviceName, config, callback) {
-           callback(ERR_EV);
-         }
-       },
+       env = {deploy: willError},
        mockView = {
          fire: NO_OP,
          _deployCallback: function() {
@@ -418,7 +408,7 @@
          }
        };
 
-        views.charm.prototype.on_charm_deploy.apply(mockView, [ERR_EV]);
+        views.charm.prototype.on_charm_deploy.call(mockView, ERR_EV);
         assert.isTrue(notified);
       });
 

=== modified file 'test/test_env.js'
--- test/test_env.js	2013-01-24 04:11:52 +0000
+++ test/test_env.js	2013-01-24 19:23:21 +0000
@@ -10,7 +10,7 @@
   var Y;
 
   describe('Juju environment', function() {
-    var juju, conn, env, msg, testUtils;
+    var juju, conn, env, msg, testUtils, endpoint1, endpoint2;
 
     before(function(done) {
       Y = YUI(GlobalConfig).use(
@@ -26,9 +26,13 @@
           });
     });
 
-    after(function(done)  {
+    beforeEach(function() {
+      endpoint1 = ['service1', {name: 'relation-name-1'}];
+      endpoint2 = ['service2', {name: 'relation-name-2'}];
+    });
+
+    after(function() {
       env.destroy();
-      done();
     });
 
     it('can deploy a service', function() {
@@ -217,7 +221,7 @@
     };
 
     it('denies adding a relation if the GUI is read-only', function() {
-      assertOperationDenied('add_relation', ['haproxy:http', 'django:http']);
+      assertOperationDenied('add_relation', [endpoint1, endpoint2]);
     });
 
     it('denies adding a unit if the GUI is read-only', function() {
@@ -242,8 +246,7 @@
     });
 
     it('denies removing a relation if the GUI is read-only', function() {
-      assertOperationDenied(
-          'remove_relation', ['haproxy:http', 'django:http']);
+      assertOperationDenied('remove_relation', [endpoint1, endpoint2]);
     });
 
     it('denies removing units if the GUI is read-only', function() {

=== modified file 'test/test_topology_relation.js'
--- test/test_topology_relation.js	2013-01-23 16:46:40 +0000
+++ test/test_topology_relation.js	2013-01-24 19:23:21 +0000
@@ -39,12 +39,12 @@
 
   it('fires a "clearState" event if a drag line is clicked', function() {
     var firedEventName;
-    var fauxTopo = {
+    var topo = {
       fire: function(eventName) {
         firedEventName = eventName;
       }
     };
-    view.set('component', fauxTopo);
+    view.set('component', topo);
     view.draglineClicked(undefined, view);
     assert.equal(firedEventName, 'clearState');
   });
@@ -57,4 +57,37 @@
     assert.equal(view.render(), view);
   });
 
+  it('retrieves the current relation DOM element when removing', function() {
+    var requestedSelector;
+    var container = {
+      one: function(selector) {
+        requestedSelector = selector;
+      }
+    };
+    var env = {
+      remove_relation: function() {}
+    };
+    var topo = {
+      get: function() {
+        return env;
+      }
+    };
+    var fauxView = {
+      get: function(name) {
+        if (name === 'component') {
+          return topo;
+        } else if (name === 'container') {
+          return container;
+        }
+      }
+    };
+    var relationId = 'the ID of this relation';
+    var relation = {
+      relation_id: relationId,
+      endpoints: [null, null]
+    };
+    view.removeRelation.call(fauxView, relation, fauxView, undefined);
+    assert.equal(requestedSelector, '#' + relationId);
+  });
+
 });