Merge lp:~hatch/juju-gui/bundle-fail-gracefully into lp:juju-gui/experimental

[Jeff Pihach]

Bundle topology rendering fails gracefully

If the bundle does not provide proper xy position annotations
on any of the services in its deployer file we do not render
the bundle topology and instead remove the bundle topology
tab, selecting the Charms tab to show the charms the
bundle contains.

Because we don't have any bundles with proper positions right
now I added the flag `strictBundle` to enable this feature.

https://codereview.appspot.com/14502061/

[Jeff Pihach (hatch)]

Reviewers: mp+190769_code.launchpad.net,

Message:
Please take a look.

Description:
Bundle topology rendering fails gracefully

If the bundle does not provide proper xy position annotations
on any of the services in its deployer file we do not render
the bundle topology and instead issue a warning.

Because we don't have any bundles with proper positions right
now I added the flag strictBundle to enable this feature.

https://code.launchpad.net/~hatch/juju-gui/bundle-fail-gracefully/+merge/190769

(do not edit description out of merge proposal)

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

Affected files (+119, -23 lines):
   A [revision details]
   M app/subapps/browser/views/bundle.js
   M test/test_bundle_details_view.js

[Gary Poster (gary)]

Small test change requested, and otherwise code is LGTM. doing qa now.

https://codereview.appspot.com/14502061/diff/1/test/test_bundle_details_view.js
File test/test_bundle_details_view.js (right):

https://codereview.appspot.com/14502061/diff/1/test/test_bundle_details_view.js#newcode188
test/test_bundle_details_view.js:188: 'gui-y': ''
I wonder if we should not allow this either. shrug.

https://codereview.appspot.com/14502061/diff/1/test/test_bundle_details_view.js#newcode208
test/test_bundle_details_view.js:208: view._positionAnnotationsIncluded
= function() {
But this way you are not testing whether _postionAnnotationsIncluded
actually ever can approve something! :-) Can we just adjust the data
instead?

https://codereview.appspot.com/14502061/

[lp:~hatch/juju-gui/bundle-fail-gracefully]

1135. By Jeff Pihach on 2013-10-11

If a bundle topology data is invalid we now don't show the tab at all and instead default to the charms tab

[Jeff Pihach (hatch)]

Please take a look.

https://codereview.appspot.com/14502061/

[Jeff Pihach (hatch)]

*** Submitted:

Bundle topology rendering fails gracefully

If the bundle does not provide proper xy position annotations
on any of the services in its deployer file we do not render
the bundle topology and instead remove the bundle topology
tab, selecting the Charms tab to show the charms the
bundle contains.

Because we don't have any bundles with proper positions right
now I added the flag `strictBundle` to enable this feature.

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

https://codereview.appspot.com/14502061/

=== modified file 'app/subapps/browser/views/bundle.js'
--- app/subapps/browser/views/bundle.js	2013-10-11 17:51:10 +0000
+++ app/subapps/browser/views/bundle.js	2013-10-11 22:52:00 +0000
@@ -150,27 +150,67 @@
         return !/\.svg$/.test(fileName);
       });
       var content = this.template(attrs);
-
       var node = this.get('container').setHTML(content);
       var renderTo = this.get('renderTo');
       var options = {size: [480, 360]};
       this.hideIndicator(renderTo);
-      this.environment = new views.BundleTopology(Y.mix({
-        db: this.fakebackend.db,
-        container: node.one('#bws-bundle'), // Id because of Y.TabView
-        store: this.get('store')
-      }, options));
-
-      this.environment.render();
+
+      var showTopo = true;
+      // remove the flag in the test(test_bundle_details_view.js)
+      // when this flag is no longer needed.
+      if (window.flags && window.flags.strictBundle) {
+        showTopo = this._positionAnnotationsIncluded(attrs.data.services);
+      }
+      if (showTopo) {
+        this.environment = new views.BundleTopology(Y.mix({
+          db: this.fakebackend.db,
+          container: node.one('#bws-bundle'), // Id because of Y.TabView
+          store: this.get('store')
+        }, options));
+        this.environment.render();
+      } else {
+        // Remove the bundle tab so it doesn't get PE'd when
+        // we instantiate the tabview.
+        node.one('#bws-bundle').remove();
+        node.one('a[href=#bws-bundle]').get('parentNode').remove();
+      }
+
       renderTo.setHTML(node);
 
       this._setupTabview();
