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',