Adds search functionality

Adds search functionality

This adds text search from the search widget
-Search widget propogates search text to browser subapp
-Subapp routes data to search view
-Search view does query from text
-Search view can render search results

As a driveby, this moves repeated apiFailure code out to an extension.

There is an issue outstanding:
-The querystring can be eaten by the routing code in some cases. A follow up
branch is in progress for this problem.

https://code.launchpad.net/~jcsackett/juju-gui/search-routing-and-view/+merge/160110

(do not edit description out of merge proposal)

[j.c.sackett, 2013-04-22 13:39:38]

Please take a look.

[rharding, 2013-04-22 14:08:14]

LGTM With some comments

- I really think the apiFailure as extension makes a lot of sense and will be
cleaner.

- I think that renderSearchResults, while longer clears up some confusion we had
as search is two parts. The widget/input, and the rendering view of results. 

- We also need to make sure to note that we need a way to 'undo' search. There
needs to be a way to reset back to the default views and get editorial content.
I had thought an empty search input valueChange would do it, but it's been
brought up that the search term being empty doesn't mean 'do no search'.

https://codereview.appspot.com/8910043/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/8910043/diff/1/app/modules-debug.js#newcode310
app/modules-debug.js:310: fullpath: '/juju-ui/subapps/browser/views/utils.js'
Can we add this to the existing view utils? Maybe it's worthwhile to sync this
code up as long as it doesn't collide with anything.

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

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:209: if (params.id && params.id !== 'search') {
I wonder if there's some way to make this work like a python web app and hit
only the first route. Then we'd not need to worry about the /search and /*id
collision since we've gone this route.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
File app/subapps/browser/templates/search.handlebars (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
app/subapps/browser/templates/search.handlebars:2: <h3>{{ count }} results
found</h3>
check the css used in the other headings and try to match up. Huw's got a
class='section-title' we should make sure to reuse and help him out a step.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:71:
Y.juju.browser.views.utils.apiFailure(data, request, this);
Maybe this is a diff bug or something, but this might be great as an extension
you can stick on the view so that the right scope is there and you can tack it
onto any view.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/edito...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:149: /*
typo?

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:33: results.map(function(charm) {
does it make sense to use a charm-container here with a larger default count?

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:48: apiFailure: function(data, request) {
yea, so this is where an extension would remove the need to create the
map-through function call.

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js
File app/widgets/charm-search.js (right):

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:4: /** * The widget used across Browser view to
manage the search box and the
typo

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:101: var val = ev.currentTarget.get('value');
I think on valueChange the new value is in the ev itself. ev.newVal? Maybe I'm
wrong.

[jeff.pihach, 2013-04-22 14:20:55]

LGTM - I'd like to see that apiFailure util moved into an extension as it's not
really a utility method because it requires the view instance to be passed in

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

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:209: if (params.id && params.id !== 'search') {
That could be why the *id is undocumented as it brings with it this issue :)

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:371: if (this._viewState.viewmode ===
'fullscreen') {
because this is a route callback the req object will have the param for
:viewmode which you should probably use here.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/utils.js
File app/subapps/browser/views/utils.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/utils...
app/subapps/browser/views/utils.js:25: ns.apiFailure = function(data, request,
view) {
This should probably be an extension to the views because it isn't really a
utility method as it requires the view to function.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:173:
Y.juju.browser.views.utils.apiFaiure(data, request, this);
see comment about view extension in utils

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_widget.js
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_widget...
test/test_browser_search_widget.js:64: it('should supports clearing search
string', function() {
should support clearning
small typo.

[curtis, 2013-04-22 14:33:07]

Hi Jon. Thank you for the branch. I have a question about a test.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
File app/subapps/browser/templates/search.handlebars (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
app/subapps/browser/templates/search.handlebars:3: <div
class="search-results"></div>
the div is not subordinate to the heading, it does not need to be indented.

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_view.js
File test/test_browser_search_view.js (right):

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_view.j...
test/test_browser_search_view.js:71: });
Do we need a query string test for the cause when there isn't one? I see
renderSearch in app/subapps/browser/browser.js will perform the search with with
{text: ''}.

[j.c.sackett, 2013-04-22 20:54:45]

Comments addressed; changes coming in submission.

https://codereview.appspot.com/8910043/diff/1/app/modules-debug.js
File app/modules-debug.js (right):

https://codereview.appspot.com/8910043/diff/1/app/modules-debug.js#newcode310
app/modules-debug.js:310: fullpath: '/juju-ui/subapps/browser/views/utils.js'
On 2013/04/22 14:08:15, rharding wrote:
> Can we add this to the existing view utils? Maybe it's worthwhile to sync this
> code up as long as it doesn't collide with anything.

Done.

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

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:209: if (params.id && params.id !== 'search') {
On 2013/04/22 14:20:55, jeff.pihach wrote:
> That could be why the *id is undocumented as it brings with it this issue :)

I've got a follow up branch to deal with the two outstanding issues referenced
in this proposal; I'll investigate this in that branch.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:371: if (this._viewState.viewmode ===
'fullscreen') {
On 2013/04/22 14:20:55, jeff.pihach wrote:
> because this is a route callback the req object will have the param for
> :viewmode which you should probably use here.
Done, thanks.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
File app/subapps/browser/templates/search.handlebars (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
app/subapps/browser/templates/search.handlebars:2: <h3>{{ count }} results
found</h3>
On 2013/04/22 14:08:15, rharding wrote:
> check the css used in the other headings and try to match up. Huw's got a
> class='section-title' we should make sure to reuse and help him out a step.

Done, thanks.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/templates/s...
app/subapps/browser/templates/search.handlebars:3: <div
class="search-results"></div>
On 2013/04/22 14:33:07, curtis wrote:
> the div is not subordinate to the heading, it does not need to be indented.

Fixed, thanks

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:71:
Y.juju.browser.views.utils.apiFailure(data, request, this);
On 2013/04/22 14:08:15, rharding wrote:
> Maybe this is a diff bug or something, but this might be great as an extension
> you can stick on the view so that the right scope is there and you can tack it
> onto any view. 

Done.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/edito...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:149: /*
On 2013/04/22 14:08:15, rharding wrote:
> typo?
Fixed.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
File app/subapps/browser/views/search.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:33: results.map(function(charm) {
On 2013/04/22 14:08:15, rharding wrote:
> does it make sense to use a charm-container here with a larger default count?

I don't think so; the container header &c will look redundant with the search
header.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/searc...
app/subapps/browser/views/search.js:48: apiFailure: function(data, request) {
On 2013/04/22 14:08:15, rharding wrote:
> yea, so this is where an extension would remove the need to create the
> map-through function call. 

Yup, extension implemented.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/utils.js
File app/subapps/browser/views/utils.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/utils...
app/subapps/browser/views/utils.js:25: ns.apiFailure = function(data, request,
view) {
On 2013/04/22 14:20:55, jeff.pihach wrote:
> This should probably be an extension to the views because it isn't really a
> utility method as it requires the view to function.

Agreed; done.

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/view.js
File app/subapps/browser/views/view.js (right):

https://codereview.appspot.com/8910043/diff/1/app/subapps/browser/views/view....
app/subapps/browser/views/view.js:173:
Y.juju.browser.views.utils.apiFaiure(data, request, this);
On 2013/04/22 14:20:55, jeff.pihach wrote:
> see comment about view extension in utils

Yup, done.

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js
File app/widgets/charm-search.js (right):

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:4: /** * The widget used across Browser view to
manage the search box and the
On 2013/04/22 14:08:15, rharding wrote:
> typo
Fixed.

https://codereview.appspot.com/8910043/diff/1/app/widgets/charm-search.js#new...
app/widgets/charm-search.js:101: var val = ev.currentTarget.get('value');
On 2013/04/22 14:08:15, rharding wrote:
> I think on valueChange the new value is in the ev itself. ev.newVal? Maybe I'm
> wrong.

You're right; I've made the replacement.

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_view.js
File test/test_browser_search_view.js (right):

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_view.j...
test/test_browser_search_view.js:71: });
On 2013/04/22 14:33:07, curtis wrote:
> Do we need a query string test for the cause when there isn't one? I see
> renderSearch in app/subapps/browser/browser.js will perform the search with
with
> {text: ''}.

I've added a test that ensures it doesn't die on an empty string.

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_widget.js
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/8910043/diff/1/test/test_browser_search_widget...
test/test_browser_search_widget.js:64: it('should supports clearing search
string', function() {
On 2013/04/22 14:20:55, jeff.pihach wrote:
> should support clearning
> small typo.
Fixed.

[curtis, 2013-04-22 21:01:35]

LGTM, thanks.

[j.c.sackett, 2013-04-23 13:50:51]

*** Submitted:

Adds search functionality

This adds text search from the search widget
-Search widget propogates search text to browser subapp
-Subapp routes data to search view
-Search view does query from text
-Search view can render search results

As a driveby, this moves repeated apiFailure code out to an extension.

There is an issue outstanding:
-The querystring can be eaten by the routing code in some cases. A follow up
branch is in progress for this problem.

R=rharding, jeff.pihach, curtis
CC=
https://codereview.appspot.com/8910043