+      if (!showTopo) {
+        // Select the charms tab as the landing tab if
+        // we aren't showing the bundle topology.
+        this.tabview.selectChild(2);
+      }
       this._dispatchTabEvents(this.tabview);
 
       this.set('rendered', true);
     },
 
     /**
+      Determines if all of the services in the bundle
+      have position annotations.
+
+      @method _positionAnnotationsIncluded
+      @param {Object} services An object of all of the services in the bundle.
+      @return {Boolean} Weather all services have position annotations or not.
+    */
+    _positionAnnotationsIncluded: function(services) {
+      // Some returns true if it's stopped early, this inverts before returning.
+      return !Object.keys(services).some(function(key) {
+        var annotations = services[key].annotations;
+        // If there is no annotations for the position coords
+        // return true stopping the 'some' loop.
+        if (!annotations ||
+            !annotations['gui-x'] ||
+            !annotations['gui-y']) {
+          return true;
+        }
+      });
+    },
+
+    /**
       Renders the loading indicator into the DOM and then calls
       the _prepareData method to fetch/parse the bundle data for
       the real view rendering.
 
=== modified file 'test/test_bundle_details_view.js'
--- test/test_bundle_details_view.js	2013-10-11 20:13:50 +0000
+++ test/test_bundle_details_view.js	2013-10-11 22:52:00 +0000
@@ -60,6 +60,9 @@
       entityId: data.id,
       renderTo: container
     });
+    view._setupLocalFakebackend = function() {
+      this.fakebackend = utils.makeFakeBackend();
+    };
   });
 
   afterEach(function() {
@@ -89,9 +92,6 @@
   });
 
   it('displays the bundle data in a tabview', function(done) {
-    view._setupLocalFakebackend = function() {
-      this.fakebackend = utils.makeFakeBackend();
-    };
     view.after('renderedChange', function(e) {
       assert.isNotNull(container.one('.yui3-tabview'));
       done();
@@ -112,9 +112,6 @@
         done();
       }
     });
-    view._setupLocalFakebackend = function() {
-      this.fakebackend = utils.makeFakeBackend();
-    };
     view.after('renderedChange', function(e) {
       container.one('a.readme').simulate('click');
     });
@@ -134,9 +131,6 @@
         done();
       }
     });
-    view._setupLocalFakebackend = function() {
-      this.fakebackend = utils.makeFakeBackend();
-    };
     view.after('renderedChange', function(e) {
       container.one('a.code').simulate('click');
       var codeNode = container.one('#bws-code');
@@ -148,9 +142,6 @@
   });
 
   it('renders the proper charm icons into the header', function(done) {
-    view._setupLocalFakebackend = function() {
-      this.fakebackend = utils.makeFakeBackend();
-    };
     view.after('renderedChange', function(e) {
       assert.equal(
           container.one('.header .details .charms').all('img').size(),
@@ -161,9 +152,6 @@
   });
 
   it('deploys a bundle when \'add\' button is clicked', function(done) {
-    view._setupLocalFakebackend = function() {
-      this.fakebackend = utils.makeFakeBackend();
-    };
     // app.js sets this to its deploy bundle method so
     // as long as it's called it's successful.
     view.set('deployBundle', function(data) {
@@ -176,4 +164,78 @@
     view.render();
   });
 
+  it('fails gracefully if services don\'t provide xy annotations',
+     function(done) {
+       window.flags = { strictBundle: true };
+       view._parseData = function() {
+         return new Y.Promise(function(resolve) { resolve(); });
+       };
+       view.set('entity', {
+          getAttrs: function() {
+            return {
+              charm_metadata: {},
+              files: [],
+              data: {
+                services: {
+                  foo: {
+                    annotations: {
+                      'gui-x': '',
+                      'gui-y': ''
+                    }
+                  },
+                  bar: {}
+                }
+              }
+            };
+          }});
+       view.after('renderedChange', function(e) {
+         assert.isNull(container.one('#bws-bundle'));
+         assert.isNull(container.one('a[href=#bws-bundle]'));
+         // Check that the charms tab is the landing tab
+         assert.equal(view.tabview.get('selection').get('index'), 2);
+         window.flags = {};
+         done();
+       });
+       view.render();
+     });
+
+  it('renders the bundle topology into the view', function(done) {
+    window.flags = { strictBundle: true };
+    view._parseData = function() {
+      return new Y.Promise(function(resolve) { resolve(); });
+    };
+    view.set('entity', {
+      getAttrs: function() {
+        return {
+          charm_metadata: {},
+          files: [],
+          data: {
+            services: {
+              foo: {
+                annotations: {
+                  'gui-x': '1',
+                  'gui-y': '2'
+                }
+              },
+              bar: {
+                annotations: {
+                  'gui-x': '3',
+                  'gui-y': '4'
+                }
+              }
+            }
+          }
+        };
+      }});
+    view.after('renderedChange', function(e) {
+      assert.isNotNull(container.one('.topology-canvas'));
+      // Check that the bundle topology tab is the landing tab.
+      assert.equal(view.tabview.get('selection').get('index'), 0);
+      window.flags = {};
+      done();
+    });
+    view.render();
+  });
+
+
 });