Fix 5982 - Do fetch after route change is complete

[Bargs]

Fixes #5982
Fixes #5986

@spalger summed up the root cause of the issue well in #6788:

In order to react to the url changing we listen for the $routeUpdate event on the $rootScope. This event is fired as a part of the transition process before the $location service has flushed it's changes to the browser. Because of this, any changes that happen in the same tick are merged together as a single change. This is generally desirable, but since we are using $location.replace() pretty liberally when the State detects that it has been removed from the URL we need to actually do this in the next tick. This way the $location.replace() call will never be merged with the location change that triggered the update.

This PR takes the same approach, but instead of using undefined behavior in $evalAsync to schedule the query string replacement in the next tick, it simply does the replacement after the route change is complete based on the routeChangeSuccess event.

[spalger]

Oh snap, hope this fixes it! Will take a look soon

[Bargs]

I hope so too! Some design changes in add data have really made this important. The only thing I'm not sure of is if there are any unintended side effects, but I played around with it some and didn't notice any issues.

The one test failure simply asserts that the state object gets destroyed on $routeChangeStart. There's no description why in the commit message though. Looks like @w33ble added the test so he might have some insight into how safe this change is as well.

[Bargs]

If it's not safe to destroy the state in $routeChangeSuccess, I can always create an additional handler for the fetch and leave the destroy action in its current handler.

[w33ble]

Looks like @w33ble added the test so he might have some insight into how safe this change is as well.

w33ble committed on Sep 30, 2014

Man, that was a long time ago. Back when I had even less of an idea what I was doing than I have now ;)

I suspect I was just trying to make sure that the appState was being cleaned up, not specifically that it was being cleaned up during that event. I think it's safe to update the test to $emit('$routeChangeSuccess') instead, I don't think the actual event matters, just the cleanup.

[spalger]

I can always create an additional handler for the fetch and leave the destroy action in its current handler.

^ this

[Bargs]

Thanks for walking down memory lane @w33ble :)

I left destruction on routeChangeStart since @spalger agreed it was the safer option. Tests should pass now, should be ready for review.

[spalger]

⚠️ super nit approaching ⚠️

can you swap these event handlers so that start comes before success?

[Bargs]

Your wish is my command :)

Updated.

[spalger]

LGTM! Now to figure out how far back we should backport this...

[Bargs]

Good point, @epixa what's our policy on backporting now?

[epixa]

I wouldn't bother backporting it. The bug has been around for awhile and is a P2. As you pointed out, it's really only a bigger deal now because of the add data ui, which is 5.0+.

[Bargs]

Works for me.