Move handling of COMMENT route from diff view to router
The diff view handling the COMMENT router leads to a lot of special
casing. Translating the comment ID to a proper diff URL is rather the
responsibility of the router. Let's move it there.
Apart from just moving there is one small behavior change: We do not
show a toast about the chosen patchsets when initially navigating to
a diff. That seemed to be unnecessary complexity that is not really
beneficial for the user. If the comment was made on ps 5 and we are
showing ps5-vs-latest, then there is no reason to call this out. The
patch range picker shows that information and the user is expecting
it.
This change is also important for moving code from diff view into the
change model. For example we want to allow the diff view to just
observe the patch range in the change model instead of maintaining its
own `patchRange` property. This is done in a later change.
Release-Notes: skip
Google-Bug-Id: b/247042673
Change-Id: I66988bd2b2517b53dc8060ed920e67dfa2ea0d5d
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 ce6e933..d8761bf 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
@@ -31,6 +31,13 @@
import {ChangeChildView, ChangeViewState} from '../../../models/views/change';
import {PatchRangeParams} from '../../../utils/url-util';
import {testResolver} from '../../../test/common-test-setup';
+import {
+ createComment,
+ createDiff,
+ createParsedChange,
+ createRevision,
+} from '../../../test/test-data-generators';
+import {ParsedChangeInfo} from '../../../types/types';
suite('gr-router tests', () => {
let router: GrRouter;
@@ -1209,25 +1216,74 @@
assert.isFalse(redirectStub.called);
});
- test('comment route', () => {
- const url = '/c/gerrit/+/264833/comment/00049681_f34fd6a9/';
+ test('comment route base..1', async () => {
+ const change: ParsedChangeInfo = createParsedChange();
+ const repo = change.project;
+ const changeNum = change._number;
+ const ps = 1 as RevisionPatchSetNum;
+ const line = 23;
+ const id = '00049681_f34fd6a9' as UrlEncodedCommentId;
+ stubRestApi('getChangeDetail').resolves(change);
+ stubRestApi('getDiffComments').resolves({
+ filepath: [{...createComment(), id, patch_set: ps, line}],
+ });
+
+ const url = `/c/${repo}/+/${changeNum}/comment/${id}/`;
const groups = url.match(_testOnly_RoutePattern.COMMENT);
- assert.deepEqual(groups!.slice(1), [
- 'gerrit', // project
- '264833', // changeNum
- '00049681_f34fd6a9', // commentId
- ]);
- assertctxToParams(
- {params: groups!.slice(1)} as any,
- 'handleCommentRoute',
- {
- repo: 'gerrit' as RepoName,
- changeNum: 264833 as NumericChangeId,
- commentId: '00049681_f34fd6a9' as UrlEncodedCommentId,
- view: GerritView.CHANGE,
- childView: ChangeChildView.DIFF,
- diffView: {commentLink: true},
- }
+ assert.deepEqual(groups!.slice(1), [repo, `${changeNum}`, id]);
+
+ await router.handleCommentRoute({params: groups!.slice(1)} as any);
+ assert.isTrue(redirectStub.calledOnce);
+ assert.equal(
+ redirectStub.lastCall.args[0],
+ `/c/${repo}/+/${changeNum}/${ps}/filepath#${line}`
+ );
+ });
+
+ test('comment route 1..2', async () => {
+ const change: ParsedChangeInfo = {
+ ...createParsedChange(),
+ revisions: {
+ abc: createRevision(1),
+ def: createRevision(2),
+ },
+ };
+ const repo = change.project;
+ const changeNum = change._number;
+ const ps = 1 as RevisionPatchSetNum;
+ const line = 23;
+ const id = '00049681_f34fd6a9' as UrlEncodedCommentId;
+
+ stubRestApi('getChangeDetail').resolves(change);
+ stubRestApi('getDiffComments').resolves({
+ filepath: [{...createComment(), id, patch_set: ps, line}],
+ });
+ const diffStub = stubRestApi('getDiff');
+
+ const url = `/c/${repo}/+/${changeNum}/comment/${id}/`;
+ const groups = url.match(_testOnly_RoutePattern.COMMENT);
+
+ // If getDiff() returns a diff with changes, then we will compare
+ // the patchset of the comment (1) against latest (2).
+ diffStub.onFirstCall().resolves(createDiff());
+ await router.handleCommentRoute({params: groups!.slice(1)} as any);
+ assert.isTrue(redirectStub.calledOnce);
+ assert.equal(
+ redirectStub.lastCall.args[0],
+ `/c/${repo}/+/${changeNum}/${ps}..2/filepath#b${line}`
+ );
+
+ // If getDiff() returns an unchanged diff, then we will compare
+ // the patchset of the comment (1) against base.
+ diffStub.onSecondCall().resolves({
+ ...createDiff(),
+ content: [],
+ });
+ await router.handleCommentRoute({params: groups!.slice(1)} as any);
+ assert.isTrue(redirectStub.calledTwice);
+ assert.equal(
+ redirectStub.lastCall.args[0],
+ `/c/${repo}/+/${changeNum}/${ps}/filepath#${line}`
);
});
