Derive isOnParent from existing comment's side
isOnParent on a thread is only used to set the side value when storing
new drafts on the server. All comments of a thread can be assumed to
have the same side set. So when adding a new draft to an existing thread,
we can determine isOnParent just by looking at the first comment's side
value. If side is not set, it defaults to REVISION:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-info
Change-Id: I80ff9460e9869705c3416cdb52bca17b2faf54d4
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 9788b65..7b568d1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -140,11 +140,8 @@
}
const threads = this._createThreads(allComments);
this._threadEls = threads.map(thread => {
- // TODO(oler): Figure out how to determine that here already - for now,
- // this is set later.
- const isOnParent = false;
return Gerrit.createThreadElement(
- thread, isOnParent, this._parentIndex, this._changeNum,
+ thread, this._parentIndex, this._changeNum,
this._path, this._projectName);
});
}
@@ -430,6 +427,7 @@
patchNum: comment.patch_set,
rootId: comment.id || comment.__draftID,
lineNum: comment.line,
+ isOnParent: comment.side === 'PARENT',
};
if (comment.range) {
newThread.range = Object.assign({}, comment.range);
@@ -460,26 +458,6 @@
};
/**
- * Returns whether the comments on the given line are on a (merge) parent.
- *
- * @param {string} firstCommentSide
- * @param {{basePatchNum: number, patchNum: number}} patchRange
- * @param {GrDiffLine} line The line the comments are on.
- * @param {string=} opt_side
- * @return {boolean} True iff the comments on the given line are on a (merge)
- * parent.
- */
- GrDiffBuilder.prototype._determineIsOnParent = function(
- firstCommentSide, patchRange, line, opt_side) {
- return ((line.type === GrDiffLine.Type.REMOVE ||
- opt_side === GrDiffBuilder.Side.LEFT) &&
- (patchRange.basePatchNum === 'PARENT' ||
- Gerrit.PatchSetBehavior.isMergeParent(
- patchRange.basePatchNum))) ||
- firstCommentSide === 'PARENT';
- };
-
- /**
* @param {!GrDiffLine} line
* @param {!GrDiffBuilder.Side=} side The side (LEFT, RIGHT, BOTH) for which to return
* the thread group (default: BOTH).
@@ -495,10 +473,7 @@
const threadGroupEl =
document.createElement('gr-diff-comment-thread-group');
- const patchRange = this._comments.meta.patchRange;
for (const threadEl of threadElsForGroup) {
- threadEl.isOnParent = this._determineIsOnParent(
- threadEl.comments[0].side, patchRange, line, side);
Polymer.dom(threadGroupEl).appendChild(threadEl);
}
if (side) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 1e1e99b..bd65703 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -210,7 +210,7 @@
line: 1,
}];
- expectedThreadGroups = [
+ expectedThreads = [
{
start_datetime: '2015-12-24 15:00:10.396000000',
commentSide: 'left',
@@ -237,30 +237,60 @@
end_character: 2,
},
lineNum: 1,
+ isOnParent: false,
},
];
assert.deepEqual(
builder._createThreads(comments),
- expectedThreadGroups);
+ expectedThreads);
});
- test('multiple comments at same location but not threaded', () => {
- const comments = [
- {
- id: 'sallys_confession',
- message: 'i like you, jack',
- updated: '2015-12-23 15:00:20.396000000',
- __commentSide: 'left',
- }, {
- id: 'jacks_reply',
- message: 'i like you, too',
- updated: '2015-12-24 15:01:20.396000000',
- __commentSide: 'left',
- },
- ];
- assert.equal(builder._createThreads(comments).length, 2);
- });
+ test('_createThreads does not thread unrelated comments at same location',
+ () => {
+ const comments = [
+ {
+ id: 'sallys_confession',
+ message: 'i like you, jack',
+ updated: '2015-12-23 15:00:20.396000000',
+ __commentSide: 'left',
+ }, {
+ id: 'jacks_reply',
+ message: 'i like you, too',
+ updated: '2015-12-24 15:01:20.396000000',
+ __commentSide: 'left',
+ },
+ ];
+ assert.equal(builder._createThreads(comments).length, 2);
+ });
+
+ test('_createThreads derives isOnParent using side from first comment',
+ () => {
+ const comments = [
+ {
+ id: 'sallys_confession',
+ message: 'i like you, jack',
+ updated: '2015-12-23 15:00:20.396000000',
+ // line: 1,
+ // __commentSide: 'left',
+ }, {
+ id: 'jacks_reply',
+ message: 'i like you, too',
+ updated: '2015-12-24 15:01:20.396000000',
+ // __commentSide: 'left',
+ // line: 1,
+ in_reply_to: 'sallys_confession',
+ },
+ ];
+
+ assert.equal(builder._createThreads(comments)[0].isOnParent, false);
+
+ comments[0].side = 'REVISION';
+ assert.equal(builder._createThreads(comments)[0].isOnParent, false);
+
+ comments[0].side = 'PARENT';
+ assert.equal(builder._createThreads(comments)[0].isOnParent, true);
+ });
test('_createElement classStr applies all classes', () => {
const node = builder._createElement('div', 'test classes');
@@ -437,9 +467,9 @@
test('comment thread group creation', () => {
const l3 = {id: 'l3', line: 3, updated: '2016-08-09 00:42:32.000000000',
- __commentSide: 'left'};
+ __commentSide: 'left', side: 'PARENT'};
const l5 = {id: 'l5', line: 5, updated: '2016-08-09 00:42:32.000000000',
- __commentSide: 'left', patch_set: '2'};
+ __commentSide: 'left', patch_set: '2', side: 'REVISION'};
const r5 = {id: 'r5', line: 5, updated: '2016-08-09 00:42:32.000000000',
__commentSide: 'right', patch_set: '3'};
@@ -458,14 +488,13 @@
right: [r5],
}, parentIndex, changeNum, path, projectName, prefs);
- function checkThreadGroupProps(threadGroupEl, patchNum, isOnParent,
+ function checkThreadGroupProps(threadGroupEl, patchNum,
comments) {
const threadEls = Polymer.dom(threadGroupEl).queryDistributedElements(
'gr-diff-comment-thread');
assert.equal(threadEls.length, comments.length);
for (let i=0; i < comments.length; i++) {
const threadEl = threadEls[i];
- assert.equal(threadEl.isOnParent, isOnParent);
const comment = comments[i];
assert.equal(threadEl.patchNum, comment.patch_set);
@@ -480,28 +509,28 @@
line.beforeNumber = 5;
line.afterNumber = 5;
let threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
- checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
- checkThreadGroupProps(threadGroupEl, '3', true, [l5]);
+ checkThreadGroupProps(threadGroupEl, '3', [l5]);
builder._comments.meta.patchRange.basePatchNum = '1';
threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
threadEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
- checkThreadGroupProps(threadEl, '1', false, [l5]);
+ checkThreadGroupProps(threadEl, '1', [l5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
- checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [r5]);
builder._comments.meta.patchRange.basePatchNum = 'PARENT';
@@ -509,13 +538,13 @@
line.beforeNumber = 5;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, '3', true, [l5, r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [l5, r5]);
line = new GrDiffLine(GrDiffLine.Type.ADD);
line.beforeNumber = 3;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
- checkThreadGroupProps(threadGroupEl, '3', false, [l3, r5]);
+ checkThreadGroupProps(threadGroupEl, '3', [l3, r5]);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
index 3e71f14..9eb3fa2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
@@ -36,18 +36,17 @@
// easier to read.
/**
* @param {Object} thread
- * @param {boolean} isOnParent
* @param {number} parentIndex
* @param {number} changeNum
* @param {string} path
* @param {string} projectName
*/
window.Gerrit.createThreadElement = function(
- thread, isOnParent, parentIndex, changeNum, path, projectName) {
+ thread, parentIndex, changeNum, path, projectName) {
const threadEl = document.createElement('gr-diff-comment-thread');
threadEl.comments = thread.comments;
threadEl.commentSide = thread.commentSide;
- threadEl.isOnParent = isOnParent;
+ threadEl.isOnParent = thread.isOnParent;
threadEl.parentIndex = parentIndex;
threadEl.changeNum = changeNum;
threadEl.patchNum = thread.patchNum;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
index 55a88d8..90f50a8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
@@ -113,7 +113,7 @@
];
for (const thread of threads) {
Polymer.dom(element).appendChild(Gerrit.createThreadElement(
- thread, false, 1, '2', 'some/path', 'some_project'));
+ thread, 1, '2', 'some/path', 'some_project'));
}
flushAsynchronousOperations();
@@ -134,7 +134,7 @@
];
for (const thread of threads) {
const threadEl = Gerrit.createThreadElement(
- thread, false, 1, 2, 'some/path', 'Some Project');
+ thread, 1, 2, 'some/path', 'Some Project');
Polymer.dom(element).appendChild(threadEl);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
index b6524ec..68b4616 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
@@ -486,7 +486,8 @@
commentSide,
patchNum: patchForNewThreads,
range,
- }, isOnParent, this._parentIndex, this.changeNum,
+ isOnParent,
+ }, this._parentIndex, this.changeNum,
this.path, this.projectName);
Polymer.dom(threadGroupEl).appendChild(threadEl);
}
