Merge lp:~rharding/juju-gui/routing-issues into lp:juju-gui/experimental

[Richard Harding]

Fixes routing issues around search and charm tabs

- See bug #1205468
- See bug #1200743

Summary
-------
There are two problems. The first is that when doing some view state changes
we need to also clear the hash so that it doesn't carry around. We also clear
the charm id when doing searches in fullscreen mode. This helps the browser
app ack that there were changes in state and show the correct view.

The second issue was that clicking a tab on the fullscreen charm details after
a search causes two #bws-readme (for instance) to be added. One is before the
query string and one is after the querysting. This is caused by our double
dispatch and the fact that our routing code builds urls with query strings
after the hash of the url. This is not proper. The Y.App adds it to the end of
the url. In this way we ended up with it in both places.

Our routing code would then assume the whole #bws-readme?text=apache2 was the
hash of the url and that there was no query string. All kinds of trouble came
out of this.

Tests are added to verify the changes work as expected given our sample bad
urls.

QA
---
To QA simpler go through the steps in the two bugs and it should work as
expected. Other QA would be to verify that other usage is not adversely
effected by moving the hash to be at the end of the url while the querystring
is immediately after the path.

https://codereview.appspot.com/12036043/

[Richard Harding (rharding)]

Reviewers: mp+177401_code.launchpad.net,

Message:
Please take a look.

Description:
Fixes routing issues around search and charm tabs

- See bug #1205468
- See bug #1200743

Summary
-------
There are two problems. The first is that when doing some view state
changes
we need to also clear the hash so that it doesn't carry around. We also
clear
the charm id when doing searches in fullscreen mode. This helps the
browser
app ack that there were changes in state and show the correct view.

The second issue was that clicking a tab on the fullscreen charm details
after
a search causes two #bws-readme (for instance) to be added. One is
before the
query string and one is after the querysting. This is caused by our
double
dispatch and the fact that our routing code builds urls with query
strings
after the hash of the url. This is not proper. The Y.App adds it to the
end of
the url. In this way we ended up with it in both places.

Our routing code would then assume the whole #bws-readme?text=apache2
was the
hash of the url and that there was no query string. All kinds of trouble
came
out of this.

Tests are added to verify the changes work as expected given our sample
bad
urls.

QA
---
To QA simpler go through the steps in the two bugs and it should work as
expected. Other QA would be to verify that other usage is not adversely
effected by moving the hash to be at the end of the url while the
querystring
is immediately after the path.

https://code.launchpad.net/~rharding/juju-gui/routing-issues/+merge/177401

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/assets/javascripts/ns-routing-app-extension.js
   M app/subapps/browser/browser.js
   M app/subapps/browser/views/view.js
   M app/widgets/charm-search.js
   M test/test_browser_app.js
   M test/test_routing.js

[j.c.sackett (jcsackett)]

LGTM, thanks.

https://codereview.appspot.com/12036043/diff/1/app/assets/javascripts/ns-routing-app-extension.js
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/12036043/diff/1/app/assets/javascripts/ns-routing-app-extension.js#newcode104
app/assets/javascripts/ns-routing-app-extension.js:104: var hashIndex =
qs.indexOf('#');
Thanks for using hashIndex instead of hashIdx. Deryck is, I'm sure,
proud.

https://codereview.appspot.com/12036043/

[Jeff Pihach (hatch)]

QA No Go

Go to http://localhost:8888/fullscreen/precise/cinder-8/
Click, Readme, Interfaces
Click JuJu (top left)
Click Browse
Url will now be http://localhost:8888/fullscree/##bws-interfaces

Can't repro on jujucharms.com because I can't get past the readme step,
so I'm not sure if this was pre-existing or a side effect of this
branch.

https://codereview.appspot.com/12036043/

[lp:~rharding/juju-gui/routing-issues]

906. By Richard Harding on 2013-07-29

Make sure to reset the subapp state when showing root view

907. By Richard Harding on 2013-07-29

Make that safe

908. By Richard Harding on 2013-07-29

lint

909. By Richard Harding on 2013-07-29

