Code review comment for lp://staging/~gary/juju-gui/bug1167967

Reviewers: mp+158468_code.launchpad.net,

Message:
Please take a look.

Description:
Fix bug 1167967 (hopefully)

This branch attempts to fix the referenced bug. I was unable to
duplicate the symptoms locally exactly, but duplicated them closely
enough that I am optimistic that these changes address the issue.
Regardless, they seem like reasonable changes.

I also fixed a couple of landscape issues I saw with newly created
services without annotations.

The aspect of this branch I wonder most if reviewers (particularly Ben)
will like is the change to the route url algorithm. If it's not clear
why I chose it, I'm happy to discuss my rationale and other options.

Thank you.

https://code.launchpad.net/~gary/juju-gui/bug1167967/+merge/158468

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/app.js
   M app/assets/javascripts/ns-routing-app-extension.js
   M app/views/charm.js
   M app/views/landscape.js
   M app/views/service.js
   M app/views/utils.js
   M test/test_app_hotkeys.js
   M test/test_charm_view.js
   M test/test_landscape.js
   M test/test_routing.js
   M test/test_service_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: app/app.js
=== modified file 'app/app.js'
--- app/app.js 2013-04-09 20:50:46 +0000
+++ app/app.js 2013-04-11 19:46:11 +0000
@@ -184,7 +184,7 @@

        Y.detachAll('window-alt-E-pressed');
        Y.on('window-alt-E-pressed', function(data) {
- this.fire('navigateTo', { url: '/' });
+ this.fire('navigateTo', { url: '/:gui:/' });
          data.preventDefault = true;
        }, this);

Index: test/test_app_hotkeys.js
=== modified file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js 2013-04-08 18:23:28 +0000
+++ test/test_app_hotkeys.js 2013-04-11 19:46:11 +0000
@@ -50,7 +50,7 @@
    it('should listen for alt-E events', function() {
      var altEtriggered = false;
      app.on('navigateTo', function(ev) {
- if (ev && ev.url === '/') {
+ if (ev && ev.url === '/:gui:/') {
          altEtriggered = true;
        }
        // Avoid URL change performed by additional listeners.

Index: test/test_charm_view.js
=== modified file 'test/test_charm_view.js'
--- test/test_charm_view.js 2013-02-15 11:58:05 +0000
+++ test/test_charm_view.js 2013-04-11 19:46:11 +0000
@@ -88,7 +88,7 @@
          env: env});
        var redirected = false;
        charmView.on('navigateTo', function(e) {
- assert.equal('/', e.url);
+ assert.equal('/:gui:/', e.url);
          redirected = true;
        });
        var deployInput = charmView.get('container').one('#charm-deploy');

Index: app/views/charm.js
=== modified file 'app/views/charm.js'
--- app/views/charm.js 2013-03-19 21:58:04 +0000
+++ app/views/charm.js 2013-04-11 19:46:11 +0000
@@ -97,7 +97,7 @@
        // The deploy call generates an event chain leading to a call to
        // `app.on_database_changed()`, which re-dispatches the current view.
        // For this reason we need to redirect to the root page right now.
- this.fire('navigateTo', {url: '/'});
+ this.fire('navigateTo', {url: '/:gui:/'});
        env.deploy(charmUrl, serviceName, config,
            Y.bind(this._deployCallback, this)
        );

Index: app/assets/javascripts/ns-routing-app-extension.js
=== modified file 'app/assets/javascripts/ns-routing-app-extension.js'
--- app/assets/javascripts/ns-routing-app-extension.js 2013-03-21 15:32:06
+0000
+++ app/assets/javascripts/ns-routing-app-extension.js 2013-04-11 19:46:11
+0000
@@ -167,7 +167,9 @@
        var keys = Y.Object.keys(base).sort();
        Y.each(keys, function(ns) {
          url = slash(url);
- url += ':' + ns + ':' + base[ns];
+ if (base[ns] !== '/') {
+ url += ':' + ns + ':' + base[ns];
+ }
        });

        url = slash(url);

Index: test/test_landscape.js
=== modified file 'test/test_landscape.js'
--- test/test_landscape.js 2013-03-26 21:20:28 +0000
+++ test/test_landscape.js 2013-04-11 19:46:11 +0000
@@ -158,6 +158,14 @@
      node.one('.updates-control').getStyle('display').should.equal('none');
      node.one('.restart-control').getStyle('display').should.equal('block');

+ // We handle missing annotations on a service.
+ mysql.set('annotations', {});
+ landscape.update();
+ views.utils.updateLandscapeBottomBar(landscape, env, mysql, node,
+ 'service');
+ node.one('.machine-control').getStyle('display').should.equal('none');
+
+ // We handle normal unit annotations.
      unit.annotations = {'landscape-computer': '+unit:mysql-0'};
      landscape.update();

@@ -171,6 +179,13 @@
      node.one('.updates-control').getStyle('display').should.equal('none');
      node.one('.restart-control').getStyle('display').should.equal('none');

+ // We handle completely missing annotations on a unit.
+ delete unit.annotations;
+ landscape.update();
+ views.utils.updateLandscapeBottomBar(landscape, env, unit, node,
+ 'unit');
+ node.one('.machine-control').getStyle('display').should.equal('none');
+
      node.remove();
    });

Index: app/views/landscape.js
=== modified file 'app/views/landscape.js'
--- app/views/landscape.js 2013-04-09 20:22:01 +0000
+++ app/views/landscape.js 2013-04-11 19:46:11 +0000
@@ -128,7 +128,8 @@
          }
          url += slash(modelAnnotation);
        } else if (model.name === 'serviceUnit') {
- modelAnnotation = model.annotations['landscape-computer'];
+ modelAnnotation = (
+ model.annotations && model.annotations['landscape-computer']);
          if (!modelAnnotation) {
            console.warn('Unit missing the landscape-computer annotation!');
            return undefined;

Index: test/test_routing.js
=== modified file 'test/test_routing.js'
--- test/test_routing.js 2013-04-08 18:23:28 +0000
+++ test/test_routing.js 2013-04-11 19:46:11 +0000
@@ -46,6 +46,12 @@
      var u = router.url(match);
      assert(u === '/charms/precise/mediawiki/:inspector:/services/mysql/');

+ // Root keys are implicit.
+ u = router.url({foo: '/'});
+ assert.strictEqual(u, '/');
+ u = router.url({foo: '/shazam', bar: '/'});
+ assert.strictEqual(u, '/:foo:/shazam/');
+
      // Sorted keys.
      u = router.url({charmstore: '/', gamma: 'g', a: 'alpha', b: 'beta'});
      assert(u === '/:a:alpha/:b:beta/:gamma:g/');

Index: app/views/service.js
=== modified file 'app/views/service.js'
--- app/views/service.js 2013-04-04 07:22:16 +0000
+++ app/views/service.js 2013-04-11 19:46:11 +0000
@@ -225,7 +225,8 @@
              ));
          this.panel.hide();
          this.panel.destroy();
