opendev/gerrit
Code Issues Proposed changes

Allow porting comments to contribute to comment count string

Browse Source 
Ported comments now contribute to the thread, unresolved and draft count
shown on the file list and the diff view dropdown.
For the file list count and the file dropdown in diff view, we have a
path and patchset range already defined.
We then figure out if the ported thread will be rendered or not and add
it to the count accordingly.

Also refactor to
* Use getThreadsBySideForFile to calculate thread count which
automatically includes ported comments since we are now calculating
these numbers based on threads and not comments.

* Merge comment string computed by gr-file-list and gr-diff-view and
move it to gr-comment-api.

Change-Id: Ibf6959abf5a0cf437a4964ef32e35f6304d0de86
This commit is contained in:
Dhruv Srivastava
2020-12-14 08:39:32 +01:00
parent e48fa286d6
commit 430c1dd3d6
9 changed files with 245 additions and 253 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
View File

@@ -610,44 +610,16 @@ export class GrFileList extends KeyboardShortcutMixin(
_computeCommentsString(
changeComments?: ChangeComments,
patchRange?: PatchRange,
path?: string
file?: NormalizedFileInfo
) {
if (
changeComments === undefined ||
patchRange === undefined ||
path === undefined
file?.__path === undefined
) {
return '';
}
const unresolvedCount =
changeComments.computeUnresolvedNum({
patchNum: patchRange.basePatchNum,
path,
}) +
changeComments.computeUnresolvedNum({
patchNum: patchRange.patchNum,
path,
});
const commentThreadCount =
changeComments.computeCommentThreadCount({
patchNum: patchRange.basePatchNum,
path,
}) +
changeComments.computeCommentThreadCount({
patchNum: patchRange.patchNum,
path,
});
const commentString = pluralize(commentThreadCount, 'comment');
const unresolvedString =
unresolvedCount === 0 ? '' : `${unresolvedCount} unresolved`;
return (
commentString +
// Add a space if both comments and unresolved
(commentString && unresolvedString ? ' ' : '') +
// Add parentheses around unresolved if it exists.
(unresolvedString ? `(${unresolvedString})` : '')
);
return changeComments.computeCommentsString(patchRange, file.__path, file);
}
/**

3
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.ts
View File

@@ -456,8 +456,7 @@ export const htmlTemplate = html`
>
<span
><!--
-->[[_computeCommentsString(changeComments, patchRange,
file.__path)]]<!--
-->[[_computeCommentsString(changeComments, patchRange, file)]]<!--
--></span
>
<span class="noCommentsScreenReaderText">

140
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
View File

@@ -28,8 +28,8 @@ import {runA11yAudit} from '../../../test/a11y-test-utils.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
import {TestKeyboardShortcutBinder} from '../../../test/test-utils.js';
import {Shortcut} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.js';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api.js';
import {createCommentThreads} from '../../../utils/comment-util.js';
import {createChangeComments} from '../../../test/test-data-generators.js';
const commentApiMock = createCommentApiMockWithTemplateElement(
'gr-file-list-comment-api-mock', html`
@@ -352,101 +352,7 @@ suite('gr-file-list tests', () => {
});
test('comment filtering', () => {
const comments = {
'/COMMIT_MSG': [
{
patch_set: 1,
message: 'Done',
updated: '2017-02-08 16:40:49',
id: '1',
},
{
patch_set: 1,
message: 'oh hay',
updated: '2017-02-09 16:40:49',
id: '2',
},
{
patch_set: 2,
message: 'hello',
updated: '2017-02-10 16:40:49',
id: '3',
},
],
'myfile.txt': [
{
patch_set: 1,
message: 'good news!',
updated: '2017-02-08 16:40:49',
id: '4',
},
{
patch_set: 2,
message: 'wat!?',
updated: '2017-02-09 16:40:49',
id: '5',
},
{
patch_set: 2,
message: 'hi',
updated: '2017-02-10 16:40:49',
id: '6',
},
],
'unresolved.file': [
{
patch_set: 2,
message: 'wat!?',
updated: '2017-02-09 16:40:49',
id: '7',
unresolved: true,
},
{
patch_set: 2,
message: 'hi',
updated: '2017-02-10 16:40:49',
id: '8',
in_reply_to: '7',
unresolved: false,
},
{
patch_set: 2,
message: 'good news!',
updated: '2017-02-08 16:40:49',
id: '9',
unresolved: true,
},
],
};
const drafts = {
'/COMMIT_MSG': [
{
patch_set: 1,
message: 'hi',
updated: '2017-02-15 16:40:49',
id: '10',
unresolved: true,
},
{
patch_set: 1,
message: 'fyi',
updated: '2017-02-15 16:40:49',
id: '11',
unresolved: false,
},
],
'unresolved.file': [
{
patch_set: 1,
message: 'hi',
updated: '2017-02-11 16:40:49',
id: '12',
unresolved: false,
},
],
};
element.changeComments = new ChangeComments(comments, {}, drafts, 123);
element.changeComments = createChangeComments();
const parentTo1 = {
basePatchNum: 'PARENT',
patchNum: 1,
@@ -462,12 +368,6 @@ suite('gr-file-list tests', () => {
patchNum: 2,
};
assert.equal(
element._computeCommentsString(element.changeComments, parentTo1,
'/COMMIT_MSG', 'comment'), '2 comments (1 unresolved)');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'/COMMIT_MSG', 'comment'), '3 comments (1 unresolved)');
assert.equal(
element._computeCommentsStringMobile(element.changeComments, parentTo1
, '/COMMIT_MSG'), '2c');
@@ -486,12 +386,6 @@ suite('gr-file-list tests', () => {
assert.equal(
element._computeDraftsStringMobile(element.changeComments, _1To2,
'unresolved.file'), '1d');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo1,
'myfile.txt', 'comment'), '1 comment');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'myfile.txt', 'comment'), '3 comments');
assert.equal(
element._computeCommentsStringMobile(
element.changeComments,
@@ -513,12 +407,6 @@ suite('gr-file-list tests', () => {
assert.equal(
element._computeDraftsStringMobile(element.changeComments, _1To2,
'myfile.txt'), '');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo1,
'file_added_in_rev2.txt', 'comment'), '');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'file_added_in_rev2.txt', 'comment'), '');
assert.equal(
element._computeCommentsStringMobile(
element.changeComments,
@@ -540,12 +428,6 @@ suite('gr-file-list tests', () => {
assert.equal(
element._computeDraftsStringMobile(element.changeComments, _1To2,
'file_added_in_rev2.txt'), '');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo2,
'/COMMIT_MSG', 'comment'), '1 comment');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'/COMMIT_MSG', 'comment'), '3 comments (1 unresolved)');
assert.equal(
element._computeCommentsStringMobile(
element.changeComments,
@@ -570,12 +452,6 @@ suite('gr-file-list tests', () => {
assert.equal(
element._computeDraftsStringMobile(element.changeComments, _1To2,
'/COMMIT_MSG'), '2d');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo2,
'myfile.txt', 'comment'), '2 comments');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'myfile.txt', 'comment'), '3 comments');
assert.equal(
element._computeCommentsStringMobile(
element.changeComments,
@@ -591,18 +467,6 @@ suite('gr-file-list tests', () => {
assert.equal(
element._computeDraftsStringMobile(element.changeComments, _1To2,
'myfile.txt'), '');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo2,
'file_added_in_rev2.txt', 'comment'), '');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'file_added_in_rev2.txt', 'comment'), '');
assert.equal(
element._computeCommentsString(element.changeComments, parentTo2,
'unresolved.file', 'comment'), '2 comments (1 unresolved)');
assert.equal(
element._computeCommentsString(element.changeComments, _1To2,
'unresolved.file', 'comment'), '2 comments (1 unresolved)');
});
test('_reviewedTitle', () => {

41
polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
View File

@@ -29,6 +29,7 @@ import {
UrlEncodedCommentId,
NumericChangeId,
PathToCommentsInfoMap,
FileInfo,
} from '../../../types/common';
import {hasOwnProperty} from '../../../utils/common-util';
import {
@@ -51,6 +52,7 @@ import {PatchSetFile, PatchNumOnly, isPatchSetFile} from '../../../types/types';
import {appContext} from '../../../services/app-context';
import {CommentSide, Side} from '../../../constants/constants';
import {KnownExperimentId} from '../../../services/flags/flags';
import {pluralize} from '../../../utils/string-util';
export type CommentIdToCommentThreadMap = {
[urlEncodedCommentId: string]: CommentThread;
@@ -485,6 +487,45 @@ export class ChangeComments {
return this._commentObjToArray(allDrafts).length;
}
/**
* @param includeUnmodified Included unmodified status of the file in the
* comment string or not. For files we opt of chip instead of a string.
* @param filterPatchset Only count threads which belong to this patchset
*/
computeCommentsString(
patchRange?: PatchRange,
path?: string,
changeFileInfo?: FileInfo,
includeUnmodified?: boolean
) {
if (!path) return '';
if (!patchRange) return '';
const threads = this.getThreadsBySideForFile({path}, patchRange);
const commentThreadCount = threads.filter(thread => !isDraftThread(thread))
.length;
const unresolvedCount = threads.reduce((cnt, thread) => {
if (isUnresolved(thread)) cnt += 1;
return cnt;
}, 0);
const commentThreadString = pluralize(commentThreadCount, 'comment');
const unresolvedString =
unresolvedCount === 0 ? '' : `${unresolvedCount} unresolved`;
const unmodifiedString =
includeUnmodified && changeFileInfo?.status === 'U' ? 'no changes' : '';
return (
commentThreadString +
// Add a space if both comments and unresolved
(commentThreadString && unresolvedString ? ' ' : '') +
// Add parentheses around unresolved if it exists.
(unresolvedString ? `(${unresolvedString})` : '') +
(unmodifiedString ? `(${unmodifiedString})` : '')
);
}
/**
* Computes a number of unresolved comment threads in a given file and path.
*/

87
polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.js
View File

@@ -20,7 +20,7 @@ import './gr-comment-api.js';
import {ChangeComments} from './gr-comment-api.js';
import {CommentSide} from '../../../constants/constants.js';
import {isInRevisionOfPatchRange, isInBaseOfPatchRange, createCommentThreads, isDraftThread, isUnresolved} from '../../../utils/comment-util.js';
import {createDraft, createComment} from '../../../test/test-data-generators.js';
import {createDraft, createComment, createChangeComments} from '../../../test/test-data-generators.js';
const basicFixture = fixtureFromElement('gr-comment-api');
@@ -192,7 +192,7 @@ suite('gr-comment-api tests', () => {
portedComments = {
'karma.conf.js': [{
...comment1,
patchNum: 4,
patch_set: 4,
range: {
start_line: 136,
start_character: 16,
@@ -278,6 +278,17 @@ suite('gr-comment-api tests', () => {
).length, 0);
});
test('ported comments contribute to comment count', () => {
assert.equal(changeComments.computeCommentsString(
{basePatchNum: 'PARENT', patchNum: 2}, 'karma.conf.js',
{__path: 'karma.conf.js'}), '2 comments (1 unresolved)');
// comment1 is ported over to patchset 4
assert.equal(changeComments.computeCommentsString(
{basePatchNum: 'PARENT', patchNum: 4}, 'karma.conf.js',
{__path: 'karma.conf.js'}), '1 comment (1 unresolved)');
});
test('drafts are ported over', () => {
changeComments = new ChangeComments(
{}/* comments */,
@@ -612,6 +623,78 @@ suite('gr-comment-api tests', () => {
element._changeComments.computeUnresolvedNum(1, 'path'), 0);
});
test('computeCommentsString', () => {
const changeComments = createChangeComments();
const parentTo1 = {
basePatchNum: 'PARENT',
patchNum: 1,
};
const parentTo2 = {
basePatchNum: 'PARENT',
patchNum: 2,
};
const _1To2 = {
basePatchNum: 1,
patchNum: 2,
};
assert.equal(
changeComments.computeCommentsString(parentTo1, '/COMMIT_MSG',
{__path: '/COMMIT_MSG'}), '2 comments (1 unresolved)');
assert.equal(
changeComments.computeCommentsString(parentTo1, '/COMMIT_MSG',
{__path: '/COMMIT_MSG', status: 'U'}, true),
'2 comments (1 unresolved)(no changes)');
assert.equal(
changeComments.computeCommentsString(_1To2, '/COMMIT_MSG',
{__path: '/COMMIT_MSG'}), '3 comments (1 unresolved)');
assert.equal(
changeComments.computeCommentsString(parentTo1, 'myfile.txt',
{__path: 'myfile.txt'}), '1 comment');
assert.equal(
changeComments.computeCommentsString(_1To2, 'myfile.txt',
{__path: 'myfile.txt'}), '3 comments');
assert.equal(
changeComments.computeCommentsString(parentTo1,
'file_added_in_rev2.txt',
{__path: 'file_added_in_rev2.txt'}), '');
assert.equal(
changeComments.computeCommentsString(_1To2,
'file_added_in_rev2.txt',
{__path: 'file_added_in_rev2.txt'}), '');
assert.equal(
changeComments.computeCommentsString(parentTo2, '/COMMIT_MSG',
{__path: '/COMMIT_MSG'}), '1 comment');
assert.equal(
changeComments.computeCommentsString(_1To2, '/COMMIT_MSG',
{__path: '/COMMIT_MSG'}), '3 comments (1 unresolved)');
assert.equal(
changeComments.computeCommentsString(parentTo2, 'myfile.txt',
{__path: 'myfile.txt'}), '2 comments');
assert.equal(
changeComments.computeCommentsString(_1To2, 'myfile.txt',
{__path: 'myfile.txt'}), '3 comments');
assert.equal(
changeComments.computeCommentsString(parentTo2,
'file_added_in_rev2.txt',
{__path: 'file_added_in_rev2.txt'}), '');
assert.equal(
changeComments.computeCommentsString(_1To2,
'file_added_in_rev2.txt',
{__path: 'file_added_in_rev2.txt'}), '');
assert.equal(
changeComments.computeCommentsString(parentTo2, 'unresolved.file',
{__path: 'unresolved.file'}), '2 comments (1 unresolved)');
assert.equal(
changeComments.computeCommentsString(_1To2, 'unresolved.file',
{__path: 'unresolved.file'}), '2 comments (1 unresolved)');
});
test('computeCommentThreadCount', () => {
assert.equal(element._changeComments
.computeCommentThreadCount({

46
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
View File

@@ -39,7 +39,6 @@ import {
KeyboardShortcutMixin,
Shortcut,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {pluralize} from '../../../utils/string-util';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {appContext} from '../../../services/app-context';
import {
@@ -181,9 +180,7 @@ export class GrDiffView extends KeyboardShortcutMixin(
@property({
type: Array,
computed:
'_formatFilesForDropdown(_files, ' +
'_patchRange.patchNum, _changeComments)',
computed: '_formatFilesForDropdown(_files, _patchRange, _changeComments)',
})
_formattedFiles?: DropdownItem[];
@@ -1306,11 +1303,11 @@ export class GrDiffView extends KeyboardShortcutMixin(
_formatFilesForDropdown(
files?: Files,
patchNum?: PatchSetNum,
patchRange?: PatchRange,
changeComments?: ChangeComments
): DropdownItem[] {
if (!files) return [];
if (!patchNum) return [];
if (!patchRange) return [];
if (!changeComments) return [];
const dropdownContent: DropdownItem[] = [];
@@ -1319,46 +1316,17 @@ export class GrDiffView extends KeyboardShortcutMixin(
text: computeDisplayPath(path),
mobileText: computeTruncatedPath(path),
value: path,
bottomText: this._computeCommentString(
changeComments,
patchNum,
bottomText: changeComments.computeCommentsString(
patchRange,
path,
files.changeFilesByPath[path]
files.changeFilesByPath[path],
/* includeUnmodified= */ true
),
});
}
return dropdownContent;
}
_computeCommentString(
changeComments?: ChangeComments,
patchNum?: PatchSetNum,
path?: string,
changeFileInfo?: FileInfo
) {
if (!changeComments) return '';
if (!path) return '';
if (!changeFileInfo) return '';
const unresolvedCount = changeComments.computeUnresolvedNum({
patchNum,
path,
});
const commentThreadCount = changeComments.computeCommentThreadCount({
patchNum,
path,
});
const commentThreadString = pluralize(commentThreadCount, 'comment');
const unresolvedString =
unresolvedCount === 0 ? '' : `${unresolvedCount} unresolved`;
const unmodifiedString = changeFileInfo.status === 'U' ? 'no changes' : '';
return [unmodifiedString, commentThreadString, unresolvedString]
.filter(v => v && v.length > 0)
.join(', ');
}
_computePrefsButtonHidden(
prefs?: DiffPreferencesInfo,
prefsDisabled?: boolean

42
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
View File

@@ -150,6 +150,7 @@ suite('gr-diff-view tests', () => {
},
]},
computeCommentThreadCount: () => {},
computeCommentsString: () => '',
computeUnresolvedNum: () => {},
getPaths: () => {},
getThreadsBySideForFile: () => [],
@@ -905,44 +906,6 @@ suite('gr-diff-view tests', () => {
assert.isTrue(overlayOpenStub.called);
});
test('_computeCommentString', done => {
const path = '/test';
element.$.commentAPI.loadAll().then(comments => {
const commentThreadCountStub =
sinon.stub(comments, 'computeCommentThreadCount');
const unresolvedCountStub =
sinon.stub(comments, 'computeUnresolvedNum');
commentThreadCountStub.withArgs({patchNum: 1, path}).returns(0);
commentThreadCountStub.withArgs({patchNum: 2, path}).returns(1);
commentThreadCountStub.withArgs({patchNum: 3, path}).returns(2);
commentThreadCountStub.withArgs({patchNum: 4, path}).returns(0);
unresolvedCountStub.withArgs({patchNum: 1, path}).returns(1);
unresolvedCountStub.withArgs({patchNum: 2, path}).returns(0);
unresolvedCountStub.withArgs({patchNum: 3, path}).returns(2);
unresolvedCountStub.withArgs({patchNum: 4, path}).returns(0);
assert.equal(element._computeCommentString(comments, 1, path, {}),
'1 unresolved');
assert.equal(
element._computeCommentString(comments, 2, path, {status: 'M'}),
'1 comment');
assert.equal(
element._computeCommentString(comments, 2, path, {status: 'U'}),
'no changes, 1 comment');
assert.equal(
element._computeCommentString(comments, 3, path, {status: 'A'}),
'2 comments, 2 unresolved');
assert.equal(
element._computeCommentString(
comments, 4, path, {status: 'M'}
), '');
assert.equal(
element._computeCommentString(comments, 4, path, {status: 'U'}),
'no changes');
done();
});
});
suite('url params', () => {
setup(() => {
sinon.stub(element, '_getFiles');
@@ -962,9 +925,6 @@ suite('gr-diff-view tests', () => {
basePatchNum: PARENT,
patchNum: 10,
};
// computeCommentThreadCount is an empty function hence stubbing
// function that depends on it's return value
sinon.stub(element, '_computeCommentString').returns('');
element._change = {_number: 42};
element._files = getFilesFromFileList(
['chell.go', 'glados.txt', 'wheatley.md',

1
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.ts
View File

@@ -367,6 +367,7 @@ export class GrPatchRangeSelect extends GestureEventListeners(
);
}
// TODO(dhruvsri): have ported comments contribute to this count
_computePatchSetCommentsString(
changeComments: ChangeComments,
patchNum: PatchSetNum

104
polygerrit-ui/app/test/test-data-generators.ts
View File

@@ -91,6 +91,7 @@ import {CommitInfoWithRequiredCommit} from '../elements/change/gr-change-metadat
import {WebLinkInfo} from '../types/diff';
import {UIComment, UIDraft} from '../utils/comment-util';
import {GerritView} from '../services/router/router-model';
import {ChangeComments} from '../elements/diff/gr-comment-api/gr-comment-api';
export function dateToTimestamp(date: Date): Timestamp {
const nanosecondSuffix = '.000000000';
@@ -465,3 +466,106 @@ export function createDraft(): UIDraft {
__editing: false,
};
}
export function createChangeComments(): ChangeComments {
const comments = {
'/COMMIT_MSG': [
{
...createComment(),
message: 'Done',
updated: '2017-02-08 16:40:49' as Timestamp,
id: '1' as UrlEncodedCommentId,
},
{
...createComment(),
message: 'oh hay',
updated: '2017-02-09 16:40:49' as Timestamp,
id: '2' as UrlEncodedCommentId,
},
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'hello',
updated: '2017-02-10 16:40:49' as Timestamp,
id: '3' as UrlEncodedCommentId,
},
],
'myfile.txt': [
{
...createComment(),
message: 'good news!',
updated: '2017-02-08 16:40:49' as Timestamp,
id: '4' as UrlEncodedCommentId,
},
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'wat!?',
updated: '2017-02-09 16:40:49' as Timestamp,
id: '5' as UrlEncodedCommentId,
},
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'hi',
updated: '2017-02-10 16:40:49' as Timestamp,
id: '6' as UrlEncodedCommentId,
},
],
'unresolved.file': [
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'wat!?',
updated: '2017-02-09 16:40:49' as Timestamp,
id: '7' as UrlEncodedCommentId,
unresolved: true,
},
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'hi',
updated: '2017-02-10 16:40:49' as Timestamp,
id: '8' as UrlEncodedCommentId,
in_reply_to: '7' as UrlEncodedCommentId,
unresolved: false,
},
{
...createComment(),
patch_set: 2 as PatchSetNum,
message: 'good news!',
updated: '2017-02-08 16:40:49' as Timestamp,
id: '9' as UrlEncodedCommentId,
unresolved: true,
},
],
};
const drafts = {
'/COMMIT_MSG': [
{
...createDraft(),
message: 'hi',
updated: '2017-02-15 16:40:49' as Timestamp,
id: '10' as UrlEncodedCommentId,
unresolved: true,
},
{
...createDraft(),
message: 'fyi',
updated: '2017-02-15 16:40:49' as Timestamp,
id: '11' as UrlEncodedCommentId,
unresolved: false,
},
],
'unresolved.file': [
{
...createDraft(),
message: 'hi',
updated: '2017-02-11 16:40:49' as Timestamp,
id: '12' as UrlEncodedCommentId,
unresolved: false,
},
],
};
return new ChangeComments(comments, {}, drafts, {}, {});
}