Add robot comments to PolyGerrit

This change adds an API request to get robot comments for displaying
inline in the diff view. They are styled in a different color, contain
build and robotId information, and a "please fix" action rather than the
standard set of actions.

Feature: Issue 5089
Change-Id: I1f5954a2ed01920bb7c3dc897e3285687ff7d3ca
This commit is contained in:
Becky Siegel
2016-12-07 13:36:49 -08:00
parent f9937b173b
commit 35a7682262
10 changed files with 281 additions and 59 deletions

View File

@@ -25,7 +25,6 @@ limitations under the License.
background-color: #ffd;
border: 1px solid #bbb;
display: block;
padding: 0 .7em;
white-space: normal;
}
</style>
@@ -38,10 +37,11 @@ limitations under the License.
draft="[[comment.__draft]]"
show-actions="[[_showActions]]"
project-config="[[projectConfig]]"
on-reply="_handleCommentReply"
on-comment-discard="_handleCommentDiscard"
on-ack="_handleCommentAck"
on-done="_handleCommentDone"></gr-diff-comment>
on-create-ack-comment="_handleCommentAck"
on-create-done-comment="_handleCommentDone"
on-create-fix-comment="_handleCommentFix"
on-create-reply-comment="_handleCommentReply"></gr-diff-comment>
</template>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>

View File

