opendev/gerrit
Code Issues Proposed changes

Centralized comment requests

Browse Source 
Formerly, gr-diff would make its own REST calls for diff comments in the
patch range and path being viewed. However, these calls by the diff view
were redundant to the more general comment requests made by either
gr-diff-view (which requests all comments to support skipping to the
next file with comments) or gr-change-view (which requests comments
individually for each inline diff).

With this change, the diff component no longer loads comments for
itself, but is rather provided the comments from its host view via a
public property. In this way the host view can load the more-general
comment data, and filter it down for the diff view's params.

Introduces the gr-comment-api component to simplify loading and
filtering diff comments for use by diff views. By using this new
component:
* The diff view makes only one request for comments (including drafts
  and robot comments) to support skipping to the next comment and
  displaying comments in the diff view.
* The change view makes only one request for comments for all inline
  diffs.

Bug: Issue 5299
Change-Id: Ia14a01619f1f9881aa0d253fd3f49af9a17f3dce
This commit is contained in:
Wyatt Allen
2017-07-26 09:32:03 -07:00
parent 68c14fc93b
commit dc8782d762
14 changed files with 484 additions and 300 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
View File

@@ -19,6 +19,7 @@ limitations under the License.
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
<link rel="import" href="../../diff/gr-diff/gr-diff.html">
<link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
@@ -410,6 +411,7 @@ limitations under the License.
focus-on-move
cursor-target-class="selected"></gr-cursor-manager>
<gr-reporting id="reporting"></gr-reporting>
<gr-comment-api id="commentAPI"></gr-comment-api>
</template>
<script src="gr-file-list.js"></script>
</dom-module>

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

