Provide a 'deploy' button in quicksearch results.

Provide a 'deploy' button in quicksearch results.

- Add the control to the tokens for both charms and bundles
- Wire up a new event that the search widget can fire EVT_DEPLOY
- Make views that extend view.js (fullscree/sidebar) watch for that event and
build a proper deploy command.
- Make sure those views get access to the deploy and bundleDeploy helpers from
the browser.js
- Deploying does not actual perform a selection so no search or details takes
place. It's an alternate behavior for quicksearch.


Several drive-by fixes for things. Documented in the reviewer comments.


QA:

The button is feature flagged due to the fact that this quick deploy doesn't
involve the charm cache and causes the inspector to 'hang' for a sec when you
hit the deploy button. It's not an ideal experience.

http://gui:8888/:flags:/searchDeploy

Search for apa and then click on the + next to apache2. It should deploy to
the canvas. No text is in the quicksearch box, no charm highlighted, etc.

You can also QA this with a bundle.

http://gui:8888/:flags:/searchDeploy/charmworldv3/

Now enter "TestB" and get the TestBundle result. Press the + to deploy the
bundle.

https://code.launchpad.net/~rharding/juju-gui/deploy-quicksearch/+merge/193059

(do not edit description out of merge proposal)

[rharding, 2013-10-29 18:23:20]

Please take a look.

[rharding, 2013-10-29 18:31:54]

