opendev/gerrit
Code Issues Proposed changes

Use comment API to determine comment strings in patch range select

Browse Source 
Change-Id: Ic81175fe428c550504e2ecf20026547e71db03fc
This commit is contained in:
Becky Siegel
2017-11-09 11:37:09 -08:00
parent c94e62d285
commit 0853fd04b5
3 changed files with 83 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
View File

@@ -18,7 +18,7 @@ limitations under the License.
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../shared/gr-dropdown-list/gr-dropdown-list.html">
<link rel="import" href="../../shared/gr-count-string-formatter/gr-count-string-formatter.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
<dom-module id="gr-patch-range-select">

89
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
View File

@@ -42,13 +42,7 @@
'basePatchNum, _sortedRevisions, changeComments)',
},
changeNum: String,
// In the case of a patch range select (like diff view) comments should
// be an empty array, so that the patch and base content computed values
// get triggered.
changeComments: {
type: Object,
value: () => { return {}; },
},
changeComments: Object,
/** @type {{ meta_a: !Array, meta_b: !Array}} */
filesWeblinks: Object,
patchNum: String,
@@ -73,10 +67,9 @@
basePatch.num, patchNum, _sortedRevisions),
triggerText: `Patchset ${basePatchNum}`,
text: `Patchset ${basePatchNum}` +
this._computePatchSetCommentsString(changeComments.comments,
basePatchNum),
this._computePatchSetCommentsString(changeComments, basePatchNum),
mobileText: this._computeMobileText(basePatchNum,
changeComments.comments, _sortedRevisions),
changeComments, _sortedRevisions),
bottomText: `${this._computePatchSetDescription(
_sortedRevisions, basePatchNum)}`,
value: basePatch.num,
@@ -89,9 +82,9 @@
return dropdownContent;
},
_computeMobileText(patchNum, comments, revisions) {
_computeMobileText(patchNum, changeComments, revisions) {
return `${patchNum}` +
`${this._computePatchSetCommentsString(comments, patchNum)}` +
`${this._computePatchSetCommentsString(changeComments, patchNum)}` +
`${this._computePatchSetDescription(revisions, patchNum, true)}`;
},
@@ -107,8 +100,8 @@
patchNum,
text: `${patchNum === 'edit' ? '': 'Patchset '}${patchNum}` +
`${this._computePatchSetCommentsString(
changeComments.comments, patchNum)}`,
mobileText: this._computeMobileText(patchNum, changeComments.comments,
changeComments, patchNum)}`,
mobileText: this._computeMobileText(patchNum, changeComments,
_sortedRevisions),
bottomText: `${this._computePatchSetDescription(
_sortedRevisions, patchNum)}`,
@@ -152,66 +145,26 @@
this.findSortedIndex(patchNum, sortedRevisions);
},
// Copied from gr-file-list
// @todo(beckysiegel) clean up.
_getCommentsForPath(comments, patchNum, path) {
return (comments[path] || []).filter(c => {
return this.patchNumEquals(c.patch_set, patchNum);
});
},
// Copied from gr-file-list
// @todo(beckysiegel) clean up.
_computeUnresolvedNum(comments, drafts, patchNum, path) {
comments = this._getCommentsForPath(comments, patchNum, path);
drafts = this._getCommentsForPath(drafts, patchNum, path);
comments = comments.concat(drafts);
_computePatchSetCommentsString(changeComments, patchNum) {
if (!changeComments) { return; }
// Create an object where every comment ID is the key of an unresolved
// comment.
const commentCount = changeComments.computeCommentCount(patchNum);
const commentString = GrCountStringFormatter.computePluralString(
commentCount, 'comment');
const idMap = comments.reduce((acc, comment) => {
if (comment.unresolved) {
acc[comment.id] = true;
}
return acc;
}, {});
const unresolvedCount = changeComments.computeUnresolvedNum(patchNum);
const unresolvedString = GrCountStringFormatter.computeString(
unresolvedCount, 'unresolved');
// Set false for the comments that are marked as parents.
for (const comment of comments) {
idMap[comment.in_reply_to] = false;
if (!commentString.length && !unresolvedString.length) {
return '';
}
// The unresolved comments are the comments that still have true.
const unresolvedLeaves = Object.keys(idMap).filter(key => {
return idMap[key];
});
return unresolvedLeaves.length;
},
_computePatchSetCommentsString(allComments, patchNum) {
// todo (beckysiegel) get comment strings for diff view also.
if (!allComments) { return ''; }
let numComments = 0;
let numUnresolved = 0;
for (const file in allComments) {
if (allComments.hasOwnProperty(file)) {
numComments += this._getCommentsForPath(
allComments, patchNum, file).length;
numUnresolved += this._computeUnresolvedNum(
allComments, {}, patchNum, file);
}
}
let commentsStr = '';
if (numComments > 0) {
commentsStr = ' (' + numComments + ' comments';
if (numUnresolved > 0) {
commentsStr += ', ' + numUnresolved + ' unresolved';
}
commentsStr += ')';
}
return commentsStr;
return ` (${commentString}` +
// Add a comma + space if both comments and unresolved
(commentString && unresolvedString ? ', ' : '') +
`${unresolvedString})`;
},
/**

115
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html
View File

@@ -23,13 +23,24 @@ limitations under the License.
<link rel="import" href="../../../test/common-test-setup.html"/>
<script src="../../../bower_components/page/page.js"></script>
<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
<link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
<link rel="import" href="gr-patch-range-select.html">
<script>void(0);</script>
<dom-module id="comment-api-mock">
<template>
<gr-patch-range-select id="patchRange" auto
change-comments="[[_changeComments]]"></gr-patch-range-select>
<gr-comment-api id="commentAPI"></gr-comment-api>
</template>
<script src="../../diff/gr-comment-api/gr-comment-api-mock.js"></script>
</dom-module>
<test-fixture id="basic">
<template>
<gr-patch-range-select auto></gr-patch-range-select>
<comment-api-mock></comment-api-mock>
</template>
</test-fixture>
@@ -37,10 +48,25 @@ limitations under the License.
suite('gr-patch-range-select tests', () => {
let element;
let sandbox;
let commentApiWrapper;
setup(() => {
element = fixture('basic');
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getDiffComments() { return Promise.resolve({}); },
getDiffRobotComments() { return Promise.resolve({}); },
getDiffDrafts() { return Promise.resolve({}); },
});
// Element must be wrapped in an element with direct access to the
// comment API.
commentApiWrapper = fixture('basic');
element = commentApiWrapper.$.patchRange;
// Stub methods on the changeComments object after changeComments has
// been initalized.
return commentApiWrapper.loadComments();
});
teardown(() => sandbox.restore());
@@ -80,23 +106,12 @@ limitations under the License.
});
test('_computeBaseDropdownContent', () => {
const comments = {};
const availablePatches = [
{num: 'edit'},
{num: 3},
{num: 2},
{num: 1},
];
const revisions = [
{
commit: {},
_number: 2,
description: 'description',
},
{commit: {}},
{commit: {}},
{commit: {}},
];
const patchNum = 1;
const sortedRevisions = [
{_number: 3},
@@ -143,7 +158,8 @@ limitations under the License.
},
];
assert.deepEqual(element._computeBaseDropdownContent(availablePatches,
patchNum, sortedRevisions, revisions, comments), expectedResult);
patchNum, sortedRevisions, element.changeComments),
expectedResult);
});
test('_computeBaseDropdownContent called when patchNum updates', () => {
@@ -170,36 +186,32 @@ limitations under the License.
assert.equal(element._computeBaseDropdownContent.callCount, 1);
});
test('_computeBaseDropdownContent called when comments update', () => {
element.revisions = [
test('_computeBaseDropdownContent called when changeComments update',
done => {
element.revisions = [
{commit: {}},
{commit: {}},
{commit: {}},
{commit: {}},
];
element.availablePatches = [
];
element.availablePatches = [
{num: 'edit'},
{num: 3},
{num: 2},
{num: 1},
];
element.patchNum = 2;
element.basePatchNum = 'PARENT';
flushAsynchronousOperations();
];
element.patchNum = 2;
element.basePatchNum = 'PARENT';
flushAsynchronousOperations();
// Should be recomputed for each available patch
sandbox.stub(element, '_computeBaseDropdownContent');
assert.equal(element._computeBaseDropdownContent.callCount, 0);
element.set('changeComments', {
comments: {
file: [{
message: 'test',
patch_set: 2,
}],
},
});
assert.equal(element._computeBaseDropdownContent.callCount, 1);
});
// Should be recomputed for each available patch
sandbox.stub(element, '_computeBaseDropdownContent');
assert.equal(element._computeBaseDropdownContent.callCount, 0);
commentApiWrapper.loadComments().then().then(() => {
assert.equal(element._computeBaseDropdownContent.callCount, 1);
done();
});
});
test('_computePatchDropdownContent called when basePatchNum updates', () => {
element.revisions = [
@@ -224,7 +236,7 @@ limitations under the License.
assert.equal(element._computePatchDropdownContent.callCount, 1);
});
test('_computePatchDropdownContent called when comments update', () => {
test('_computePatchDropdownContent called when comments update', done => {
element.revisions = [
{commit: {}},
{commit: {}},
@@ -244,19 +256,12 @@ limitations under the License.
// Should be recomputed for each available patch
sandbox.stub(element, '_computePatchDropdownContent');
assert.equal(element._computePatchDropdownContent.callCount, 0);
element.set('changeComments', {
comments: {
file: [{
message: 'test',
patch_set: 2,
}],
},
commentApiWrapper.loadComments().then().then(() => {
done();
});
assert.equal(element._computePatchDropdownContent.callCount, 1);
});
test('_computePatchDropdownContent', () => {
const comments = {};
const availablePatches = [
{num: 'edit'},
{num: 3},
@@ -307,7 +312,8 @@ limitations under the License.
];
assert.deepEqual(element._computePatchDropdownContent(availablePatches,
basePatchNum, sortedRevisions, comments), expectedResult);
basePatchNum, sortedRevisions, element.changeComments),
expectedResult);
});
test('filesWeblinks', () => {
@@ -335,7 +341,7 @@ limitations under the License.
test('_computePatchSetCommentsString', () => {
// Test string with unresolved comments.
comments = {
element.changeComments._comments = {
foo: [{
id: '27dcee4d_f7b77cfa',
message: 'test',
@@ -355,17 +361,18 @@ limitations under the License.
abc: [],
};
assert.equal(element._computePatchSetCommentsString(comments, 1),
' (3 comments, 1 unresolved)');
assert.equal(element._computePatchSetCommentsString(
element.changeComments, 1), ' (3 comments, 1 unresolved)');
// Test string with no unresolved comments.
delete comments['foo'];
assert.equal(element._computePatchSetCommentsString(comments, 1),
' (2 comments)');
delete element.changeComments._comments['foo'];
assert.equal(element._computePatchSetCommentsString(
element.changeComments, 1), ' (2 comments)');
// Test string with no comments.
delete comments['bar'];
assert.equal(element._computePatchSetCommentsString(comments, 1), '');
delete element.changeComments._comments['bar'];
assert.equal(element._computePatchSetCommentsString(
element.changeComments, 1), '');
});
test('patch-range-change fires', () => {