opendev/gerrit
Code Issues Proposed changes

Replace instead of append to history on /q/ redirect

Browse Source 
Currently, if someone navigates to /q/<change num> or a similar
pattern, they are redirected to the change, but the /q/<change num>
remains in the browser history, effectively messing up the back
button behavior (you hit back and it just redirects you forward
again).

Use window.location.replace to get the same behavior you would get
by using an HTTP redirect, which doesn’t break the back button.

Bug: Issue 7278
Change-Id: I352f741b170df477ccd267dc4d6bc951f0b8affa
This commit is contained in:
Andrew Bonventre
2017-10-12 16:53:31 -04:00
parent 9ecbb7c3d5
commit 6cabe1fc17
2 changed files with 27 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
View File

@@ -139,7 +139,8 @@
for (const query in LookupQueryPatterns) {
if (LookupQueryPatterns.hasOwnProperty(query) &&
this._query.match(LookupQueryPatterns[query])) {
page.show('/c/' + changes[0]._number);
this._replaceCurrentLocation(
Gerrit.Nav.getUrlForChange(changes[0]));
return;
}
}
@@ -149,6 +150,10 @@
});
},
_replaceCurrentLocation(url) {
window.location.replace(url);
},
_getChanges() {
return this.$.restAPI.getChanges(this._changesPerPage, this._query,
this._offset);

29
polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html
View File

@@ -102,6 +102,7 @@ limitations under the License.
});
test('_computeNavLink with path', () => {
const oldCanonicalPath = window.CANONICAL_PATH;
window.CANONICAL_PATH = '/r';
const query = 'status:open';
let offset = 0;
@@ -119,6 +120,7 @@ limitations under the License.
assert.equal(
element._computeNavLink(query, offset, direction, changesPerPage),
'/r/q/status:open,10');
window.CANONICAL_PATH = oldCanonicalPath;
});
test('_hidePrevArrow', () => {
@@ -183,11 +185,22 @@ limitations under the License.
});
suite('query based navigation', () => {
setup(() => {
sandbox.stub(Gerrit.Nav, 'getUrlForChange', () => '/r/c/1');
});
teardown(done => {
flush(() => {
sandbox.restore();
done();
});
});
test('Searching for a change ID redirects to change', done => {
sandbox.stub(element, '_getChanges')
.returns(Promise.resolve([{_number: 1}]));
sandbox.stub(page, 'show', url => {
assert.equal(url, '/c/1');
sandbox.stub(element, '_replaceCurrentLocation', url => {
assert.equal(url, '/r/c/1');
done();
});
@@ -197,8 +210,8 @@ limitations under the License.
test('Searching for a change num redirects to change', done => {
sandbox.stub(element, '_getChanges')
.returns(Promise.resolve([{_number: 1}]));
sandbox.stub(page, 'show', url => {
assert.equal(url, '/c/1');
sandbox.stub(element, '_replaceCurrentLocation', url => {
assert.equal(url, '/r/c/1');
done();
});
@@ -208,8 +221,8 @@ limitations under the License.
test('Commit hash redirects to change', done => {
sandbox.stub(element, '_getChanges')
.returns(Promise.resolve([{_number: 1}]));
sandbox.stub(page, 'show', url => {
assert.equal(url, '/c/1');
sandbox.stub(element, '_replaceCurrentLocation', url => {
assert.equal(url, '/r/c/1');
done();
});
@@ -219,7 +232,7 @@ limitations under the License.
test('Searching for an invalid change ID searches', () => {
sandbox.stub(element, '_getChanges')
.returns(Promise.resolve([]));
const stub = sandbox.stub(page, 'show');
const stub = sandbox.stub(element, '_replaceCurrentLocation');
element.params = {view: Gerrit.Nav.View.SEARCH, query: CHANGE_ID};
flushAsynchronousOperations();
@@ -230,7 +243,7 @@ limitations under the License.
test('Change ID with multiple search results searches', () => {
sandbox.stub(element, '_getChanges')
.returns(Promise.resolve([{}, {}]));
const stub = sandbox.stub(page, 'show');
const stub = sandbox.stub(element, '_replaceCurrentLocation');
element.params = {view: Gerrit.Nav.View.SEARCH, query: CHANGE_ID};
flushAsynchronousOperations();