Remove the feature of redirecting x..x to x
Theoretically you may enter a URL with a patchset range of `x..x`, which
we currently immediately redirect to just `x`, which is nice, but it
adds a fair amount of complexity and hinders the router refactoring
substantially. It will be trivial to bring this feature back once the
refactoring is completed, but TBH even if we forget, there will
unlikely be any user who cares or runs into this.
Release-Notes: skip
Google-Bug-Id: b/244279450
Change-Id: I4740fb25475fe98104633855dd74ee646b5354ec
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index c27776a..50b4792 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -57,6 +57,7 @@
import {GroupDetailView} from '../../../models/views/group';
import {DiffViewState} from '../../../models/views/diff';
import {ChangeViewState} from '../../../models/views/change';
+import {EditViewState} from '../../../models/views/edit';
const RoutePattern = {
ROOT: '/',
@@ -283,7 +284,7 @@
this.startRouter();
}
- setParams(params: AppElementParams | GenerateUrlParameters) {
+ setParams(params: AppElementParams) {
this.routerModel.updateState({
view: params.view,
changeNum: 'changeNum' in params ? params.changeNum : undefined,
@@ -420,29 +421,23 @@
}
/**
- * Normalizes the params object, and determines if the URL needs to be
- * modified to fit the proper schema.
- *
+ * Normalizes the patchset numbers of the params object.
*/
normalizePatchRangeParams(params: PatchRangeParams) {
- if (params.basePatchNum === undefined) {
- return false;
- }
- const hasPatchNum = params.patchNum !== undefined;
- let needsRedirect = false;
+ if (params.basePatchNum === undefined) return;
// Diffing a patch against itself is invalid, so if the base and revision
// patches are equal clear the base.
if (params.patchNum && params.basePatchNum === params.patchNum) {
- needsRedirect = true;
params.basePatchNum = PARENT;
- } else if (!hasPatchNum) {
- // Regexes set basePatchNum instead of patchNum when only one is
- // specified. Redirect is not needed in this case.
+ return;
+ }
+ // Regexes set basePatchNum instead of patchNum when only one is
+ // specified.
+ if (params.patchNum === undefined) {
params.patchNum = params.basePatchNum as RevisionPatchSetNum;
params.basePatchNum = PARENT;
}
- return needsRedirect;
}
/**
@@ -1467,7 +1462,8 @@
assertIsDefined(params.project, 'project');
this.reporting.setRepoName(params.project);
this.reporting.setChangeId(changeNum);
- this.redirectOrNavigate(params);
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
}
handleCommentRoute(ctx: PageContextWithQueryMap) {
@@ -1481,7 +1477,8 @@
};
this.reporting.setRepoName(params.project ?? '');
this.reporting.setChangeId(changeNum);
- this.redirectOrNavigate(params);
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
}
handleCommentsRoute(ctx: PageContextWithQueryMap) {
@@ -1495,7 +1492,8 @@
assertIsDefined(params.project);
this.reporting.setRepoName(params.project);
this.reporting.setChangeId(changeNum);
- this.redirectOrNavigate(params);
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
}
handleDiffRoute(ctx: PageContextWithQueryMap) {
@@ -1516,7 +1514,8 @@
}
this.reporting.setRepoName(params.project ?? '');
this.reporting.setChangeId(changeNum);
- this.redirectOrNavigate(params);
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
}
handleChangeLegacyRoute(ctx: PageContextWithQueryMap) {
@@ -1544,7 +1543,7 @@
// Parameter order is based on the regex group number matched.
const project = ctx.params[0] as RepoName;
const changeNum = Number(ctx.params[1]) as NumericChangeId;
- this.redirectOrNavigate({
+ const params: EditViewState = {
project,
changeNum,
// for edit view params, patchNum cannot be undefined
@@ -1552,7 +1551,9 @@
path: ctx.params[3],
lineNum: Number(ctx.hash),
view: GerritView.EDIT,
- });
+ };
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
this.reporting.setRepoName(project);
this.reporting.setChangeId(changeNum);
}
@@ -1577,25 +1578,13 @@
location.href.replace(/[?&]forceReload=true/, '')
);
}
- this.redirectOrNavigate(params);
+ this.normalizePatchRangeParams(params);
+ this.setParams(params);
this.reporting.setRepoName(project);
this.reporting.setChangeId(changeNum);
}
- /**
- * Normalize the patch range params for a the change or diff view and
- * redirect if URL upgrade is needed.
- */
- private redirectOrNavigate(params: GenerateUrlParameters & PatchRangeParams) {
- const needsRedirect = this.normalizePatchRangeParams(params);
- if (needsRedirect) {
- this.redirect(this.generateUrl(params));
- } else {
- this.setParams(params);
- }
- }
-
handleAgreementsRoute() {
this.redirect('/settings/#Agreements');
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index 136ae2e..2fdda8d 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -328,16 +328,14 @@
basePatchNum: 4 as BasePatchSetNum,
patchNum: 4 as RevisionPatchSetNum,
};
- const needsRedirect = router.normalizePatchRangeParams(params);
- assert.isTrue(needsRedirect);
+ router.normalizePatchRangeParams(params);
assert.equal(params.basePatchNum, PARENT);
assert.equal(params.patchNum, 4 as RevisionPatchSetNum);
});
test('range n.. normalizes to n', () => {
const params: PatchRangeParams = {basePatchNum: 4 as BasePatchSetNum};
- const needsRedirect = router.normalizePatchRangeParams(params);
- assert.isFalse(needsRedirect);
+ router.normalizePatchRangeParams(params);
assert.equal(params.basePatchNum, PARENT);
assert.equal(params.patchNum, 4 as RevisionPatchSetNum);
});
@@ -1197,8 +1195,6 @@
});
suite('handleChangeRoute', () => {
- let normalizeRangeStub: sinon.SinonStub;
-
function makeParams(
_path: string,
_hash: string
@@ -1218,23 +1214,10 @@
}
setup(() => {
- normalizeRangeStub = sinon.stub(router, 'normalizePatchRangeParams');
stubRestApi('setInProjectLookup');
});
- test('needs redirect', () => {
- normalizeRangeStub.returns(true);
- sinon.stub(router, 'generateUrl').returns('foo');
- const ctx = makeParams('', '');
- router.handleChangeRoute(ctx);
- assert.isTrue(normalizeRangeStub.called);
- assert.isFalse(setParamsStub.called);
- assert.isTrue(redirectStub.calledOnce);
- assert.isTrue(redirectStub.calledWithExactly('foo'));
- });
-
test('change view', () => {
- normalizeRangeStub.returns(false);
sinon.stub(router, 'generateUrl').returns('foo');
const ctx = makeParams('', '');
assertDataToParams(ctx, 'handleChangeRoute', {
@@ -1245,11 +1228,9 @@
patchNum: 7 as RevisionPatchSetNum,
});
assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRangeStub.called);
});
test('params', () => {
- normalizeRangeStub.returns(false);
sinon.stub(router, 'generateUrl').returns('foo');
const ctx = makeParams('', '');
ctx.queryMap.set('tab', 'checks');
@@ -1270,8 +1251,6 @@
});
suite('handleDiffRoute', () => {
- let normalizeRangeStub: sinon.SinonStub;
-
function makeParams(
path: string,
hash: string
@@ -1294,23 +1273,10 @@
}
setup(() => {
- normalizeRangeStub = sinon.stub(router, 'normalizePatchRangeParams');
stubRestApi('setInProjectLookup');
});
- test('needs redirect', () => {
- normalizeRangeStub.returns(true);
- sinon.stub(router, 'generateUrl').returns('foo');
- const ctx = makeParams('', '');
- router.handleDiffRoute(ctx);
- assert.isTrue(normalizeRangeStub.called);
- assert.isFalse(setParamsStub.called);
- assert.isTrue(redirectStub.calledOnce);
- assert.isTrue(redirectStub.calledWithExactly('foo'));
- });
-
test('diff view', () => {
- normalizeRangeStub.returns(false);
sinon.stub(router, 'generateUrl').returns('foo');
const ctx = makeParams('foo/bar/baz', 'b44');
assertDataToParams(ctx, 'handleDiffRoute', {
@@ -1324,7 +1290,6 @@
lineNum: 44,
});
assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRangeStub.called);
});
test('comment route', () => {
@@ -1370,10 +1335,6 @@
});
test('handleDiffEditRoute', () => {
- const normalizeRangeSpy = sinon.spy(
- router,
- 'normalizePatchRangeParams'
- );
stubRestApi('setInProjectLookup');
const ctx = {
...createPageContext(),
@@ -1396,17 +1357,10 @@
router.handleDiffEditRoute(ctx);
assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRangeSpy.calledOnce);
- assert.deepEqual(normalizeRangeSpy.lastCall.args[0], appParams);
- assert.isFalse(normalizeRangeSpy.lastCall.returnValue);
assert.deepEqual(setParamsStub.lastCall.args[0], appParams);
});
test('handleDiffEditRoute with lineNum', () => {
- const normalizeRangeSpy = sinon.spy(
- router,
- 'normalizePatchRangeParams'
- );
stubRestApi('setInProjectLookup');
const ctx = {
...createPageContext(),
@@ -1429,17 +1383,10 @@
router.handleDiffEditRoute(ctx);
assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRangeSpy.calledOnce);
- assert.deepEqual(normalizeRangeSpy.lastCall.args[0], appParams);
- assert.isFalse(normalizeRangeSpy.lastCall.returnValue);
assert.deepEqual(setParamsStub.lastCall.args[0], appParams);
});
test('handleChangeEditRoute', () => {
- const normalizeRangeSpy = sinon.spy(
- router,
- 'normalizePatchRangeParams'
- );
stubRestApi('setInProjectLookup');
const ctx = {
...createPageContext(),
@@ -1461,9 +1408,6 @@
router.handleChangeEditRoute(ctx);
assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRangeSpy.calledOnce);
- assert.deepEqual(normalizeRangeSpy.lastCall.args[0], appParams);
- assert.isFalse(normalizeRangeSpy.lastCall.returnValue);
assert.deepEqual(setParamsStub.lastCall.args[0], appParams);
});
});
