opendev/gerrit
Code Issues Proposed changes

Merge "Make messages more conversational"

Browse Source
This commit is contained in:
Kasper Nilsson
2018-08-24 19:45:55 +00:00
committed by Gerrit Code Review
parent b0f3fe4f51 512f404821
commit 18ad1e9355
3 changed files with 243 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
polygerrit-ui/app/elements/change/gr-message/gr-message.html
View File

@@ -178,20 +178,34 @@ limitations under the License.
<gr-account-label
account="[[author]]"
hide-avatar></gr-account-label>
<template is="dom-repeat" items="[[_getScores(message)]]" as="score">
<span class$="score [[_computeScoreClass(score, labelExtremes)]]">
[[score.label]] [[score.value]]
</span>
<template is="dom-if" if="[[_successfulParse]]">
<template is="dom-if" if="[[_parsedVotes.length]]">voted</template>
<template is="dom-repeat" items="[[_parsedVotes]]" as="score">
<span class$="score [[_computeScoreClass(score, labelExtremes)]]">
[[score.label]] [[score.value]]
</span>
</template>
[[_computeConversationalString(_parsedVotes, _parsedPatchNum, _parsedCommentCount)]]
</template>
</div>
<template is="dom-if" if="[[message.message]]">
<div class="content">
<div class="message hideOnOpen">[[message.message]]</div>
<gr-formatted-text
no-trailing-margin
class="message hideOnCollapsed"
content="[[message.message]]"
config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
<template is="dom-if" if="[[_successfulParse]]">
<div class="message hideOnOpen">[[_parsedChangeMessage]]</div>
<gr-formatted-text
no-trailing-margin
class="message hideOnCollapsed"
content="[[_parsedChangeMessage]]"
config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
</template>
<template is="dom-if" if="[[!_successfulParse]]">
<div class="message hideOnOpen">[[message.message]]</div>
<gr-formatted-text
no-trailing-margin
class="message hideOnCollapsed"
content="[[message.message]]"
config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
</template>
<div class="replyContainer" hidden$="[[!showReplyButton]]" hidden>
<gr-button link small on-tap="_handleReplyTap">Reply</gr-button>
</div>

91
polygerrit-ui/app/elements/change/gr-message/gr-message.js
View File

@@ -17,7 +17,8 @@
(function() {
'use strict';
const PATCH_SET_PREFIX_PATTERN = /^Patch Set \d+: /;
const PATCH_SET_PREFIX_PATTERN = /^Patch Set (\d)+:[ ]?/;
const COMMENTS_COUNT_PATTERN = /^\((\d+)( inline)? comments?\)$/;
const LABEL_TITLE_SCORE_PATTERN = /^([A-Za-z0-9-]+)([+-]\d+)$/;
Polymer({
@@ -101,10 +102,17 @@
type: Boolean,
value: false,
},
_parsedPatchNum: String,
_parsedCommentCount: String,
_parsedVotes: Array,
_parsedChangeMessage: String,
_successfulParse: Boolean,
},
observers: [
'_updateExpandedClass(message.expanded)',
'_consumeMessage(message.message)',
],
ready() {
@@ -184,19 +192,6 @@
return event.type === 'REVIEWER_UPDATE';
},
_getScores(message) {
if (!message.message) { return []; }
const line = message.message.split('\n', 1)[0];
const patchSetPrefix = PATCH_SET_PREFIX_PATTERN;
if (!line.match(patchSetPrefix)) { return []; }
const scoresRaw = line.split(patchSetPrefix)[1];
if (!scoresRaw) { return []; }
return scoresRaw.split(' ')
.map(s => s.match(LABEL_TITLE_SCORE_PATTERN))
.filter(ms => ms && ms.length === 3)
.map(ms => ({label: ms[1], value: ms[2]}));
},
_computeScoreClass(score, labelExtremes) {
const classes = [];
if (score.value > 0) {
@@ -260,5 +255,73 @@
e.stopPropagation();
this.set('message.expanded', !this.message.expanded);
},
/**
* Attempts to consume a change message to create a shorter and more legible
* format. If the function encounters unexpected characters at any point, it
* sets the _successfulParse flag to false and terminates, causing the UI to
* fall back to displaying the entirety of the change message.
*
* A successful parse results in a one-liner that reads:
* `${AVATAR} voted ${VOTES} and left ${NUM} comment(s) on ${PATCHSET}`
*
* @param {string} text
*/
_consumeMessage(text) {
this._parsedPatchNum = '';
this._parsedCommentCount = '';
this._parsedChangeMessage = '';
this._parsedVotes = [];
if (!text) {
// No message body means nothing to parse.
this._successfulParse = false;
return;
}
const lines = text.split('\n');
const messageLines = lines.shift().split(PATCH_SET_PREFIX_PATTERN);
if (!messageLines[1]) {
// Message is in an unexpected format.
this._successfulParse = false;
return;
}
this._parsedPatchNum = messageLines[1];
if (messageLines[2]) {
// Content after the colon is always vote information. If it is in the
// most up to date schema, parse it. Otherwise, cancel the parsing
// completely.
let match;
for (const score of messageLines[2].split(' ')) {
match = score.match(LABEL_TITLE_SCORE_PATTERN);
if (!match || match.length !== 3) {
this._successfulParse = false;
return;
}
this._parsedVotes.push({label: match[1], value: match[2]});
}
}
// Remove empty line.
lines.shift();
if (lines.length) {
const commentMatch = lines[0].match(COMMENTS_COUNT_PATTERN);
if (commentMatch) {
this._parsedCommentCount = commentMatch[1];
// Remove comment line and the following empty line.
lines.splice(0, 2);
}
this._parsedChangeMessage = lines.join('\n');
}
this._successfulParse = true;
},
_computeConversationalString(votes, patchNum, commentCount) {
let clause = ' on Patch Set ' + patchNum;
if (commentCount) {
let commentStr = ' comment';
if (parseInt(commentCount, 10) > 1) { commentStr += 's'; }
clause = ' left ' + commentCount + commentStr + clause;
if (votes.length) { clause = ' and' + clause; }
}
return clause;
},
});
})();