The # is part of the url so it's part of the hash

[Richard Harding (rharding)]

Please take a look.

https://codereview.appspot.com/12036043/

[lp:~rharding/juju-gui/routing-issues]

910. By Richard Harding on 2013-07-29

Update the back to also clear the hash

911. By Richard Harding on 2013-07-29

Remove the unneeded code

[Richard Harding (rharding)]

Please take a look.

https://codereview.appspot.com/12036043/

[Jeff Pihach (hatch)]

LGTM Thanks for getting these blockers/criticals fixed!
QA OK

This branch exposed an issue where the router is not dispatching when
the hash changes. https://bugs.launchpad.net/juju-gui/+bug/1206239

https://codereview.appspot.com/12036043/

[Richard Harding (rharding)]

*** Submitted:

Fixes routing issues around search and charm tabs

- See bug #1205468
- See bug #1200743

Summary
-------
There are two problems. The first is that when doing some view state
changes
we need to also clear the hash so that it doesn't carry around. We also
clear
the charm id when doing searches in fullscreen mode. This helps the
browser
app ack that there were changes in state and show the correct view.

The second issue was that clicking a tab on the fullscreen charm details
after
a search causes two #bws-readme (for instance) to be added. One is
before the
query string and one is after the querysting. This is caused by our
double
dispatch and the fact that our routing code builds urls with query
strings
after the hash of the url. This is not proper. The Y.App adds it to the
end of
the url. In this way we ended up with it in both places.

Our routing code would then assume the whole #bws-readme?text=apache2
was the
hash of the url and that there was no query string. All kinds of trouble
came
out of this.

Tests are added to verify the changes work as expected given our sample
bad
urls.

QA
---
To QA simpler go through the steps in the two bugs and it should work as
expected. Other QA would be to verify that other usage is not adversely
effected by moving the hash to be at the end of the url while the
querystring
is immediately after the path.

R=j.c.sackett, jeff.pihach
CC=
https://codereview.appspot.com/12036043

https://codereview.appspot.com/12036043/

