opendev/gerrit
Code Issues Proposed changes

Implement isOnParent as functions rather than an attribute on comments

Browse Source 
Previously, the rest api interface set '__isOnParent' for comments.
When comments were added, the property from the comment thread
'isOnParent' was passed as a property. When the draft was saved, it was
expecting '__isOnParent' rather than 'isOnParent' and this caused
comments to show up on the wrong side after saved/refreshed, because
'__isOnParent' was undefined.

Instead of changing to either '__isOnParent' which would be strange to
pass as an attribute or 'isOnParent' which would look like it came from
the api directly, this change introduces isOnParent functions so that
translation doesn't need to be done with the API.

Bug: Issue 5831
Change-Id: I3b849ba5878275cda0a39638626a12bd51341a29
This commit is contained in:
Becky Siegel
2017-03-21 14:56:19 -07:00
parent cb2f7009c5
commit 0727efa3d1
9 changed files with 25 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.js
View File

@@ -34,11 +34,15 @@
return '/c/' + changeNum + '/' + patchNum + '/' + file;
},
_isOnParent: function(comment) {
return comment.side === 'PARENT';
},
_computeDiffLineURL: function(file, changeNum, patchNum, comment) {
var diffURL = this._computeFileDiffURL(file, changeNum, patchNum);
if (comment.line) {
diffURL += '#';
if (comment.__isOnParent) { diffURL += 'b'; }
if (this._isOnParent(comment)) { diffURL += 'b'; }
diffURL += comment.line;
}
return diffURL;
@@ -51,7 +55,7 @@
},
_computePatchDisplayName: function(comment) {
if (comment.__isOnParent) {
if (this._isOnParent(comment)) {
return 'Base, ';
}
if (comment.patch_set != this.patchNum) {

8
polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list_test.html
View File

@@ -64,14 +64,14 @@ limitations under the License.
});
test('_computeDiffLineURL', function() {
var comment = {line: 123, side: 'REIVISION', patch_set: 10};
var comment = {line: 123, side: 'REVISION', patch_set: 10};
var expected = '/c/<change>/<patch>/<file>#123';
var actual = element._computeDiffLineURL('<file>', '<change>', '<patch>',
comment);
assert.equal(actual, expected);
comment.line = 321;
comment.__isOnParent = true;
comment.side = 'PARENT';
expected = '/c/<change>/<patch>/<file>#b321';
actual = element._computeDiffLineURL('<file>', '<change>', '<patch>',
@@ -79,7 +79,7 @@ limitations under the License.
});
test('_computePatchDisplayName', function() {
var comment = {line: 123, side: 'REIVISION', patch_set: 10};
var comment = {line: 123, side: 'REVISION', patch_set: 10};
element.patchNum = 10;
assert.equal(element._computePatchDisplayName(comment), '');
@@ -87,7 +87,7 @@ limitations under the License.
element.patchNum = 9;
assert.equal(element._computePatchDisplayName(comment), 'PS10, ');
comment.__isOnParent = true;
comment.side = 'PARENT';
assert.equal(element._computePatchDisplayName(comment), 'Base, ');
});
});

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

@@ -356,7 +356,7 @@
}
var patchNum = this._comments.meta.patchRange.patchNum;
var isOnParent = comments[0].__isOnParent || false ;
var isOnParent = comments[0].side === 'PARENT' || false;
if (line.type === GrDiffLine.Type.REMOVE ||
opt_side === GrDiffBuilder.Side.LEFT) {
if (this._comments.meta.patchRange.basePatchNum === 'PARENT') {

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

@@ -59,7 +59,7 @@ limitations under the License.
draft="[[comment.__draft]]"
show-actions="[[_showActions]]"
comment-side="[[comment.__commentSide]]"
is-on-parent="[[isOnParent]]"
side="[[comment.side]]"
project-config="[[projectConfig]]"
on-create-fix-comment="_handleCommentFix"
on-comment-discard="_handleCommentDiscard"></gr-diff-comment>

7
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
View File

@@ -272,7 +272,7 @@
__date: new Date(),
path: this.path,
patchNum: this.patchNum,
__isOnParent: this.__isOnParent,
side: this._getSide(this.isOnParent),
__commentSide: this.commentSide,
};
if (opt_lineNum) {
@@ -289,6 +289,11 @@
return d;
},
_getSide: function(isOnParent) {
if (isOnParent) { return 'PARENT'; }
return 'REVISION';
},
_handleCommentDiscard: function(e) {
var diffCommentEl = Polymer.dom(e).rootTarget;
var comment = diffCommentEl.comment;

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

@@ -137,12 +137,13 @@
this.isRobotComment = !!comment.robot_id;
},
isOnParent: function() {
return this.side === 'PARENT';
},
save: function() {
this.comment.message = this._messageText;
// Translate {Boolean} __isOnParent to {String} side for the REST API
// format.
this.comment.side = this.comment.__isOnParent ? 'PARENT' : null;
this.disabled = true;
this._eraseDraftComment();
@@ -153,7 +154,6 @@
return this.$.restAPI.getResponseObject(response).then(function(obj) {
var comment = obj;
comment.__isOnParent = comment.side === 'PARENT';
comment.__draft = true;
// Maintain the ephemeral draft ID for identification by other
// elements.
@@ -407,18 +407,16 @@
},
_saveDraft: function(draft) {
draft.side = draft.__isOnParent ? 'PARENT' : null;
return this.$.restAPI.saveDiffDraft(this.changeNum, this.patchNum, draft);
},
_deleteDraft: function(draft) {
draft.side = draft.__isOnParent ? 'PARENT' : null;
return this.$.restAPI.deleteDiffDraft(this.changeNum, this.patchNum,
draft);
},
_getPatchNum: function() {
return this.isOnParent ? 'PARENT' : this.patchNum;
return this.isOnParent() ? 'PARENT' : this.patchNum;
},
_loadLocalDraft: function(changeNum, patchNum, comment) {

5
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
View File

@@ -153,10 +153,10 @@ limitations under the License.
});
test('_getPatchNum', function() {
element.isOnParent = true;
element.side = 'PARENT';
element.patchNum = 1;
assert.equal(element._getPatchNum(), 'PARENT');
element.isOnParent = false;
element.side = 'REVISION';
assert.equal(element._getPatchNum(), 1);
});
@@ -508,7 +508,6 @@ limitations under the License.
assert.deepEqual(fireStub.lastCall.args[1], {
comment: {
__commentSide: 'right',
__isOnParent: false,
__draft: true,
__draftID: 'temp_draft_id',
__editing: false,

5
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@@ -781,7 +781,6 @@
function onlyParent(c) { return c.side == PARENT_PATCH_NUM; }
function withoutParent(c) { return c.side != PARENT_PATCH_NUM; }
function setPath(c) { c.path = opt_path; }
function setIsOnParent(c) { c.__isOnParent = true; }
var promises = [];
var comments;
@@ -801,10 +800,6 @@
if (opt_basePatchNum == PARENT_PATCH_NUM) {
baseComments = comments.filter(onlyParent);
baseComments.forEach(setPath);
// Translate {String} side to {Boolean} __isOnParent for readability
// in the code.
baseComments.forEach(setIsOnParent);
}
comments = comments.filter(withoutParent);

1
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

@@ -150,7 +150,6 @@ limitations under the License.
assert.equal(obj.baseComments.length, 1);
assert.deepEqual(obj.baseComments[0], {
side: 'PARENT',
__isOnParent: true,
message: 'how did this work in the first place?',
path: 'sieve.go',
updated: '2017-02-03 22:33:28.000000000',