- this.fire('navigateTo', {url: '/'});
+ this.fire('navigateTo', {url: '/:gui:/'});
+ db.fire('update');
        }
      }
    };

Index: app/views/utils.js
=== modified file 'app/views/utils.js'
--- app/views/utils.js 2013-04-11 12:47:10 +0000
+++ app/views/utils.js 2013-04-11 19:46:11 +0000
@@ -422,20 +422,22 @@

      if (envAnnotations['landscape-url']) {
        controls.show();
- machine.show();
- machine.one('a').setAttribute('href',
- landscape.getLandscapeURL(model));
-
- if (annotations['landscape-security-upgrades']) {
- updates.show();
- updates.one('a').setAttribute('href',
- landscape.getLandscapeURL(model, 'security'));
- }
-
- if (annotations['landscape-needs-reboot']) {
- restart.show();
- restart.one('a').setAttribute('href',
- landscape.getLandscapeURL(model, 'reboot'));
+ var baseLandscapeURL = landscape.getLandscapeURL(model);
+ if (baseLandscapeURL) {
+ machine.show();
+ machine.one('a').setAttribute('href', baseLandscapeURL);
+
+ if (annotations['landscape-security-upgrades']) {
+ updates.show();
+ updates.one('a').setAttribute('href',
+ landscape.getLandscapeURL(model, 'security'));
+ }
+
+ if (annotations['landscape-needs-reboot']) {
+ restart.show();
+ restart.one('a').setAttribute('href',
+ landscape.getLandscapeURL(model, 'reboot'));
+ }
        }
      }
    };

Index: test/test_service_view.js
=== modified file 'test/test_service_view.js'
--- test/test_service_view.js 2013-03-21 15:32:06 +0000
+++ test/test_service_view.js 2013-04-11 19:46:11 +0000
@@ -349,20 +349,23 @@
           destroy.simulate('click');
           var called = false;
           view.on('navigateTo', function(ev) {
- assert.equal('/', ev.url);
+ assert.equal('/:gui:/', ev.url);
             called = true;
           });
           var callbacks = Y.Object.values(env._txn_callbacks);
           callbacks.length.should.equal(1);
- // Since we don't have an app to listen to this event and tell the
- // view to re-render, we need to do it ourselves.
- db.on('update', view.render, view);
+ var dbUpdated = false;
+ db.on('update', function() {
+ dbUpdated = true;
+ });
           callbacks[0]({result: true});
           var _ =
expect(db.services.getById(service.get('id'))).to.not.exist;
           db.relations.map(function(u) {return u.get('id');})
          .should.eql(['relation-0000000001']);
           // Catch show environment event.
           called.should.equal(true);
+ // The db should be updated.
+ dbUpdated.should.equal(true);
         });

      it('should send an expose RPC call when exposeService is invoked',