fdf0db7a2b
The UI does not allow creating multiple comments at the same diff location, but instead encourages contributing to the comment threads already at a location. The uniqueness of thread locations was leveraged to simplify the collection of comments into threads by mapping their locations to keys. However, the REST API does permit multiple threads starting from the same diff location, so this expectation in the key-based UI threading logic can be wrong. In such scenarios, an unresolved thread may be mistakenly inserted into a resolved thread, resulting in a comment that is not resolvable through the UI. With this change, the `getCommentThreads` method is updated to associate comments into threads via the `in_reply_to` graph rather than location map in the same way that the `unresolved_comment_count` detail property is determined server-side. Although this corrects the issue in the `getCommentThreads` method, as of this change, the diffs do not yet use it to thread comments. Migrating diffs over to use it will be done in follow-up. Bug: Issue 6779 Change-Id: I9427ccc716acdf7ef4c63eba48d2287875fa5534
610 lines
21 KiB
HTML
610 lines
21 KiB
HTML
<!DOCTYPE html>
|
|
<!--
|
|
Copyright (C) 2017 The Android Open Source Project
|
|
|
|
Licensed under the Apache License, Version 2.0 (the "License");
|
|
you may not use this file except in compliance with the License.
|
|
You may obtain a copy of the License at
|
|
|
|
http://www.apache.org/licenses/LICENSE-2.0
|
|
|
|
Unless required by applicable law or agreed to in writing, software
|
|
distributed under the License is distributed on an "AS IS" BASIS,
|
|
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
|
See the License for the specific language governing permissions and
|
|
limitations under the License.
|
|
-->
|
|
|
|
<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
|
|
<title>gr-comment-api</title>
|
|
|
|
<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
|
|
<script src="../../../bower_components/web-component-tester/browser.js"></script>
|
|
<link rel="import" href="../../../test/common-test-setup.html"/>
|
|
|
|
<link rel="import" href="./gr-comment-api.html">
|
|
|
|
<script>void(0);</script>
|
|
|
|
<test-fixture id="basic">
|
|
<template>
|
|
<gr-comment-api></gr-comment-api>
|
|
</template>
|
|
</test-fixture>
|
|
|
|
<script>
|
|
suite('gr-comment-api tests', () => {
|
|
const PARENT = 'PARENT';
|
|
|
|
let element;
|
|
let sandbox;
|
|
|
|
setup(() => {
|
|
sandbox = sinon.sandbox.create();
|
|
element = fixture('basic');
|
|
});
|
|
|
|
teardown(() => { sandbox.restore(); });
|
|
|
|
test('loads logged-out', () => {
|
|
const changeNum = 1234;
|
|
|
|
sandbox.stub(element.$.restAPI, 'getLoggedIn')
|
|
.returns(Promise.resolve(false));
|
|
sandbox.stub(element.$.restAPI, 'getDiffComments')
|
|
.returns(Promise.resolve({
|
|
'foo.c': [{id: '123', message: 'foo bar', in_reply_to: '321'}],
|
|
}));
|
|
sandbox.stub(element.$.restAPI, 'getDiffRobotComments')
|
|
.returns(Promise.resolve({'foo.c': [{id: '321', message: 'done'}]}));
|
|
sandbox.stub(element.$.restAPI, 'getDiffDrafts')
|
|
.returns(Promise.resolve({}));
|
|
|
|
return element.loadAll(changeNum).then(() => {
|
|
assert.isTrue(element.$.restAPI.getDiffComments.calledWithExactly(
|
|
changeNum));
|
|
assert.isTrue(element.$.restAPI.getDiffRobotComments.calledWithExactly(
|
|
changeNum));
|
|
assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
|
|
changeNum));
|
|
assert.isOk(element._changeComments._comments);
|
|
assert.isOk(element._changeComments._robotComments);
|
|
assert.deepEqual(element._changeComments._drafts, {});
|
|
});
|
|
});
|
|
|
|
test('loads logged-in', () => {
|
|
const changeNum = 1234;
|
|
|
|
sandbox.stub(element.$.restAPI, 'getLoggedIn')
|
|
.returns(Promise.resolve(true));
|
|
sandbox.stub(element.$.restAPI, 'getDiffComments')
|
|
.returns(Promise.resolve({
|
|
'foo.c': [{id: '123', message: 'foo bar', in_reply_to: '321'}],
|
|
}));
|
|
sandbox.stub(element.$.restAPI, 'getDiffRobotComments')
|
|
.returns(Promise.resolve({'foo.c': [{id: '321', message: 'done'}]}));
|
|
sandbox.stub(element.$.restAPI, 'getDiffDrafts')
|
|
.returns(Promise.resolve({'foo.c': [{id: '555', message: 'ack'}]}));
|
|
|
|
return element.loadAll(changeNum).then(() => {
|
|
assert.isTrue(element.$.restAPI.getDiffComments.calledWithExactly(
|
|
changeNum));
|
|
assert.isTrue(element.$.restAPI.getDiffRobotComments.calledWithExactly(
|
|
changeNum));
|
|
assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
|
|
changeNum));
|
|
assert.isOk(element._changeComments._comments);
|
|
assert.isOk(element._changeComments._robotComments);
|
|
assert.notDeepEqual(element._changeComments._drafts, {});
|
|
});
|
|
});
|
|
|
|
suite('reloadDrafts', () => {
|
|
let commentStub;
|
|
let robotCommentStub;
|
|
let draftStub;
|
|
setup(() => {
|
|
commentStub = sandbox.stub(element.$.restAPI, 'getDiffComments')
|
|
.returns(Promise.resolve({}));
|
|
robotCommentStub = sandbox.stub(element.$.restAPI,
|
|
'getDiffRobotComments').returns(Promise.resolve({}));
|
|
draftStub = sandbox.stub(element.$.restAPI, 'getDiffDrafts')
|
|
.returns(Promise.resolve({}));
|
|
});
|
|
|
|
test('without loadAll first', done => {
|
|
assert.isNotOk(element._changeComments);
|
|
sandbox.spy(element, 'loadAll');
|
|
element.reloadDrafts().then(() => {
|
|
assert.isTrue(element.loadAll.called);
|
|
assert.isOk(element._changeComments);
|
|
assert.equal(commentStub.callCount, 1);
|
|
assert.equal(robotCommentStub.callCount, 1);
|
|
assert.equal(draftStub.callCount, 1);
|
|
done();
|
|
});
|
|
});
|
|
|
|
test('with loadAll first', done => {
|
|
assert.isNotOk(element._changeComments);
|
|
element.loadAll().then(() => {
|
|
assert.isOk(element._changeComments);
|
|
assert.equal(commentStub.callCount, 1);
|
|
assert.equal(robotCommentStub.callCount, 1);
|
|
assert.equal(draftStub.callCount, 1);
|
|
return element.reloadDrafts();
|
|
}).then(() => {
|
|
assert.isOk(element._changeComments);
|
|
assert.equal(commentStub.callCount, 1);
|
|
assert.equal(robotCommentStub.callCount, 1);
|
|
assert.equal(draftStub.callCount, 2);
|
|
done();
|
|
});
|
|
});
|
|
});
|
|
|
|
suite('_changeComment methods', () => {
|
|
setup(done => {
|
|
const changeNum = 1234;
|
|
stub('gr-rest-api-interface', {
|
|
getDiffComments() { return Promise.resolve({}); },
|
|
getDiffRobotComments() { return Promise.resolve({}); },
|
|
getDiffDrafts() { return Promise.resolve({}); },
|
|
});
|
|
element.loadAll(changeNum).then(() => {
|
|
done();
|
|
});
|
|
});
|
|
|
|
test('_isInBaseOfPatchRange', () => {
|
|
const comment = {patch_set: 1};
|
|
const patchRange = {basePatchNum: 1, patchNum: 2};
|
|
assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
|
|
patchRange.basePatchNum = PARENT;
|
|
assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
|
|
comment.side = PARENT;
|
|
assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
|
|
comment.patch_set = 2;
|
|
assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
|
|
patchRange.basePatchNum = -2;
|
|
comment.side = PARENT;
|
|
comment.parent = 1;
|
|
assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
|
|
comment.parent = 2;
|
|
assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
|
|
patchRange));
|
|
});
|
|
|
|
test('_isInRevisionOfPatchRange', () => {
|
|
const comment = {patch_set: 123};
|
|
const patchRange = {basePatchNum: 122, patchNum: 124};
|
|
assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
|
|
comment, patchRange));
|
|
|
|
patchRange.patchNum = 123;
|
|
assert.isTrue(element._changeComments._isInRevisionOfPatchRange(
|
|
comment, patchRange));
|
|
|
|
comment.side = PARENT;
|
|
assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
|
|
comment, patchRange));
|
|
});
|
|
|
|
test('_isInPatchRange', () => {
|
|
const patchRange1 = {basePatchNum: 122, patchNum: 124};
|
|
const patchRange2 = {basePatchNum: 123, patchNum: 125};
|
|
const patchRange3 = {basePatchNum: 124, patchNum: 125};
|
|
|
|
const isInBasePatchStub = sandbox.stub(element._changeComments,
|
|
'_isInBaseOfPatchRange');
|
|
const isInRevisionPatchStub = sandbox.stub(element._changeComments,
|
|
'_isInRevisionOfPatchRange');
|
|
|
|
isInBasePatchStub.withArgs({}, patchRange1).returns(true);
|
|
isInBasePatchStub.withArgs({}, patchRange2).returns(false);
|
|
isInBasePatchStub.withArgs({}, patchRange3).returns(false);
|
|
|
|
isInRevisionPatchStub.withArgs({}, patchRange1).returns(false);
|
|
isInRevisionPatchStub.withArgs({}, patchRange2).returns(true);
|
|
isInRevisionPatchStub.withArgs({}, patchRange3).returns(false);
|
|
|
|
assert.isTrue(element._changeComments._isInPatchRange({}, patchRange1));
|
|
assert.isTrue(element._changeComments._isInPatchRange({}, patchRange2));
|
|
assert.isFalse(element._changeComments._isInPatchRange({},
|
|
patchRange3));
|
|
});
|
|
|
|
suite('comment ranges and paths', () => {
|
|
function makeTime(mins) {
|
|
return `2013-02-26 15:0${mins}:43.986000000`;
|
|
}
|
|
|
|
setup(() => {
|
|
element._changeComments._drafts = {
|
|
'file/one': [
|
|
{
|
|
id: 11,
|
|
patch_set: 2,
|
|
side: PARENT,
|
|
line: 1,
|
|
updated: makeTime(3),
|
|
},
|
|
{
|
|
id: 12,
|
|
in_reply_to: 2,
|
|
patch_set: 2,
|
|
line: 1,
|
|
updated: makeTime(3),
|
|
},
|
|
],
|
|
};
|
|
element._changeComments._robotComments = {
|
|
'file/one': [
|
|
{
|
|
id: 1,
|
|
patch_set: 2,
|
|
side: PARENT,
|
|
line: 1,
|
|
updated: makeTime(1),
|
|
range: {
|
|
start_line: 1,
|
|
start_character: 2,
|
|
end_line: 2,
|
|
end_character: 2,
|
|
},
|
|
}, {
|
|
id: 2,
|
|
in_reply_to: 4,
|
|
patch_set: 2,
|
|
unresolved: true,
|
|
line: 1,
|
|
updated: makeTime(2),
|
|
},
|
|
],
|
|
};
|
|
element._changeComments._comments = {
|
|
'file/one': [
|
|
{id: 3, patch_set: 2, side: PARENT, line: 2, updated: makeTime(1)},
|
|
{id: 4, patch_set: 2, line: 1, updated: makeTime(1)},
|
|
],
|
|
'file/two': [
|
|
{id: 5, patch_set: 2, line: 2, updated: makeTime(1)},
|
|
{id: 6, patch_set: 3, line: 2, updated: makeTime(1)},
|
|
],
|
|
'file/three': [
|
|
{
|
|
id: 7,
|
|
patch_set: 2,
|
|
side: PARENT,
|
|
unresolved: true,
|
|
line: 1,
|
|
updated: makeTime(1),
|
|
},
|
|
{id: 8, patch_set: 3, line: 1, updated: makeTime(1)},
|
|
],
|
|
'file/four': [
|
|
{id: 9, patch_set: 5, side: PARENT, line: 1, updated: makeTime(1)},
|
|
{id: 10, patch_set: 5, line: 1, updated: makeTime(1)},
|
|
],
|
|
};
|
|
});
|
|
|
|
test('getPaths', () => {
|
|
const patchRange = {basePatchNum: 1, patchNum: 4};
|
|
let paths = element._changeComments.getPaths(patchRange);
|
|
assert.equal(Object.keys(paths).length, 0);
|
|
|
|
patchRange.basePatchNum = PARENT;
|
|
patchRange.patchNum = 3;
|
|
paths = element._changeComments.getPaths(patchRange);
|
|
assert.notProperty(paths, 'file/one');
|
|
assert.property(paths, 'file/two');
|
|
assert.property(paths, 'file/three');
|
|
assert.notProperty(paths, 'file/four');
|
|
|
|
patchRange.patchNum = 2;
|
|
paths = element._changeComments.getPaths(patchRange);
|
|
assert.property(paths, 'file/one');
|
|
assert.property(paths, 'file/two');
|
|
assert.property(paths, 'file/three');
|
|
assert.notProperty(paths, 'file/four');
|
|
|
|
paths = element._changeComments.getPaths();
|
|
assert.property(paths, 'file/one');
|
|
assert.property(paths, 'file/two');
|
|
assert.property(paths, 'file/three');
|
|
assert.property(paths, 'file/four');
|
|
});
|
|
|
|
test('getCommentsBySideForPath', () => {
|
|
const patchRange = {basePatchNum: 1, patchNum: 3};
|
|
let path = 'file/one';
|
|
let comments = element._changeComments.getCommentsBySideForPath(path,
|
|
patchRange);
|
|
assert.equal(comments.meta.changeNum, 1234);
|
|
assert.equal(comments.left.length, 0);
|
|
assert.equal(comments.right.length, 0);
|
|
|
|
path = 'file/two';
|
|
comments = element._changeComments.getCommentsBySideForPath(path,
|
|
patchRange);
|
|
assert.equal(comments.left.length, 0);
|
|
assert.equal(comments.right.length, 1);
|
|
|
|
patchRange.basePatchNum = 2;
|
|
comments = element._changeComments.getCommentsBySideForPath(path,
|
|
patchRange);
|
|
assert.equal(comments.left.length, 1);
|
|
assert.equal(comments.right.length, 1);
|
|
|
|
patchRange.basePatchNum = PARENT;
|
|
path = 'file/three';
|
|
comments = element._changeComments.getCommentsBySideForPath(path,
|
|
patchRange);
|
|
assert.equal(comments.left.length, 0);
|
|
assert.equal(comments.right.length, 1);
|
|
});
|
|
|
|
test('getAllCommentsForPath', () => {
|
|
let path = 'file/one';
|
|
let comments = element._changeComments.getAllCommentsForPath(path);
|
|
assert.deepEqual(comments.length, 4);
|
|
path = 'file/two';
|
|
comments = element._changeComments.getAllCommentsForPath(path, 2);
|
|
assert.deepEqual(comments.length, 1);
|
|
});
|
|
|
|
test('getAllDraftsForPath', () => {
|
|
const path = 'file/one';
|
|
const drafts = element._changeComments.getAllDraftsForPath(path);
|
|
assert.deepEqual(drafts.length, 2);
|
|
});
|
|
|
|
test('computeUnresolvedNum', () => {
|
|
assert.equal(element._changeComments
|
|
.computeUnresolvedNum(2, 'file/one'), 0);
|
|
assert.equal(element._changeComments
|
|
.computeUnresolvedNum(1, 'file/one'), 0);
|
|
assert.equal(element._changeComments
|
|
.computeUnresolvedNum(2, 'file/three'), 1);
|
|
});
|
|
|
|
test('computeCommentCount', () => {
|
|
assert.equal(element._changeComments
|
|
.computeCommentCount(2, 'file/one'), 4);
|
|
assert.equal(element._changeComments
|
|
.computeCommentCount(1, 'file/one'), 0);
|
|
assert.equal(element._changeComments
|
|
.computeCommentCount(2, 'file/three'), 1);
|
|
});
|
|
|
|
test('computeDraftCount', () => {
|
|
assert.equal(element._changeComments
|
|
.computeDraftCount(2, 'file/one'), 2);
|
|
assert.equal(element._changeComments
|
|
.computeDraftCount(1, 'file/one'), 0);
|
|
assert.equal(element._changeComments
|
|
.computeDraftCount(2, 'file/three'), 0);
|
|
});
|
|
|
|
test('getAllPublishedComments', () => {
|
|
let publishedComments = element._changeComments
|
|
.getAllPublishedComments();
|
|
assert.equal(Object.keys(publishedComments).length, 4);
|
|
assert.equal(Object.keys(publishedComments[['file/one']]).length, 4);
|
|
assert.equal(Object.keys(publishedComments[['file/two']]).length, 2);
|
|
publishedComments = element._changeComments
|
|
.getAllPublishedComments(2);
|
|
assert.equal(Object.keys(publishedComments[['file/one']]).length, 4);
|
|
assert.equal(Object.keys(publishedComments[['file/two']]).length, 1);
|
|
});
|
|
|
|
test('getAllComments', () => {
|
|
let comments = element._changeComments.getAllComments();
|
|
assert.equal(Object.keys(comments).length, 4);
|
|
assert.equal(Object.keys(comments[['file/one']]).length, 4);
|
|
assert.equal(Object.keys(comments[['file/two']]).length, 2);
|
|
comments = element._changeComments.getAllComments(false, 2);
|
|
assert.equal(Object.keys(comments).length, 4);
|
|
assert.equal(Object.keys(comments[['file/one']]).length, 4);
|
|
assert.equal(Object.keys(comments[['file/two']]).length, 1);
|
|
// Include drafts
|
|
comments = element._changeComments.getAllComments(true);
|
|
assert.equal(Object.keys(comments).length, 4);
|
|
assert.equal(Object.keys(comments[['file/one']]).length, 6);
|
|
assert.equal(Object.keys(comments[['file/two']]).length, 2);
|
|
comments = element._changeComments.getAllComments(true, 2);
|
|
assert.equal(Object.keys(comments).length, 4);
|
|
assert.equal(Object.keys(comments[['file/one']]).length, 6);
|
|
assert.equal(Object.keys(comments[['file/two']]).length, 1);
|
|
});
|
|
|
|
test('computeAllThreads', () => {
|
|
const expectedThreads = [
|
|
{
|
|
comments: [
|
|
{
|
|
id: 5,
|
|
patch_set: 2,
|
|
line: 2,
|
|
__path: 'file/two',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
patchNum: 2,
|
|
path: 'file/two',
|
|
line: 2,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 3,
|
|
patch_set: 2,
|
|
side: 'PARENT',
|
|
line: 2,
|
|
__path: 'file/one',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
commentSide: 'PARENT',
|
|
patchNum: 2,
|
|
path: 'file/one',
|
|
line: 2,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 1,
|
|
patch_set: 2,
|
|
side: 'PARENT',
|
|
line: 1,
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
range: {
|
|
start_line: 1,
|
|
start_character: 2,
|
|
end_line: 2,
|
|
end_character: 2,
|
|
},
|
|
__path: 'file/one',
|
|
},
|
|
],
|
|
commentSide: 'PARENT',
|
|
patchNum: 2,
|
|
path: 'file/one',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 9,
|
|
patch_set: 5,
|
|
side: 'PARENT',
|
|
line: 1,
|
|
__path: 'file/four',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
commentSide: 'PARENT',
|
|
patchNum: 5,
|
|
path: 'file/four',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 8,
|
|
patch_set: 3,
|
|
line: 1,
|
|
__path: 'file/three',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
patchNum: 3,
|
|
path: 'file/three',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 7,
|
|
patch_set: 2,
|
|
side: 'PARENT',
|
|
unresolved: true,
|
|
line: 1,
|
|
__path: 'file/three',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
commentSide: 'PARENT',
|
|
patchNum: 2,
|
|
path: 'file/three',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 4,
|
|
patch_set: 2,
|
|
line: 1,
|
|
__path: 'file/one',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
{
|
|
id: 2,
|
|
in_reply_to: 4,
|
|
patch_set: 2,
|
|
unresolved: true,
|
|
line: 1,
|
|
__path: 'file/one',
|
|
updated: '2013-02-26 15:02:43.986000000',
|
|
},
|
|
{
|
|
id: 12,
|
|
in_reply_to: 2,
|
|
patch_set: 2,
|
|
line: 1,
|
|
__path: 'file/one',
|
|
__draft: true,
|
|
updated: '2013-02-26 15:03:43.986000000',
|
|
},
|
|
],
|
|
patchNum: 2,
|
|
path: 'file/one',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 6,
|
|
patch_set: 3,
|
|
line: 2,
|
|
__path: 'file/two',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
patchNum: 3,
|
|
path: 'file/two',
|
|
line: 2,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 10,
|
|
patch_set: 5,
|
|
line: 1,
|
|
__path: 'file/four',
|
|
updated: '2013-02-26 15:01:43.986000000',
|
|
},
|
|
],
|
|
patchNum: 5,
|
|
path: 'file/four',
|
|
line: 1,
|
|
}, {
|
|
comments: [
|
|
{
|
|
id: 11,
|
|
patch_set: 2,
|
|
side: 'PARENT',
|
|
line: 1,
|
|
__path: 'file/one',
|
|
__draft: true,
|
|
updated: '2013-02-26 15:03:43.986000000',
|
|
},
|
|
],
|
|
commentSide: 'PARENT',
|
|
patchNum: 2,
|
|
path: 'file/one',
|
|
line: 1,
|
|
},
|
|
];
|
|
const threads = element._changeComments.getAllThreadsForChange();
|
|
assert.deepEqual(threads, expectedThreads);
|
|
});
|
|
});
|
|
});
|
|
});
|
|
</script>
|