Navigate change legacy routes to current change route
Legacy change route is missing the project so lookup the project
from the changeNum and route to the exising current change url.
Google-bug-id: b/199258043
Change-Id: I1b5ec90a89b37f640999d95cee710d2b4d689543
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 4a6f009..8e70eb9 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -177,8 +177,8 @@
CHANGE_ID_QUERY: /^\/id\/(I[0-9a-f]{40})$/,
- // Matches /c/<changeNum>/[<basePatchNum>..][<patchNum>][/].
- CHANGE_LEGACY: /^\/c\/(\d+)\/?(((-?\d+|edit)(\.\.(\d+|edit))?))?\/?$/,
+ // Matches /c/<changeNum>/[*][/].
+ CHANGE_LEGACY: /^\/c\/(\d+)\/(.*)$/,
CHANGE_NUMBER_LEGACY: /^\/(\d+)\/?/,
// Matches
@@ -210,10 +210,6 @@
// Matches /c/<project>/+/<changeNum>/[<patchNum|edit>]/<path>,edit[#lineNum]
DIFF_EDIT: /^\/c\/(.+)\/\+\/(\d+)\/(\d+|edit)\/(.+),edit(#\d+)?$/,
- // Matches non-project-relative
- // /c/<changeNum>/[<basePatchNum>..]<patchNum>/<path>.
- DIFF_LEGACY: /^\/c\/(\d+)\/((-?\d+|edit)(\.\.(\d+|edit))?)\/(.+)/,
-
// Matches diff routes using @\d+ to specify a file name (whether or not
// the project name is included).
// eslint-disable-next-line max-len
@@ -287,15 +283,6 @@
type QueryStringItem = [string, string]; // [key, value]
-type GenerateUrlLegacyChangeViewParameters = Omit<
- GenerateUrlChangeViewParameters,
- 'project'
->;
-type GenerateUrlLegacyDiffViewParameters = Omit<
- GenerateUrlDiffViewParameters,
- 'project'
->;
-
interface PatchRangeParams {
patchNum?: PatchSetNum;
basePatchNum?: BasePatchSetNum;
@@ -679,37 +666,6 @@
}
/**
- * Given a set of params without a project, gets the project from the rest
- * API project lookup and then sets the app params.
- */
- _normalizeLegacyRouteParams(
- params: Readonly<
- | GenerateUrlLegacyChangeViewParameters
- | GenerateUrlLegacyDiffViewParameters
- >
- ) {
- if (!params.changeNum) {
- return Promise.resolve();
- }
-
- return this.restApiService
- .getFromProjectLookup(params.changeNum)
- .then(project => {
- // Show a 404 and terminate if the lookup request failed. Attempting
- // to redirect after failing to get the project loops infinitely.
- if (!project) {
- this._show404();
- return;
- }
- const updatedParams:
- | GenerateUrlChangeViewParameters
- | GenerateUrlDiffViewParameters = {...params, project};
- this._normalizePatchRangeParams(updatedParams);
- this._redirect(this._generateUrl(updatedParams));
- });
- }
-
- /**
* Normalizes the params object, and determines if the URL needs to be
* modified to fit the proper schema.
*
@@ -1100,8 +1056,6 @@
this._mapRoute(RoutePattern.CHANGE_LEGACY, '_handleChangeLegacyRoute');
- this._mapRoute(RoutePattern.DIFF_LEGACY, '_handleDiffLegacyRoute');
-
this._mapRoute(RoutePattern.AGREEMENTS, '_handleAgreementsRoute', true);
this._mapRoute(
@@ -1666,41 +1620,26 @@
}
_handleChangeLegacyRoute(ctx: PageContextWithQueryMap) {
- // Parameter order is based on the regex group number matched.
- const params: GenerateUrlLegacyChangeViewParameters = {
- changeNum: Number(ctx.params[0]) as NumericChangeId,
- basePatchNum: convertToPatchSetNum(ctx.params[3]) as BasePatchSetNum,
- patchNum: convertToPatchSetNum(ctx.params[5]),
- view: GerritView.CHANGE,
- querystring: ctx.querystring,
- };
-
- this._normalizeLegacyRouteParams(params);
+ const changeNum = Number(ctx.params[0]) as NumericChangeId;
+ if (!changeNum) {
+ this._show404();
+ return;
+ }
+ this.restApiService.getFromProjectLookup(changeNum).then(project => {
+ // Show a 404 and terminate if the lookup request failed. Attempting
+ // to redirect after failing to get the project loops infinitely.
+ if (!project) {
+ this._show404();
+ return;
+ }
+ this._redirect(`/c/${project}/+/${changeNum}/${ctx.params[1]}`);
+ });
}
_handleLegacyLinenum(ctx: PageContextWithQueryMap) {
this._redirect(ctx.path.replace(LEGACY_LINENUM_PATTERN, '#$1'));
}
- _handleDiffLegacyRoute(ctx: PageContextWithQueryMap) {
- // Parameter order is based on the regex group number matched.
- const params: GenerateUrlLegacyDiffViewParameters = {
- changeNum: Number(ctx.params[0]) as NumericChangeId,
- basePatchNum: convertToPatchSetNum(ctx.params[2]) as BasePatchSetNum,
- patchNum: convertToPatchSetNum(ctx.params[4]),
- path: ctx.params[5],
- view: GerritView.DIFF,
- };
-
- const address = this._parseLineAddress(ctx.hash);
- if (address) {
- params.leftSide = address.leftSide;
- params.lineNum = address.lineNum;
- }
-
- this._normalizeLegacyRouteParams(params);
- }
-
_handleDiffEditRoute(ctx: PageContextWithQueryMap) {
// Parameter order is based on the regex group number matched.
const project = ctx.params[0] as RepoName;
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
index 4c7855b..b91bf0c 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.js
@@ -192,7 +192,6 @@
'_handleDiffRoute',
'_handleDefaultRoute',
'_handleChangeLegacyRoute',
- '_handleDiffLegacyRoute',
'_handleDocumentationRedirectRoute',
'_handleDocumentationSearchRoute',
'_handleDocumentationSearchRedirectRoute',
@@ -536,66 +535,6 @@
});
suite('param normalization', () => {
- let projectLookupStub;
- let generateUrlStub;
-
- setup(() => {
- projectLookupStub = stubRestApi('getFromProjectLookup');
- generateUrlStub = sinon.stub(element, '_generateUrl');
- });
-
- suite('_normalizeLegacyRouteParams', () => {
- let rangeStub;
- let redirectStub;
- let show404Stub;
-
- setup(() => {
- rangeStub = sinon.stub(element, '_normalizePatchRangeParams')
- .returns(Promise.resolve());
- redirectStub = sinon.stub(element, '_redirect');
- show404Stub = sinon.stub(element, '_show404');
- });
-
- test('w/o changeNum', () => {
- projectLookupStub.returns(Promise.resolve('foo/bar'));
- const params = {};
- return element._normalizeLegacyRouteParams(params).then(() => {
- assert.isFalse(generateUrlStub.calledOnce);
- assert.isFalse(projectLookupStub.called);
- assert.isFalse(rangeStub.called);
- assert.isFalse(redirectStub.called);
- assert.isFalse(show404Stub.called);
- });
- });
-
- test('w/ changeNum', () => {
- projectLookupStub.returns(Promise.resolve('foo/bar'));
- const params = {changeNum: 1234};
-
- return element._normalizeLegacyRouteParams(params).then(() => {
- assert.isTrue(generateUrlStub.calledOnce);
- const updatedParams = generateUrlStub.lastCall.args[0];
- assert.isTrue(projectLookupStub.called);
- assert.isTrue(rangeStub.called);
- assert.equal(updatedParams.project, 'foo/bar');
- assert.isTrue(redirectStub.calledOnce);
- assert.isFalse(show404Stub.called);
- });
- });
-
- test('halts on project lookup failure', () => {
- projectLookupStub.returns(Promise.resolve(undefined));
- const params = {changeNum: 1234};
- return element._normalizeLegacyRouteParams(params).then(() => {
- assert.isFalse(generateUrlStub.calledOnce);
- assert.isTrue(projectLookupStub.called);
- assert.isFalse(rangeStub.called);
- assert.isFalse(redirectStub.called);
- assert.isTrue(show404Stub.calledOnce);
- });
- });
- });
-
suite('_normalizePatchRangeParams', () => {
test('range n..n normalizes to n', () => {
const params = {basePatchNum: 4, patchNum: 4};
@@ -1367,58 +1306,19 @@
assert.isTrue(redirectStub.calledWithExactly('/c/12345'));
});
- test('_handleChangeLegacyRoute', () => {
- const normalizeRouteStub = sinon.stub(element,
- '_normalizeLegacyRouteParams');
+ test('_handleChangeLegacyRoute', async () => {
+ stubRestApi('getFromProjectLookup').returns(Promise.resolve('project'));
const ctx = {
params: [
1234, // 0 Change number
- null, // 1 Unused
- null, // 2 Unused
- 6, // 3 Base patch number
- null, // 4 Unused
- 9, // 5 Patch number
+ 'comment/6789',
],
querystring: '',
};
element._handleChangeLegacyRoute(ctx);
- assert.isTrue(normalizeRouteStub.calledOnce);
- assert.deepEqual(normalizeRouteStub.lastCall.args[0], {
- changeNum: 1234,
- basePatchNum: 6,
- patchNum: 9,
- view: GerritView.CHANGE,
- querystring: '',
- });
- });
-
- test('_handleDiffLegacyRoute', () => {
- const normalizeRouteStub = sinon.stub(element,
- '_normalizeLegacyRouteParams');
- const ctx = {
- params: [
- 1234, // 0 Change number
- null, // 1 Unused
- 3, // 2 Base patch number
- null, // 3 Unused
- 8, // 4 Patch number
- 'foo/bar', // 5 Diff path
- ],
- path: '/c/1234/3..8/foo/bar',
- hash: 'b123',
- };
- element._handleDiffLegacyRoute(ctx);
- assert.isFalse(redirectStub.called);
- assert.isTrue(normalizeRouteStub.calledOnce);
- assert.deepEqual(normalizeRouteStub.lastCall.args[0], {
- changeNum: 1234,
- basePatchNum: 3,
- patchNum: 8,
- view: GerritView.DIFF,
- path: 'foo/bar',
- lineNum: 123,
- leftSide: true,
- });
+ await flush();
+ assert.isTrue(redirectStub.calledWithExactly('/c/project/+/1234' +
+ '/comment/6789'));
});
test('_handleLegacyLinenum w/ @321', () => {