=== modified file 'app/app.js'
--- app/app.js	2013-07-25 16:41:41 +0000
+++ app/app.js	2013-07-29 18:11:23 +0000
@@ -1144,6 +1144,11 @@
     */
     showRootView: function() {
       this._navigate('/', { overrideAllNamespaces: true });
+      // Reset the view state of the subapps.
+      var subapps = this.get('subApps');
+      if (subapps.charmstore) {
+        subapps.charmstore.initState();
+      }
     },
 
     /**

=== modified file 'app/assets/javascripts/ns-routing-app-extension.js'
--- app/assets/javascripts/ns-routing-app-extension.js	2013-06-11 17:55:57 +0000
+++ app/assets/javascripts/ns-routing-app-extension.js	2013-07-29 18:11:23 +0000
@@ -97,7 +97,16 @@
      * @method getQS
      **/
     getQS: function(url) {
-      return url.split('?')[1];
+      var qs = url.split('?')[1];
+      if (qs) {
+        // The query string picks up the hash due to the simple nature of this
+        // generation. Remove it.
+        var hashIndex = qs.indexOf('#');
+        if (hashIndex !== -1) {
+          qs = qs.substr(0, hashIndex);
+        }
+      }
+      return qs;
     },
 
     /**
@@ -131,8 +140,8 @@
         href: url
       };
       var origin = this._regexUrlOrigin.exec(url);
+      result.hash = this.getHash(url);
       result.search = this.getQS(url);
-      result.hash = this.getHash(url);
 
       if (origin) {
         // Take the match.
@@ -144,11 +153,11 @@
       }
 
       if (result.search) {
-        result.pathname = result.pathname.substr(
-            0,
-            (result.pathname.length - result.search.length) - 1);
-      }
-
+        result.pathname = result.pathname.replace('?' + result.search, '');
+      }
+      if (result.hash) {
+        result.pathname = result.pathname.replace('#' + result.hash, '');
+      }
       return result;
     },
 
@@ -173,10 +182,6 @@
       result.hash = parts.hash;
       result.search = parts.search;
 
-      // If there was a hash we need to split it off url
-      if (result.hash) {
-        url = url.slice(0, url.indexOf('#'));
-      }
       parts = url.split(this._fragment);
       // Example output
       // '/foo/bar'.split(this._fragment)
@@ -268,14 +273,12 @@
       });
 
       url = slash(url);
-
+      if (components.search) {
+        url += '?' + components.search;
+      }
       if (components.hash) {
         url += '#' + components.hash;
       }
-
-      if (components.search) {
-        url += '?' + components.search;
-      }
       return url;
     },
 

=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js	2013-07-19 18:56:15 +0000
+++ app/subapps/browser/browser.js	2013-07-29 18:11:23 +0000
@@ -120,6 +120,10 @@
           qs: this._viewState.querystring
         });
       }
+
+      if (this._viewState.hash) {
+        url = url + this._viewState.hash;
+      }
       return url;
     },
 
@@ -144,22 +148,6 @@
     },
 
     /**
-       Create an initial subapp state for later url generation.
-
-       @method _initState
-     */
-    _initState: function() {
-      this._oldState = {
-        charmID: null,
-        hash: null,
-        querystring: null,
-        search: null,
-        viewmode: null
-      };
-      this._viewState = Y.merge(this._oldState, {});
-    },
-
-    /**
       Registers Handlebars helpers that need access to subapp data like the
       store instance.
 
@@ -430,7 +418,7 @@
         search: null,
         interesting: null
       };
-      this._initState();
+      this.initState();
       this._filter = new models.browser.Filter();
 
       this._registerSubappHelpers();
@@ -449,6 +437,22 @@
     },
 
     /**
+       Create an initial subapp state for later url generation.
+
+       @method initState
+     */
+    initState: function() {
+      this._oldState = {
+        charmID: null,
+        querystring: null,
+        hash: null,
+        search: null,
+        viewmode: null
+      };
+      this._viewState = Y.merge(this._oldState, {});
+    },
+
+    /**
        Render the charm details view
 
        @method renderCharmDetails

=== modified file 'app/subapps/browser/views/charm.js'
--- app/subapps/browser/views/charm.js	2013-07-24 17:10:03 +0000
+++ app/subapps/browser/views/charm.js	2013-07-29 18:11:23 +0000
@@ -269,7 +269,8 @@
       ev.halt();
       this.fire('viewNavigate', {
         change: {
-          charmID: null
+          charmID: null,
+          hash: null
         }
       });
     },

=== modified file 'app/subapps/browser/views/view.js'
--- app/subapps/browser/views/view.js	2013-07-24 17:17:25 +0000
+++ app/subapps/browser/views/view.js	2013-07-29 18:11:23 +0000
@@ -149,6 +149,7 @@
     _goHome: function(ev) {
       var change = {
         charmID: undefined,
+        hash: undefined,
         search: false,
         filter: {
           clear: true
@@ -204,6 +205,14 @@
         }
       };
 
+      // If we're in fullscreen and you did a search we clear the charmID to
+      // help make sure that we show you the search results you just asked for
+      // properly.
+      if (this.isFullscreen()) {
+        change.charmID = undefined;
+        change.hash = undefined;
+      }
+
       // Perhaps there's more to this change than just a search change. This
       // might come from places, such as autocomplete, which are a search
       // change, but also want to select a charm id as well.

=== modified file 'app/widgets/charm-search.js'
--- app/widgets/charm-search.js	2013-07-29 16:01:59 +0000
+++ app/widgets/charm-search.js	2013-07-29 18:11:23 +0000
@@ -271,7 +271,7 @@
       );
 
       // Make sure the UI around the autocomplete search input is setup.
-      if (window.flags.ac) {
+      if (window.flags && window.flags.ac) {
         this._setupAutocomplete();
 
         // Override a couple of autocomplete events to help perform our

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-07-24 15:23:57 +0000
+++ test/test_browser_app.js	2013-07-29 18:11:23 +0000
@@ -157,6 +157,7 @@
       view.on('viewNavigate', function(ev) {
         assert.equal(ev.change.search, false);
         assert.equal(ev.change.filter.clear, true);
+        assert.equal(ev.change.hash, undefined);
         done();
       });
 
@@ -166,6 +167,26 @@
       });
     });
 
+    it('resets charmid and hash on search', function(done) {
+      var container = Y.one('#subapp-browser'),
+          fakeStore = new Y.juju.Charmworld2({});
+      view = new FullScreen({
+        charmID: 'precise/jenkins-13'
+      });
+
+      view.on('viewNavigate', function(ev) {
+        assert.equal(ev.change.filter.text, 'test search');
+        assert.equal(ev.change.charmID, undefined);
+        assert.equal(ev.change.hash, undefined);
+        done();
+      });
+
+      view.render(container);
+      view._searchChanged({
+        newVal: 'test search'
+      });
+    });
+
   });
 })();
 
@@ -517,6 +538,28 @@
         filter: undefined
       });
       assert.equal(url, 'fullscreen');
+
+      // It takes into account hash.
+      url = app._getStateUrl({
+        viewmode: 'fullscreen',
+        charmID: undefined,
+        hash: '#bws-readme',
+        search: undefined,
+        filter: undefined
+      });
+      assert.equal(url, 'fullscreen#bws-readme');
+
+      // It always puts the hash last.
+      url = app._getStateUrl({
+        viewmode: 'fullscreen',
+        charmID: 'precise/jenkins-2',
+        hash: '#bws-readme',
+        search: true,
+        querystring: 'text=jenkins'
+      });
+      assert.equal(
+          url,
+          'fullscreen/search/precise/jenkins-2?text=jenkins#bws-readme');
     });
 
     it('/charm/id router ignores other urls', function() {
@@ -1147,5 +1190,6 @@
       });
       assert.equal(url, '/search?text=mysql');
     });
+
   });
 })();

=== modified file 'test/test_routing.js'
--- test/test_routing.js	2013-06-19 15:44:25 +0000
+++ test/test_routing.js	2013-07-29 18:11:23 +0000
@@ -137,10 +137,8 @@
        // argument to combine, values from the original url
        // are discarded.
        url = router.combine('/foo/bar?world=gone+away',
-                            '/:inspector:/foo/#hello?world=beater');
-       url.should.equal('/foo/bar/:inspector:/foo/#hello?world=beater');
-
-
+                            '/:inspector:/foo/?world=beater#hello');
+       url.should.equal('/foo/bar/:inspector:/foo/?world=beater#hello');
      });
 
   it('should be able to split qualified urls', function() {
@@ -200,7 +198,6 @@
     assert.equal(u, '/:gui:/services/mysql/:gui:/services/mediawiki/');
   });
 
-
   it('should allow combining routes in a given namespace', function() {
     var router = juju.Router('charmstore');
     var match = router.parse('/');
@@ -225,7 +222,6 @@
     var u = router.url(match);
     assert.equal(u, '/:gui:/services/mysql/:gui:/services/mediawiki/');
 
-
     // Combine works as well (note the flag, like with parse this can be an
     // object).
     u = router.combine('/:gui:/services/mysql/', ':gui:/services/mediawiki/',
@@ -234,6 +230,24 @@
 
   });
 
+  it('should properly parse the QS of a given url', function() {
+    var router = juju.Router('charmstore');
+    var qs = router.getQS(
+        '/fullscreen/search/precise/jenkins-5/?text=jenkins#bws-readme');
+    assert.equal(qs, 'text=jenkins');
+  });
+
+  it('should split a url into components properly', function() {
+    var router = juju.Router('charmstore');
+    var components = router.split(
+        '/fullscreen/search/precise/jenkins-5/?text=jenkins#bws-readme');
+    assert.equal(
+        components.pathname,
+        '/fullscreen/search/precise/jenkins-5/');
+    assert.equal(components.hash, 'bws-readme');
+    assert.equal(components.search, 'text=jenkins');
+  });
+
 });
 
 describe('Juju Gui Routing', function() {