@@ -161,6 +161,9 @@
});
}));
// Load all comments for the change.
promises.push(this.$.commentAPI.loadAll(this.changeNum));
this._localPrefs = this.$.storage.getPreferences();
promises.push(this._getDiffPreferences().then(prefs => {
this.diffPrefs = prefs;
@@ -879,6 +882,9 @@
console.log('Expanding diff', 1 + initialCount - paths.length, 'of',
initialCount, ':', paths[0]);
const diffElem = this._findDiffByPath(paths[0], diffElements);
diffElem.comments = this.$.commentAPI.getCommentsForPath(paths[0],
this.patchRange, this.projectConfig);
const promises = [diffElem.reload()];
if (this._isLoggedIn) {
promises.push(this._reviewFile(paths[0]));

13
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
View File

@@ -60,6 +60,12 @@ limitations under the License.
stub('gr-diff', {
reload() { return Promise.resolve(); },
});
stub('gr-comment-api', {
loadAll() { return Promise.resolve(); },
getPaths() { return {}; },
getCommentsForPath() { return {meta: {}, left: [], right: []}; },
});
element = fixture('basic');
element.numFilesShown = 200;
saveStub = sandbox.stub(element, '_saveReviewedState',
@@ -965,7 +971,7 @@ limitations under the License.
const setupDiff = function(diff) {
const mock = document.createElement('mock-diff-response');
diff._diff = mock.diffResponse;
diff._comments = {
diff.comments = {
left: [],
right: [],
};
@@ -1013,6 +1019,11 @@ limitations under the License.
stub('gr-diff', {
reload() { return Promise.resolve(); },
});
stub('gr-comment-api', {
loadAll() { return Promise.resolve(); },
getPaths() { return {}; },
getCommentsForPath() { return {meta: {}, left: [], right: []}; },
});
element = fixture('basic');
element.numFilesShown = 75;
element.selectedIndex = 0;

26
polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.html Normal file
View File

@@ -0,0 +1,26 @@
<!--
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.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<dom-module id="gr-comment-api">
<template>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
<script src="gr-comment-api.js"></script>
</dom-module>

174
polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js Normal file
View File

@@ -0,0 +1,174 @@
// 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.
(function() {
'use strict';
const PARENT = 'PARENT';
Polymer({
is: 'gr-comment-api',
properties: {
_changeNum: Number,
_comments: Object,
_drafts: Object,
_robotComments: Object,
},
behaviors: [
Gerrit.PatchSetBehavior,
],
/**
* Load all comments (with drafts and robot comments) for the given change
* number. The returned promise resolves when the comments have loaded, but
* does not yield the comment data.
*
* @param {!number} changeNum
* @return {!Promise}
*/
loadAll(changeNum) {
this._changeNum = changeNum;
// Reset comment arrays.
this._comments = undefined;
this._drafts = undefined;
this._robotComments = undefined;
const promises = [];
promises.push(this.$.restAPI.getDiffComments(changeNum)
.then(comments => { this._comments = comments; }));
promises.push(this.$.restAPI.getDiffRobotComments(changeNum)
.then(robotComments => { this._robotComments = robotComments; }));
promises.push(this.$.restAPI.getLoggedIn()
.then(loggedIn => {
if (!loggedIn) { return Promise.resolve({}); }
return this.$.restAPI.getDiffDrafts(changeNum);
})
.then(drafts => { this._drafts = drafts; }));
return Promise.all(promises);
},
/**
* Get an object mapping file paths to a boolean representing whether that
* path contains diff comments in the given patch set (including drafts and
* robot comments).
*
* Paths with comments are mapped to true, whereas paths without comments
* are not mapped.
*
* @param {!Object} patchRange The patch-range object containing patchNum
* and basePatchNum properties to represent the range.
* @return {Object}
*/
getPaths(patchRange) {
const responses = [this._comments, this._drafts, this._robotComments];
const commentMap = {};
for (const response of responses) {
for (const path in response) {
if (response.hasOwnProperty(path) &&
response[path].some(c => this._isInPatchRange(c, patchRange))) {
commentMap[path] = true;
}
}
}
return commentMap;
},
/**
* Get the comments (with drafts and robot comments) for a path and
* patch-range. Returns an object with left and right properties mapping to
* arrays of comments in on either side of the patch range for that path.
*
* @param {!string} path
* @param {!Object} patchRange The patch-range object containing patchNum
* and basePatchNum properties to represent the range.
* @param {Object} opt_projectConfig Optional project config object to
* include in the meta sub-object.
* @return {Object}
*/
getCommentsForPath(path, patchRange, opt_projectConfig) {
const comments = this._comments[path] || [];
const drafts = this._drafts[path] || [];
const robotComments = this._robotComments[path] || [];
drafts.forEach(d => { d.__draft = true; });
const all = comments.concat(drafts).concat(robotComments);
const baseComments = all.filter(c =>
this._isInBaseOfPatchRange(c, patchRange));
const revisionComments = all.filter(c =>
this._isInRevisionOfPatchRange(c, patchRange));
return {
meta: {
changeNum: this._changeNum,
path,
patchRange,
projectConfig: opt_projectConfig,
},
left: baseComments,
right: revisionComments,
};
},
/**
* Whether the given comment should be included in the base side of the
* given patch range.
* @param {!Object} comment
* @param {!Object} range
* @return {boolean}
*/
_isInBaseOfPatchRange(comment, range) {
// If the base of the range is the parent of the patch:
if (range.basePatchNum === PARENT &&
comment.side === PARENT &&
this.patchNumEquals(comment.patch_set, range.patchNum)) {
return true;
}
// If the base of the range is not the parent of the patch:
if (range.basePatchNum !== PARENT &&
comment.side !== PARENT &&
this.patchNumEquals(comment.patch_set, range.basePatchNum)) {
return true;
}
return false;
},
/**
* Whether the given comment should be included in the revision side of the
* given patch range.
* @param {!Object} comment
* @param {!Object} range
* @return {boolean}
*/
_isInRevisionOfPatchRange(comment, range) {
return comment.side !== PARENT &&
this.patchNumEquals(comment.patch_set, range.patchNum);
},
/**
* Whether the given comment should be included in the given patch range.
* @param {!Object} comment
* @param {!Object} range
* @return {boolean}
*/
_isInPatchRange(comment, range) {
return this._isInBaseOfPatchRange(comment, range) ||
this._isInRevisionOfPatchRange(comment, range);
},
});
})();

194
polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html Normal file
View File

@@ -0,0 +1,194 @@
<!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');
return element.loadAll(changeNum).then(() => {
assert.isTrue(element.$.restAPI.getDiffComments.calledWithExactly(
changeNum));
assert.isTrue(element.$.restAPI.getDiffRobotComments.calledWithExactly(
changeNum));
assert.isFalse(element.$.restAPI.getDiffDrafts.called);
assert.isOk(element._comments);
assert.isOk(element._robotComments);
assert.deepEqual(element._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._comments);
assert.isOk(element._robotComments);
assert.notDeepEqual(element._drafts, {});
});
});
test('_isInBaseOfPatchRange', () => {
const comment = {patch_set: 1};
const patchRange = {basePatchNum: 1, patchNum: 2};
assert.isTrue(element._isInBaseOfPatchRange(comment, patchRange));
patchRange.basePatchNum = PARENT;
assert.isFalse(element._isInBaseOfPatchRange(comment, patchRange));
comment.side = PARENT;
assert.isFalse(element._isInBaseOfPatchRange(comment, patchRange));
comment.patch_set = 2;
assert.isTrue(element._isInBaseOfPatchRange(comment, patchRange));
});
test('_isInRevisionOfPatchRange', () => {
const comment = {patch_set: 123};
const patchRange = {basePatchNum: 122, patchNum: 124};
assert.isFalse(element._isInRevisionOfPatchRange(comment, patchRange));
patchRange.patchNum = 123;
assert.isTrue(element._isInRevisionOfPatchRange(comment, patchRange));
comment.side = PARENT;
assert.isFalse(element._isInRevisionOfPatchRange(comment, patchRange));
});
suite('comment ranges and paths', () => {
setup(() => {
element._changeNum = 1234;
element._drafts = {};
element._robotComments = {};
element._comments = {
'file/one': [
{patch_set: 2, side: PARENT},
{patch_set: 2},
],
'file/two': [
{patch_set: 2},
{patch_set: 3},
],
'file/three': [
{patch_set: 2, side: PARENT},
{patch_set: 3},
],
};
});
test('getPaths', () => {
const patchRange = {basePatchNum: 1, patchNum: 4};
let paths = element.getPaths(patchRange);
assert.equal(Object.keys(paths).length, 0);
patchRange.basePatchNum = PARENT;
patchRange.patchNum = 3;
paths = element.getPaths(patchRange);
assert.notProperty(paths, 'file/one');
assert.property(paths, 'file/two');
assert.property(paths, 'file/three');
patchRange.patchNum = 2;
paths = element.getPaths(patchRange);
assert.property(paths, 'file/one');
assert.property(paths, 'file/two');
assert.property(paths, 'file/three');
});
test('getCommentsForPath', () => {
const patchRange = {basePatchNum: 1, patchNum: 3};
let path = 'file/one';
let comments = element.getCommentsForPath(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.getCommentsForPath(path, patchRange);
assert.equal(comments.left.length, 0);
assert.equal(comments.right.length, 1);
patchRange.basePatchNum = 2;
comments = element.getCommentsForPath(path, patchRange);
assert.equal(comments.left.length, 1);
assert.equal(comments.right.length, 1);
patchRange.basePatchNum = PARENT;
path = 'file/three';
comments = element.getCommentsForPath(path, patchRange);
assert.equal(comments.left.length, 0);
assert.equal(comments.right.length, 1);
});
});
});
</script>

27
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

@@ -59,6 +59,7 @@ limitations under the License.
// Register the diff with the cursor.
cursorElement.push('diffs', diffElement);
diffElement.comments = {left: [], right: []};
diffElement.$.restAPI.getDiffPreferences().then(prefs => {
diffElement.prefs = prefs;
});
@@ -67,19 +68,8 @@ limitations under the License.
return Promise.resolve(mockDiffResponse.diffResponse);
});
sandbox.stub(diffElement, '_getDiffComments', () => {
return Promise.resolve({baseComments: [], comments: []});
});
sandbox.stub(diffElement, '_getDiffDrafts', () => {
return Promise.resolve({baseComments: [], comments: []});
});
sandbox.stub(diffElement, '_getDiffRobotComments', () => {
return Promise.resolve({baseComments: [], comments: []});
});
const setupDone = () => {
cursorElement._updateStops();
cursorElement.moveToFirstChunk();
diffElement.removeEventListener('render', setupDone);
done();
@@ -218,12 +208,13 @@ limitations under the License.
const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber');
const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
diffElement.addEventListener('render', () => {
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
assert.isFalse(moveToNumStub.called);
assert.isTrue(moveToChunkStub.called);
done();
});
}
diffElement.addEventListener('render', renderHandler);
diffElement.reload();
});
@@ -231,13 +222,15 @@ limitations under the License.
const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber');
const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
diffElement.addEventListener('render', () => {
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
assert.isFalse(moveToChunkStub.called);
assert.isTrue(moveToNumStub.called);
assert.equal(moveToNumStub.lastCall.args[0], 10);
assert.equal(moveToNumStub.lastCall.args[1], 'right');
done();
});
}
diffElement.addEventListener('render', renderHandler);
cursorElement.initialLineNumber = 10;
cursorElement.side = 'right';

