Fixes #1174358 remove dupe code/driveby route fix

Fixes #1174358 remove dupe code/driveby route fix

- subapp views search and editorial share some common event handling code as
you can select, activate, etc on both views.

- Performs a drive by fix for making REALLY sure that the Y.Router uses the
QueryString.parse method to parse query strings. Per call with Gary, we just
directly monkey patch Y.Router when our own route code loads.
- Remove the old hack of prefixing the module list in index.html.
- Add a test in the route code that Y.Router._parseQuery will in fact parse a
list-value query string.

https://code.launchpad.net/~rharding/juju-gui/dupe-code-1174358/+merge/163575

(do not edit description out of merge proposal)

[rharding, 2013-05-13 17:52:26]

Please take a look.

[rharding, 2013-05-13 18:01:21]

Please take a look.

[gary.poster, 2013-05-13 18:20:33]

LGTM.  I skimmed the meat of the now-less-duplicated classes, and did not do QA.
 I'm hoping Jon can correct both of those limitations.

Thanks

Gary

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

https://codereview.appspot.com/9036049/diff/1010/app/assets/javascripts/ns-ro...
app/assets/javascripts/ns-routing-app-extension.js:8: Y.Router._parseQuery =
Y.QueryString.parse;
+1

https://codereview.appspot.com/9036049/diff/1010/app/assets/javascripts/ns-ro...
app/assets/javascripts/ns-routing-app-extension.js:504: 'router'
Thanks. :-)

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
File app/subapps/browser/views/charmresults.js (right):

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:145: activeID: {
trivial, but might as well collapse as you do elsewhere into activeId: {},

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:49: apiFailure: function(data, request) {
This is in the superclass also, isn't it?  I didn't see a difference.  If so,
you can delete as well.

[j.c.sackett, 2013-05-13 18:34:05]

Rick--

Not sure about implementing this as a parent class; as it's not a complete view,
it feels like it might make more sense as an extension, like apiFailingView.

At a minimum, it shouldn't use the apiFailingView extension; just it's children,
as they call it with their own error messages. I've noted that in comments.

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
File app/subapps/browser/views/charmresults.js (right):

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:50: this.on('activeIDChange',
function(ev) {
We should probaby catch this and detach it on destroy, right? Maybe just use
eventmanager?

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:109: this._apiFailure(data, request,
'Failed to load editorial content.');
This is the wrong place for this; leave it in the editorial view, since the
error message is just for editorial. Search should have its own version which
mentions an error loading search results.

[rharding, 2013-05-14 10:54:08]

JC, I've tried to look at this as an extension and I'm not really sold. The
charmresults hooks into the Y.View apis with things like the events property and
defined the ATTRS shared between the child views. While I can do these things in
an extension with this.addAttr and the like, it's kind of messy. It doesn't have
clear bounds on where it's api starts/ends like an extension does. 

What I could see is maybe trying to name this somehow to show it's incomplete?
Or maybe to add a render method that states that it's not meant to be used on
its own?

What do you think?

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
File app/subapps/browser/views/charmresults.js (right):

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:50: this.on('activeIDChange',
function(ev) {
On 2013/05/13 18:34:05, j.c.sackett wrote:
> We should probaby catch this and detach it on destroy, right? Maybe just use
> eventmanager?

Definitely, thanks.

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:109: this._apiFailure(data, request,
'Failed to load editorial content.');
On 2013/05/13 18:34:05, j.c.sackett wrote:
> This is the wrong place for this; leave it in the editorial view, since the
> error message is just for editorial. Search should have its own version which
> mentions an error loading search results.

Yep, thanks. I'll keep this on its own between the two.

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ch...
app/subapps/browser/views/charmresults.js:145: activeID: {
On 2013/05/13 18:20:33, gary.poster wrote:
> trivial, but might as well collapse as you do elsewhere into activeId: {},

Sure thing.

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ed...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9036049/diff/1010/app/subapps/browser/views/ed...
app/subapps/browser/views/editorial.js:49: apiFailure: function(data, request) {
On 2013/05/13 18:20:33, gary.poster wrote:
> This is in the superclass also, isn't it?  I didn't see a difference.  If so,
> you can delete as well.

As JC points out, the message isn't common and so I'll keep these in each one.

[j.c.sackett, 2013-05-14 13:17:56]

On 2013/05/14 10:54:08, rharding wrote:
> JC, I've tried to look at this as an extension and I'm not really sold. The
> charmresults hooks into the Y.View apis with things like the events property
and
> defined the ATTRS shared between the child views. While I can do these things
in
> an extension with this.addAttr and the like, it's kind of messy. It doesn't
have
> clear bounds on where it's api starts/ends like an extension does. 
> 
> What I could see is maybe trying to name this somehow to show it's incomplete?
> Or maybe to add a render method that states that it's not meant to be used on
> its own?

Yeah, I buy your argument that it does have to hook into the Y.View stuff. It
feels awkward, but it's probably the best solution. Given the comment string
says it's not a full view, I don't think renaming or providing a render
method to indicate it's incomplete is necessary.

> On 2013/05/13 18:34:05, j.c.sackett wrote:
> > This is the wrong place for this; leave it in the editorial view, since the
> > error message is just for editorial. Search should have its own version
which
> > mentions an error loading search results.
> 
> Yep, thanks. I'll keep this on its own between the two.

Thanks.

[rharding, 2013-05-14 13:27:44]

Please take a look.

[j.c.sackett, 2013-05-14 14:03:12]

LGTM. Thanks for those changes.

[rharding, 2013-05-14 14:20:16]

*** Submitted:

Fixes #1174358 remove dupe code/driveby route fix

- subapp views search and editorial share some common event handling code as
you can select, activate, etc on both views.

- Performs a drive by fix for making REALLY sure that the Y.Router uses the
QueryString.parse method to parse query strings. Per call with Gary, we just
directly monkey patch Y.Router when our own route code loads.
- Remove the old hack of prefixing the module list in index.html.
- Add a test in the route code that Y.Router._parseQuery will in fact parse a
list-value query string.

R=gary.poster, j.c.sackett
CC=
https://codereview.appspot.com/9036049