Merge lp:~jcsackett/juju-gui/search-fixes into lp:juju-gui/experimental

[j.c.sackett]

Fixes several problems with search

-Querystring was eaten on viewmode changes or when a search url was pasted
into browser.
-Editorial and Search could have collisions in fullscreen.
-Charm add button didn't work on search results charm tokens.

Note: this reproduced (and now has merged) work in https://code.launchpad.net/~rharding/juju-gui/charm-details-1172050

https://codereview.appspot.com/8940043/

[j.c.sackett (jcsackett)]

Reviewers: mp+160706_code.launchpad.net,

Message:
Please take a look.

Description:
Fixes several problems with search

-Querystring was eaten on viewmode changes or when a search url was
pasted
into browser.
-Editorial and Search could have collisions in fullscreen.
-Charm add button didn't work on search results charm tokens.

https://code.launchpad.net/~jcsackett/juju-gui/search-fixes/+merge/160706

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/subapps/browser/browser.js
   M app/subapps/browser/views/editorial.js
   M app/subapps/browser/views/search.js
   M test/test_browser_app.js
   M test/test_browser_editorial.js
   M test/test_browser_search_view.js

[j.c.sackett (jcsackett)]

Please take a look.

https://codereview.appspot.com/8940043/

[Jeff Pihach (hatch)]

LGTM thanks, with trivial

https://codereview.appspot.com/8940043/diff/3001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/8940043/diff/3001/app/subapps/browser/browser.js#newcode24
app/subapps/browser/browser.js:24: * Show or hide the details panel.
We decided against the *'s in a Friday call.