2
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
View File

@@ -24,6 +24,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-fixed-panel/gr-fixed-panel.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
<link rel="import" href="../gr-comment-api/gr-comment-api.html">
<link rel="import" href="../gr-diff-cursor/gr-diff-cursor.html">
<link rel="import" href="../gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../gr-diff/gr-diff.html">
@@ -337,6 +338,7 @@ limitations under the License.
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
<gr-diff-cursor id="cursor"></gr-diff-cursor>
<gr-comment-api id="commentAPI"></gr-comment-api>
</template>
<script src="gr-diff-view.js"></script>
</dom-module>

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

@@ -21,6 +21,8 @@
const COMMENT_SAVE = 'Try again when all comments have saved.';
const PARENT = 'PARENT';
const DiffSides = {
LEFT: 'left',
RIGHT: 'right',
@@ -99,6 +101,8 @@
*/
_commentMap: Object,
_commentsForDiff: Object,
/**
* Object to contain the path of the next and previous file in the current
* change and patch range that has comments.
@@ -464,7 +468,7 @@
this._changeNum = value.changeNum;
this._patchRange = {
patchNum: value.patchNum,
basePatchNum: value.basePatchNum || 'PARENT',
basePatchNum: value.basePatchNum || PARENT,
};
this._path = value.path;
@@ -493,14 +497,13 @@
promises.push(this._getChangeDetail(this._changeNum));
promises.push(this._loadComments());
Promise.all(promises).then(() => {
this._loading = false;
this.$.diff.comments = this._commentsForDiff;
this.$.diff.reload();
});
this._loadCommentMap().then(commentMap => {
this._commentMap = commentMap;
});
},
_changeViewStateChanged(changeViewState) {
@@ -557,7 +560,7 @@
_patchRangeStr(patchRange) {
let patchStr = patchRange.patchNum;
if (patchRange.basePatchNum != null &&
patchRange.basePatchNum != 'PARENT') {
patchRange.basePatchNum != PARENT) {
patchStr = patchRange.basePatchNum + '..' + patchRange.patchNum;
}
return patchStr;
@@ -584,7 +587,7 @@
for (const rev of Object.values(revisions || {})) {
latestPatchNum = Math.max(latestPatchNum, rev._number);
}
if (patchRange.basePatchNum !== 'PARENT' ||
if (patchRange.basePatchNum !== PARENT ||
parseInt(patchRange.patchNum, 10) !== latestPatchNum) {
patchNum = patchRange.patchNum;
basePatchNum = patchRange.basePatchNum;
@@ -717,33 +720,12 @@
return url;
},
/**
* Request all comments (and drafts and robot comments) for the current
* change and construct the map of file paths that have comments for the
* current patch range.
* @return {Promise} A promise that yields a comment map object.
*/
_loadCommentMap() {
const filterByRange = comment =>
this.patchNumEquals(comment.patch_set, this._patchRange.patchNum) ||
this.patchNumEquals(comment.patch_set,
this._patchRange.basePatchNum);
_loadComments() {
return this.$.commentAPI.loadAll(this._changeNum).then(() => {
this._commentMap = this.$.commentAPI.getPaths(this._patchRange);
return Promise.all([
this.$.restAPI.getDiffComments(this._changeNum),
this._getDiffDrafts(),
this.$.restAPI.getDiffRobotComments(this._changeNum),
]).then(results => {
const commentMap = {};
for (const response of results) {
for (const path in response) {
if (response.hasOwnProperty(path) &&
response[path].filter(filterByRange).length) {
commentMap[path] = true;
}
}
}
return commentMap;
this._commentsForDiff = this.$.commentAPI.getCommentsForPath(this._path,
this._patchRange, this._projectConfig);
});
},

50
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
View File

@@ -56,7 +56,7 @@ limitations under the License.
getDiffChangeDetail() { return Promise.resolve(null); },
getChangeFiles() { return Promise.resolve({}); },
saveFileReviewed() { return Promise.resolve(); },
getDiffRobotComments() { return Promise.resolve(); },
getDiffRobotComments() { return Promise.resolve({}); },
getDiffDrafts() { return Promise.resolve(); },
});
element = fixture('basic');
@@ -639,57 +639,41 @@ limitations under the License.
assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
});
suite('_loadCommentMap', () => {
suite('_loadComments', () => {
test('empty', done => {
stub('gr-rest-api-interface', {
getDiffComments() { return Promise.resolve({}); },
stub('gr-comment-api', {
loadAll() { return Promise.resolve(); },
getPaths() { return {}; },
getCommentsForPath() { return {meta: {}}; },
});
element._loadCommentMap().then(map => {
assert.equal(Object.keys(map).length, 0);
element._loadComments().then(() => {
assert.equal(Object.keys(element._commentMap).length, 0);
done();
});
});
test('paths in patch range', done => {
stub('gr-rest-api-interface', {
getDiffComments() {
return Promise.resolve({
test('has paths', done => {
stub('gr-comment-api', {
loadAll() { return Promise.resolve(); },
getPaths() {
return {
'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
});
};
},
getCommentsForPath() { return {meta: {}}; },
});
element._changeNum = '42';
element._patchRange = {
basePatchNum: '3',
patchNum: '5',
};
element._loadCommentMap().then(map => {
assert.deepEqual(Object.keys(map),
element._loadComments().then(() => {
assert.deepEqual(Object.keys(element._commentMap),
['path/to/file/one.cpp', 'path-to/file/two.py']);
done();
});
});
test('empty for paths outside patch range', done => {
stub('gr-rest-api-interface', {
getDiffComments() {
return Promise.resolve({
'path/to/file/one.cpp': [{patch_set: PARENT, message: 'lorem'}],
'path-to/file/two.py': [{patch_set: 2, message: 'ipsum'}],
});
},
});
element._changeNum = '42';
element._patchRange = {
basePatchNum: '3',
patchNum: '5',
};
element._loadCommentMap().then(map => {
assert.equal(Object.keys(map).length, 0);
done();
});
});
});
suite('_computeCommentSkips', () => {

4
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -225,10 +225,10 @@ limitations under the License.
<gr-diff-highlight
id="highlights"
logged-in="[[_loggedIn]]"
comments="{{_comments}}">
comments="{{comments}}">
<gr-diff-builder
id="diffBuilder"
comments="[[_comments]]"
comments="[[comments]]"
diff="[[_diff]]"
diff-path="[[path]]"
view-mode="[[viewMode]]"

93
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
View File

@@ -77,6 +77,7 @@
reflectToAttribute: true,
},
noRenderOnPrefsChange: Boolean,
comments: Object,
_loggedIn: {
type: Boolean,
value: false,
@@ -101,7 +102,6 @@
type: String,
value: '',
},
_comments: Object,
_baseImage: Object,
_revisionImage: Object,
@@ -152,10 +152,6 @@
return this._loadDiffAssets();
}));
promises.push(this._getDiffCommentsAndDrafts().then(comments => {
this._comments = comments;
}));
return Promise.all(promises).then(() => {
if (this.prefs) {
return this._renderDiffTable();
@@ -373,7 +369,7 @@
const comment = e.detail.comment;
const side = e.detail.comment.__commentSide;
const idx = this._findDraftIndex(comment, side);
this.set(['_comments', side, idx], comment);
this.set(['comments', side, idx], comment);
},
_handleCommentUpdate(e) {
@@ -384,9 +380,9 @@
idx = this._findDraftIndex(comment, side);
}
if (idx !== -1) { // Update draft or comment.
this.set(['_comments', side, idx], comment);
this.set(['comments', side, idx], comment);
} else { // Create new draft.
this.push(['_comments', side], comment);
this.push(['comments', side], comment);
}
},
@@ -396,24 +392,24 @@
idx = this._findDraftIndex(comment, side);
}
if (idx !== -1) {
this.splice('_comments.' + side, idx, 1);
this.splice('comments.' + side, idx, 1);
}
},
_findCommentIndex(comment, side) {
if (!comment.id || !this._comments[side]) {
if (!comment.id || !this.comments[side]) {
return -1;
}
return this._comments[side].findIndex(item => {
return this.comments[side].findIndex(item => {
return item.id === comment.id;
});
},
_findDraftIndex(comment, side) {
if (!comment.__draftID || !this._comments[side]) {
if (!comment.__draftID || !this.comments[side]) {
return -1;
}
return this._comments[side].findIndex(item => {
return this.comments[side].findIndex(item => {
return item.__draftID === comment.__draftID;
});
},
@@ -463,7 +459,7 @@
this.updateStyles(stylesToUpdate);
if (this._diff && this._comments && !this.noRenderOnPrefsChange) {
if (this._diff && this.comments && !this.noRenderOnPrefsChange) {
this._renderDiffTable();
}
},
@@ -477,7 +473,7 @@
}
this._showWarning = false;
return this.$.diffBuilder.render(this._comments, this._getBypassPrefs());
return this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
},
/**
@@ -519,73 +515,6 @@
});
},
_getDiffComments() {
return this.$.restAPI.getDiffComments(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
},
_getDiffDrafts() {
return this._getLoggedIn().then(loggedIn => {
if (!loggedIn) {
return Promise.resolve({baseComments: [], comments: []});
}
return this.$.restAPI.getDiffDrafts(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
});
},
_getDiffRobotComments() {
return this.$.restAPI.getDiffRobotComments(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
},
_getDiffCommentsAndDrafts() {
const promises = [];
promises.push(this._getDiffComments());
promises.push(this._getDiffDrafts());
promises.push(this._getDiffRobotComments());
return Promise.all(promises).then(results => {
return Promise.resolve({
comments: results[0],
drafts: results[1],
robotComments: results[2],
});
}).then(this._normalizeDiffCommentsAndDrafts.bind(this));
},
_normalizeDiffCommentsAndDrafts(results) {
function markAsDraft(d) {
d.__draft = true;
return d;
}
const baseDrafts = results.drafts.baseComments.map(markAsDraft);
const drafts = results.drafts.comments.map(markAsDraft);
const baseRobotComments = results.robotComments.baseComments;
const robotComments = results.robotComments.comments;
return Promise.resolve({
meta: {
path: this.path,
changeNum: this.changeNum,
patchRange: this.patchRange,
projectConfig: this.projectConfig,
},
left: results.comments.baseComments.concat(baseDrafts)
.concat(baseRobotComments),
right: results.comments.comments.concat(drafts)
.concat(robotComments),
});
},
_getLoggedIn() {
return this.$.restAPI.getLoggedIn();
},

144
polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
View File

@@ -53,8 +53,6 @@ limitations under the License.
// Stub the network calls into requests that never resolve.
sandbox.stub(element, '_getDiff', () => new Promise(() => {}));
sandbox.stub(element, '_getDiffCommentsAndDrafts',
() => new Promise(() => {}));
element.reload();
assert.isTrue(cancelStub.called);
@@ -105,29 +103,6 @@ limitations under the License.
element.$$('.diffContainer').classList.contains('displayLine'));
});
test('get drafts', done => {
element.patchRange = {basePatchNum: 0, patchNum: 0};
const getDraftsStub = sandbox.stub(element.$.restAPI, 'getDiffDrafts');
element._getDiffDrafts().then(result => {
assert.deepEqual(result, {baseComments: [], comments: []});
sinon.assert.notCalled(getDraftsStub);
done();
});
});
test('get robot comments', done => {
element.patchRange = {basePatchNum: 0, patchNum: 0};
const getDraftsStub = sandbox.stub(element.$.restAPI,
'getDiffRobotComments');
element._getDiffDrafts().then(result => {
assert.deepEqual(result, {baseComments: [], comments: []});
sinon.assert.notCalled(getDraftsStub);
done();
});
});
test('loads files weblinks', done => {
sandbox.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({
@@ -149,7 +124,7 @@ limitations under the License.
});
test('remove comment', () => {
element._comments = {
element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -176,7 +151,7 @@ limitations under the License.
element._removeComment({});
// Using JSON.stringify because Safari 9.1 (11601.5.17.1) doesn’t seem
// to believe that one object deepEquals another even when they do :-/.
assert.equal(JSON.stringify(element._comments), JSON.stringify({
assert.equal(JSON.stringify(element.comments), JSON.stringify({
meta: {
changeNum: '42',
patchRange: {
@@ -202,7 +177,7 @@ limitations under the License.
element._removeComment({id: 'bc2', side: 'PARENT',
__commentSide: 'left'});
assert.deepEqual(element._comments, {
assert.deepEqual(element.comments, {
meta: {
changeNum: '42',
patchRange: {
@@ -226,7 +201,7 @@ limitations under the License.
});
element._removeComment({id: 'd2', __commentSide: 'right'});
assert.deepEqual(element._comments, {
assert.deepEqual(element.comments, {
meta: {
changeNum: '42',
patchRange: {
@@ -354,6 +329,7 @@ limitations under the License.
() => Promise.resolve(mockComments)));
element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
element.comments = {left: [], right: []};
});
test('renders image diffs with same file name', done => {
@@ -691,7 +667,7 @@ limitations under the License.
const setupDiff = function() {
const mock = document.createElement('mock-diff-response');
element._diff = mock.diffResponse;
element._comments = {
element.comments = {
left: [],
right: [],
};
@@ -747,102 +723,6 @@ limitations under the License.
element = fixture('basic');
});
test('get drafts', done => {
element.patchRange = {basePatchNum: 0, patchNum: 0};
const draftsResponse = {
baseComments: [{id: 'foo'}],
comments: [{id: 'bar'}],
};
sandbox.stub(element.$.restAPI, 'getDiffDrafts',
() => Promise.resolve(draftsResponse));
element._getDiffDrafts().then(result => {
assert.deepEqual(result, draftsResponse);
done();
});
});
test('get comments and drafts', done => {
const comments = {
baseComments: [
{id: 'bc1', __commentSide: 'left'},
{id: 'bc2', __commentSide: 'left'},
],
comments: [
{id: 'c1', __commentSide: 'right'},
{id: 'c2', __commentSide: 'right'},
],
};
sandbox.stub(element, '_getDiffComments',
() => Promise.resolve(comments));
const drafts = {
baseComments: [
{id: 'bd1', __commentSide: 'left'},
{id: 'bd2', __commentSide: 'left'},
],
comments: [
{id: 'd1', __commentSide: 'right'},
{id: 'd2', __commentSide: 'right'},
],
};
sandbox.stub(element, '_getDiffDrafts', () => Promise.resolve(drafts));
const robotComments = {
baseComments: [
{id: 'br1', __commentSide: 'left'},
{id: 'br2', __commentSide: 'left'},
],
comments: [
{id: 'r1', __commentSide: 'right'},
{id: 'r2', __commentSide: 'right'},
],
};
sandbox.stub(element,
'_getDiffRobotComments', () => Promise.resolve(robotComments));
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
patchNum: 3,
};
element.path = '/path/to/foo';
element.projectConfig = {foo: 'bar'};
element._getDiffCommentsAndDrafts().then(result => {
assert.deepEqual(result, {
meta: {
changeNum: '42',
patchRange: {
basePatchNum: 'PARENT',
patchNum: 3,
},
path: '/path/to/foo',
projectConfig: {foo: 'bar'},
},
left: [
{id: 'bc1', __commentSide: 'left'},
{id: 'bc2', __commentSide: 'left'},
{id: 'bd1', __draft: true, __commentSide: 'left'},
{id: 'bd2', __draft: true, __commentSide: 'left'},
{id: 'br1', __commentSide: 'left'},
{id: 'br2', __commentSide: 'left'},
],
right: [
{id: 'c1', __commentSide: 'right'},
{id: 'c2', __commentSide: 'right'},
{id: 'd1', __draft: true, __commentSide: 'right'},
{id: 'd2', __draft: true, __commentSide: 'right'},
{id: 'r1', __commentSide: 'right'},
{id: 'r2', __commentSide: 'right'},
],
});
done();
});
});
test('addDraftAtLine', done => {
const fakeLineEl = {getAttribute: sandbox.stub().returns(42)};
sandbox.stub(element, '_selectLine');
@@ -868,7 +748,7 @@ limitations under the License.
change_type: 'MODIFIED',
content: [{skip: 66}],
};
element._comments = {
element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -912,7 +792,7 @@ limitations under the License.
suite('handle comment-update', () => {
setup(() => {
element._comments = {
element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -941,7 +821,7 @@ limitations under the License.
const comment = {__draft: true, __draftID: 'tempID', side: 'PARENT',
__commentSide: 'left'};
element.fire('comment-update', {comment});
assert.include(element._comments.left, comment);
assert.include(element.comments.left, comment);
});
test('saving a draft', () => {
@@ -953,10 +833,10 @@ limitations under the License.
side: 'PARENT',
__commentSide: 'left',
};
element._comments.left.push(comment);
element.comments.left.push(comment);
comment.id = id;
element.fire('comment-update', {comment});
const drafts = element._comments.left.filter(item => {
const drafts = element.comments.left.filter(item => {
return item.__draftID === draftID;
});
assert.equal(drafts.length, 1);
@@ -1007,7 +887,7 @@ limitations under the License.
() => Promise.resolve());
const mock = document.createElement('mock-diff-response');
element._diff = mock.diffResponse;
element._comments = {left: [], right: []};
element.comments = {left: [], right: []};
element.noRenderOnPrefsChange = true;
});

1
polygerrit-ui/app/test/index.html
View File

@@ -70,6 +70,7 @@ limitations under the License.
'core/gr-router/gr-router_test.html',
'core/gr-reporting/gr-reporting_test.html',
'core/gr-search-bar/gr-search-bar_test.html',
'diff/gr-comment-api/gr-comment-api_test.html',
'diff/gr-diff-builder/gr-diff-builder_test.html',
'diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html',
'diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html',