Bug fix: routes being dispatched multiple times

Bug fix: routes being dispatched multiple times

Ever since we added namespace aware routing we have had
an issue which caused routes to be dispatched multiple
times. This stops that from happening while maintaining
the proper browser history entries allowing back/forward
functionality.

https://code.launchpad.net/~hatch/juju-gui/multi-dispatch/+merge/159916

(do not edit description out of merge proposal)

[jeff.pihach, 2013-04-20 03:46:15]

Please take a look.

[gary.poster, 2013-04-22 12:46:00]

LGTM, with possible addition in navigate per my comment, at your discretion. 
For my "I'm confused" comment, please explain it to me over some channel or
other, but don't consider it a blocker.  I QA'd as you described and duplicated
your results, and I also could not find a situation in which the change broke
the app.

Thank you

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:272: navigate: function(url,
options) {
This dumps the this._hasSameOrigin check from the YUI source.  That seems like a
reasonable check, if not super important for us.  Why did you drop it?

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:298: if
(Y.App.prototype._navigate.call(this, result, options)) {
I'm confused.  result has all namespaces, not just the one we explicitly
changed.  I would have guessed that we only wanted the one we changed here. 
Why?

[jeff.pihach, 2013-04-22 14:42:39]

Thanks for the review!

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:272: navigate: function(url,
options) {
It was removed to avoid the _resolveURL call which was causing issues during
hacking. I will look into it to see if the issues are still there or if it was
just a side effect.

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:298: if
(Y.App.prototype._navigate.call(this, result, options)) {
The way that we had it before would push the namespaced url into the history and
then replace it with the full one. This pushes the full namespaced url into it
once removing the multiple 'replace' updates that come afterwards triggering the
multi dispatches.

[bcsaller, 2013-04-22 15:52:44]

LGTM

Did basic QA and it seems to work fine, thanks for tracking this down. :)

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
File app/assets/javascripts/ns-routing-app-extension.js (right):

https://codereview.appspot.com/8883043/diff/1/app/assets/javascripts/ns-routi...
app/assets/javascripts/ns-routing-app-extension.js:298: if
(Y.App.prototype._navigate.call(this, result, options)) {
You can just return the call directly here, no?

[jeff.pihach, 2013-04-22 16:41:19]

*** Submitted:

Bug fix: routes being dispatched multiple times

Ever since we added namespace aware routing we have had
an issue which caused routes to be dispatched multiple
times. This stops that from happening while maintaining
the proper browser history entries allowing back/forward
functionality.

R=gary.poster, bcsaller
CC=
https://codereview.appspot.com/8883043