@@ -14,6 +14,8 @@
(function() {
'use strict';
var NEWLINE_PATTERN = /\n/g;
Polymer({
is: 'gr-diff-comment-thread',
@@ -149,41 +151,50 @@
}
},
_createReplyComment: function(parent, content, opt_isEditing) {
var reply = this._newReply(parent.id, parent.line, content);
if (opt_isEditing) {
reply.__editing = true;
}
this.push('comments', reply);
if (!opt_isEditing) {
// Allow the reply to render in the dom-repeat.
this.async(function() {
var commentEl = this._commentElWithDraftID(reply.__draftID);
commentEl.save();
}, 1);
}
},
_handleCommentReply: function(e) {
var comment = e.detail.comment;
var quoteStr;
if (e.detail.quote) {
var msg = comment.message;
var quoteStr = msg.split('\n').map(
function(line) { return '> ' + line; }).join('\n') + '\n\n';
quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n';
}
var reply = this._newReply(comment.id, comment.line, quoteStr);
reply.__editing = true;
this.push('comments', reply);
this._createReplyComment(comment, quoteStr, true);
},
_handleCommentAck: function(e) {
var comment = e.detail.comment;
var reply = this._newReply(comment.id, comment.line, 'Ack');
this.push('comments', reply);
// Allow the reply to render in the dom-repeat.
this.async(function() {
var commentEl = this._commentElWithDraftID(reply.__draftID);
commentEl.save();
}.bind(this), 1);
this._createReplyComment(comment, 'Ack');
},
_handleCommentDone: function(e) {
var comment = e.detail.comment;
var reply = this._newReply(comment.id, comment.line, 'Done');
this.push('comments', reply);
this._createReplyComment(comment, 'Done');
},
// Allow the reply to render in the dom-repeat.
this.async(function() {
var commentEl = this._commentElWithDraftID(reply.__draftID);
commentEl.save();
}.bind(this), 1);
_handleCommentFix: function(e) {
var comment = e.detail.comment;
var msg = comment.message;
var quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n';
var response = quoteStr + 'Please Fix';
this._createReplyComment(comment, response);
},
_commentElWithDraftID: function(id) {

View File

@@ -152,7 +152,7 @@ limitations under the License.
test('reply', function(done) {
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('reply', function() {
commentEl.addEventListener('create-reply-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -161,13 +161,14 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('reply', {comment: commentEl.comment}, {bubbles: false});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('quote reply', function(done) {
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('reply', function() {
commentEl.addEventListener('create-reply-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -176,8 +177,37 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('reply', {comment: commentEl.comment, quote: true},
{bubbles: false});
commentEl.fire('create-reply-comment', {comment: commentEl.comment,
quote: true}, {bubbles: false});
});
test('quote reply multiline', function(done) {
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?\nIt might be!',
updated: '2015-12-08 19:48:33.843000000',
}];
flushAsynchronousOperations();
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-reply-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
assert.equal(drafts.length, 1);
assert.equal(drafts[0].message,
'> is this a crossover episode!?\n> It might be!\n\n');
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment,
quote: true}, {bubbles: false});
});
test('ack', function(done) {
@@ -185,7 +215,7 @@ limitations under the License.
element.patchNum = '1';
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('ack', function() {
commentEl.addEventListener('create-ack-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -194,7 +224,8 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('ack', {comment: commentEl.comment}, {bubbles: false});
commentEl.fire('create-ack-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('done', function(done) {
@@ -202,7 +233,7 @@ limitations under the License.
element.patchNum = '1';
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('done', function() {
commentEl.addEventListener('create-done-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -211,7 +242,27 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('done', {comment: commentEl.comment}, {bubbles: false});
commentEl.fire('create-done-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('please fix', function(done) {
element.changeNum = '42';
element.patchNum = '1';
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-fix-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
assert.equal(drafts.length, 1);
assert.equal(
drafts[0].message, '> is this a crossover episode!?\n\nPlease Fix');
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-fix-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('discard', function(done) {

View File

@@ -28,17 +28,20 @@ limitations under the License.
:host {
display: block;
font-family: var(--font-family);
margin: .7em 0;
padding: .7em .7em;
--iron-autogrow-textarea: {
padding: 2px;
};
}
:host([disabled]) {
:host[disabled] {
pointer-events: none;
}
:host([disabled]) .container {
:host[disabled] .container {
opacity: .5;
}
:host[is-robot-comment] {
background-color: #cfe8fc;
}
.header {
cursor: pointer;
display: flex;
@@ -93,7 +96,7 @@ limitations under the License.
.danger .action {
margin-right: 0;
}
.container:not(.draft) .actions :not(.reply):not(.quote):not(.ack):not(.done) {
.container:not(.draft) .actions .hideOnPublished {
display: none;
}
.draft .reply,
@@ -124,6 +127,20 @@ limitations under the License.
.show-hide {
margin-left: .4em;
}
.robotId {
color: #808080;
margin-bottom: .8em;
margin-top: -.4em;
}
.runIdInformation {
margin-bottom: .5em;
}
.robotRun {
margin-left: .5em;
}
.robotRunLink {
margin-left: .5em;
}
input.show-hide {
display: none;
}
@@ -178,6 +195,11 @@ limitations under the License.
</label>
</div>
</div>
<template is="dom-if" if="[[comment.robot_id]]">
<div class="robotId" hidden$="[[collapsed]]"">
[[comment.robot_id]]
</div>
</template>
<iron-autogrow-textarea
id="editTextarea"
class="editMessage"
@@ -190,19 +212,36 @@ limitations under the License.
content="[[comment.message]]"
collapsed="[[collapsed]]"
config="[[projectConfig.commentlinks]]"></gr-formatted-text>
<div class="actions" hidden$="[[!showActions]]">
<div hidden$="[[!comment.robot_run_id]]">
<div class="runIdInformation" hidden$="[[collapsed]]">
Run ID:
<a class="robotRunLink" href$="[[comment.url]]">
<span class="robotRun">[[comment.robot_run_id]]</span>
</a>
</div>
</div>
<div class="actions humanActions" hidden$="[[!_showHumanActions]]">
<gr-button class="action reply" on-tap="_handleReply">Reply</gr-button>
<gr-button class="action quote" on-tap="_handleQuote">Quote</gr-button>
<gr-button class="action ack" on-tap="_handleAck">Ack</gr-button>
<gr-button class="action done" on-tap="_handleDone">Done</gr-button>
<gr-button class="action edit" on-tap="_handleEdit">Edit</gr-button>
<gr-button class="action save" on-tap="_handleSave"
<gr-button class="action done" on-tap="_handleDone">
Done</gr-button>
<gr-button class="action edit hideOnPublished" on-tap="_handleEdit">
Edit</gr-button>
<gr-button class="action save hideOnPublished" on-tap="_handleSave"
disabled$="[[_computeSaveDisabled(_messageText)]]">Save</gr-button>
<gr-button class="action cancel" on-tap="_handleCancel" hidden>Cancel</gr-button>
<gr-button class="action cancel hideOnPublished"
on-tap="_handleCancel" hidden>Cancel</gr-button>
<div class="danger">
<gr-button class="action discard" on-tap="_handleDiscard">Discard</gr-button>
<gr-button class="action discard hideOnPublished"
on-tap="_handleDiscard">Discard</gr-button>
</div>
</div>
<div class="actions robotActions" hidden$="[[!_showRobotActions]]">
<gr-button class="action fix" on-tap="_handleFix">
Please Fix
</gr-button>
</div>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>

View File

@@ -22,19 +22,25 @@
/**
* Fired when the Reply action is triggered.
*
* @event reply
* @event create-reply-comment
*/
/**
* Fired when the Ack action is triggered.
*
* @event ack
* @event create-ack-comment
*/
/**
* Fired when the Done action is triggered.
*
* @event done
* @event create-done-comment
*/
/**
* Fired when the create fix comment action is triggered.
*
* @event create-fix-comment
*/
/**
@@ -70,6 +76,11 @@
notify: true,
observer: '_commentChanged',
},
isRobotComment: {
type: Boolean,
value: false,
reflectToAttribute: true,
},
disabled: {
type: Boolean,
value: false,
@@ -85,8 +96,11 @@
value: false,
observer: '_editingChanged',
},
hasChildren: Boolean,
patchNum: String,
showActions: Boolean,
_showHumanActions: Boolean,
_showRobotActions: Boolean,
collapsed: {
type: Boolean,
value: true,
@@ -105,6 +119,8 @@
observers: [
'_commentMessageChanged(comment.message)',
'_loadLocalDraft(changeNum, patchNum, comment)',
'_isRobotComment(comment)',
'_calculateActionstoShow(showActions, isRobotComment)',
],
attached: function() {
@@ -121,6 +137,15 @@
return collapsed ? '◀' : '▼';
},
_calculateActionstoShow: function(showActions, isRobotComment) {
this._showHumanActions = showActions && !isRobotComment;
this._showRobotActions = showActions && isRobotComment;
},
_isRobotComment: function(comment) {
this.isRobotComment = !!comment.robot_id;
},
save: function() {
this.comment.message = this._messageText;
this.disabled = true;
@@ -290,23 +315,32 @@
_handleReply: function(e) {
e.preventDefault();
this.fire('reply', this._getEventPayload(), {bubbles: false});
this.fire('create-reply-comment', this._getEventPayload(),
{bubbles: false});
},
_handleQuote: function(e) {
e.preventDefault();
this.fire(
'reply', this._getEventPayload({quote: true}), {bubbles: false});
this.fire( 'create-reply-comment', this._getEventPayload({quote: true}),
{bubbles: false});
},
_handleFix: function(e) {
e.preventDefault();
this.fire('create-fix-comment', this._getEventPayload({quote: true}),
{bubbles: false});
},
_handleAck: function(e) {
e.preventDefault();
this.fire('ack', this._getEventPayload(), {bubbles: false});
this.fire('create-ack-comment', this._getEventPayload(),
{bubbles: false});
},
_handleDone: function(e) {
e.preventDefault();
this.fire('done', this._getEventPayload(), {bubbles: false});
this.fire('create-done-comment', this._getEventPayload(),
{bubbles: false});
},
_handleEdit: function(e) {

View File

@@ -99,7 +99,7 @@ limitations under the License.
});
test('proper event fires on reply', function(done) {
element.addEventListener('reply', function(e) {
element.addEventListener('create-reply-comment', function(e) {
assert.ok(e.detail.comment);
done();
});
@@ -107,7 +107,7 @@ limitations under the License.
});
test('proper event fires on quote', function(done) {
element.addEventListener('reply', function(e) {
element.addEventListener('create-reply-comment', function(e) {
assert.ok(e.detail.comment);
assert.isTrue(e.detail.quote);
done();
@@ -116,19 +116,26 @@ limitations under the License.
});
test('proper event fires on ack', function(done) {
element.addEventListener('ack', function(e) {
element.addEventListener('create-ack-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.ack'));
});
test('proper event fires on done', function(done) {
element.addEventListener('done', function(e) {
element.addEventListener('create-done-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.done'));
});
test('proper event fires on fix', function(done) {
element.addEventListener('create-fix-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.fix'));
});
test('clicking on date link does not trigger nav', function() {
var showStub = sinon.stub(page, 'show');
var dateEl = element.$$('.date');
@@ -229,9 +236,12 @@ limitations under the License.
test('button visibility states', function() {
element.showActions = false;
assert.isTrue(element.$$('.actions').hasAttribute('hidden'));
assert.isTrue(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.showActions = true;
assert.isFalse(element.$$('.actions').hasAttribute('hidden'));
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.draft = true;
assert.isTrue(isVisible(element.$$('.edit')), 'edit is visible');
@@ -242,6 +252,8 @@ limitations under the License.
assert.isFalse(isVisible(element.$$('.quote')), 'quote is not visible');
assert.isFalse(isVisible(element.$$('.ack')), 'ack is not visible');
assert.isFalse(isVisible(element.$$('.done')), 'done is not visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.editing = true;
assert.isFalse(isVisible(element.$$('.edit')), 'edit is not visible');
@@ -252,6 +264,8 @@ limitations under the License.
assert.isFalse(isVisible(element.$$('.quote')), 'quote is not visible');
assert.isFalse(isVisible(element.$$('.ack')), 'ack is not visible');
assert.isFalse(isVisible(element.$$('.done')), 'done is not visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.draft = false;
element.editing = false;
@@ -264,11 +278,26 @@ limitations under the License.
assert.isTrue(isVisible(element.$$('.quote')), 'quote is visible');
assert.isTrue(isVisible(element.$$('.ack')), 'ack is visible');
assert.isTrue(isVisible(element.$$('.done')), 'done is visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.comment.id = 'foo';
element.draft = true;
element.editing = true;
assert.isTrue(isVisible(element.$$('.cancel')), 'cancel is visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
element.isRobotComment = true;
element.draft = true;
assert.isTrue(element.$$('.humanActions').hasAttribute('hidden'));
assert.isFalse(element.$$('.robotActions').hasAttribute('hidden'));
// It is not expected to see Robot comment drafts, but if they appear,
// they will behave the same as non-drafts.
element.draft = false;
assert.isTrue(element.$$('.humanActions').hasAttribute('hidden'));
assert.isFalse(element.$$('.robotActions').hasAttribute('hidden'));
});
test('collapsible drafts', function() {

View File

@@ -70,6 +70,10 @@ limitations under the License.
return Promise.resolve({baseComments: [], comments: []});
});
sinon.stub(diffElement, '_getDiffRobotComments', function() {
return Promise.resolve({baseComments: [], comments: []});
});
var setupDone = function() {
cursorElement.moveToFirstChunk();
done();

View File

@@ -453,14 +453,24 @@
}.bind(this));
},
_getDiffRobotComments: function() {
return this.$.restAPI.getDiffRobotComments(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
},
_getDiffCommentsAndDrafts: function() {
var promises = [];
promises.push(this._getDiffComments());
promises.push(this._getDiffDrafts());
promises.push(this._getDiffRobotComments());
return Promise.all(promises).then(function(results) {
return Promise.resolve({
comments: results[0],
drafts: results[1],
robotComments: results[2],
});
}).then(this._normalizeDiffCommentsAndDrafts.bind(this));
},
@@ -472,6 +482,9 @@
}
var baseDrafts = results.drafts.baseComments.map(markAsDraft);
var drafts = results.drafts.comments.map(markAsDraft);
var baseRobotComments = results.robotComments.baseComments;
var robotComments = results.robotComments.comments;
return Promise.resolve({
meta: {
path: this.path,
@@ -479,8 +492,10 @@
patchRange: this.patchRange,
projectConfig: this.projectConfig,
},
left: results.comments.baseComments.concat(baseDrafts),
right: results.comments.comments.concat(drafts),
left: results.comments.baseComments.concat(baseDrafts)
.concat(baseRobotComments),
right: results.comments.comments.concat(drafts)
.concat(robotComments),
});
},

View File

@@ -77,6 +77,19 @@ limitations under the License.
});
});
test('get robot comments', function(done) {
element.patchRange = {basePatchNum: 0, patchNum: 0};
var getDraftsStub = sinon.stub(element.$.restAPI,
'getDiffRobotComments');
element._getDiffDrafts().then(function(result) {
assert.deepEqual(result, {baseComments: [], comments: []});
sinon.assert.notCalled(getDraftsStub);
getDraftsStub.restore();
done();
});
});
test('loads files weblinks', function(done) {
var diffStub = sinon.stub(element.$.restAPI, 'getDiff').returns(
Promise.resolve({
@@ -403,9 +416,24 @@ limitations under the License.
{id: 'd2'},
],
};
var diffDraftsStub = sinon.stub(element, '_getDiffDrafts',
function() { return Promise.resolve(drafts); });
var robotComments = {
baseComments: [
{id: 'br1'},
{id: 'br2'},
],
comments: [
{id: 'r1'},
{id: 'r2'},
],
};
var diffRobotCommentStub = sinon.stub(element, '_getDiffRobotComments',
function() { return Promise.resolve(robotComments); });
element.changeNum = '42';
element.patchRange = {
basePatchNum: 'PARENT',
@@ -430,17 +458,22 @@ limitations under the License.
{id: 'bc2'},
{id: 'bd1', __draft: true},
{id: 'bd2', __draft: true},
{id: 'br1'},
{id: 'br2'},
],
right: [
{id: 'c1'},
{id: 'c2'},
{id: 'd1', __draft: true},
{id: 'd2', __draft: true},
{id: 'r1'},
{id: 'r2'},
],
});
diffCommentsStub.restore();
diffDraftsStub.restore();
diffRobotCommentStub.restore();
done();
});
});

View File

@@ -671,6 +671,12 @@
opt_patchNum, opt_path);
},
getDiffRobotComments: function(changeNum, basePatchNum, patchNum,
opt_path) {
return this._getDiffComments(changeNum, '/robotcomments', basePatchNum,
patchNum, opt_path);
},
getDiffDrafts: function(changeNum, opt_basePatchNum, opt_patchNum,
opt_path) {
return this._getDiffComments(changeNum, '/drafts', opt_basePatchNum,