Merge lp:~hatch/juju-gui/inspector-relation-status into lp:juju-gui/experimental

[Jeff Pihach]

Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a relation
error the entire relation is shown as in error.

https://codereview.appspot.com/33130043/

[Jeff Pihach (hatch)]

Reviewers: mp+196770_code.launchpad.net,

Message:
Please take a look.

Description:
Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a
relation
error the entire relation is shown as in error.

To QA:
Sandbox:
Deploy Mysql & Wordpress with 50 units each
Relate them
See the inspector relations panel, there should be a green icon
Wait for a relation error
See the inspector relations panel, there should be a red icon
Reduce the units on your inspector to 1
See the inspector relations panel, there should be a green icon
   because it's very likely the unit in error is gone

LXC
Deploy two instances of failtester
set ones relation fail setting to true
relate them
the inspector relations panel should show an error
resolve the error
the inspector relations panel should show green

https://code.launchpad.net/~hatch/juju-gui/inspector-relation-status/+merge/196770

(do not edit description out of merge proposal)

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

Affected files (+77, -82 lines):
   A [revision details]
   M app/models/models.js
   M app/templates/service-relations-list.handlebars
   M app/templates/service-relations-viewlet.handlebars
   D app/templates/service-relations.handlebars
   M app/views/utils.js
   M app/views/viewlets/service-relations.js
   M lib/views/juju-inspector.less
   M test/test_model.js

[Jeff Pihach (hatch)]

reviewer notes

https://codereview.appspot.com/33130043/diff/1/app/templates/service-relations.handlebars
File app/templates/service-relations.handlebars (left):

https://codereview.appspot.com/33130043/diff/1/app/templates/service-relations.handlebars#oldcode1
app/templates/service-relations.handlebars:1: {{> service-header}}
Template from old inspector

https://codereview.appspot.com/33130043/

[Jeff Pihach (hatch)]

Please take a look.

https://codereview.appspot.com/33130043/

[lp:~hatch/juju-gui/inspector-relation-status]

1214. By Jeff Pihach on 2013-11-26

fixed issue where the first error status delta would get ignored for relation errors

[Jeff Pihach (hatch)]

