Merge lp:~makyo/juju-gui/sub-rel-counter-1107999 into lp:juju-gui/experimental

[Madison Scott-Clary]

Correctly count subordinate relations

Subordinate relations were not being properly counted with the new comparison when it came to new services or relations. This was due to object inequality after a component's update method was called. Comparison switched to service modelIds, and test added.

https://codereview.appspot.com/7234048/

[Madison Scott-Clary (makyo)]

Reviewers: mp+145217_code.launchpad.net,

Message:
Please take a look.

Description:
Correctly count subordinate relations

Subordinate relations were not being properly counted with the new
comparison when it came to new services or relations. This was due to
object inequality after a component's update method was called.
Comparison switched to service modelIds, and test added.

https://code.launchpad.net/~makyo/juju-gui/sub-rel-counter-1107999/+merge/145217

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/views/topology/relation.js
   M test/test_environment_view.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-01-25 12:05:33 +0000
+++ test/test_environment_view.js 2013-01-28 18:14:07 +0000
@@ -193,7 +193,7 @@
          db: db,
          env: env
        });
- var tmp_data = {
+ var addSubordinate = {
          result: [
            ['service', 'add', {
              'subordinate': true,
@@ -206,7 +206,20 @@
              'endpoints':
               [['wordpress', {'role': 'server', 'name': 'juju-info'}],
                ['puppet2', {'role': 'client', 'name': 'juju-info'}]],
- 'id': 'relation-0000000007'
+ 'id': 'new-relation-0000000008'
+ }]
+ ],
+ op: 'delta'
+ };
+ var addRelation = {
+ result: [
+ ['relation', 'add', {
+ 'interface': 'juju-info',
+ 'scope': 'container',
+ 'endpoints':
+ [['mediawiki', {'role': 'server', 'name': 'juju-info'}],
+ ['puppet', {'role': 'client', 'name': 'juju-info'}]],
+ 'id': 'new-relation-0000000009'
            }]
          ],
          op: 'delta'
@@ -216,21 +229,28 @@

        var relationModule = view.topo.modules.RelationModule;

- function validateRelationCount(serviceNode, module) {
+ function validateRelationCount(serviceNode, module, count) {
          var service = d3.select(serviceNode.getDOMNode()).datum();
- return module.subordinateRelationsForService(service).length === 1;
+ return module.subordinateRelationsForService(service)
+ .length === count;
        }

        container.all('.subordinate.service').each(function(service) {
- validateRelationCount(service, relationModule).should.equal(true);
+ validateRelationCount(service, relationModule,
1).should.equal(true);
        });

- db.on_delta({ data: tmp_data });
- view.render();
+ db.on_delta({ data: addSubordinate });
+ view.update();

        container.all('.subordinate.service').each(function(service) {
- validateRelationCount(service, relationModule).should.equal(true);
+ validateRelationCount...

[Brad Crittenden (bac)]

Land as is. Thanks.

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

https://codereview.appspot.com/7234048/diff/1/app/views/topology/relation.js#newcode257
app/views/topology/relation.js:257:
vis.selectAll('.service.subordinate')
Hurrah for simplicity

https://codereview.appspot.com/7234048/

[Nicola Larosa (teknico)]

Land as is.

Very nice, thanks.

https://codereview.appspot.com/7234048/

[Madison Scott-Clary (makyo)]

*** Submitted:

Correctly count subordinate relations

Subordinate relations were not being properly counted with the new
comparison when it came to new services or relations. This was due to
object inequality after a component's update method was called.
Comparison switched to service modelIds, and test added.

R=bac, teknico
CC=
https://codereview.appspot.com/7234048

https://codereview.appspot.com/7234048/

=== modified file 'app/views/topology/relation.js'
--- app/views/topology/relation.js	2013-01-24 19:29:08 +0000
+++ app/views/topology/relation.js	2013-01-28 18:20:25 +0000
@@ -254,10 +254,7 @@
       var vis = topo.vis;
       var self = this;
 
-      vis.selectAll('.service')
-        .filter(function(d) {
-            return d.subordinate;
-          })
+      vis.selectAll('.service.subordinate')
         .selectAll('.sub-rel-block tspan')
         .text(function(d) {
             return self.subordinateRelationsForService(d).length;
@@ -771,7 +768,8 @@
      */
     subordinateRelationsForService: function(service) {
       return this.relations.filter(function(relation) {
-        return (relation.source === service || relation.target === service) &&
+        return (relation.source.modelId() === service.modelId() ||
+            relation.target.modelId() === service.modelId()) &&
             relation.isSubordinate;
       });
     },

=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2013-01-25 12:05:33 +0000
+++ test/test_environment_view.js	2013-01-28 18:20:25 +0000
@@ -193,7 +193,7 @@
         db: db,
         env: env
       });
-      var tmp_data = {
+      var addSubordinate = {
         result: [
           ['service', 'add', {
             'subordinate': true,
@@ -206,7 +206,20 @@
             'endpoints':
              [['wordpress', {'role': 'server', 'name': 'juju-info'}],
               ['puppet2', {'role': 'client', 'name': 'juju-info'}]],
-            'id': 'relation-0000000007'
+            'id': 'new-relation-0000000008'
+          }]
+        ],
+        op: 'delta'
+      };
+      var addRelation = {
+        result: [
+          ['relation', 'add', {
+            'interface': 'juju-info',
+            'scope': 'container',
+            'endpoints':
+             [['mediawiki', {'role': 'server', 'name': 'juju-info'}],
+              ['puppet', {'role': 'client', 'name': 'juju-info'}]],
+            'id': 'new-relation-0000000009'
           }]
         ],
         op: 'delta'
@@ -216,21 +229,28 @@
 
       var relationModule = view.topo.modules.RelationModule;
 
-      function validateRelationCount(serviceNode, module) {
+      function validateRelationCount(serviceNode, module, count) {
         var service = d3.select(serviceNode.getDOMNode()).datum();
-        return module.subordinateRelationsForService(service).length === 1;
+        return module.subordinateRelationsForService(service)
+          .length === count;
       }
 
       container.all('.subordinate.service').each(function(service) {
-        validateRelationCount(service, relationModule).should.equal(true);
+        validateRelationCount(service, relationModule, 1).should.equal(true);
       });
 
-      db.on_delta({ data: tmp_data });
-      view.render();
+      db.on_delta({ data: addSubordinate });
+      view.update();
 
       container.all('.subordinate.service').each(function(service) {
-        validateRelationCount(service, relationModule).should.equal(true);
+        validateRelationCount(service, relationModule, 1).should.equal(true);
       });
+
+      db.on_delta({ data: addRelation });
+      view.update();
+
+      validateRelationCount(container.one('.subordinate.service'),
+          relationModule, 2).should.equal(true);
     });
 
     it('must not duplicate nodes when services are added', function() {
@@ -238,27 +258,27 @@
         container: container,
         db: db,
         env: env
-      }),
-          tmp_data = {
-            result: [
-              ['service', 'add', {
-                'subordinate': true,
-                'charm': 'cs:precise/puppet-2',
-                'id': 'puppet2'
-              }],
-              ['service', 'add', {
-                'charm': 'cs:precise/mysql-6',
-                'id': 'mysql2'
-              }],
-              ['unit', 'add', {
-                'machine': 0,
-                'agent-state': 'started',
-                'public-address': '192.168.122.222',
-                'id': 'mysql2/0'
-              }]
-            ],
-            op: 'delta'
-          };
+      });
+      var tmp_data = {
+        result: [
+          ['service', 'add', {
+            'subordinate': true,
+            'charm': 'cs:precise/puppet-2',
+            'id': 'puppet2'
+          }],
+          ['service', 'add', {
+            'charm': 'cs:precise/mysql-6',
+            'id': 'mysql2'
+          }],
+          ['unit', 'add', {
+            'machine': 0,
+            'agent-state': 'started',
+            'public-address': '192.168.122.222',
+            'id': 'mysql2/0'
+          }]
+        ],
+        op: 'delta'
+      };
       view.render();
 
       db.on_delta({ data: tmp_data });