Merge lp:~frankban/juju-gui/inspector-who into lp:juju-gui/experimental

[Francesco Banconi]

Fix links in the unit details view.

Handle the juju-core "port/protocol" ports.
Privilege the HTTPS port if applicable.

Also fixed the z-index of the charm browser tab,
so that when the sidebar and the unit details view
appear together in a narrow browser it is still
possible to close the sidebar.

QA:
- deploy the GUI with this branch in a juju-core env:
  juju bootstrap --debug
  juju deploy juju-gui
  juju set juju-gui juju-gui-source=lp:~frankban/juju-gui/inspector-who
- wait for the unit to be ready;
- visit the juju-gui unit detail view;

You should be able to close the sidebar view.
The GUI address links to the right URL (https://).
The ports link to the right URLs without the trailing "/tcp".

https://codereview.appspot.com/14439054/

[Francesco Banconi (frankban)]

Reviewers: mp+190379_code.launchpad.net,

Message:
Please take a look.

Description:
Fix links in the unit details view.

Handle the juju-core "port/protocol" ports.
Privilege the HTTPS port if applicable.

Also fixed the z-index of the charm browser tab,
so that when the sidebar and the unit details view
appear together in a narrow browser it is still
possible to close the sidebar.

QA:
- deploy the GUI with this branch in a juju-core env:
   juju bootstrap --debug
   juju deploy juju-gui
   juju set juju-gui juju-gui-source=lp:~frankban/juju-gui/inspector-who
- wait for the unit to be ready;
- visit the juju-gui unit detail view;

You should be able to close the sidebar view.
The GUI address links to the right URL (https://).
The ports link to the right URLs without the trailing "/tcp".

https://code.launchpad.net/~frankban/juju-gui/inspector-who/+merge/190379

(do not edit description out of merge proposal)

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

Affected files (+249, -41 lines):
   A [revision details]
   M app/views/utils.js
   M app/views/viewlets/unit-details.js
   M lib/views/browser/browser-icon.less
   M test/test_unit_detail_viewlet.js
   M test/test_utils.js

[Jeff Pihach (hatch)]

Thanks for this - great tests!
QA'd on local provider - all looks good

https://codereview.appspot.com/14439054/

[Jeff Pihach (hatch)]

oops, LGTM

https://codereview.appspot.com/14439054/

[Francesco Banconi (frankban)]

*** Submitted:

Fix links in the unit details view.

Handle the juju-core "port/protocol" ports.
Privilege the HTTPS port if applicable.

Also fixed the z-index of the charm browser tab,
so that when the sidebar and the unit details view
appear together in a narrow browser it is still
possible to close the sidebar.

QA:
- deploy the GUI with this branch in a juju-core env:
   juju bootstrap --debug
   juju deploy juju-gui
   juju set juju-gui juju-gui-source=lp:~frankban/juju-gui/inspector-who
- wait for the unit to be ready;
- visit the juju-gui unit detail view;

You should be able to close the sidebar view.
The GUI address links to the right URL (https://).
The ports link to the right URLs without the trailing "/tcp".

R=jeff.pihach
CC=
https://codereview.appspot.com/14439054

https://codereview.appspot.com/14439054/

[Francesco Banconi (frankban)]

Thanks for the review and QA Jeff!

https://codereview.appspot.com/14439054/

=== modified file 'app/views/utils.js'
--- app/views/utils.js	2013-10-10 04:28:39 +0000
+++ app/views/utils.js	2013-10-10 14:30:29 +0000
@@ -1365,6 +1365,65 @@
     var charmUrl = candidate.charm || candidate.get('charm');
     return utils.isGuiCharmUrl(charmUrl);
   };
+  /**
+   * Normalize the list of open ports on the unit.
+   * The list of open ports can be received in different formats based on the
+   * Juju implementations. In essence, while pyJuju sends e.g. [80, 42],
+   * juju-core includes the protocol information, e.g. ['80/tcp', '47/udp'].
+   *
+   * @method normalizeUnitPorts
+   * @param {Array} ports The list of open ports on a unit.
+   * @return {Array} A list of objects each one with the following properties:
+   *   - port (int): the port number;
+   *   - protocol (string): 'tcp' or 'udp'.
+   */
+  utils.normalizeUnitPorts = function(ports) {
+    if (!ports) {
+      return [];
+    }
+    return Y.Array.map(ports, function(port) {
+      var splitted = port.toString().split('/');
+      return {port: parseInt(splitted[0], 10), protocol: splitted[1] || 'tcp'};
+    });
+  };
+
+  /**
+   * Parse a list of normalized open ports.
+   *
+   * @method parseUnitPorts
+   * @param {String} ipAddress The unit IP address.
+   * @param {Array} normalizedPorts A list of normalized unit ports
+   *   (see utils.normalizeUnitPorts above).
+   * @return {Array} An array of two elements. The first element is a data
+   *   object describing the IP address. The second element is a list of data
+       objects describing each open port. A data object is an object with the
+       following properties:
+   *   - text (string): the text to show in the template;
+   *   - href (string, optional): the URL where the text can link to (
+         if applicable).
+   */
+  utils.parseUnitPorts = function(ipAddress, normalizedPorts) {
+    var httpHref, httpsHref;
+    var ipAddressData = {text: ipAddress};
+    var portDataList = [];
+    normalizedPorts.forEach(function(normalizedPort) {
+      var port = normalizedPort.port;
+      var protocol = normalizedPort.protocol;
+      var portData = {text: port + '/' + protocol};
+      if (protocol === 'tcp') {
+        if (port === 443) {
+          portData.href = httpsHref = 'https://' + ipAddress + '/';
+        } else if (port === 80) {
+          portData.href = httpHref = 'http://' + ipAddress + '/';
+        } else {
+          portData.href = 'http://' + ipAddress + ':' + port + '/';
+        }
+      }
+      portDataList.push(portData);
+    });
+    ipAddressData.href = httpsHref || httpHref;
+    return [ipAddressData, portDataList];
+  };
 
   Y.Handlebars.registerHelper('unitState', function(relation_errors,
       agent_state) {

=== modified file 'app/views/viewlets/unit-details.js'
--- app/views/viewlets/unit-details.js	2013-09-24 20:27:28 +0000
+++ app/views/viewlets/unit-details.js	2013-10-10 14:30:29 +0000
@@ -31,50 +31,50 @@
 
     @method updateAddress
     @param {Y.Node} node The node to be updated.
-    @param {String} value The IP address.
-    @param {Array} value An array of the ports exposed.
+    @param {String} ipAddress The IP address.
+    @param {Array} openPorts An array of the ports exposed.
     @return {undefined} Mutates the node.
     */
-  var updateAddress = function(node, value, open_ports) {
+  var updateAddress = function(node, ipAddress, openPorts) {
     node.empty();
-    if (!value) { return; }
-    var protocol;
-    var protocols = {443: 'https', 80: 'http'};
-    if (open_ports && open_ports.length) {
-      Object.keys(protocols).some(function(port) {
-        if (open_ports.indexOf(port) >= 0 ||
-            open_ports.indexOf(parseInt(port, 10)) >= 0) {
-          protocol = protocols[port];
-          return true; // Short-circuits the loop.
-        }
-      });
+    if (!ipAddress) {
+      return;
     }
-    var infoNode;
-    if (protocol) {
-      infoNode = Y.Node.create('<a></a>').setAttrs({
-        href: protocol + '://' + value + '/',
+    var ports = utils.normalizeUnitPorts(openPorts);
+    var parsed = utils.parseUnitPorts(ipAddress, ports);
+    var ipAddressData = parsed[0];
+    var portDataList = parsed[1];
+    // Render the IP address fragment.
+    var ipAddressNode;
+    if (ipAddressData.href) {
+      ipAddressNode = Y.Node.create('<a></a>').setAttrs({
+        href: ipAddressData.href,
         target: '_blank',
-        text: value
+        text: ipAddressData.text
       });
     } else {
-      infoNode = Y.Node.create(value);
+      ipAddressNode = Y.Node.create(ipAddressData.text);
     }
-    node.append(infoNode);
-    if (open_ports && open_ports.length) {
+    node.append(ipAddressNode);
+    // Render the ports fragment.
+    if (portDataList.length) {
       // YUI 3 trims HTML because IE (<10?) does it. :-/
       node.append('&nbsp;| Ports&nbsp;');
-      var previous;
-      open_ports.forEach(function(port) {
-        if (previous) {
+      Y.Array.each(portDataList, function(portData, index) {
+        if (index) {
           node.append(',&nbsp;');
         }
-        previous = port;
-        var protocol = protocols[port.toString()] || 'http';
-        node.append(Y.Node.create('<a></a>').setAttrs({
-          href: protocol + '://' + value + ':' + port + '/',
-          target: '_blank',
-          text: port
-        }));
+        var portNode;
+        if (portData.href) {
+          portNode = Y.Node.create('<a></a>').setAttrs({
+            href: portData.href,
+            target: '_blank',
+            text: portData.text
+          });
+        } else {
+          portNode = Y.Node.create(portData.text);
+        }
+        node.append(portNode);
       });
     }
   };

=== modified file 'lib/views/browser/browser-icon.less'
--- lib/views/browser/browser-icon.less	2013-10-01 14:35:09 +0000
+++ lib/views/browser/browser-icon.less	2013-10-10 14:30:29 +0000
@@ -10,7 +10,7 @@
     box-shadow: 3px 0 2px rgba(0, 0, 0, 0.1);
     border-top-right-radius: 4px;
     border-bottom-right-radius: 4px;
-    z-index: 600;
+    z-index: 610;
 }
 .content-visible .bws-icon {
     left: @bws-sidebar-width + @bws-panel-width;

=== modified file 'test/test_unit_detail_viewlet.js'
--- test/test_unit_detail_viewlet.js	2013-09-27 16:26:18 +0000
+++ test/test_unit_detail_viewlet.js	2013-10-10 14:30:29 +0000
@@ -84,18 +84,37 @@
       var node = Y.Node.create('<span>Delete me</span>');
       updateAddress(node, '10.0.0.1', [8080]);
       assert.strictEqual(
-          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 8080');
-      assert.strictEqual(node.all('a').size(), 1);
-      assert.strictEqual(node.one('a').get('href'), 'http://10.0.0.1:8080/');
-      assert.strictEqual(node.one('a').get('target'), '_blank');
-      assert.strictEqual(node.one('a').get('text'), '8080');
+          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 8080/tcp');
+      assert.strictEqual(node.all('a').size(), 1);
+      assert.strictEqual(node.one('a').get('href'), 'http://10.0.0.1:8080/');
+      assert.strictEqual(node.one('a').get('target'), '_blank');
+      assert.strictEqual(node.one('a').get('text'), '8080/tcp');
+    });
+
+    it('mutates a node with an address and a port/tcp pair', function() {
+      var node = Y.Node.create('<span>Delete me</span>');
+      updateAddress(node, '10.0.0.1', ['8080/tcp']);
+      assert.strictEqual(
+          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 8080/tcp');
+      assert.strictEqual(node.all('a').size(), 1);
+      assert.strictEqual(node.one('a').get('href'), 'http://10.0.0.1:8080/');
+      assert.strictEqual(node.one('a').get('target'), '_blank');
+      assert.strictEqual(node.one('a').get('text'), '8080/tcp');
+    });
+
+    it('mutates a node with an address and a port/udp pair', function() {
+      var node = Y.Node.create('<span>Delete me</span>');
+      updateAddress(node, '10.0.0.1', ['8080/udp']);
+      assert.strictEqual(
+          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 8080/udp');
+      assert.strictEqual(node.all('a').size(), 0);
     });
 
     it('mutates a node with an address and http port', function() {
       var node = Y.Node.create('<span>Delete me</span>');
       updateAddress(node, '10.0.0.1', [80]);
       assert.strictEqual(
-          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 80');
+          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 80/tcp');
       assert.strictEqual(node.all('a').size(), 2);
       assert.strictEqual(node.one('a').get('href'), 'http://10.0.0.1/');
       assert.strictEqual(node.one('a').get('target'), '_blank');
@@ -106,7 +125,7 @@
       var node = Y.Node.create('<span>Delete me</span>');
       updateAddress(node, '10.0.0.1', [443]);
       assert.strictEqual(
-          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 443');
+          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 443/tcp');
       assert.strictEqual(node.all('a').size(), 2);
       assert.strictEqual(node.one('a').get('href'), 'https://10.0.0.1/');
       assert.strictEqual(node.one('a').get('target'), '_blank');
@@ -117,13 +136,26 @@
       var node = Y.Node.create('<span>Delete me</span>');
       updateAddress(node, '10.0.0.1', [443, 8080]);
       assert.strictEqual(
-          node.get('text').replace(/\s/g, ' '), '10.0.0.1 | Ports 443, 8080');
+          node.get('text').replace(/\s/g, ' '),
+          '10.0.0.1 | Ports 443/tcp, 8080/tcp');
       assert.strictEqual(node.all('a').size(), 3);
       assert.strictEqual(node.one('a').get('href'), 'https://10.0.0.1/');
       assert.strictEqual(node.one('a').get('target'), '_blank');
       assert.strictEqual(node.one('a').get('text'), '10.0.0.1');
     });
 
+    it('mutates a node with multiple port/protocol pairs', function() {
+      var node = Y.Node.create('<span>Delete me</span>');
+      updateAddress(node, '10.0.0.1', ['42/tcp', '47/udp']);
+      assert.strictEqual(
+          node.get('text').replace(/\s/g, ' '),
+          '10.0.0.1 | Ports 42/tcp, 47/udp');
+      assert.strictEqual(node.all('a').size(), 1);
+      assert.strictEqual(node.one('a').get('href'), 'http://10.0.0.1:42/');
+      assert.strictEqual(node.one('a').get('target'), '_blank');
+      assert.strictEqual(node.one('a').get('text'), '42/tcp');
+    });
+
     it('instantiates correctly when bound', function() {
       db.environment.set('annotations', {
         'landscape-url': 'http://landscape.example.com',

=== modified file 'test/test_utils.js'
--- test/test_utils.js	2013-10-10 04:28:39 +0000
+++ test/test_utils.js	2013-10-10 14:30:29 +0000
@@ -373,6 +373,121 @@
 
   });
 
+  describe('normalizeUnitPorts', function() {
+    var normalizeUnitPorts;
+
+    before(function() {
+      normalizeUnitPorts = utils.normalizeUnitPorts;
+    });
+
+    it('normalizes juju-core ports', function() {
+      var expected = [
+        {port: 80, protocol: 'tcp'},
+        {port: 42, protocol: 'udp'}
+      ];
+      var obtained = normalizeUnitPorts(['80/tcp', '42/udp']);
+      assert.deepEqual(obtained, expected);
+    });
+
+    it('normalizes pyJuju ports', function() {
+      var expected = [
+        {port: 80, protocol: 'tcp'},
+        {port: 443, protocol: 'tcp'},
+        {port: 8080, protocol: 'tcp'}
+      ];
+      var obtained = normalizeUnitPorts([80, '443', 8080]);
+      assert.deepEqual(obtained, expected);
+    });
+
+    it('returns an empty list if no ports are passed', function() {
+      assert.deepEqual(normalizeUnitPorts([]), []);
+      assert.deepEqual(normalizeUnitPorts(undefined), []);
+    });
+
+  });
+
+  describe('parseUnitPorts', function() {
+    var parseUnitPorts;
+
+    before(function() {
+      parseUnitPorts = utils.parseUnitPorts;
+    });
+
+    it('parses generic ports', function() {
+      var expected = [
+        {text: '10.0.3.1', href: undefined},
+        [
+          {text: '42/tcp', href: 'http://10.0.3.1:42/'},
+          {text: '47/tcp', href: 'http://10.0.3.1:47/'}
+        ]
+      ];
+      var ports = [{port: 42, protocol: 'tcp'}, {port: 47, protocol: 'tcp'}];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', ports), expected);
+    });
+
+    it('parses the HTTP port', function() {
+      var expected = [
+        {text: '10.0.3.1', href: 'http://10.0.3.1/'},
+        [
+          {text: '80/tcp', href: 'http://10.0.3.1/'},
+          {text: '47/tcp', href: 'http://10.0.3.1:47/'}
+        ]
+      ];
+      var ports = [{port: 80, protocol: 'tcp'}, {port: 47, protocol: 'tcp'}];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', ports), expected);
+    });
+
+    it('parses the HTTPS port', function() {
+      var expected = [
+        {text: '10.0.3.1', href: 'https://10.0.3.1/'},
+        [
+          {text: '42/tcp', href: 'http://10.0.3.1:42/'},
+          {text: '443/tcp', href: 'https://10.0.3.1/'}
+        ]
+      ];
+      var ports = [{port: 42, protocol: 'tcp'}, {port: 443, protocol: 'tcp'}];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', ports), expected);
+    });
+
+    it('privileges the HTTPS port', function() {
+      var expected = [
+        {text: '10.0.3.1', href: 'https://10.0.3.1/'},
+        [
+          {text: '42/tcp', href: 'http://10.0.3.1:42/'},
+          {text: '443/tcp', href: 'https://10.0.3.1/'},
+          {text: '80/tcp', href: 'http://10.0.3.1/'}
+        ]
+      ];
+      var ports = [
+        {port: 42, protocol: 'tcp'},
+        {port: 443, protocol: 'tcp'},
+        {port: 80, protocol: 'tcp'}
+      ];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', ports), expected);
+    });
+
+    it('avoid linking UDP ports', function() {
+      var expected = [
+        {text: '10.0.3.1', href: 'https://10.0.3.1/'},
+        [
+          {text: '42/udp'},
+          {text: '443/tcp', href: 'https://10.0.3.1/'}
+        ]
+      ];
+      var ports = [
+        {port: 42, protocol: 'udp'},
+        {port: 443, protocol: 'tcp'}
+      ];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', ports), expected);
+    });
+
+    it('handles no open ports', function() {
+      var expected = [{text: '10.0.3.1', href: undefined}, []];
+      assert.deepEqual(parseUnitPorts('10.0.3.1', []), expected);
+    });
+
+  });
+
 });
 
 (function() {