Merge "Centralized comment requests"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 36d6f0e..aca7a2e 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -19,6 +19,7 @@
<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 @@
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>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index c38b324..05e658f 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -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]));
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index 4791dc3..fcf8c74 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -60,6 +60,12 @@
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 @@
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 @@
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;
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;
});
diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html
index fd055c6..355a7b6 100644
--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@@ -71,6 +71,7 @@
'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',