https://codereview.appspot.com/8940043/diff/3001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/8940043/diff/3001/test/test_browser_app.js#newcode514
test/test_browser_app.js:514: it('no querystring still searched',
function() {
non descriptive test name.

https://codereview.appspot.com/8940043/

[Madison Scott-Clary (makyo)]

LGTM with little comment change. Thanks for the branch.

https://codereview.appspot.com/8940043/diff/3001/app/subapps/browser/browser.js
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/8940043/diff/3001/app/subapps/browser/browser.js#newcode496
app/subapps/browser/browser.js:496: // If no details in the route then
hide the div for
Minor:

// If there are no details in the route, then hide the div for

https://codereview.appspot.com/8940043/

[lp:~jcsackett/juju-gui/search-fixes]

591. By j.c.sackett on 2013-04-24

Comments conform.

592. By j.c.sackett on 2013-04-24

Comment typo fixed.

593. By j.c.sackett on 2013-04-24

More descriptive test name.

[j.c.sackett (jcsackett)]

*** Submitted:

Fixes several problems with search

-Querystring was eaten on viewmode changes or when a search url was
pasted
into browser.
-Editorial and Search could have collisions in fullscreen.
-Charm add button didn't work on search results charm tokens.

Note: this reproduced (and now has merged) work in
https://code.launchpad.net/~rharding/juju-gui/charm-details-1172050

R=jeff.pihach, matthew.scott
CC=
https://codereview.appspot.com/8940043

https://codereview.appspot.com/8940043/

=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js	2013-04-23 16:20:42 +0000
+++ app/subapps/browser/browser.js	2013-04-24 18:15:32 +0000
@@ -2,22 +2,20 @@
 
 
 /**
- * SubApp for the Browser
- *
- * @module juju
- * @submodule subapps
- *
+   SubApp for the Browser
+
+   @module juju
+   @submodule subapps
  */
 YUI.add('subapp-browser', function(Y) {
   var ns = Y.namespace('juju.subapps'),
       models = Y.namespace('juju.models');
 
   /**
-   * Browser Sub App for the Juju Gui.
-   *
-   * @class Browser
-   * @extends {juju.SubApp}
-   *
+     Browser Sub App for the Juju Gui.
+
+     @class Browser
+     @extends {juju.SubApp}
    */
   ns.Browser = Y.Base.create('subapp-browser', Y.juju.SubApp, [], {
     /**
@@ -25,7 +23,6 @@
 
         @method _detailsVisible
         @param {Boolean} visible set the panel to hide or show.
-
      */
     _detailsVisible: function(visible) {
       var detailsNode = Y.one('.bws-view-data');
@@ -45,7 +42,6 @@
 
         @method _getStateUrl
         @param {Object} change the values to change in the current state.
-
      */
     _getStateUrl: function(change) {
       var urlParts = ['/bws'];
@@ -70,12 +66,11 @@
     },
 
     /**
-     * Generate a standard shared set of cfg all Views can expect to see.
-     *
-     * @method _getViewCfg
-     * @param {Object} cfg additional config to merge into the default view
-     * config.
-     *
+       Generate a standard shared set of cfg all Views can expect to see.
+
+       @method _getViewCfg
+       @param {Object} cfg additional config to merge into the default view
+       config.
      */
     _getViewCfg: function(cfg) {
       return Y.merge(cfg, {
@@ -88,24 +83,23 @@
        Create an initial subapp state for later url generation.
 
        @method _initState
-
      */
     _initState: function() {
       this._oldState = {
-        viewmode: null,
+        charmID: null,
+        querystring: null,
         search: null,
-        charmID: null
+        viewmode: null
       };
       this._viewState = Y.merge(this._oldState, {});
     },
 
     /**
-      Determine if we should render the charm details based on the current
-      state.
-
-      @return {Boolean} true if should show.
-
-    */
+       Determine if we should render the charm details based on the current
+       state.
+
+       @return {Boolean} true if should show.
+     */
     _shouldShowCharm: function() {
       if (
           this._viewState.charmID &&
@@ -121,12 +115,11 @@
     },
 
     /**
-      Determine if we should render the editorial content based on the current
-      state.
-
-      @return {Boolean} true if should show.
-
-    */
+       Determine if we should render the editorial content based on the current
+       state.
+
+       @return {Boolean} true if should show.
+     */
     _shouldShowEditorial: function() {
       if (
           !this._viewState.search &&
@@ -139,19 +132,19 @@
     },
 
     /**
-      Determine if we should render the search results based on the current
-      state.
-
-      @return {Boolean} true if should show.
-
-    */
+       Determine if we should render the search results based on the current
+       state.
+
+       @return {Boolean} true if should show.
+     */
     _shouldShowSearch: function() {
       if (
           this._viewState.search &&
           (
            this._hasStateChanged('search') ||
            this._hasStateChanged('viewmode') ||
-           this._hasStateChanged('querystring')
+           this._hasStateChanged('querystring') ||
+           (this._hasStateChanged('charmID') && !this._viewState.charmID)
           )
       ) {
         return true;
@@ -161,11 +154,10 @@
     },
 
     /**
-      Verify that a particular part of the state has changed.
-
-      @method _hasStateChanged
-      @param {String} field the part of the state to check.
-
+       Verify that a particular part of the state has changed.
+
+       @method _hasStateChanged
+       @param {String} field the part of the state to check.
      */
     _hasStateChanged: function(field) {
       if (this._oldState[field] === this._viewState[field]) {
@@ -176,11 +168,10 @@
     },
 
     /**
-      Update the oldState with the viewState now that we're done processing
-      the request.
-
-      @method _saveState
-
+       Update the oldState with the viewState now that we're done processing
+       the request.
+
+       @method _saveState
      */
     _saveState: function() {
       this._oldState = Y.merge(
@@ -193,17 +184,21 @@
        be.
 
        @method _updateState
-       @param {String} path the requested path.
-       @param {Object} params the params from the request payload.
-
+       @param {Object} req the request payload.
      */
-    _updateState: function(path, params) {
+    _updateState: function(req) {
       // Update the viewmode. Every request has a viewmode.
+      var path = req.path,
+          params = req.params,
+          query = req.query;
+
       this._viewState.viewmode = params.viewmode;
 
       // Check for a charm id in the request.
       if (params.id && params.id !== 'search') {
-        this._viewState.charmID = params.id;
+        // Make sure we clear out any accidental matching of search/ in the
+        // url.
+        this._viewState.charmID = params.id.replace(/^search\//, '');
       } else {
         this._viewState.charmID = null;
       }
@@ -214,12 +209,19 @@
       } else {
         this._viewState.search = false;
       }
+
+      // Check if there's a query string to set.
+      if (query) {
+        // Store it as a straight string.
+        this._viewState.querystring = Y.QueryString.stringify(query);
+      } else {
+        this._viewState.querystring = null;
+      }
     },
 
     /**
-     * The available Views run from this sub app.
-     * @attribute views
-     *
+       The available Views run from this sub app.
+       @attribute views
      */
     views: {
       fullscreen: {
@@ -236,7 +238,6 @@
        Cleanup after ourselves on destroy.
 
        @method destructor
-
      */
     destructor: function() {
       this._cacheCharms.destroy();
@@ -244,11 +245,10 @@
     },
 
     /**
-     * General app initializer
-     *
-     * @method initializer
-     * @param {Object} cfg general init config object.
-     *
+       General app initializer
+
+       @method initializer
+       @param {Object} cfg general init config object.
      */
     initializer: function(cfg) {
       // Hold onto charm data so we can pass model instances to other views when
@@ -269,16 +269,15 @@
     },
 
     /**
-     * Render the charm details view
-     *
-     * @method renderCharmDetails
-     * @param {Request} req current request object.
-     * @param {Response} res current response object.
-     * @param {function} next callable for the next route in the chain.
-     *
+       Render the charm details view
+
+       @method renderCharmDetails
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
      */
     renderCharmDetails: function(req, res, next) {
-      var charmID = req.params.id;
+      var charmID = this._viewState.charmID;
       var extraCfg = {
         charmID: charmID,
         container: Y.Node.create('<div class="charmview"/>'),
@@ -305,16 +304,15 @@
     },
 
     /**
-      Render editorial content into the parent view when required.
-
-      The parent view is either fullscreen/sidebar which determines how the
-      editorial content is to be rendered.
-
-      @method renderEditorial
-      @param {Request} req current request object.
-      @param {Response} res current response object.
-      @param {function} next callable for the next route in the chain.
-
+       Render editorial content into the parent view when required.
+
+       The parent view is either fullscreen/sidebar which determines how the
+       editorial content is to be rendered.
+
+       @method renderEditorial
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
      */
     renderEditorial: function(req, res, next) {
       // If loading the interesting content then it's not a search going on.
@@ -353,19 +351,19 @@
     },
 
     /**
-     * Render search results
-     *
-     * @method renderSearchResults
-     * @param {Request} req current request object.
-     * @param {Response} res current response object.
-     * @param {function} next callable for the next route in the chain.
+       Render search results
+
+       @method renderSearchResults
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
      */
     renderSearchResults: function(req, res, next) {
       var container = this.get('container'),
           extraCfg = {},
           query;
-      if (this._viewState.querystring) {
-        query = Y.QueryString.parse(this._viewState.querystring);
+      if (req.query) {
+        query = req.query;
       } else {
         // If there's no querystring, we need a default "empty" search.
         query = {text: ''};
@@ -385,13 +383,12 @@
     },
 
     /**
-     * Render the fullscreen view to the client.
-     *
-     * @method fullscreen
-     * @param {Request} req current request object.
-     * @param {Response} res current response object.
-     * @param {function} next callable for the next route in the chain.
-     *
+       Render the fullscreen view to the client.
+
+       @method fullscreen
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
      */
     fullscreen: function(req, res, next) {
       // If we've switched to viewmode fullscreen, we need to render it.
@@ -402,6 +399,20 @@
         this._fullscreen = this.showView('fullscreen', this._getViewCfg());
       }
 
+      // Regardless of the results below we need to clear out the old
+      // subviews.
+      if (this._editorial) {
+        this._editorial.destroy();
+      }
+
+      if (this._details) {
+        this._details.destroy();
+      }
+
+      if (this._search) {
+        this._search.destroy();
+      }
+
       // If we've changed the charmID or the viewmode has changed and we have
       // a charmID, render charmDetails.
       if (this._shouldShowCharm()) {
@@ -411,7 +422,7 @@
         // Render search results if search is in the url and the viewmode or
         // the search has been changed in the state.
         this.renderSearchResults(req, res, next);
-      } else if (!this._viewState.charmID) {
+      } else if (!this._viewState.search && !this._viewState.charmID) {
         // Render the editorial in fullscreen only if we don't have a charmid
         this.renderEditorial(req, res, next);
       }
@@ -422,13 +433,12 @@
     },
 
     /**
-     * Handle the route for the sidebar view.
-     *
-     * @method sidebar
-     * @param {Request} req current request object.
-     * @param {Response} res current response object.
-     * @param {function} next callable for the next route in the chain.
-     *
+       Handle the route for the sidebar view.
+
+       @method sidebar
+       @param {Request} req current request object.
+       @param {Response} res current response object.
+       @param {function} next callable for the next route in the chain.
      */
     sidebar: function(req, res, next) {
       // If we've switched to viewmode sidebar, we need to render it.
@@ -440,10 +450,20 @@
       // Render search results if search is in the url and the viewmode or the
       // search has been changed in the state.
       if (this._shouldShowSearch()) {
+        // Showing search implies that other sidebar content is destroyed.
+        if (this._editorial) {
+          this._editorial.destroy();
+        }
+
         this.renderSearchResults(req, res, next);
       }
 
       if (this._shouldShowEditorial()) {
+        // Showing editorial implies that other sidebar content is destroyed.
+        if (this._search) {
+          this._search.destroy();
+        }
+
         this.renderEditorial(req, res, next);
       }
 
@@ -454,7 +474,7 @@
         this.renderCharmDetails(req, res, next);
       }
 
-      // If the sidebar is the final part of the route, then hide the div for
+      // If there are no details in the route then hide the div for
       // viewing the charm details.
       if (!this._viewState.charmID) {
         this._detailsVisible(false);
@@ -474,17 +494,16 @@
     },
 
     /**
-        Dispatch to the correct viewmode based on the route that was hit.
+       Dispatch to the correct viewmode based on the route that was hit.
 
        @method routeView
        @param {Request} req current request object.
        @param {Response} res current response object.
        @param {function} next callable for the next route in the chain.
-
      */
     routeView: function(req, res, next) {
       // Update the state for the rest of things to figure out what to do.
-      this._updateState(req.path, req.params);
+      this._updateState(req);
       this[req.params.viewmode](req, res, next);
     }
 
@@ -494,7 +513,6 @@
          @attribute container
          @default '#subapp-browser'
          @type {String}
-
        */
       container: {
         value: '#subapp-browser'
@@ -504,17 +522,15 @@
          @attribute store
          @default Charmworld0
          @type {Charmworld0}
-
        */
       store: {
         /**
-          We keep one instance of the store and will work on caching results
-          at the app level so that routes can share api calls. However, in
-          tests there's no config for talking to the api so we have to watch
-          out in test runs and allow the store to be broken.
-
-          method store.valueFn
-
+           We keep one instance of the store and will work on caching results
+           at the app level so that routes can share api calls. However, in
+           tests there's no config for talking to the api so we have to watch
+           out in test runs and allow the store to be broken.
+
+           method store.valueFn
         */
         valueFn: function() {
           var url = '';
@@ -533,7 +549,6 @@
          @attribute routes
          @default Array of subapp routes.
          @type {Array}
-
        */
       routes: {
         value: [
@@ -549,7 +564,6 @@
          @attribute urlNamespace
          @default 'charmstore'
          @type {String}
-
        */
       urlNamespace: {
         value: 'charmstore'
@@ -562,7 +576,6 @@
          @attribute deploy
          @default undefined
          @type {Function}
-
        */
       deploy: {}
 
@@ -573,7 +586,7 @@
   requires: [
     'juju-charm-store',
     'juju-models',
-    'querystring-parse',
+    'querystring',
     'sub-app',
     'subapp-browser-charmview',
     'subapp-browser-editorial',

=== modified file 'app/subapps/browser/views/editorial.js'
--- app/subapps/browser/views/editorial.js	2013-04-24 06:18:42 +0000
+++ app/subapps/browser/views/editorial.js	2013-04-24 18:15:32 +0000
@@ -60,17 +60,14 @@
       var charmID = charm.getData('charmid');
 
       // Update the UI for the active one.
-      this._updateActive(ev.currentTarget);
+      if (!this.get('isFullscreen')) {
+        this._updateActive(ev.currentTarget);
+      }
+
       var change = {
         charmID: charmID
       };
 
-      if (this.get('isFullscreen')) {
-        change.viewmode = 'fullscreen';
-      } else {
-        change.viewmode = 'sidebar';
-      }
-
       this.fire('viewNavigate', {change: change});
     },
 

=== modified file 'app/subapps/browser/views/search.js'
--- app/subapps/browser/views/search.js	2013-04-23 13:31:29 +0000
+++ app/subapps/browser/views/search.js	2013-04-24 18:15:32 +0000
@@ -17,7 +17,52 @@
   ns.BrowserSearchView = Y.Base.create('browser-view-searchview', Y.View, [
     views.utils.apiFailingView
   ], {
+    events: {
+      '.charm-token': {
+        click: '_handleCharmSelection'
+      }
+    },
+
     template: views.Templates.search,
+
+    /**
+        When selecting a charm from the list make sure we re-route the app to
+        the details view with that charm selected.
+
+        @method _handleCharmSelection
+        @param {Event} ev the click event handler for the charm selected.
+
+     */
+    _handleCharmSelection: function(ev) {
+      ev.halt();
+      var charm = ev.currentTarget;
+      var charmID = charm.getData('charmid');
+
+      // Update the UI for the active one.
+      if (!this.get('isFullscreen')) {
+        this._updateActive(ev.currentTarget);
+      }
+      var change = {
+        charmID: charmID
+      };
+      this.fire('viewNavigate', {change: change});
+    },
+
+    /**
+      Update the node in the editorial list marked as 'active'.
+
+      @method _updateActive
+      @param {Node} clickTarget the charm-token clicked on to activate.
+
+    */
+    _updateActive: function(clickTarget) {
+      // Remove the active class from any nodes that have it.
+      Y.all('.yui3-charmtoken.active').removeClass('active');
+
+      // Add it to the current node.
+      clickTarget.ancestor('.yui3-charmtoken').addClass('active');
+    },
+
     /**
      * Renders the search results from the the store query.
      *
@@ -30,6 +75,9 @@
           tplNode = Y.Node.create(tpl),
           container = tplNode.one('.search-results');
 
+      // Set the container so that our events will delegate based off of it.
+      this.set('container', container);
+
       results.map(function(charm) {
         var ct = new widgets.browser.CharmToken(charm.getAttrs());
         ct.render(container);

=== modified file 'test/test_browser_app.js'
--- test/test_browser_app.js	2013-04-23 19:36:04 +0000
+++ test/test_browser_app.js	2013-04-24 18:15:32 +0000
@@ -468,6 +468,66 @@
       assert.deepEqual(hits, expected);
     });
 
+    it('changing the query string dispatches correctly', function() {
+      var req = {
+        path: '/bws/fullscreen/search/',
+        params: {
+          viewmode: 'fullscreen'
+        },
+        query: {
+          text: 'test'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Reset the hits and we should not redraw anything to update the view.
+      resetHits();
+      req.query.text = 'test2';
+
+      var expected = Y.merge(hits, {
+        renderSearchResults: true
+      });
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('no change to query string does not redraw', function() {
+      var req = {
+        path: '/bws/fullscreen/search/',
+        params: {
+          viewmode: 'fullscreen'
+        },
+        query: {
+          text: 'test'
+        }
+      };
+      browser.routeView(req, undefined, function() {});
+
+      // Reset the hits and we should not redraw anything to update the view.
+      resetHits();
+
+      var expected = Y.merge(hits);
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
+    it('handles searches with no querystring', function() {
+      var req = {
+        path: '/bws/fullscreen/search/',
+        params: {
+          viewmode: 'fullscreen'
+        }
+      };
+
+      var expected = Y.merge(hits, {
+        fullscreen: true,
+        renderSearchResults: true
+      });
+
+      browser.routeView(req, undefined, function() {});
+      assert.deepEqual(hits, expected);
+    });
+
   });
 })();
 

=== modified file 'test/test_browser_editorial.js'
--- test/test_browser_editorial.js	2013-04-24 16:42:05 +0000
+++ test/test_browser_editorial.js	2013-04-24 18:15:32 +0000
@@ -106,7 +106,6 @@
 
       view.on('viewNavigate', function(ev) {
         ev.halt();
-        assert(ev.change.viewmode === 'fullscreen');
         assert(ev.change.charmID === 'precise/ceph-7');
         done();
       });
@@ -134,7 +133,6 @@
 
       view.on('viewNavigate', function(ev) {
         ev.halt();
-        assert(ev.change.viewmode === 'sidebar');
         assert(ev.change.charmID === 'precise/ceph-7');
         done();
       });

=== modified file 'test/test_browser_search_view.js'
--- test/test_browser_search_view.js	2013-04-22 21:04:27 +0000
+++ test/test_browser_search_view.js	2013-04-24 18:15:32 +0000
@@ -12,6 +12,7 @@
         'json',
         'juju-charm-store',
         'node',
+        'node-event-simulate',
         'subapp-browser-searchview',
         function(Y) {
           done();
@@ -75,4 +76,27 @@
     view.render();
     assert.equal('charms?text=', apiURL);
   });
+
+  it('clicking a charm navigates for fullscreen', function(done) {
+    view.render();
+    view.on('viewNavigate', function(ev) {
+      ev.halt();
+      assert(ev.change.charmID === 'foo/bar-2');
+      done();
+    });
+
+    container.one('.charm-token').simulate('click');
+  });
+
+  it('clicking a charm navigates for sidebar', function(done) {
+    view.render();
+    view.on('viewNavigate', function(ev) {
+      ev.halt();
+      assert(ev.change.charmID === 'foo/bar-2');
+      done();
+    });
+
+    container.one('.charm-token').simulate('click');
+  });
+
 });