Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(358)

Issue 10119043: Makes navigation hash and qs safe

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by j.c.sackett
Modified:
10 years, 10 months ago
Reviewers:
mp+168153, jeff.pihach
Visibility:
Public.

Description

Makes navigation hash and qs safe This branch address two problems occuring in navigation -onLogin is eating querystrings and hashes when a url with them is entered into a location bar as part of the process of processing namespaces in the url. To remedy this, we get the processed url, then add the querystring and hash to it. -_navigate in the routing extension wasn't hash aware. We copy the getQS methods for hashes. Looking at other commits in this area of the code, it looks like this is regrettably untestable. (e.g. r667 touched this same part of onlogin and pointed out we can't handle nav tests in test-prod). If the reviewer has suggestions, I'm happy to pursue them. https://code.launchpad.net/~jcsackett/juju-gui/why-do-the-querystrings-die-why-why/+merge/168153 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Makes navigation hash and qs safe #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -3 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M app/app.js View 1 1 chunk +15 lines, -3 lines 0 comments Download
M app/assets/javascripts/ns-routing-app-extension.js View 2 chunks +13 lines, -0 lines 0 comments Download

Messages

Total messages: 5
j.c.sackett
Please take a look.
10 years, 11 months ago (2013-06-07 19:19:08 UTC) #1
jeff.pihach
LGTM to get you unblocked. As per our hangout... I feel that this is a ...
10 years, 11 months ago (2013-06-07 19:36:03 UTC) #2
j.c.sackett
On 2013/06/07 19:36:03, jeff.pihach wrote: > LGTM to get you unblocked. > > As per ...
10 years, 11 months ago (2013-06-07 19:49:47 UTC) #3
j.c.sackett
Please take a look.
10 years, 11 months ago (2013-06-07 19:54:10 UTC) #4
j.c.sackett
10 years, 10 months ago (2013-06-10 13:28:33 UTC) #5
On 2013/06/07 19:54:10, j.c.sackett wrote:
> Please take a look.

It's worth pointing out that Ben has handled the underlying problem here:
https://codereview.appspot.com/9830046/

I'll keep this up for now to track that, but if it's going to land soon then
there's no need for this to land, as his branch will unblock us.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b