659f15110f
When a new comment is being added to a line, Gerrit checks to see if
there is an existing thread on the same line (and same range, if any) so
that the comment can be appended to it if so. Otherwise, a new thread is
created.
However, following I4f7804ac02, the logic to identify the appropriate
thread by range was refactored to not use range location strings, but
use range objects instead. Problematically, there were two flaws in this
code:
1) The range object references were compared rather than their values.
2) Only new threads were being rendered with their corresponding ranges
whereas existing threads were not.
As a result, if the user attempted to add a line comment on a line with
an existing ranged comment, the ranged comment's thread would be
identified as the destination (because the new comment has no range and
the existing thread's range was not being set). Appending the range-less
comment to a ranged thread resulted in incoherent data and the draft
would be unsavable.
With this change, the logic uses value equality to match ranges and the
`gr-diff-comment-thread-group#_getThreads` method is updated to set the
range on existing threads.
Bug: Issue 8410
Change-Id: If34e0d46a5c1af81bec82125217088fb574a2f61
304 lines
8.8 KiB
HTML
304 lines
8.8 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-diff-comment-thread-group</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"/>
|
|
<script src="../../../scripts/util.js"></script>
|
|
|
|
<link rel="import" href="gr-diff-comment-thread-group.html">
|
|
|
|
<script>void(0);</script>
|
|
|
|
<test-fixture id="basic">
|
|
<template>
|
|
<gr-diff-comment-thread-group></gr-diff-comment-thread-group>
|
|
</template>
|
|
</test-fixture>
|
|
|
|
<script>
|
|
suite('gr-diff-comment-thread-group tests', () => {
|
|
let element;
|
|
let sandbox;
|
|
|
|
setup(() => {
|
|
sandbox = sinon.sandbox.create();
|
|
stub('gr-rest-api-interface', {
|
|
getLoggedIn() { return Promise.resolve(false); },
|
|
});
|
|
element = fixture('basic');
|
|
});
|
|
|
|
teardown(() => {
|
|
sandbox.restore();
|
|
});
|
|
|
|
test('_getThreads', () => {
|
|
element.patchForNewThreads = 3;
|
|
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',
|
|
in_reply_to: 'sallys_confession',
|
|
},
|
|
];
|
|
|
|
let expectedThreadGroups = [
|
|
{
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
commentSide: 'left',
|
|
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',
|
|
in_reply_to: 'sallys_confession',
|
|
}],
|
|
rootId: 'sallys_confession',
|
|
patchNum: 3,
|
|
},
|
|
];
|
|
|
|
assert.deepEqual(element._getThreads(comments), expectedThreadGroups);
|
|
|
|
// Patch num should get inherited from comment rather
|
|
comments.push({
|
|
id: 'betsys_confession',
|
|
message: 'i like you, jack',
|
|
updated: '2015-12-24 15:00:10.396000000',
|
|
range: {
|
|
start_line: 1,
|
|
start_character: 1,
|
|
end_line: 1,
|
|
end_character: 2,
|
|
},
|
|
__commentSide: 'left',
|
|
});
|
|
|
|
expectedThreadGroups = [
|
|
{
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
commentSide: 'left',
|
|
comments: [{
|
|
id: 'sallys_confession',
|
|
message: 'i like you, jack',
|
|
updated: '2015-12-23 15:00:20.396000000',
|
|
__commentSide: 'left',
|
|
}, {
|
|
id: 'jacks_reply',
|
|
in_reply_to: 'sallys_confession',
|
|
message: 'i like you, too',
|
|
updated: '2015-12-24 15:01:20.396000000',
|
|
__commentSide: 'left',
|
|
}],
|
|
patchNum: 3,
|
|
rootId: 'sallys_confession',
|
|
},
|
|
{
|
|
start_datetime: '2015-12-24 15:00:10.396000000',
|
|
commentSide: 'left',
|
|
comments: [{
|
|
id: 'betsys_confession',
|
|
message: 'i like you, jack',
|
|
updated: '2015-12-24 15:00:10.396000000',
|
|
range: {
|
|
start_line: 1,
|
|
start_character: 1,
|
|
end_line: 1,
|
|
end_character: 2,
|
|
},
|
|
__commentSide: 'left',
|
|
}],
|
|
patchNum: 3,
|
|
rootId: 'betsys_confession',
|
|
range: {
|
|
start_line: 1,
|
|
start_character: 1,
|
|
end_line: 1,
|
|
end_character: 2,
|
|
},
|
|
},
|
|
];
|
|
|
|
assert.deepEqual(element._getThreads(comments), expectedThreadGroups);
|
|
});
|
|
|
|
test('multiple comments at same location but not threaded', () => {
|
|
element.patchForNewThreads = 3;
|
|
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(element._getThreads(comments).length, 2);
|
|
});
|
|
|
|
test('_sortByDate', () => {
|
|
let threadGroups = [
|
|
{
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
comments: [],
|
|
locationRange: 'line',
|
|
},
|
|
{
|
|
start_datetime: '2015-12-22 15:00:10.396000000',
|
|
comments: [],
|
|
locationRange: 'range-1-1-1-2',
|
|
},
|
|
];
|
|
|
|
let expectedResult = [
|
|
{
|
|
start_datetime: '2015-12-22 15:00:10.396000000',
|
|
comments: [],
|
|
locationRange: 'range-1-1-1-2',
|
|
}, {
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
comments: [],
|
|
locationRange: 'line',
|
|
},
|
|
];
|
|
|
|
assert.deepEqual(element._sortByDate(threadGroups), expectedResult);
|
|
|
|
// When a comment doesn't have a date, the one without the date should be
|
|
// last.
|
|
threadGroups = [
|
|
{
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
comments: [],
|
|
locationRange: 'line',
|
|
},
|
|
{
|
|
comments: [],
|
|
locationRange: 'range-1-1-1-2',
|
|
},
|
|
];
|
|
|
|
expectedResult = [
|
|
{
|
|
start_datetime: '2015-12-23 15:00:20.396000000',
|
|
comments: [],
|
|
locationRange: 'line',
|
|
},
|
|
{
|
|
comments: [],
|
|
locationRange: 'range-1-1-1-2',
|
|
},
|
|
];
|
|
|
|
assert.deepEqual(element._sortByDate(threadGroups), expectedResult);
|
|
});
|
|
|
|
test('_calculateLocationRange', () => {
|
|
const comment = {__commentSide: 'left'};
|
|
const range = {
|
|
start_line: 1,
|
|
start_character: 2,
|
|
end_line: 3,
|
|
end_character: 4,
|
|
};
|
|
assert.equal(
|
|
element._calculateLocationRange(range, comment),
|
|
'range-1-2-3-4-left');
|
|
});
|
|
|
|
test('thread groups are updated when comments change', () => {
|
|
const commentsChangedStub = sandbox.stub(element, '_commentsChanged');
|
|
element.comments = [];
|
|
element.comments.push({
|
|
id: 'sallys_confession',
|
|
message: 'i like you, jack',
|
|
updated: '2015-12-23 15:00:20.396000000',
|
|
});
|
|
assert(commentsChangedStub.called);
|
|
});
|
|
|
|
test('addNewThread', () => {
|
|
const locationRange = 'range-1-2-3-4';
|
|
element._threads = [{locationRange: 'line'}];
|
|
element.addNewThread(locationRange);
|
|
assert(element._threads.length, 2);
|
|
});
|
|
|
|
test('_getPatchNum', () => {
|
|
element.patchForNewThreads = 3;
|
|
const comment = {
|
|
id: 'sallys_confession',
|
|
message: 'i like you, jack',
|
|
updated: '2015-12-23 15:00:20.396000000',
|
|
};
|
|
assert.equal(element._getPatchNum(comment), 3);
|
|
comment.patchNum = 4;
|
|
assert.equal(element._getPatchNum(comment), 4);
|
|
});
|
|
|
|
test('removeThread', () => {
|
|
const locationRange = 'range-1-2-3-4';
|
|
element._threads = [
|
|
{locationRange: 'range-1-2-3-4', comments: []},
|
|
{locationRange: 'line', comments: []},
|
|
];
|
|
flushAsynchronousOperations();
|
|
element.removeThread(locationRange);
|
|
flushAsynchronousOperations();
|
|
assert(element._threads.length, 1);
|
|
});
|
|
|
|
test('_rangesEqual', () => {
|
|
const range1 =
|
|
{startLine: 123, startChar: 345, endLine: 234, endChar: 456};
|
|
const range2 =
|
|
{startLine: 1, startChar: 2, endLine: 3, endChar: 4};
|
|
|
|
assert.isTrue(element._rangesEqual(null, null));
|
|
assert.isTrue(element._rangesEqual(null, undefined));
|
|
assert.isTrue(element._rangesEqual(undefined, null));
|
|
assert.isTrue(element._rangesEqual(undefined, undefined));
|
|
|
|
assert.isFalse(element._rangesEqual(range1, null));
|
|
assert.isFalse(element._rangesEqual(null, range1));
|
|
assert.isFalse(element._rangesEqual(range1, range2));
|
|
|
|
assert.isTrue(element._rangesEqual(range1, Object.assign({}, range1)));
|
|
});
|
|
});
|
|
</script>
|