Update gr-comment-api
Previously, nested components (that are never standalone) were making
their own requests for comments, creating unecessary duplicate API
requests.
Some requests were made to the rest API directly (for example,
gr-change-view requested comments this way), and some were through the
gr-comment-api, which also helped handle comment manipulation and
reorganization.
Instead of ad-hoc comment requesting, move all comment requests to the
top level (standalone) component, and use an object prototype to
generate _changeComments as a property on gr-comment-api. This is an
immutable object that can be passed to the inner components
(file list, etc), and includes the methods needed to manipulate
comments into the forms
necessary.
When a child component needs to trigger a refresh of the comments, fire
an event that the parent event handles (see gr-file-list l.867).
Bug: Issue 6953
Change-Id: Ic4b6cf16520baae65d8cf956c311a60f2a70a2e1
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 8b15880..7a29eea3 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -21,6 +21,7 @@
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../styles/shared-styles.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
+<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
<link rel="import" href="../../diff/gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../../edit/gr-edit-constants.html">
<link rel="import" href="../../plugins/gr-endpoint-decorator/gr-endpoint-decorator.html">
@@ -433,7 +434,7 @@
all-patch-sets="[[_allPatchSets]]"
change="[[_change]]"
change-num="[[_changeNum]]"
- comments="[[_comments]]"
+ comments="[[_changeComments.comments]]"
commit-info="[[_commitInfo]]"
change-url="[[_computeChangeUrl(_change)]]"
edit-loaded="[[_editLoaded]]"
@@ -456,7 +457,7 @@
change="[[_change]]"
change-num="[[_changeNum]]"
patch-range="{{_patchRange}}"
- comments="[[_comments]]"
+ change-comments="[[_changeComments]]"
drafts="[[_diffDrafts]]"
revisions="[[_sortedRevisions]]"
project-config="[[_projectConfig]]"
@@ -467,7 +468,8 @@
files-expanded="{{_filesExpanded}}"
file-list-increment="{{_numFilesShown}}"
on-files-shown-changed="_setShownFiles"
- on-file-action-tap="_handleFileActionTap"></gr-file-list>
+ on-file-action-tap="_handleFileActionTap"
+ on-reload-comments="_reloadCommentsWithCallback"></gr-file-list>
</section>
<gr-endpoint-decorator name="change-view-integration">
</gr-endpoint-decorator>
@@ -476,7 +478,7 @@
change-num="[[_changeNum]]"
messages="[[_change.messages]]"
reviewer-updates="[[_change.reviewer_updates]]"
- comments="[[_comments]]"
+ comments="[[_changeComments.comments]]"
project-name="[[_change.project]]"
show-reply-buttons="[[_loggedIn]]"
on-reply="_handleMessageReply"></gr-messages-list>
@@ -511,6 +513,7 @@
</gr-overlay>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
+ <gr-comment-api id="commentAPI"></gr-comment-api>
</template>
<script src="gr-change-view.js"></script>
</dom-module>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index c658e8d..5da411e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -96,6 +96,8 @@
type: Object,
value: {},
},
+ /** @type {?} */
+ _changeComments: Object,
_canStartReview: {
type: Boolean,
computed: '_computeCanStartReview(_loggedIn, _change, _account)',
@@ -1006,12 +1008,6 @@
});
},
- _getComments() {
- return this.$.restAPI.getDiffComments(this._changeNum).then(comments => {
- this._comments = comments;
- });
- },
-
_getEdit() {
return this.$.restAPI.getChangeEdit(this._changeNum, true);
},
@@ -1051,30 +1047,28 @@
});
},
- _reloadDiffDrafts() {
- this._diffDrafts = {};
- this._getDiffDrafts().then(() => {
- if (this.$.replyOverlay.opened) {
- this.async(() => { this.$.replyOverlay.center(); }, 1);
- }
+ _reloadCommentsWithCallback(e) {
+ return this._reloadComments().then(() => {
+ return e.detail.resolve();
});
},
+ _reloadComments() {
+ return this.$.commentAPI.loadAll(this._changeNum)
+ .then(comments => {
+ this._changeComments = this.$.commentAPI._changeComments;
+ });
+ },
+
_reload() {
this._loading = true;
this._relatedChangesCollapsed = true;
- this._getLoggedIn().then(loggedIn => {
- if (!loggedIn) { return; }
-
- this._reloadDiffDrafts();
- });
-
const detailCompletes = this._getChangeDetail().then(() => {
this._loading = false;
this._getProjectConfig();
});
- this._getComments();
+ this._reloadComments();
if (this._patchRange.patchNum) {
return Promise.all([
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index 129b4b3..915a229 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -245,6 +245,15 @@
});
});
+ test('comments are reloaded when reload-comments fired', () => {
+ const reloadStub = sandbox.stub(element.$.commentAPI, 'loadAll')
+ .returns(Promise.resolve());
+ element.$.fileList.fire('reload-comments', {
+ resolve: () => { return Promise.resolve(); },
+ });
+ assert.isTrue(reloadStub.called);
+ });
+
test('download tap calls _handleOpenDownloadDialog', () => {
sandbox.stub(element, '_handleOpenDownloadDialog');
element.$.actions.fire('download-tap');
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 7080fc6..4ee0f31 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
@@ -21,7 +21,6 @@
<link rel="import" href="../../../styles/shared-styles.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="../../edit/gr-edit-file-controls/gr-edit-file-controls.html">
@@ -267,17 +266,17 @@
</span>
<div class="comments desktop">
<span class="drafts">
- [[_computeDraftsString(drafts, patchRange.patchNum, file.__path)]]
+ [[_computeDraftsString(changeComments.drafts, patchRange.patchNum, file.__path)]]
</span>
- [[_computeCommentsString(comments, patchRange.patchNum, file.__path)]]
- [[_computeUnresolvedString(comments, drafts, patchRange.patchNum, file.__path)]]
+ [[_computeCommentsString(changeComments.comments, patchRange.patchNum, file.__path)]]
+ [[_computeUnresolvedString(changeComments.comments, changeComments.drafts, patchRange.patchNum, file.__path)]]
</div>
<div class="comments mobile">
<span class="drafts">
- [[_computeDraftsStringMobile(drafts, patchRange.patchNum,
+ [[_computeDraftsStringMobile(changeComments.drafts, patchRange.patchNum,
file.__path)]]
</span>
- [[_computeCommentsStringMobile(comments, patchRange.patchNum,
+ [[_computeCommentsStringMobile(changeComments.comments, patchRange.patchNum,
file.__path)]]
</div>
<div class$="[[_computeClass('stats', file.__path)]]">
@@ -402,7 +401,6 @@
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 f7b1af9..6569088 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
@@ -32,12 +32,19 @@
Polymer({
is: 'gr-file-list',
+ /**
+ * Fired when a comment refresh should get triggered
+ *
+ * @event reload-comments
+ */
+
properties: {
/** @type {?} */
patchRange: Object,
patchNum: String,
changeNum: String,
- comments: Object,
+ /** @type {?} */
+ changeComments: Object,
drafts: Object,
// Already sorted by the change-view.
revisions: Array,
@@ -193,9 +200,6 @@
});
}));
- // 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;
@@ -272,11 +276,7 @@
const timerName = 'Update ' + this._expandedFilePaths.length +
' diffs with new prefs';
this._renderInOrder(this._expandedFilePaths, this.diffs,
- this._expandedFilePaths.length)
- .then(() => {
- this.$.reporting.timeEnd(timerName);
- this.$.diffCursor.handleDiffUpdate();
- });
+ this._expandedFilePaths.length, timerName);
},
_forEachDiff(fn) {
@@ -853,11 +853,7 @@
// Required so that the newly created diff view is included in this.diffs.
Polymer.dom.flush();
- this._renderInOrder(newPaths, this.diffs, newPaths.length)
- .then(() => {
- this.$.reporting.timeEnd(timerName);
- this.$.diffCursor.handleDiffUpdate();
- });
+ this._renderInOrder(newPaths, this.diffs, newPaths.length, timerName);
this._updateDiffCursor();
this.$.diffCursor.handleDiffUpdate();
},
@@ -870,30 +866,35 @@
* @param {!NodeList<!Object>} diffElements (GrDiffElement)
* @param {number} initialCount The total number of paths in the pass. This
* is used to generate log messages.
+ * @param {string} timerName the timer to stop after the render has
+ * completed
* @return {!Promise}
*/
- _renderInOrder(paths, diffElements, initialCount) {
+ _renderInOrder(paths, diffElements, initialCount, timerName) {
let iter = 0;
- return this.$.commentAPI.loadAll(this.changeNum)
- .then(() => {
- return this.asyncForeach(paths, path => {
- iter++;
- console.log('Expanding diff', iter, 'of', initialCount, ':',
- path);
- const diffElem = this._findDiffByPath(path, diffElements);
- diffElem.comments = this.$.commentAPI.getCommentsForPath(path,
- this.patchRange, this.projectConfig);
- const promises = [diffElem.reload()];
- if (this._isLoggedIn) {
- promises.push(this._reviewFile(path));
- }
- return Promise.all(promises);
- });
- })
- .then(() => {
- console.log('Finished expanding', initialCount, 'diff(s)');
- });
+ return (new Promise(resolve => {
+ this.fire('reload-comments', {resolve});
+ })).then(() => {
+ return this.asyncForeach(paths, path => {
+ iter++;
+ console.log('Expanding diff', iter, 'of', initialCount, ':',
+ path);
+ const diffElem = this._findDiffByPath(path, diffElements);
+ diffElem.comments = this.changeComments.getCommentsForPath(path,
+ this.patchRange, this.projectConfig);
+ const promises = [diffElem.reload()];
+ if (this._isLoggedIn) {
+ promises.push(this._reviewFile(path));
+ }
+ return Promise.all(promises);
+ }).then(() => {
+ this._nextRenderParams = null;
+ console.log('Finished expanding', initialCount, 'diff(s)');
+ this.$.reporting.timeEnd(timerName);
+ this.$.diffCursor.handleDiffUpdate();
+ });
+ });
},
/**
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 be03e38..5854f2f 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
@@ -22,6 +22,7 @@
<script src="../../../bower_components/web-component-tester/browser.js"></script>
<link rel="import" href="../../../test/common-test-setup.html"/>
<script src="../../../bower_components/page/page.js"></script>
+<link rel="import" href="../../diff/gr-comment-api/gr-comment-api.html">
<script src="../../../scripts/util.js"></script>
<link rel="import" href="../../shared/gr-rest-api-interface/mock-diff-response_test.html">
@@ -29,25 +30,39 @@
<script>void(0);</script>
+<dom-module id="comment-api-mock">
+ <template>
+ <gr-file-list id="fileList"
+ change-comments="[[_changeComments]]"
+ on-reload-comments="_reloadCommentsWithCallback"></gr-file-list>
+ <gr-comment-api id="commentAPI"></gr-comment-api>
+ </template>
+ <script src="../../diff/gr-comment-api/gr-comment-api-mock.js"></script>
+</dom-module>
+
<test-fixture id="basic">
<template>
- <gr-file-list></gr-file-list>
+ <comment-api-mock></comment-api-mock>
</template>
</test-fixture>
<script>
suite('gr-file-list tests', () => {
let element;
+ let commentApiWrapper;
let sandbox;
let saveStub;
- let loadCommentStub;
+ let loadCommentSpy;
- setup(() => {
+ setup(done => {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(true); },
getPreferences() { return Promise.resolve({}); },
fetchJSON() { return Promise.resolve({}); },
+ getDiffComments() { return Promise.resolve({}); },
+ getDiffRobotComments() { return Promise.resolve({}); },
+ getDiffDrafts() { return Promise.resolve({}); },
});
stub('gr-date-formatter', {
_loadTimeFormat() { return Promise.resolve(''); },
@@ -55,17 +70,25 @@
stub('gr-diff', {
reload() { return Promise.resolve(); },
});
- stub('gr-comment-api', {
- getPaths() { return {}; },
- getCommentsForPath() { return {meta: {}, left: [], right: []}; },
+
+ // Element must be wrapped in an element with direct access to the
+ // comment API.
+ commentApiWrapper = fixture('basic');
+ element = commentApiWrapper.$.fileList;
+ loadCommentSpy = sandbox.spy(commentApiWrapper.$.commentAPI, 'loadAll');
+
+ // Stub methods on the changeComments object after changeComments has
+ // been initalized.
+ commentApiWrapper.loadComments().then(() => {
+ sandbox.stub(element.changeComments, 'getPaths').returns({});
+ sandbox.stub(element.changeComments, 'getCommentsForPath')
+ .returns({meta: {}, left: [], right: []});
+ done();
});
- element = fixture('basic');
element.numFilesShown = 200;
saveStub = sandbox.stub(element, '_saveReviewedState',
() => { return Promise.resolve(); });
- loadCommentStub = sandbox.stub(element.$.commentAPI, 'loadAll',
- () => { return Promise.resolve(); });
});
teardown(() => {
@@ -875,7 +898,7 @@
element._renderInOrder(['p2', 'p1', 'p0'], diffs, 3)
.then(() => {
assert.isFalse(reviewStub.called);
- assert.isTrue(loadCommentStub.called);
+ assert.isTrue(loadCommentSpy.called);
done();
});
});
@@ -998,11 +1021,14 @@
return diffs;
};
- setup(() => {
+ setup(done => {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getLoggedIn() { return Promise.resolve(true); },
getPreferences() { return Promise.resolve({}); },
+ getDiffComments() { return Promise.resolve({}); },
+ getDiffRobotComments() { return Promise.resolve({}); },
+ getDiffDrafts() { return Promise.resolve({}); },
});
stub('gr-date-formatter', {
_loadTimeFormat() { return Promise.resolve(''); },
@@ -1010,12 +1036,21 @@
stub('gr-diff', {
reload() { return Promise.resolve(); },
});
- stub('gr-comment-api', {
- loadAll() { return Promise.resolve(); },
- getPaths() { return {}; },
- getCommentsForPath() { return {meta: {}, left: [], right: []}; },
+
+ // Element must be wrapped in an element with direct access to the
+ // comment API.
+ commentApiWrapper = fixture('basic');
+ element = commentApiWrapper.$.fileList;
+ loadCommentSpy = sandbox.spy(commentApiWrapper.$.commentAPI, 'loadAll');
+
+ // Stub methods on the changeComments object after changeComments has
+ // been initalized.
+ commentApiWrapper.loadComments().then(() => {
+ sandbox.stub(element.changeComments, 'getPaths').returns({});
+ sandbox.stub(element.changeComments, 'getCommentsForPath')
+ .returns({meta: {}, left: [], right: []});
+ done();
});
- element = fixture('basic');
element.numFilesShown = 75;
element.selectedIndex = 0;
element._files = [
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api-mock.js b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api-mock.js
new file mode 100644
index 0000000..cfcafd3
--- /dev/null
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api-mock.js
@@ -0,0 +1,41 @@
+// 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';
+
+ Polymer({
+ is: 'comment-api-mock',
+
+ properties: {
+ _changeComments: Object,
+ },
+
+ loadComments() {
+ return this._reloadComments();
+ },
+
+ _reloadCommentsWithCallback(e) {
+ return this._reloadComments().then(() => {
+ return e.detail.resolve();
+ });
+ },
+
+ _reloadComments() {
+ return this.$.commentAPI.loadAll(this._changeNum)
+ .then(comments => {
+ this._changeComments = this.$.commentAPI._changeComments;
+ });
+ },
+ });
+})();
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
index ef39e1f..bf608e8 100644
--- 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
@@ -16,18 +16,191 @@
const PARENT = 'PARENT';
+ /**
+ * Construct a change comments object, which can be data-bound to child
+ * elements of that which uses the gr-comment-api.
+ *
+ * @param {!Object} comments
+ * @param {!Object} robotComments
+ * @param {!Object} drafts
+ * @param {number} changeNum
+ * @constructor
+ */
+ function ChangeComments(comments, robotComments, drafts, changeNum) {
+ this._comments = comments;
+ this._robotComments = robotComments;
+ this._drafts = drafts;
+ this._changeNum = changeNum;
+ }
+
+ ChangeComments.prototype = {
+ get comments() {
+ return this._comments;
+ },
+ get drafts() {
+ return this._drafts;
+ },
+ get robotComments() {
+ return this._robotComments;
+ },
+ };
+
+ ChangeComments.prototype._patchNumEquals =
+ Gerrit.PatchSetBehavior.patchNumEquals;
+
+ /**
+ * 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=} opt_patchRange The patch-range object containing patchNum
+ * and basePatchNum properties to represent the range.
+ * @return {!Object}
+ */
+ ChangeComments.prototype.getPaths = function(opt_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 => {
+ // If don't care about patch range, we know that the path exists.
+ if (!opt_patchRange) { return true; }
+ return this._isInPatchRange(c, opt_patchRange);
+ })) {
+ commentMap[path] = true;
+ }
+ }
+ }
+ return commentMap;
+ };
+
+ /**
+ * Gets all the comments and robot comments for the given change.
+ *
+ * @return {!Object}
+ */
+ ChangeComments.prototype.getAllPublishedComments = function() {
+ const paths = this.getPaths();
+ const publishedComments = {};
+ for (const path of Object.keys(paths)) {
+ publishedComments[path] = this.getAllCommentsForPath(path);
+ }
+ return publishedComments;
+ };
+
+ /**
+ * 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
+ * @return {!Object}
+ */
+ ChangeComments.prototype.getAllCommentsForPath = function(path) {
+ const comments = this._comments[path] || [];
+ const robotComments = this._robotComments[path] || [];
+ return comments.concat(robotComments);
+ };
+
+ /**
+ * 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}
+ */
+ ChangeComments.prototype.getCommentsForPath = function(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}
+ */
+ ChangeComments.prototype._isInBaseOfPatchRange = function(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}
+ */
+ ChangeComments.prototype._isInRevisionOfPatchRange =
+ function(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|undefined}
+ */
+ ChangeComments.prototype._isInPatchRange = function(comment, range) {
+ return this._isInBaseOfPatchRange(comment, range) ||
+ this._isInRevisionOfPatchRange(comment, range);
+ };
+
Polymer({
is: 'gr-comment-api',
properties: {
- /** @type {number} */
- _changeNum: Number,
- /** @type {!Object|undefined} */
- _comments: Object,
- /** @type {!Object|undefined} */
- _drafts: Object,
- /** @type {!Object|undefined} */
- _robotComments: Object,
+ _changeComments: Object,
+ },
+
+ listeners: {
+ 'reload-comments': 'loadAll',
},
behaviors: [
@@ -43,132 +216,15 @@
* @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.getDiffDrafts(changeNum)
- .then(drafts => { this._drafts = drafts; }));
+ promises.push(this.$.restAPI.getDiffComments(changeNum));
+ promises.push(this.$.restAPI.getDiffRobotComments(changeNum));
+ promises.push(this.$.restAPI.getDiffDrafts(changeNum));
- 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|undefined}
- */
- _isInPatchRange(comment, range) {
- return this._isInBaseOfPatchRange(comment, range) ||
- this._isInRevisionOfPatchRange(comment, range);
+ return Promise.all(promises).then(([comments, robotComments, drafts]) => {
+ this._changeComments = new ChangeComments(comments,
+ robotComments, drafts, changeNum);
+ });
},
});
})();
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
index 09403a4..eae249d 100644
--- 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
@@ -67,9 +67,9 @@
changeNum));
assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
changeNum));
- assert.isOk(element._comments);
- assert.isOk(element._robotComments);
- assert.deepEqual(element._drafts, {});
+ assert.isOk(element._changeComments._comments);
+ assert.isOk(element._changeComments._robotComments);
+ assert.deepEqual(element._changeComments._drafts, {});
});
});
@@ -94,102 +94,144 @@
changeNum));
assert.isTrue(element.$.restAPI.getDiffDrafts.calledWithExactly(
changeNum));
- assert.isOk(element._comments);
- assert.isOk(element._robotComments);
- assert.notDeepEqual(element._drafts, {});
+ assert.isOk(element._changeComments._comments);
+ assert.isOk(element._changeComments._robotComments);
+ assert.notDeepEqual(element._changeComments._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},
- ],
- };
+ suite('_changeComment methods', () => {
+ setup(done => {
+ const changeNum = 1234;
+ element.loadAll(changeNum).then(() => {
+ done();
+ });
});
- test('getPaths', () => {
- const patchRange = {basePatchNum: 1, patchNum: 4};
- let paths = element.getPaths(patchRange);
- assert.equal(Object.keys(paths).length, 0);
+ test('_isInBaseOfPatchRange', () => {
+ const comment = {patch_set: 1};
+ const patchRange = {basePatchNum: 1, patchNum: 2};
+ assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+ patchRange));
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');
+ assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+ patchRange));
- patchRange.patchNum = 2;
- paths = element.getPaths(patchRange);
- assert.property(paths, 'file/one');
- assert.property(paths, 'file/two');
- assert.property(paths, 'file/three');
+ comment.side = PARENT;
+ assert.isFalse(element._changeComments._isInBaseOfPatchRange(comment,
+ patchRange));
+
+ comment.patch_set = 2;
+ assert.isTrue(element._changeComments._isInBaseOfPatchRange(comment,
+ patchRange));
});
- 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);
+ test('_isInRevisionOfPatchRange', () => {
+ const comment = {patch_set: 123};
+ const patchRange = {basePatchNum: 122, patchNum: 124};
+ assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+ comment, patchRange));
- path = 'file/two';
- comments = element.getCommentsForPath(path, patchRange);
- assert.equal(comments.left.length, 0);
- assert.equal(comments.right.length, 1);
+ patchRange.patchNum = 123;
+ assert.isTrue(element._changeComments._isInRevisionOfPatchRange(
+ comment, patchRange));
- patchRange.basePatchNum = 2;
- comments = element.getCommentsForPath(path, patchRange);
- assert.equal(comments.left.length, 1);
- assert.equal(comments.right.length, 1);
+ comment.side = PARENT;
+ assert.isFalse(element._changeComments._isInRevisionOfPatchRange(
+ comment, patchRange));
+ });
- patchRange.basePatchNum = PARENT;
- path = 'file/three';
- comments = element.getCommentsForPath(path, patchRange);
- assert.equal(comments.left.length, 0);
- assert.equal(comments.right.length, 1);
+ suite('comment ranges and paths', () => {
+ setup(() => {
+ element._changeComments._drafts = {};
+ element._changeComments._robotComments = {
+ 'file/one': [
+ {patch_set: 2, side: PARENT},
+ {patch_set: 2},
+ ],
+ };
+ element._changeComments._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},
+ ],
+ 'file/four': [
+ {patch_set: 5, side: PARENT},
+ {patch_set: 5},
+ ],
+ };
+ });
+
+ test('getPaths', () => {
+ const patchRange = {basePatchNum: 1, patchNum: 4};
+ let paths = element._changeComments.getPaths(patchRange);
+ assert.equal(Object.keys(paths).length, 0);
+
+ patchRange.basePatchNum = PARENT;
+ patchRange.patchNum = 3;
+ paths = element._changeComments.getPaths(patchRange);
+ assert.notProperty(paths, 'file/one');
+ assert.property(paths, 'file/two');
+ assert.property(paths, 'file/three');
+ assert.notProperty(paths, 'file/four');
+
+ patchRange.patchNum = 2;
+ paths = element._changeComments.getPaths(patchRange);
+ assert.property(paths, 'file/one');
+ assert.property(paths, 'file/two');
+ assert.property(paths, 'file/three');
+ assert.notProperty(paths, 'file/four');
+
+ paths = element._changeComments.getPaths();
+ assert.property(paths, 'file/one');
+ assert.property(paths, 'file/two');
+ assert.property(paths, 'file/three');
+ assert.property(paths, 'file/four');
+ });
+
+ test('getCommentsForPath', () => {
+ const patchRange = {basePatchNum: 1, patchNum: 3};
+ let path = 'file/one';
+ let comments = element._changeComments.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._changeComments.getCommentsForPath(path,
+ patchRange);
+ assert.equal(comments.left.length, 0);
+ assert.equal(comments.right.length, 1);
+
+ patchRange.basePatchNum = 2;
+ comments = element._changeComments.getCommentsForPath(path,
+ patchRange);
+ assert.equal(comments.left.length, 1);
+ assert.equal(comments.right.length, 1);
+
+ patchRange.basePatchNum = PARENT;
+ path = 'file/three';
+ comments = element._changeComments.getCommentsForPath(path,
+ patchRange);
+ assert.equal(comments.left.length, 0);
+ assert.equal(comments.right.length, 1);
+ });
+
+ test('getAllCommentsForPath', () => {
+ const path = 'file/one';
+ const comments = element._changeComments.getAllCommentsForPath(path);
+ assert.deepEqual(comments.length, 4);
+ });
});
});
});
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 b97d974..6f25c55 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
@@ -71,6 +71,7 @@
* }}
*/
_change: Object,
+ _changeComments: Object,
_changeNum: String,
_diff: Object,
_fileList: {
@@ -750,13 +751,23 @@
_loadComments() {
return this.$.commentAPI.loadAll(this._changeNum).then(() => {
- this._commentMap = this.$.commentAPI.getPaths(this._patchRange);
+ this._changeComments = this.$.commentAPI._changeComments;
+ this._commentMap = this._getPaths(this._patchRange);
- this._commentsForDiff = this.$.commentAPI.getCommentsForPath(this._path,
+ this._commentsForDiff = this._getCommentsForPath(this._path,
this._patchRange, this._projectConfig);
});
},
+ _getPaths(patchRange) {
+ return this._changeComments.getPaths(patchRange);
+ },
+
+ _getCommentsForPath(path, patchRange, projectConfig) {
+ return this._changeComments.getCommentsForPath(path, patchRange,
+ projectConfig);
+ },
+
_getDiffDrafts() {
return this.$.restAPI.getDiffDrafts(this._changeNum);
},
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 fa37eb9..26f91801 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
@@ -47,7 +47,7 @@
const PARENT = 'PARENT';
- setup(() => {
+ setup(done => {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
@@ -56,10 +56,14 @@
getDiffChangeDetail() { return Promise.resolve(null); },
getChangeFiles() { return Promise.resolve({}); },
saveFileReviewed() { return Promise.resolve(); },
+ getDiffComments() { return Promise.resolve({}); },
getDiffRobotComments() { return Promise.resolve({}); },
- getDiffDrafts() { return Promise.resolve(); },
+ getDiffDrafts() { return Promise.resolve({}); },
});
element = fixture('basic');
+ element._loadComments().then(() => {
+ done();
+ });
});
teardown(() => {
@@ -484,9 +488,6 @@
});
test('file review status', done => {
- stub('gr-rest-api-interface', {
- getDiffComments() { return Promise.resolve({}); },
- });
const saveReviewedStub = sandbox.stub(element, '_saveReviewedState',
() => Promise.resolve());
sandbox.stub(element.$.diff, 'reload');
@@ -529,9 +530,6 @@
});
test('hash is determined from params', done => {
- stub('gr-rest-api-interface', {
- getDiffComments() { return Promise.resolve({}); },
- });
sandbox.stub(element.$.diff, 'reload');
sandbox.stub(element, '_initCursor');
@@ -679,11 +677,6 @@
suite('_loadComments', () => {
test('empty', done => {
- stub('gr-comment-api', {
- loadAll() { return Promise.resolve(); },
- getPaths() { return {}; },
- getCommentsForPath() { return {meta: {}}; },
- });
element._loadComments().then(() => {
assert.equal(Object.keys(element._commentMap).length, 0);
done();
@@ -691,16 +684,11 @@
});
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: {}}; },
+ sandbox.stub(element, '_getPaths').returns({
+ 'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
+ 'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
});
+ sandbox.stub(element, '_getCommentsForPath').returns({meta: {}});
element._changeNum = '42';
element._patchRange = {
basePatchNum: '3',
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 9a7a59e..24829f8 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1422,15 +1422,23 @@
* @param {number|string=} opt_basePatchNum
* @param {number|string=} opt_patchNum
* @param {string=} opt_path
+ * @return {!Promise<!Object>}
*/
getDiffComments(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
return this._getDiffComments(changeNum, '/comments', opt_basePatchNum,
opt_patchNum, opt_path);
},
- getDiffRobotComments(changeNum, basePatchNum, patchNum, opt_path) {
- return this._getDiffComments(changeNum, '/robotcomments', basePatchNum,
- patchNum, opt_path);
+ /**
+ * @param {number|string} changeNum
+ * @param {number|string=} opt_basePatchNum
+ * @param {number|string=} opt_patchNum
+ * @param {string=} opt_path
+ * @return {!Promise<!Object>}
+ */
+ getDiffRobotComments(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
+ return this._getDiffComments(changeNum, '/robotcomments',
+ opt_basePatchNum, opt_patchNum, opt_path);
},
/**
@@ -1442,7 +1450,7 @@
* @param {number|string=} opt_basePatchNum
* @param {number|string=} opt_patchNum
* @param {string=} opt_path
- * @return {!Promise<?Object>}
+ * @return {!Promise<!Object>}
*/
getDiffDrafts(changeNum, opt_basePatchNum, opt_patchNum, opt_path) {
return this.getLoggedIn().then(loggedIn => {
@@ -1481,6 +1489,7 @@
* @param {number|string=} opt_basePatchNum
* @param {number|string=} opt_patchNum
* @param {string=} opt_path
+ * @return {!Promise<!Object>}
*/
_getDiffComments(changeNum, endpoint, opt_basePatchNum,
opt_patchNum, opt_path) {
@@ -1489,7 +1498,7 @@
* Helper function to make promises more legible.
*
* @param {string|number=} opt_patchNum
- * @return {!Object} Diff comments response.
+ * @return {!Promise<!Object>} Diff comments response.
*/
const fetchComments = opt_patchNum => {
return this._getChangeURLAndFetch(changeNum, endpoint, opt_patchNum);