Review comments

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:542: extraCfg.deploy = this.get('deployBundle');
Only set one deploy method on the details view. They can share the same code for
this. Just the deploy method needs to be different.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:708: extraCfg.deploy = this.get('deploy');
pass the deploy methods into the sidebar/fullscreen views so that they can use
quicksearch to perform a deploy (vs the typical path from details view or
drag/drop)

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:800: deployBundle: this.get('deployBundle')
See above

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:1165: deployBundle: {},
document all the things!

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:76: this.get('deploy')(bundle.get('data'));
so this changes to just deploy vs a diff method

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

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/view.js:93: );
no idea why there were two different "if search" checks. Block them into one.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/view.js:101: this.search.on(
this is the new event the search-widget grew to handle the deploy selection.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/view.js:140: _deployEntity: function(ev) {
mostly just sharing what the details views for bundles/charms do.

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token...
File app/templates/bundle-token.handlebars (right):

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token...
app/templates/bundle-token.handlebars:4: {{#if deployButton}}
the token only displays this when requested. The search widget passes this in
for use and other token uses don't care.

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token...
app/templates/bundle-token.handlebars:5: {{#ifFlag 'searchDeploy'}}
we only feature flag the UX since it's a UX driven feature. Simpler than FF'ing
all the code.

https://codereview.appspot.com/18920045/diff/20001/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/views/utils.js#newcode...
app/views/utils.js:1567: * {{#ifFlag 'flag_name'}} {{/ifFlag}}
Document the real usage.

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

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:212: deployButton: isCategory(charm.get('id')) ?
false : true
note that we don't set the deployButton for the category options in the result
set.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:273: this.ac._onItemClick = function(ev) {
This is the giant hack to make this all work. Clicking anything in here is touch
because it fires a AC "itemSelect" event regardless of where you click. The only
way to NOT fire an itemSelect on here is to override the original method and do
the check/work here.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:330: } else {
this is what the original method does. So had to dupe these 3 lines from YUI
itself.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:372: if (ev.result.raw.charm) {
this is a drive-by to make selecting a quicksearch result for a bundle to work.
We have to generate the 'bundle' charmId to use.

https://codereview.appspot.com/18920045/diff/20001/lib/views/browser/bws-sear...
File lib/views/browser/bws-searchbox.less (right):

https://codereview.appspot.com/18920045/diff/20001/lib/views/browser/bws-sear...
lib/views/browser/bws-searchbox.less:69: .token.tiny {
drive by fixing some alignment of the text in the quicksearch results so they're
centered better.

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_search_w...
File test/test_browser_search_widget.js (right):

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_search_w...
test/test_browser_search_widget.js:209: cleanIconHelper =
utils.stubCharmIconPath();
this can go away. Left over from debugging an issue.

[gary.poster, 2013-10-29 18:41:03]

QA thought: what is the expected/desired behavior of the search dropdown when
the deploy button is clicked?  I'm expecting that it should close.

[rharding, 2013-10-29 18:42:45]

On 2013/10/29 18:41:03, gary.poster wrote:
> QA thought: what is the expected/desired behavior of the search dropdown when
> the deploy button is clicked?  I'm expecting that it should close.

It should close when you click the deploy button. If it's not then I'd be
curious what browser/search/selection you used. It does close in my QA here.

Be aware that the input box is still 'live' so if you click into it, mouse over
it, you might get it to pop back?

[gary.poster, 2013-10-29 19:09:46]

LGTM with comments; thank you!

Gary

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:1165: deployBundle: {},
On 2013/10/29 18:31:54, rharding wrote:
> document all the things!

:-) cool

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:76: this.get('deploy')(bundle.get('data'));
On 2013/10/29 18:31:54, rharding wrote:
> so this changes to just deploy vs a diff method

The only downside to the new approach is that sometimes we separate the two
names (deploy vs deployBundle) and sometimes we merge them.  That makes it just
a bit harder to figure out what's going on while reading.  In your shoes, I'd be
tempted to rename the charm-only deploy function to "deployCharm" so as to make
a parallel with "deployBundle" and a contrast with the more mutable "deploy".  I
am sure there are other solutions, some better than this one.  Moreover, this is
just a suggestion.  What you have done is OK with me as is, and an improvement.

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

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/view.js:93: );
On 2013/10/29 18:31:54, rharding wrote:
> no idea why there were two different "if search" checks. Block them into one.

Heh. cool.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/views/...
app/subapps/browser/views/view.js:318: deploy: {},
So this is the one I'd call deployCharm.

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token...
File app/templates/bundle-token.handlebars (right):

https://codereview.appspot.com/18920045/diff/20001/app/templates/bundle-token...
app/templates/bundle-token.handlebars:5: {{#ifFlag 'searchDeploy'}}
On 2013/10/29 18:31:54, rharding wrote:
> we only feature flag the UX since it's a UX driven feature. Simpler than
FF'ing
> all the code.

Cool.

https://codereview.appspot.com/18920045/diff/20001/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/views/utils.js#newcode...
app/views/utils.js:1567: * {{#ifFlag 'flag_name'}} {{/ifFlag}}
On 2013/10/29 18:31:54, rharding wrote:
> Document the real usage. 

Uh-oh, the doc string is becoming dangerously close to being helpful. ;-)

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

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:273: this.ac._onItemClick = function(ev) {
This comment, or a variation of it, deserves to be in the code, IMO.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:275: // ignore the way autocomplete works. It's more
of a 'quick search'
'Course, I think we agreed that this is not really autocomplete after all,
anyway.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:278: if (ev.target.hasClass('search_add_to_canvas'))
{
I'm guessing we can't have an event registration for li.search_add_to_canvas or
whatever that simply catches the event first because it is a more specific
selector, and then calls ev.halt()?

If not, I suggest changing the body of this block to something like the
following.

this.hide();
this.handleDeploy(ev.target.getData());

Then handleDeploy can look at the data and do the right thing (that is, the code
you have below) within its own code.  That feels like a readability improvement
to me.

Alternatively, you could expand this to divide up on the basis of bundle vs
charm.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:377: // with entity? Something to clean up.
Entity seems simplest and reasonable.

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js#n...
test/test_browser_app.js:182: fakeStore = new Y.juju.charmworld.APIv2({});
v2?  expected v3, but doubt it matters.

[gary.poster, 2013-10-29 19:11:00]

On 2013/10/29 18:42:45, rharding wrote:
> On 2013/10/29 18:41:03, gary.poster wrote:
> > QA thought: what is the expected/desired behavior of the search dropdown
when
> > the deploy button is clicked?  I'm expecting that it should close.
> 
> It should close when you click the deploy button. If it's not then I'd be
> curious what browser/search/selection you used. It does close in my QA here.
> 
> Be aware that the input box is still 'live' so if you click into it, mouse
over
> it, you might get it to pop back?

Sounds good.  I hadn't qa'd it--you said Jeff would do that.  I was thinking
through expectations before reviewing and hoping you agreed with me.  After I
reviewed the code I saw you agreed with me.  Thanks!

[jeff.pihach, 2013-10-29 19:21:51]

Thanks for this feature! I'll be doing the QA now, just wanted to get my
comments landed so we can discuss.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
File app/subapps/browser/browser.js (right):

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:541: if (entityId.indexOf('bundle') !== -1) {
Because everywhere else that uses 'deploy'  is using it for only charms and
'deployBundle' for bundles, I feel that this adds in a level of complexity that
isn't easy to follow. I don't see any benefit to this change.

https://codereview.appspot.com/18920045/diff/20001/app/subapps/browser/browse...
app/subapps/browser/browser.js:1165: deployBundle: {},
Thanks for adding these in!

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

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:273: this.ac._onItemClick = function(ev) {
We should subclass the ac widget vs doing this monkeypatch. otherwise it will be
difficult to debug if the api changes down the road when we upgrade YUI.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:327: this.hide();
You have probably looked into this already, but just in case...Does this shut
down any pending ac requests to avoid the result list from popping up in the
future? I'm just a little concerned that we are hiding the list element but
keeping the ac functional in the background since we are short circuiting the
real code.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:331: var itemNode = ev.currentTarget;
Instead of simply copying these methods a better approach would simply be:

this._ac.prototype._onItemClick(ev);

or if you subclass it (I think):

MyNewAutocomplete.superclass.constructor._onItemClick(ev).bind(this);

This way any changes to the parent method won't require us to update this part
of the conditional.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:373: id = ev.result.raw.charm.id;
Thanks for this fix!

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js#n...
test/test_browser_app.js:182: fakeStore = new Y.juju.charmworld.APIv2({});
Shouldn't we also have tests here(and below) for the v3 api?

[jeff.pihach, 2013-10-29 19:26:55]

QA OK

Love this functionality, works great - just wish the button was a little more
clear as to its purpose and that it would be bigger so it's easier to click.

[jeff.pihach, 2013-10-29 19:31:59]

LGTM as per our discussion on IRC. Thanks for this branch, great functionality.

[rharding, 2013-10-30 12:12:55]

Please take a look.

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

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:278: if (ev.target.hasClass('search_add_to_canvas'))
{
On 2013/10/29 19:09:46, gary.poster wrote:
> I'm guessing we can't have an event registration for li.search_add_to_canvas
or
> whatever that simply catches the event first because it is a more specific
> selector, and then calls ev.halt()?
> 
> If not, I suggest changing the body of this block to something like the
> following.
> 
> this.hide();
> this.handleDeploy(ev.target.getData());
> 
> Then handleDeploy can look at the data and do the right thing (that is, the
code
> you have below) within its own code.  That feels like a readability
improvement
> to me.
> 
> Alternatively, you could expand this to divide up on the basis of bundle vs
> charm.

Agreed and in Jeff's review and discussion we agreed that this version of
'autocomplete' has gotten complex enough to be it's own module. Three's a card
to clean this up. For a start I've moved the logic into _onDeploy(ev) and called
that. It's what we'll have when moved to its own module.

https://codereview.appspot.com/18920045/diff/20001/app/widgets/charm-search.j...
app/widgets/charm-search.js:327: this.hide();
On 2013/10/29 19:21:51, jeff.pihach wrote:
> You have probably looked into this already, but just in case...Does this shut
> down any pending ac requests to avoid the result list from popping up in the
> future? I'm just a little concerned that we are hiding the list element but
> keeping the ac functional in the background since we are short circuiting the
> real code.

No, that'll have to be a new feature. From what I can tell, there's not
currently request tracking/killing in AC.

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/18920045/diff/20001/test/test_browser_app.js#n...
test/test_browser_app.js:182: fakeStore = new Y.juju.charmworld.APIv2({});
On 2013/10/29 19:09:46, gary.poster wrote:
> v2?  expected v3, but doubt it matters.

True, it's a fakestore never called so it didn't matter. It would make for one
fewer instance to change during the conversion to making v3 live though so I'll
update.

[rharding, 2013-10-30 13:28:56]

*** Submitted:

Provide a 'deploy' button in quicksearch results.

- Add the control to the tokens for both charms and bundles
- Wire up a new event that the search widget can fire EVT_DEPLOY
- Make views that extend view.js (fullscree/sidebar) watch for that event and
build a proper deploy command.
- Make sure those views get access to the deploy and bundleDeploy helpers from
the browser.js
- Deploying does not actual perform a selection so no search or details takes
place. It's an alternate behavior for quicksearch.


Several drive-by fixes for things. Documented in the reviewer comments.


QA:

The button is feature flagged due to the fact that this quick deploy doesn't
involve the charm cache and causes the inspector to 'hang' for a sec when you
hit the deploy button. It's not an ideal experience.

http://gui:8888/:flags:/searchDeploy

Search for apa and then click on the + next to apache2. It should deploy to
the canvas. No text is in the quicksearch box, no charm highlighted, etc.

You can also QA this with a bundle.

http://gui:8888/:flags:/searchDeploy/charmworldv3/

Now enter "TestB" and get the TestBundle result. Press the + to deploy the
bundle.

R=gary.poster, jeff.pihach
CC=
https://codereview.appspot.com/18920045