Centralized comment requests
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
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.html b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.html
new file mode 100644
index 0000000..68e2ff8
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.html
@@ -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>
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
new file mode 100644
index 0000000..81e5371
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.js
@@ -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);
+ },
+ });
+})();
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
new file mode 100644
index 0000000..1fdc872
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api_test.html
@@ -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>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
index 8be6e92..15a405a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
@@ -59,6 +59,7 @@
// 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 @@
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 @@
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 @@
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';
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 4bd469d..e39e6ec 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -24,6 +24,7 @@
<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 @@
<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>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index db534b4..e4901f5 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -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);
});
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index afba895..8ff75c1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -56,7 +56,7 @@
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 @@
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', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 43af472..a638b296 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -225,10 +225,10 @@
<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]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index a0abec9..cbbc7e4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -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();
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 921d285..8077e3a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -53,8 +53,6 @@
// 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 @@
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 @@
});
test('remove comment', () => {
- element._comments = {
+ element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -176,7 +151,7 @@
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 @@
element._removeComment({id: 'bc2', side: 'PARENT',
__commentSide: 'left'});
- assert.deepEqual(element._comments, {
+ assert.deepEqual(element.comments, {
meta: {
changeNum: '42',
patchRange: {
@@ -226,7 +201,7 @@
});
element._removeComment({id: 'd2', __commentSide: 'right'});
- assert.deepEqual(element._comments, {
+ assert.deepEqual(element.comments, {
meta: {
changeNum: '42',
patchRange: {
@@ -354,6 +329,7 @@
() => 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 @@
const setupDiff = function() {
const mock = document.createElement('mock-diff-response');
element._diff = mock.diffResponse;
- element._comments = {
+ element.comments = {
left: [],
right: [],
};
@@ -747,102 +723,6 @@
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 @@
change_type: 'MODIFIED',
content: [{skip: 66}],
};
- element._comments = {
+ element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -912,7 +792,7 @@
suite('handle comment-update', () => {
setup(() => {
- element._comments = {
+ element.comments = {
meta: {
changeNum: '42',
patchRange: {
@@ -941,7 +821,7 @@
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 @@
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 @@
() => Promise.resolve());
const mock = document.createElement('mock-diff-response');
element._diff = mock.diffResponse;
- element._comments = {left: [], right: []};
+ element.comments = {left: [], right: []};
element.noRenderOnPrefsChange = true;
});