142
polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
View File

@@ -169,7 +169,9 @@ limitations under the License.
author: {},
expanded: false,
message: 'Patch Set 1: Verified+1 Code-Review-2 Trybot-Ready+1',
_revision_number: 1,
};
element.comments = {};
element.labelExtremes = {
'Verified': {max: 1, min: -1},
'Code-Review': {max: 2, min: -2},
@@ -199,5 +201,145 @@ limitations under the License.
const scoreChips = Polymer.dom(element.root).querySelectorAll('.score');
assert.equal(scoreChips.length, 0);
});
suite('_consumeMessage', () => {
const assertConsumeFailed = str => {
element._consumeMessage(str);
assert.isFalse(element._successfulParse);
};
test('no message body', () => {
assertConsumeFailed('');
});
test('known old schema', () => {
const str = 'Patch Set 1: Looks good to me, approved; Verified';
assertConsumeFailed(str);
});
test('known old schema 2', () => {
const str = [
'Patch Set 2: Looks good to me',
'',
'Patch set 2 compiles, and runs as expected.',
].join('\n');
assertConsumeFailed(str);
});
test('known old schema 3', () => {
const str = 'Patch Set 2: (1 inline comment)';
assertConsumeFailed(str);
});
test('known old schema 4', () => {
const str = [
'Patch Set 2: Looks good to me',
'',
'(1 inline comment)',
].join('\n');
assertConsumeFailed(str);
});
test('just change message', () => {
const str = [
'Patch Set 2:',
'',
'I think you should reconsider this approach.',
'It really makes no sense.',
].join('\n');
element._consumeMessage(str);
assert.isTrue(element._successfulParse);
assert.equal(element._parsedPatchNum, '2');
assert.equal(element._parsedCommentCount, '');
assert.deepEqual(element._parsedVotes, []);
assert.equal(element._parsedChangeMessage, [
'I think you should reconsider this approach.',
'It really makes no sense.',
].join('\n'));
});
test('just votes', () => {
element._consumeMessage('Patch Set 2: Code-Review-Label+1 Verified+1');
assert.isTrue(element._successfulParse);
assert.equal(element._parsedPatchNum, '2');
assert.equal(element._parsedCommentCount, '');
assert.deepEqual(element._parsedVotes, [
{label: 'Code-Review-Label', value: '+1'},
{label: 'Verified', value: '+1'},
]);
assert.equal(element._parsedChangeMessage, '');
});
test('just comments', () => {
const str = [
'Patch Set 2:',
'',
'(8 comments)',
].join('\n');
element._consumeMessage(str);
assert.isTrue(element._successfulParse);
assert.equal(element._parsedPatchNum, '2');
assert.equal(element._parsedCommentCount, '8');
assert.deepEqual(element._parsedVotes, []);
assert.equal(element._parsedChangeMessage, '');
});
test('vote with comments and change message', () => {
const str = [
'Patch Set 2: Code-Review-Label+1 Verified+1',
'',
'(1 comment)',
'',
'LGTM',
'',
'Nice work, just one nit.',
].join('\n');
element._consumeMessage(str);
assert.isTrue(element._successfulParse);
assert.equal(element._parsedPatchNum, '2');
assert.equal(element._parsedCommentCount, '1');
assert.deepEqual(element._parsedVotes, [
{label: 'Code-Review-Label', value: '+1'},
{label: 'Verified', value: '+1'},
]);
assert.equal(element._parsedChangeMessage, [
'LGTM',
'',
'Nice work, just one nit.',
].join('\n'));
});
test('vote with change message', () => {
const str = [
'Patch Set 2: Code-Review-Label+2 Verified+1',
'',
'LGTM',
'',
'Nice work.',
].join('\n');
element._consumeMessage(str);
assert.isTrue(element._successfulParse);
assert.equal(element._parsedPatchNum, '2');
assert.equal(element._parsedCommentCount, '');
assert.deepEqual(element._parsedVotes, [
{label: 'Code-Review-Label', value: '+2'},
{label: 'Verified', value: '+1'},
]);
assert.equal(element._parsedChangeMessage, [
'LGTM',
'',
'Nice work.',
].join('\n'));
});
});
});
</script>