Merge lp:~hatch/juju-gui/peer-status-1255336 into lp:juju-gui/experimental

[Jeff Pihach]

Displays the relation name for peer relations

https://codereview.appspot.com/34120044/

[Jeff Pihach (hatch)]

Reviewers: mp+196964_code.launchpad.net,

Message:
Please take a look.

Description:
Displays the relation name for peer relations

https://code.launchpad.net/~hatch/juju-gui/peer-status-1255336/+merge/196964

(do not edit description out of merge proposal)

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

Affected files (+24, -4 lines):
   A [revision details]
   M app/templates/service-relations-list.handlebars
   M app/views/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: app/templates/service-relations-list.handlebars
=== modified file 'app/templates/service-relations-list.handlebars'
--- app/templates/service-relations-list.handlebars 2013-11-26 18:34:25
+0000
+++ app/templates/service-relations-list.handlebars 2013-11-27 17:14:10
+0000
@@ -4,7 +4,9 @@
  </div>
  {{/unless}}
  {{#relations}}
- <div class="relation-label {{relationStatus
far.service ../errors}}"><h3>{{far.service}}</h3></div>
+ <div class="relation-label {{relationStatus . ../errors}}">
+ <h3>{{relationServiceName .}}</h3>
+ </div>
    <h4>Interface: {{interface}}</h4>
    <h4>Name: {{near.name}}</h4>
    <h4>Role: {{near.role}}</h4>

Index: app/views/utils.js
=== modified file 'app/views/utils.js'
--- app/views/utils.js 2013-11-26 18:34:25 +0000
+++ app/views/utils.js 2013-11-27 17:14:10 +0000
@@ -1686,9 +1686,25 @@
      /*jshint debug:false */
    });

- Y.Handlebars.registerHelper('relationStatus', function(value, errors) {
- if (errors[value]) {
- return 'error';
+ // Returns 'error' if the relation in question is in error. Supports both
+ // normal and peer relations.
+ Y.Handlebars.registerHelper('relationStatus', function(relation, errors)
{
+ var serviceName = '';
+ if (!relation.far) { // It's a peer relation
+ serviceName = relation.near.service;
+ } else {
+ serviceName = relation.far.service;
+ }
+ if (errors[serviceName]) { return 'error'; }
+ });
+
+ // Returns the proper service name to support peer and normal relations
for
+ // the relations tab in the juju-gui.
+ Y.Handlebars.registerHelper('relationServiceName', function(relation) {
+ if (!relation.far) { // It's a peer relation
+ return relation.near.service;
+ } else {
+ return relation.far.service;
      }
    });

[Benji York (benji)]

This looks fine. They do need tests.

LGTM

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

https://codereview.appspot.com/34120044/diff/1/app/views/utils.js#newcode1693
app/views/utils.js:1693: if (!relation.far) { // It's a peer relation
It would be a little clearer if the negation was removed from the
condition and the then/else clauses were swapped.

https://codereview.appspot.com/34120044/diff/1/app/views/utils.js#newcode1707
app/views/utils.js:1707: return relation.far.service;
I'm not too hung up on it, but the above code is duplicated from above.
Maybe there is a nice way to factor both copies into a third place.

https://codereview.appspot.com/34120044/

[Jeff Pihach (hatch)]

Thanks for the review - Changes made and tests will be submitted.

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

https://codereview.appspot.com/34120044/diff/1/app/views/utils.js#newcode1693
app/views/utils.js:1693: if (!relation.far) { // It's a peer relation
On 2013/11/27 19:05:40, benji wrote:
> It would be a little clearer if the negation was removed from the
condition and
> the then/else clauses were swapped.

Done.

https://codereview.appspot.com/34120044/

[Jeff Pihach (hatch)]

*** Submitted:

Displays the relation name for peer relations

R=benji
CC=
https://codereview.appspot.com/34120044

https://codereview.appspot.com/34120044/

=== modified file 'app/templates/service-relations-list.handlebars'
--- app/templates/service-relations-list.handlebars	2013-11-26 18:34:25 +0000
+++ app/templates/service-relations-list.handlebars	2013-11-27 19:01:05 +0000
@@ -4,7 +4,9 @@
 </div>
 {{/unless}}
 {{#relations}}
-  <div class="relation-label {{relationStatus far.service ../errors}}"><h3>{{far.service}}</h3></div>
+  <div class="relation-label {{relationStatus . ../errors}}">
+    <h3>{{relationServiceName .}}</h3>
+  </div>
   <h4>Interface: {{interface}}</h4>
   <h4>Name: {{near.name}}</h4>
   <h4>Role: {{near.role}}</h4>

=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-11-26 18:34:25 +0000
+++ app/views/utils.js	2013-11-27 19:01:05 +0000
@@ -1686,9 +1686,25 @@
     /*jshint debug:false */
   });
 
-  Y.Handlebars.registerHelper('relationStatus', function(value, errors) {
-    if (errors[value]) {
-      return 'error';
+  // Returns 'error' if the relation in question is in error. Supports both
+  // normal and peer relations.
+  Y.Handlebars.registerHelper('relationStatus', function(relation, errors) {
+    var serviceName = '';
+    if (!relation.far) { // It's a peer relation
+      serviceName = relation.near.service;
+    } else {
+      serviceName = relation.far.service;
+    }
+    if (errors[serviceName]) { return 'error'; }
+  });
+
+  // Returns the proper service name to support peer and normal relations for
+  // the relations tab in the juju-gui.
+  Y.Handlebars.registerHelper('relationServiceName', function(relation) {
+    if (!relation.far) { // It's a peer relation
+      return relation.near.service;
+    } else {
+      return relation.far.service;
     }
   });