Prevent a case of charm details double dispatch.

Prevent a case of charm details double dispatch.

- This double dispatch caused the charm details to be rendered multiple times
  causing issues with the tabview.
- We match multiple routes in the browser subapp. We always match on * for
  instance. In the case of the * route, a url path /charm/id will match, but
  not have a req.params.id set, this causes the _viewState to think there is
  no charm id in the request. Because of this, it clears any current _details
  View object. However, a later matched route *id will find the id and cause
  it to render again. This could happen a couple of times.
- We create custom callables for the * and *id routes.
- These callables only kick in and do any work at all if they're the perfect
match for that path.
- If they do not apply, then they ignore and carry on allowing later matching
routes (more specific ones) to do their work correctly.

https://code.launchpad.net/~rharding/juju-gui/details-double-dispatch/+merge/168053

(do not edit description out of merge proposal)

[rharding, 2013-06-07 16:11:16]

Please take a look.

[j.c.sackett, 2013-06-07 16:21:41]

LGTM, but I think you have some redundant code.

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

https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js...
app/subapps/browser/browser.js:721: id = id.replace(/\/$/, '');
Didn't you update _stripViewMode to do this?

[benjamin.saller, 2013-06-07 16:27:12]

LGTM w/ some trivials.

Thanks for this. I have some suggestions you can take or leave below.

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

https://codereview.appspot.com/10086045/diff/1/app/modules-debug.js#newcode48
app/modules-debug.js:48: navigateOnHash: false,
with html5: true this should be false I think, but being explicit about this
here doesn't seem like a bad thing to me

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

https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js...
app/subapps/browser/browser.js:875: { path: '/*id/', callbacks:
'routeDirectCharmId'},
Can routeSidebarDefault normalize id so that the other methods don't have to?
Seems like there is a little bit of a DRY violation and too many methods have to
think about URL state. I'm not that familiar with this code though so I leave it
up to you.

https://codereview.appspot.com/10086045/diff/1/test/test_browser_app.js
File test/test_browser_app.js (right):

https://codereview.appspot.com/10086045/diff/1/test/test_browser_app.js#newco...
test/test_browser_app.js:243: var next = function() {};
Trivial: you can make this top level as its used a few places.

[rharding, 2013-06-07 17:06:42]

On 2013/06/07 16:21:41, j.c.sackett wrote:
> LGTM, but I think you have some redundant code.
> 
> https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js
> File app/subapps/browser/browser.js (right):
> 
>
https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js...
> app/subapps/browser/browser.js:721: id = id.replace(/\/$/, '');
> Didn't you update _stripViewMode to do this?

Thanks, I missed stripViewMode stipped /$ and I updated to also do ^/.

[rharding, 2013-06-07 17:13:59]

On 2013/06/07 16:27:12, benjamin.saller wrote:

> https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js
> File app/subapps/browser/browser.js (right):
> 
>
https://codereview.appspot.com/10086045/diff/1/app/subapps/browser/browser.js...
> app/subapps/browser/browser.js:875: { path: '/*id/', callbacks:
> 'routeDirectCharmId'},
> Can routeSidebarDefault normalize id so that the other methods don't have to?
> Seems like there is a little bit of a DRY violation and too many methods have
to
> think about URL state. I'm not that familiar with this code though so I leave
it
> up to you.

The issue is that for each matched url a different req object is passed in to be
handled. The routeSidebarDefault is only intended to handle the single url case
of /precise/myql-10 and no other routes. It builds an updated req object with
the right default viewmode and such, but it cannot effect the later route
matches like /:viewmode:/*id/

Because of the multi-dispatch and multiple routes matching everything has to
make sure it takes the current state into account when determining what work to
do.


> https://codereview.appspot.com/10086045/diff/1/test/test_browser_app.js
> File test/test_browser_app.js (right):
> 
>
https://codereview.appspot.com/10086045/diff/1/test/test_browser_app.js#newco...
> test/test_browser_app.js:243: var next = function() {};
> Trivial: you can make this top level as its used a few places.

Thanks, updated.

[rharding, 2013-06-07 17:18:31]

*** Submitted:

Prevent a case of charm details double dispatch.

- This double dispatch caused the charm details to be rendered multiple times
  causing issues with the tabview.
- We match multiple routes in the browser subapp. We always match on * for
  instance. In the case of the * route, a url path /charm/id will match, but
  not have a req.params.id set, this causes the _viewState to think there is
  no charm id in the request. Because of this, it clears any current _details
  View object. However, a later matched route *id will find the id and cause
  it to render again. This could happen a couple of times.
- We create custom callables for the * and *id routes.
- These callables only kick in and do any work at all if they're the perfect
match for that path.
- If they do not apply, then they ignore and carry on allowing later matching
routes (more specific ones) to do their work correctly.

R=j.c.sackett, benjamin.saller
CC=
https://codereview.appspot.com/10086045