reviewer notes v2

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode605
app/models/models.js:605: if (state === 'error') {
Fix for bug from first rev (see test_model)

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
The order of these is important to make sure that the first unit error,
if it's a relation error gets caught properly (bug from first rev)

https://codereview.appspot.com/33130043/

[Gary Poster (gary)]

LGTM with trivials and sandbox QA good. waiting on QA on ec2.

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode591
app/models/models.js:591: @return {Array} An array of the aggregate map
and relation errors
Thank you for improving names and docs. Nice.

https://codereview.appspot.com/33130043/diff/20001/app/models/models.js#newcode614
app/models/models.js:614: if (farService) {
this code fixes peer relation issues?

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
On 2013/11/26 20:42:45, jeff.pihach wrote:
> The order of these is important to make sure that the first unit
error, if it's
> a relation error gets caught properly (bug from first rev)

In that case could you elaborate in a code comment what's going on? All
I see in the code is "one of these has a relation error and one has an
install error," not something subtle about ordering.

https://codereview.appspot.com/33130043/

[Gary Poster (gary)]

QA OK on real environment also. Thanks! These are really cool changes
you are making.

https://codereview.appspot.com/33130043/

[Jeff Pihach (hatch)]

Thanks for the review! Changes landing

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js
File test/test_model.js (right):

https://codereview.appspot.com/33130043/diff/20001/test/test_model.js#newcode136
test/test_model.js:136: id: 'wordpress/2',
good call, done

https://codereview.appspot.com/33130043/

[Jeff Pihach (hatch)]

*** Submitted:

Adds relation status to inspector relation panel

The overall relation status is now indicated on the inspector relation
panel using the same icons as the unit status. If any unit has a
relation
error the entire relation is shown as in error.

R=gary.poster
CC=
https://codereview.appspot.com/33130043

https://codereview.appspot.com/33130043/

=== modified file 'app/models/models.js'
--- app/models/models.js	2013-11-21 23:21:49 +0000
+++ app/models/models.js	2013-11-26 20:42:30 +0000
@@ -186,7 +186,7 @@
       this._events.push(
           this.get('relations').on(
               ['*:change', '*:add', '*:remove'], function(e) {
-                this.set('aggregateRelations', e);
+                this.set('relationChangeTrigger', e);
               }, this));
     },
 
@@ -340,14 +340,16 @@
       relations: {},
 
       /**
-        An aggregate of the relations statuses that we use to trigger
-        databinding changes
+        When there is a change to the relation or to any units in the relation
+        this value is changed to trigger changes in the inspector.
 
-        @attribute aggregateRelations
+        @attribute relationChangeTrigger
         @default {}
         @type {Object}
       */
-      aggregateRelations: {},
+      relationChangeTrigger: {
+        value: {}
+      },
 
       /**
         An aggregate of the relation errors that we use to trigger
@@ -580,11 +582,14 @@
       return result;
     },
 
-    /*
-     *  Return information about the state of the set of units for a
-     *  given service in the form of a map of agent states:
-     *  state => number of units in that state
-     */
+    /**
+      Returns information about the state of the set of units for a given
+      service in the form of a map of agent states.
+
+      @method get_informative_states_for_service
+      @param {Object} service The service model.
+      @return {Array} An array of the aggregate map and relation errors
+    */
     get_informative_states_for_service: function(service) {
       var aggregate_map = {},
           relationError = {},
@@ -596,22 +601,24 @@
           aggregate_map[state] = 1;
         } else {
           aggregate_map[state] += 1;
-          if (state === 'error') {
-            // If in error status then we need to parse out why it's in error.
-            var info = unit.agent_state_info;
-            if (info !== undefined && info.indexOf('failed') > -1) {
-              // If we parse more than the relation info then split this out
-              if (info.indexOf('relation') > -1) {
-                var hook = info.split(':')[1].split('-'),
-                    interfaceName = hook.slice(0, hook.length - 2)[0].trim();
-                relationError[unit.service] = interfaceName;
+        }
+        if (state === 'error') {
+          // If in error status then we need to parse out why it's in error.
+          var info = unit.agent_state_info;
+          if (info !== undefined && info.indexOf('failed') > -1) {
+            // If we parse more than the relation info then split this out
+            if (info.indexOf('relation') > -1) {
+              var stateData = unit.agent_state_data;
+              if (stateData) {
+                var farService = stateData['remote-unit'].split('/')[0];
+                if (farService) {
+                  relationError[farService] = stateData.hook;
+                }
               }
             }
           }
         }
-
       });
-
       return [aggregate_map, relationError];
     },
 
@@ -626,7 +633,9 @@
       var previous_unit_count = service.get('unit_count');
       service.set('unit_count', sum);
       service.set('aggregated_status', aggregate[0]);
-      service.set('aggregateRelationError', aggregate[1]);
+
+      service.set('relationChangeTrigger', { error: aggregate[1] });
+
       // Set Google Analytics tracking event.
       if (previous_unit_count !== sum && window._gaq) {
         window._gaq.push(['_trackEvent', 'Service Stats', 'Update',

=== modified file 'app/templates/service-relations-list.handlebars'
--- app/templates/service-relations-list.handlebars	2013-09-19 20:11:53 +0000
+++ app/templates/service-relations-list.handlebars	2013-11-26 20:42:30 +0000
@@ -4,7 +4,7 @@
 </div>
 {{/unless}}
 {{#relations}}
-  <h3>{{far.service}}</h3>
+  <div class="relation-label {{relationStatus far.service ../errors}}"><h3>{{far.service}}</h3></div>
   <h4>Interface: {{interface}}</h4>
   <h4>Name: {{near.name}}</h4>
   <h4>Role: {{near.role}}</h4>

=== modified file 'app/templates/service-relations-viewlet.handlebars'
--- app/templates/service-relations-viewlet.handlebars	2013-09-16 20:09:02 +0000
+++ app/templates/service-relations-viewlet.handlebars	2013-11-26 20:42:30 +0000
@@ -1,4 +1,4 @@
 <div class="view-container settings-config">
   <h2>Relations</h2>
-  <div data-bind="aggregateRelations"></div>
+  <div data-bind="relationChangeTrigger"></div>
 </div>

=== removed file 'app/templates/service-relations.handlebars'
--- app/templates/service-relations.handlebars	2013-04-11 16:16:12 +0000
+++ app/templates/service-relations.handlebars	1970-01-01 00:00:00 +0000
@@ -1,40 +0,0 @@
-{{> service-header}}
-<div class="view-container">
-    <div>
-      <h5>Service Relations</h5>
-      <form id="service-relations">
-        <table class="table table-striped table-bordered">
-         <thead>
-           <tr>
-            <th>Id</th>
-            <th>Role</th>
-            <th>Remote</th>
-            <th>Scope</th>
-         </thead>
-
-         <tbody>
-           {{#relations}}
-           <tr {{#if highlight}}class="highlighted"{{/if}}>
-              <td>
-                {{ident}}
-                <button type="submit" class="btn" id="{{elementId}}" value="{{relation_id}}">
-                    Remove
-                </button>
-              </td>
-              <td>{{near.role}}</td>
-              <td>
-                {{#if far}}
-                  <a href="{{../../serviceRemoteUri}}{{far.service}}/">{{far.service}}</a>
-                {{/if}}
-              </td>
-              <td>{{scope}}</td>
-           </tr>
-           {{/relations}}
-         </tbody>
-        </table>
-      </form>
-    </div>
-</div>
-<div id="remove-modal-panel"></div>
-{{> service-footer}}
-

=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-11-21 22:42:48 +0000
+++ app/views/utils.js	2013-11-26 20:42:30 +0000
@@ -1686,6 +1686,12 @@
     /*jshint debug:false */
   });
 
+  Y.Handlebars.registerHelper('relationStatus', function(value, errors) {
+    if (errors[value]) {
+      return 'error';
+    }
+  });
+
   /*
    * Extension for views to provide an apiFailure method.
    *

=== modified file 'app/views/viewlets/service-relations.js'
--- app/views/viewlets/service-relations.js	2013-09-10 14:53:01 +0000
+++ app/views/viewlets/service-relations.js	2013-11-26 20:42:30 +0000
@@ -33,20 +33,16 @@
     template: templates['service-relations-viewlet'],
 
     bindings: {
-      aggregateRelations: {
+      relationChangeTrigger: {
         'update': function(node, value) {
           var db = this.viewlet.options.db;
           var service = this.viewlet.model;
           var relations = utils.getRelationDataForService(db, service);
           node.setHTML(templates['service-relations-list']({
-            relations: relations
+            relations: relations,
+            errors: value && value.error
           }));
         }
-      },
-      aggregateRelationError: {
-        'update': function(node, value) {
-          // Aggregate the unit statuses here to display the relation status
-        }
       }
     }
   };

=== modified file 'lib/views/juju-inspector.less'
--- lib/views/juju-inspector.less	2013-11-25 21:18:53 +0000
+++ lib/views/juju-inspector.less	2013-11-26 20:42:30 +0000
@@ -776,6 +776,17 @@
                     .transition(none);
                 }
             }
+
+            .relation-label {
+                padding-left: 10px;
+                background-repeat: no-repeat;
+                background-position: 10px 17px;
+                background-image: url("/juju-ui/assets/images/inspector-charm-running.png");
+
+                &.error {
+                    background-image: url("/juju-ui/assets/images/inspector-charm-error.png");
+                }
+            }
         }
         .status {
             display: inline-block;

=== modified file 'test/test_model.js'
--- test/test_model.js	2013-11-05 18:10:05 +0000
+++ test/test_model.js	2013-11-26 20:42:30 +0000
@@ -133,16 +133,26 @@
         id: 'wordpress/0',
         agent_state: 'pending'});
       var wp1 = new models.ServiceUnit({
+        id: 'wordpress/2',
+        agent_state: 'error',
+        agent_state_info: 'hook failed: "db-relation-changed"',
+        agent_state_data: {
+          hook: 'db-relation-changed',
+          'relation-id': 1,
+          'remote-unit': 'mysql/0'
+        }});
+      var wp2 = new models.ServiceUnit({
         id: 'wordpress/1',
-        agent_state: 'error'});
-      wordpress.get('units').add([wp0, wp1]);
+        agent_state: 'error',
+        agent_state_info: 'hook failed: "install"'});
+      wordpress.get('units').add([wp0, wp1, wp2]);
 
       assert.deepEqual(mysql.get('units')
                        .get_informative_states_for_service(mysql),
           [{'pending': 2}, {}]);
       assert.deepEqual(wordpress.get('units')
                        .get_informative_states_for_service(wordpress),
-          [{'pending': 1, 'error': 1}, {}]);
+          [{'pending': 1, 'error': 2}, { mysql: 'db-relation-changed'}]);
     });
 
     it('service unit list should update analytics when units are added',
@@ -178,20 +188,22 @@
                    models.RelationList, true);
     });
 
-    it('relation changes on service update aggregateRelations', function(done) {
-      var service = new models.Service();
-      var relations = service.get('relations');
-      var handler = relations.on(
-          '*:add', function() {
-            // This means that it will update the aggregate
-            // relations for databinding
-            handler.detach();
-            var isObject = yui.Lang.isObject;
-            assert.equal(isObject(service.get('aggregateRelations')), true);
-            done();
-          });
-      relations.add(new models.Relation());
-    });
+    it('relation changes on service update relationChangeTrigger',
+        function(done) {
+          var service = new models.Service();
+          var relations = service.get('relations');
+          var handler = relations.on(
+              '*:add', function() {
+                // This means that it will update the aggregate
+                // relations for databinding
+                handler.detach();
+                var isObject = yui.Lang.isObject;
+                assert.equal(
+                    isObject(service.get('relationChangeTrigger')), true);
+                done();
+              });
+          relations.add(new models.Relation());
+        });
 
     it('service unit objects should parse the service name from unit id',
        function() {