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.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts index 28b7e3b..ac54d35 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -10,7 +10,11 @@ } from '../../../utils/page-wrapper-utils'; import {NavigationService} from '../gr-navigation/gr-navigation'; import {getAppContext} from '../../../services/app-context'; -import {convertToPatchSetNum} from '../../../utils/patch-set-util'; +import { + computeAllPatchSets, + computeLatestPatchNum, + convertToPatchSetNum, +} from '../../../utils/patch-set-util'; import {assertIsDefined} from '../../../utils/common-util'; import { BasePatchSetNum, @@ -27,7 +31,7 @@ import {AppElement, AppElementParams} from '../../gr-app-types'; import {LocationChangeEventDetail} from '../../../types/events'; import {GerritView, RouterModel} from '../../../services/router/router-model'; -import {firePageError} from '../../../utils/event-util'; +import {fireAlert, firePageError} from '../../../utils/event-util'; import {windowLocationReload} from '../../../utils/dom-util'; import { getBaseUrl, @@ -87,6 +91,14 @@ import {SearchViewModel, SearchViewState} from '../../../models/views/search'; import {DashboardSection} from '../../../utils/dashboard-util'; import {Subscription} from 'rxjs'; +import { + addPath, + findComment, + getPatchRangeForCommentUrl, + isInBaseOfPatchRange, +} from '../../../utils/comment-util'; +import {createDiffUrl} from '../../../models/views/diff'; +import {isFileUnchanged} from '../../../embed/diff/gr-diff/gr-diff-utils'; const RoutePattern = { ROOT: '/', @@ -1470,22 +1482,57 @@ this.changeViewModel.setState(state); } - handleCommentRoute(ctx: PageContext) { + async handleCommentRoute(ctx: PageContext) { const changeNum = Number(ctx.params[1]) as NumericChangeId; - const state: ChangeViewState = { - repo: ctx.params[0] as RepoName, + const repo = ctx.params[0] as RepoName; + const commentId = ctx.params[2] as UrlEncodedCommentId; + + const comments = await this.restApiService.getDiffComments(changeNum); + const change = await this.restApiService.getChangeDetail(changeNum); + + const comment = findComment(addPath(comments), commentId); + const path = comment?.path; + const patchsets = computeAllPatchSets(change); + const latestPatchNum = computeLatestPatchNum(patchsets); + if (!comment || !path || !latestPatchNum) { + this.show404(); + return; + } + let {basePatchNum, patchNum} = getPatchRangeForCommentUrl( + comment, + latestPatchNum + ); + + if (basePatchNum !== PARENT) { + const diff = await this.restApiService.getDiff( + changeNum, + basePatchNum, + patchNum, + path + ); + if (diff && isFileUnchanged(diff)) { + fireAlert( + document, + `File is unchanged between Patchset ${basePatchNum} and ${patchNum}. + Showing diff of Base vs ${basePatchNum}.` + ); + patchNum = basePatchNum as RevisionPatchSetNum; + basePatchNum = PARENT; + } + } + + const diffUrl = createDiffUrl({ changeNum, - commentId: ctx.params[2] as UrlEncodedCommentId, - view: GerritView.CHANGE, - childView: ChangeChildView.DIFF, - diffView: {commentLink: true}, - }; - this.reporting.setRepoName(state.repo ?? ''); - this.reporting.setChangeId(changeNum); - this.normalizePatchRangeParams(state); - // Note that router model view must be updated before view models. - this.setState(state); - this.changeViewModel.setState(state); + repo, + patchNum, + basePatchNum, + diffView: { + path, + lineNum: comment.line, + leftSide: isInBaseOfPatchRange(comment, {basePatchNum, patchNum}), + }, + }); + this.redirect(diffUrl); } handleCommentsRoute(ctx: PageContext) {
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}` ); });