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}`
);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 26043b32..6eb5243 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -7,7 +7,6 @@
PatchRange,
PatchSetNum,
RobotCommentInfo,
- UrlEncodedCommentId,
PathToCommentsInfoMap,
FileInfo,
PARENT,
@@ -64,26 +63,6 @@
return this._drafts;
}
- findCommentById(
- commentId?: UrlEncodedCommentId
- ): CommentInfo | DraftInfo | undefined {
- if (!commentId) return undefined;
- const findComment = (comments: {
- [path: string]: (CommentInfo | DraftInfo)[];
- }) => {
- let comment;
- for (const path of Object.keys(comments)) {
- comment = comment || comments[path].find(c => c.id === commentId);
- }
- return comment;
- };
- return (
- findComment(this._comments) ||
- findComment(this._robotComments) ||
- findComment(this._drafts)
- );
- }
-
/**
* Get an object mapping file paths to a boolean representing whether that
* path contains diff comments in the given patch set (including drafts and
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 7f38c79..6fb19a0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -52,7 +52,6 @@
NumericChangeId,
PARENT,
PatchRange,
- PatchSetNum,
PatchSetNumber,
PreferencesInfo,
RepoName,
@@ -72,11 +71,7 @@
import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
import {GrApplyFixDialog} from '../gr-apply-fix-dialog/gr-apply-fix-dialog';
import {RevisionInfo as RevisionInfoObj} from '../../shared/revision-info/revision-info';
-import {
- CommentMap,
- getPatchRangeForCommentUrl,
- isInBaseOfPatchRange,
-} from '../../../utils/comment-util';
+import {CommentMap} from '../../../utils/comment-util';
import {
EventType,
OpenFixPreviewEvent,
@@ -1455,44 +1450,6 @@
this.initCursor(leftSide);
}
- // Private but used in tests.
- displayDiffBaseAgainstLeftToast() {
- if (!this.patchRange) return;
- fireAlert(
- this,
- `Patchset ${this.patchRange.basePatchNum} vs ` +
- `${this.patchRange.patchNum} selected. Press v + \u2190 to view ` +
- `Base vs ${this.patchRange.basePatchNum}`
- );
- }
-
- private displayDiffAgainstLatestToast(latestPatchNum?: PatchSetNum) {
- if (!this.patchRange) return;
- const leftPatchset =
- this.patchRange.basePatchNum === PARENT
- ? 'Base'
- : `Patchset ${this.patchRange.basePatchNum}`;
- fireAlert(
- this,
- `${leftPatchset} vs
- ${this.patchRange.patchNum} selected\n. Press v + \u2191 to view
- ${leftPatchset} vs Patchset ${latestPatchNum}`
- );
- }
-
- private displayToasts() {
- if (!this.patchRange) return;
- if (this.patchRange.basePatchNum !== PARENT) {
- this.displayDiffBaseAgainstLeftToast();
- return;
- }
- const latestPatchNum = computeLatestPatchNum(this.allPatchSets);
- if (this.patchRange.patchNum !== latestPatchNum) {
- this.displayDiffAgainstLatestToast(latestPatchNum);
- return;
- }
- }
-
private initCommitRange() {
let commit: CommitId | undefined;
let baseCommit: CommitId | undefined;
@@ -1539,58 +1496,25 @@
let leftSide = false;
if (!this.change) return;
if (this.viewState?.childView !== ChangeChildView.DIFF) return;
- if (this.viewState?.commentId) {
- const comment = this.changeComments?.findCommentById(
- this.viewState.commentId
- );
- if (!comment) {
- fireAlert(this, 'comment not found');
- this.getNavigation().setUrl(createChangeUrl({change: this.change}));
- return;
- }
- this.getChangeModel().updatePath(comment.path);
-
- const latestPatchNum = computeLatestPatchNum(this.allPatchSets);
- if (!latestPatchNum) throw new Error('Missing allPatchSets');
- this.patchRange = getPatchRangeForCommentUrl(comment, latestPatchNum);
- leftSide = isInBaseOfPatchRange(comment, this.patchRange);
-
- this.focusLineNum = comment.line;
- } else {
- if (this.viewState.diffView?.path) {
- this.getChangeModel().updatePath(this.viewState.diffView.path);
- }
- if (this.viewState.patchNum) {
- this.patchRange = {
- patchNum: this.viewState.patchNum,
- basePatchNum: this.viewState.basePatchNum || PARENT,
- };
- }
- if (this.viewState.diffView?.lineNum) {
- this.focusLineNum = this.viewState.diffView.lineNum;
- leftSide = !!this.viewState.diffView?.leftSide;
- }
+ if (this.viewState.diffView?.path) {
+ this.getChangeModel().updatePath(this.viewState.diffView.path);
+ }
+ if (this.viewState.patchNum) {
+ this.patchRange = {
+ patchNum: this.viewState.patchNum,
+ basePatchNum: this.viewState.basePatchNum || PARENT,
+ };
+ }
+ if (this.viewState.diffView?.lineNum) {
+ this.focusLineNum = this.viewState.diffView.lineNum;
+ leftSide = !!this.viewState.diffView?.leftSide;
}
assertIsDefined(this.patchRange, 'patchRange');
this.initLineOfInterestAndCursor(leftSide);
- if (this.viewState?.commentId) {
- // url is of type /comment/{commentId} which isn't meaningful
- this.updateUrlToDiffUrl(this.focusLineNum, leftSide);
- }
-
this.commentMap = this.getPaths();
}
- // Private but used in tests.
- isFileUnchanged(diff?: DiffInfo) {
- if (!diff || !diff.content) return false;
- return !diff.content.some(
- content =>
- (content.a && !content.common) || (content.b && !content.common)
- );
- }
-
private isSameDiffLoaded(value: ChangeViewState) {
return (
this.patchRange?.basePatchNum === value.basePatchNum &&
@@ -1660,8 +1584,7 @@
// When navigating away from the page, there is a possibility that the
// patch number is no longer a part of the URL (say when navigating to
// the top-level change info view) and therefore undefined in `params`.
- // If route is of type /comment/<commentId>/ then no patchNum is present
- if (!viewState.patchNum && !viewState.diffView?.commentLink) {
+ if (!viewState.patchNum) {
this.reporting.error(
'GrDiffView',
new Error(`Invalid diff view URL, no patchNum found: ${this.viewState}`)
@@ -1691,38 +1614,6 @@
this.reporting.diffViewDisplayed();
})
.then(() => {
- const fileUnchanged = this.isFileUnchanged(this.diff);
- if (fileUnchanged && viewState.diffView?.commentLink) {
- assertIsDefined(this.change, 'change');
- assertIsDefined(this.path, 'path');
- assertIsDefined(this.patchRange, 'patchRange');
-
- if (this.patchRange.basePatchNum === PARENT) {
- // file is unchanged between Base vs X
- // hence should not show diff between Base vs Base
- return;
- }
-
- fireAlert(
- this,
- `File is unchanged between Patchset
- ${this.patchRange.basePatchNum} and
- ${this.patchRange.patchNum}. Showing diff of Base vs
- ${this.patchRange.basePatchNum}`
- );
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.basePatchNum as RevisionPatchSetNum,
- basePatchNum: PARENT,
- diffView: {path: this.path, lineNum: this.focusLineNum},
- })
- );
- return;
- }
- if (viewState.diffView?.commentLink) {
- this.displayToasts();
- }
// If the blame was loaded for a previous file and user navigates to
// another file, then we load the blame for this file too
if (this.isBlameLoaded) this.loadBlame();
@@ -2139,15 +2030,12 @@
fireAlert(this, 'Left is already base.');
return;
}
- const lineNum = this.viewState?.diffView?.commentLink
- ? this.focusLineNum
- : undefined;
this.getNavigation().setUrl(
createDiffUrl({
change: this.change,
patchNum: this.patchRange.basePatchNum as RevisionPatchSetNum,
basePatchNum: PARENT,
- diffView: {path: this.path, lineNum},
+ diffView: {path: this.path},
})
);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index fbe999b..8e242f9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -54,10 +54,10 @@
UrlEncodedCommentId,
} from '../../../types/common';
import {CursorMoveResult} from '../../../api/core';
-import {DiffInfo, Side} from '../../../api/diff';
+import {Side} from '../../../api/diff';
import {Files, GrDiffView} from './gr-diff-view';
import {DropdownItem} from '../../shared/gr-dropdown-list/gr-dropdown-list';
-import {SinonFakeTimers, SinonStub, SinonSpy} from 'sinon';
+import {SinonFakeTimers, SinonStub} from 'sinon';
import {
changeModelToken,
ChangeModel,
@@ -198,66 +198,6 @@
});
});
- suite('comment route', () => {
- let initLineOfInterestAndCursorStub: SinonStub;
- let replaceStateStub: SinonStub;
- let viewStateChangedSpy: SinonSpy;
- setup(() => {
- initLineOfInterestAndCursorStub = sinon.stub(
- element,
- 'initLineOfInterestAndCursor'
- );
- replaceStateStub = sinon.stub(history, 'replaceState');
- sinon.stub(element, 'fetchFiles');
- stubReporting('diffViewDisplayed');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- changeModel.setState({
- change: {
- ...createParsedChange(),
- revisions: createRevisions(11),
- },
- loadingStatus: LoadingStatus.LOADED,
- });
- });
-
- test('comment url resolves to comment.patch_set vs latest', () => {
- commentsModel.setState({
- comments: {
- '/COMMIT_MSG': [
- createComment('c1', 10, 2, '/COMMIT_MSG'),
- createComment('c3', 10, PARENT, '/COMMIT_MSG'),
- ],
- },
- robotComments: {},
- drafts: {},
- portedComments: {},
- portedDrafts: {},
- discardedDrafts: [],
- });
- element.viewState = {
- ...createDiffViewState(),
- commentId: 'c1' as UrlEncodedCommentId,
- patchNum: 1 as RevisionPatchSetNum,
- diffView: {path: 'abcd', commentLink: true},
- };
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(11),
- };
- return viewStateChangedSpy.returnValues[0].then(() => {
- assert.isTrue(
- initLineOfInterestAndCursorStub.calledWithExactly(true)
- );
- assert.equal(element.focusLineNum, 10);
- assert.equal(element.patchRange?.patchNum, 11 as RevisionPatchSetNum);
- assert.equal(element.patchRange?.basePatchNum, 2 as BasePatchSetNum);
- assert.isTrue(replaceStateStub.called);
- });
- });
- });
-
test('viewState change causes blame to load if it was set to true', () => {
// Blame loads for subsequent files if it was loaded for one file
element.isBlameLoaded = true;
@@ -282,166 +222,6 @@
});
});
- test('unchanged diff X vs latest from comment links navigates to base vs X', async () => {
- commentsModel.setState({
- comments: {
- '/COMMIT_MSG': [
- createComment('c1', 10, 2, '/COMMIT_MSG'),
- createComment('c3', 10, PARENT, '/COMMIT_MSG'),
- ],
- },
- robotComments: {},
- drafts: {},
- portedComments: {},
- portedDrafts: {},
- discardedDrafts: [],
- });
- stubReporting('diffViewDisplayed');
- sinon.stub(element, 'loadBlame');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- sinon.stub(element, 'isFileUnchanged').returns(true);
- const viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- changeModel.setState({
- change: {
- ...createParsedChange(),
- revisions: createRevisions(11),
- },
- loadingStatus: LoadingStatus.LOADED,
- });
- element.viewState = {
- ...createDiffViewState(),
- commentId: 'c1' as UrlEncodedCommentId,
- diffView: {path: '/COMMIT_MSG', commentLink: true},
- };
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(11),
- };
- await viewStateChangedSpy.returnValues[0];
- assert.isTrue(setUrlStub.calledOnce);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/2//COMMIT_MSG#10'
- );
- });
-
- test('unchanged diff Base vs latest from comment does not navigate', async () => {
- commentsModel.setState({
- comments: {
- '/COMMIT_MSG': [
- createComment('c1', 10, 2, '/COMMIT_MSG'),
- createComment('c3', 10, PARENT, '/COMMIT_MSG'),
- ],
- },
- robotComments: {},
- drafts: {},
- portedComments: {},
- portedDrafts: {},
- discardedDrafts: [],
- });
- stubReporting('diffViewDisplayed');
- sinon.stub(element, 'loadBlame');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- sinon.stub(element, 'isFileUnchanged').returns(true);
- const viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- changeModel.setState({
- change: {
- ...createParsedChange(),
- revisions: createRevisions(11),
- },
- loadingStatus: LoadingStatus.LOADED,
- });
- element.viewState = {
- ...createDiffViewState(),
- commentId: 'c3' as UrlEncodedCommentId,
- diffView: {path: '/COMMIT_MSG', commentLink: true},
- };
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(11),
- };
- await viewStateChangedSpy.returnValues[0];
- assert.isFalse(setUrlStub.calledOnce);
- });
-
- test('isFileUnchanged', () => {
- let diff: DiffInfo = {
- ...createDiff(),
- content: [
- {a: ['abcd'], ab: ['ef']},
- {b: ['ancd'], a: ['xx']},
- ],
- };
- assert.equal(element.isFileUnchanged(diff), false);
- diff = {
- ...createDiff(),
- content: [{ab: ['abcd']}, {ab: ['ancd']}],
- };
- assert.equal(element.isFileUnchanged(diff), true);
- diff = {
- ...createDiff(),
- content: [
- {a: ['abcd'], ab: ['ef'], common: true},
- {b: ['ancd'], ab: ['xx']},
- ],
- };
- assert.equal(element.isFileUnchanged(diff), false);
- diff = {
- ...createDiff(),
- content: [
- {a: ['abcd'], ab: ['ef'], common: true},
- {b: ['ancd'], ab: ['xx'], common: true},
- ],
- };
- assert.equal(element.isFileUnchanged(diff), true);
- });
-
- test('diff toast to go to latest is shown and not base', async () => {
- commentsModel.setState({
- comments: {
- '/COMMIT_MSG': [
- createComment('c1', 10, 2, '/COMMIT_MSG'),
- createComment('c3', 10, PARENT, '/COMMIT_MSG'),
- ],
- },
- robotComments: {},
- drafts: {},
- portedComments: {},
- portedDrafts: {},
- discardedDrafts: [],
- });
-
- stubReporting('diffViewDisplayed');
- sinon.stub(element, 'loadBlame');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- const viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- element.change = undefined;
- changeModel.setState({
- change: {
- ...createParsedChange(),
- revisions: createRevisions(11),
- },
- loadingStatus: LoadingStatus.LOADED,
- });
- element.patchRange = {
- patchNum: 2 as RevisionPatchSetNum,
- basePatchNum: 1 as BasePatchSetNum,
- };
- sinon.stub(element, 'isFileUnchanged').returns(false);
- const toastStub = sinon.stub(element, 'displayDiffBaseAgainstLeftToast');
- element.viewState = {
- ...createDiffViewState(),
- repo: 'p' as RepoName,
- commentId: 'c1' as UrlEncodedCommentId,
- diffView: {commentLink: true},
- };
- await viewStateChangedSpy.returnValues[0];
- assert.isTrue(toastStub.called);
- });
-
test('toggle left diff with a hotkey', () => {
assertIsDefined(element.diffHost);
const toggleLeftDiffStub = sinon.stub(element.diffHost, 'toggleLeftDiff');
@@ -875,28 +655,6 @@
assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1/foo');
});
- test('handleDiffBaseAgainstLeft when initially navigating to a comment', () => {
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(10),
- };
- element.patchRange = {
- patchNum: 3 as RevisionPatchSetNum,
- basePatchNum: 1 as BasePatchSetNum,
- };
- sinon.stub(element, 'viewStateChanged');
- element.viewState = {
- ...createDiffViewState(),
- diffView: {commentLink: true},
- };
- element.focusLineNum = 10;
- element.handleDiffBaseAgainstLeft();
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/1/some/path.txt#10'
- );
- });
-
test('handleDiffRightAgainstLatest', async () => {
element.path = 'foo';
element.change = {
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
index f583c2e..4174d1d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils.ts
@@ -48,6 +48,12 @@
}, 0);
}
+export function isFileUnchanged(diff: DiffInfo) {
+ return !diff.content.some(
+ content => (content.a && !content.common) || (content.b && !content.common)
+ );
+}
+
export function getResponsiveMode(
prefs: DiffPreferencesInfo,
renderPrefs?: RenderPreferences
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
index 98b4586..25dc768 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-utils_test.ts
@@ -4,8 +4,15 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {assert} from '@open-wc/testing';
+import {DiffInfo} from '../../../api/diff';
import '../../../test/common-test-setup';
-import {createElementDiff, formatText, createTabWrapper} from './gr-diff-utils';
+import {createDiff} from '../../../test/test-data-generators';
+import {
+ createElementDiff,
+ formatText,
+ createTabWrapper,
+ isFileUnchanged,
+} from './gr-diff-utils';
const LINE_BREAK_HTML = '<span class="gr-diff br"></span>';
@@ -156,4 +163,36 @@
expectTextLength('abc\tde\t', 10, 20);
expectTextLength('\t\t\t\t\t', 20, 100);
});
+
+ test('isFileUnchanged', () => {
+ let diff: DiffInfo = {
+ ...createDiff(),
+ content: [
+ {a: ['abcd'], ab: ['ef']},
+ {b: ['ancd'], a: ['xx']},
+ ],
+ };
+ assert.equal(isFileUnchanged(diff), false);
+ diff = {
+ ...createDiff(),
+ content: [{ab: ['abcd']}, {ab: ['ancd']}],
+ };
+ assert.equal(isFileUnchanged(diff), true);
+ diff = {
+ ...createDiff(),
+ content: [
+ {a: ['abcd'], ab: ['ef'], common: true},
+ {b: ['ancd'], ab: ['xx']},
+ ],
+ };
+ assert.equal(isFileUnchanged(diff), false);
+ diff = {
+ ...createDiff(),
+ content: [
+ {a: ['abcd'], ab: ['ef'], common: true},
+ {b: ['ancd'], ab: ['xx'], common: true},
+ ],
+ };
+ assert.equal(isFileUnchanged(diff), true);
+ });
});
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 3f3c76b..317e511 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -42,6 +42,7 @@
repo: RepoName;
patchNum?: RevisionPatchSetNum;
basePatchNum?: BasePatchSetNum;
+ /** Refers to comment on COMMENTS tab in OVERVIEW. */
commentId?: UrlEncodedCommentId;
// TODO: Move properties that only apply to OVERVIEW into a submessage.
@@ -81,7 +82,6 @@
path?: string;
lineNum?: number;
leftSide?: boolean;
- commentLink?: boolean;
};
/** These properties apply to the EDIT child view only. */
diff --git a/polygerrit-ui/app/models/views/diff.ts b/polygerrit-ui/app/models/views/diff.ts
index b742c45..961c9d5 100644
--- a/polygerrit-ui/app/models/views/diff.ts
+++ b/polygerrit-ui/app/models/views/diff.ts
@@ -36,6 +36,9 @@
suffix += state.diffView.lineNum;
}
+ // TODO: Move creating of comment URLs to a separate function. We are
+ // "abusing" the `commentId` property, which should only be used for pointing
+ // to comment in the COMMENTS tab of the OVERVIEW page.
if (state.commentId) {
suffix = `/comment/${state.commentId}` + suffix;
}
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 8af5beb..a92f0f8 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -598,3 +598,17 @@
.includes(account.email)
);
}
+
+export function findComment(
+ comments: {
+ [path: string]: (CommentInfo | DraftInfo)[];
+ },
+ commentId: UrlEncodedCommentId
+) {
+ if (!commentId) return undefined;
+ let comment;
+ for (const path of Object.keys(comments)) {
+ comment = comment || comments[path].find(c => c.id === commentId);
+ }
+ return comment;
+}
