Change filtering of messages in experimental ChangeLog
Example:
All entries: https://imgur.com/a/2jhhwRc
New default view: https://imgur.com/a/GCbKVll
Old default view: https://imgur.com/a/vdRuv2c
Bug: Issue 11774, Issue 8164
Change-Id: I74b36f956fc44bc24d48c7eea1bb2eac7b6b1a86
diff --git a/polygerrit-ui/app/constants/constants.js b/polygerrit-ui/app/constants/constants.js
index cab50f6..36e0b86 100644
--- a/polygerrit-ui/app/constants/constants.js
+++ b/polygerrit-ui/app/constants/constants.js
@@ -33,3 +33,13 @@
COMMENT_THREADS: '_commentThreads',
};
+/**
+ * @enum
+ * @desc Tag names of change log messages.
+ */
+export const MessageTags = {
+ TAG_DELETE_REVIEWER: 'autogenerated:gerrit:deleteReviewer',
+ TAG_NEW_PATCHSET: 'autogenerated:gerrit:newPatchSet',
+ TAG_NEW_WIP_PATCHSET: 'autogenerated:gerrit:newWipPatchSet',
+ TAG_REVIEWER_UPDATE: 'autogenerated:gerrit:reviewerUpdate',
+};
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental.js
index 9b221ac..15dc03b 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental.js
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental.js
@@ -29,6 +29,7 @@
import {htmlTemplate} from './gr-messages-list-experimental_html.js';
import {KeyboardShortcutBehavior} from '../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.js';
import {util} from '../../../scripts/util.js';
+import {MessageTags} from '../../../constants/constants.js';
/**
* The content of the enum is also used in the UI for the button text.
@@ -40,6 +41,39 @@
COLLAPSE_ALL: 'Collapse All',
};
+function isNewPatchSet(message) {
+ if (!message || !message.tag) return false;
+ return message.tag.includes(MessageTags.TAG_NEW_PATCHSET)
+ || message.tag.includes(MessageTags.TAG_NEW_WIP_PATCHSET);
+}
+
+function hasHigherRevisionNumber(m, message) {
+ return m._revision_number && m._revision_number > message._revision_number;
+}
+
+function isNewerReviewerUpdate(m, message) {
+ if (!message || !message.tag || !m || !m.tag) return false;
+ if (m.tag != MessageTags.TAG_REVIEWER_UPDATE) return false;
+ return m.date > message.date;
+}
+
+function isImportant(message, allMessages) {
+ // Human messages don't have a tag. They are always important.
+ if (!message.tag) return true;
+
+ // Autogenerated messages are only important, if there is not a newer message
+ // with the same tag.
+ const tag = message.tag;
+ const sameTag = m =>
+ m.tag === tag || (isNewPatchSet(m) && isNewPatchSet(message));
+ return !allMessages.filter(sameTag).some(m =>
+ hasHigherRevisionNumber(m, message) || isNewerReviewerUpdate(m, message));
+}
+
+export const TEST_ONLY = {
+ isImportant,
+};
+
/**
* @extends Polymer.Element
*/
@@ -141,14 +175,6 @@
this._highlightEl(el);
}
- _isAutomated(message) {
- const isReviewerUpdate =
- !!(message.reviewer || message.type === 'REVIEWER_UPDATE');
- const isAutoGenerated =
- !!(message.tag && message.tag.startsWith('autogenerated'));
- return isReviewerUpdate || isAutoGenerated;
- }
-
_observeShowAllActivity(showAllActivity) {
// We have to call render() such that the dom-repeat filter picks up the
// change.
@@ -159,7 +185,8 @@
* Filter for the dom-repeat of combinedMessages.
*/
_isMessageVisible(message) {
- return this._showAllActivity || !this._isAutomated(message);
+ const allMessages = this._combinedMessages;
+ return this._showAllActivity || isImportant(message, allMessages);
}
/**
@@ -260,7 +287,7 @@
_isVisibleShowAllActivityToggle(messages) {
messages = messages || [];
- return messages.some(m => this._isAutomated(m));
+ return messages.some(m => !isImportant(m, messages));
}
/**
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_html.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_html.js
index 6bf4e47..225b902 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_html.js
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_html.js
@@ -59,7 +59,7 @@
<span
id="showAllActivityToggleContainer"
class="container"
- hidden$="[[!_isVisibleShowAllActivityToggle(messages)]]"
+ hidden$="[[!_isVisibleShowAllActivityToggle(_combinedMessages)]]"
>
<paper-toggle-button
id="showAllActivityToggle"
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_test.html b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_test.html
index 58e1180..6a8b34c 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_test.html
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list-experimental_test.html
@@ -47,6 +47,9 @@
import './gr-messages-list-experimental.js';
import '../../diff/gr-comment-api/gr-comment-api-mock_test.js';
import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
+import {TEST_ONLY} from './gr-messages-list-experimental.js';
+import {MessageTags} from '../../../constants/constants.js';
+
const randomMessage = function(opt_params) {
const params = opt_params || {};
const author1 = {
@@ -60,14 +63,10 @@
message: params.message || Math.random().toString(),
_revision_number: params._revision_number || 1,
author: params.author || author1,
+ tag: params.tag,
};
};
-const randomAutomated = function(opt_params) {
- return Object.assign({tag: 'autogenerated:gerrit:replace'},
- randomMessage(opt_params));
-};
-
suite('gr-messages-list-experimental tests', () => {
let element;
let messages;
@@ -88,61 +87,53 @@
email: 'marvin@sirius.org',
};
+ const createComment = function() {
+ return {
+ id: '1a2b3c4d',
+ message: 'some random test text',
+ change_message_id: '8a7b6c5d',
+ updated: '2016-01-01 01:02:03.000000000',
+ line: 1,
+ patch_set: 1,
+ author,
+ };
+ };
+
const comments = {
file1: [
{
- message: 'message text',
+ ...createComment(),
change_message_id: MESSAGE_ID_0,
- updated: '2016-09-27 00:18:03.000000000',
in_reply_to: '6505d749_f0bec0aa',
- line: 62,
- id: '6505d749_10ed44b2',
- patch_set: 2,
author: {
email: 'some@email.com',
_account_id: 123,
},
},
{
- message: 'message text',
+ ...createComment(),
+ id: '2b3c4d5e',
change_message_id: MESSAGE_ID_1,
- updated: '2016-09-27 00:18:03.000000000',
in_reply_to: 'c5912363_6b820105',
- line: 42,
- id: '450a935e_0f1c05db',
- patch_set: 2,
- author,
},
{
- message: 'message text',
+ ...createComment(),
+ id: '2b3c4d5e',
change_message_id: MESSAGE_ID_1,
- updated: '2016-09-27 00:18:03.000000000',
in_reply_to: '6505d749_f0bec0aa',
- line: 62,
- id: '6505d749_10ed44b2',
- patch_set: 2,
- author,
},
{
- message: 'message text',
- change_message_id: MESSAGE_ID_2,
- updated: '2016-09-27 00:18:03.000000000',
- line: 64,
+ ...createComment(),
id: '34ed05d749_10ed44b2',
- patch_set: 2,
- author,
+ change_message_id: MESSAGE_ID_2,
},
],
file2: [
{
- message: 'message text',
+ ...createComment(),
change_message_id: MESSAGE_ID_1,
- updated: '2016-09-27 00:18:03.000000000',
in_reply_to: 'c5912363_4b7d450a',
- line: 132,
id: '450a935e_4f260d25',
- patch_set: 2,
- author,
},
],
};
@@ -214,7 +205,7 @@
assert.isTrue([...getMessages()].filter(m => m._expanded).length == 0);
});
- test('hide messages does not appear when no automated messages', () => {
+ test('showAllActivity does not appear when all msgs are important', () => {
assert.isOk(element.shadowRoot
.querySelector('#showAllActivityToggleContainer[hidden]'));
});
@@ -264,7 +255,7 @@
._expanded);
});
- test('messages', () => {
+ test('associating messages with comments', () => {
const messages = [].concat(
randomMessage(),
{
@@ -306,6 +297,70 @@
assert.isUndefined(messageElements[2].comments.file2);
});
+ test('isImportant human message', () => {
+ const m = randomMessage();
+ assert.isTrue(TEST_ONLY.isImportant(m, []));
+ assert.isTrue(TEST_ONLY.isImportant(m, [m]));
+ });
+
+ test('isImportant even with a tag', () => {
+ const m1 = randomMessage();
+ const m2 = randomMessage({tag: 'autogenerated:gerrit1'});
+ const m3 = randomMessage({tag: 'autogenerated:gerrit2'});
+ assert.isTrue(TEST_ONLY.isImportant(m2, []));
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1, m2, m3]));
+ assert.isTrue(TEST_ONLY.isImportant(m2, [m1, m2, m3]));
+ assert.isTrue(TEST_ONLY.isImportant(m3, [m1, m2, m3]));
+ });
+
+ test('isImportant filters same tag and older revision', () => {
+ const m1 = randomMessage({tag: 'auto', _revision_number: 2});
+ const m2 = randomMessage({tag: 'auto', _revision_number: 1});
+ const m3 = randomMessage({tag: 'auto'});
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1]));
+ assert.isTrue(TEST_ONLY.isImportant(m2, [m2]));
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1, m2]));
+ assert.isFalse(TEST_ONLY.isImportant(m2, [m1, m2]));
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m3, [m1, m3]));
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1, m2, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m2, [m1, m2, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m3, [m1, m2, m3]));
+ });
+
+ test('isImportant filters newPatchSet also for wip', () => {
+ const m1 = randomMessage(
+ {tag: MessageTags.TAG_NEW_PATCHSET, _revision_number: 3});
+ const m2 = randomMessage(
+ {tag: MessageTags.TAG_NEW_PATCHSET, _revision_number: 2});
+ const m3 = randomMessage(
+ {tag: MessageTags.TAG_NEW_WIP_PATCHSET, _revision_number: 1});
+ assert.isTrue(TEST_ONLY.isImportant(m1, [m1, m2, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m2, [m1, m2, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m3, [m1, m2, m3]));
+ });
+
+ test('isImportant filters older reviewer updates', () => {
+ const m1 = randomMessage(
+ {
+ tag: MessageTags.TAG_REVIEWER_UPDATE,
+ date: '2020-04-24 10:11:02.000000000',
+ });
+ const m2 = randomMessage(
+ {
+ tag: MessageTags.TAG_REVIEWER_UPDATE,
+ date: '2020-04-25 10:11:02.000000000',
+ });
+ const m3 = randomMessage(
+ {
+ tag: MessageTags.TAG_REVIEWER_UPDATE,
+ date: '2020-04-25 10:13:02.000000000',
+ });
+ assert.isFalse(TEST_ONLY.isImportant(m1, [m1, m2, m3]));
+ assert.isFalse(TEST_ONLY.isImportant(m2, [m1, m2, m3]));
+ assert.isTrue(TEST_ONLY.isImportant(m3, [m1, m2, m3]));
+ });
+
test('messages without author do not throw', () => {
const messages = [{
_index: 5,
@@ -328,18 +383,6 @@
let sandbox;
let commentApiWrapper;
- const getMessages = function() {
- return dom(element.root).querySelectorAll('gr-message');
- };
- const getHiddenMessages = function() {
- return dom(element.root).querySelectorAll('gr-message[hidden]');
- };
-
- const randomMessageReviewer = {
- reviewer: {},
- date: '2016-01-13 20:30:33.038000',
- };
-
setup(() => {
stub('gr-rest-api-interface', {
getConfig() { return Promise.resolve({}); },
@@ -350,8 +393,11 @@
});
sandbox = sinon.sandbox.create();
- messages = _.times(2, randomAutomated);
- messages.push(randomMessageReviewer);
+ messages = [
+ randomMessage(),
+ randomMessage({tag: 'auto', _revision_number: 2}),
+ randomMessage({tag: 'auto', _revision_number: 3}),
+ ];
// Element must be wrapped in an element with direct access to the
// comment API.
@@ -374,37 +420,26 @@
.querySelector('#showAllActivityToggle[hidden]'));
});
- test('autogenerated messages are not hidden initially', () => {
- const allHiddenMessageEls = getHiddenMessages();
-
- // There are no hidden messages.
- assert.isFalse(!!allHiddenMessageEls.length);
+ test('one unimportant message is hidden initially', () => {
+ const displayedMsgs = dom(element.root).querySelectorAll('gr-message');
+ assert.equal(displayedMsgs.length, 2);
});
- test('autogenerated messages hidden after comments only toggle', () => {
- let allHiddenMessageEls = getHiddenMessages();
-
+ test('unimportant messages hidden after toggle', () => {
element._showAllActivity = true;
MockInteractions.tap(element.$.showAllActivityToggle);
flushAsynchronousOperations();
- const allMessageEls = getMessages();
- allHiddenMessageEls = getHiddenMessages();
-
- // Autogenerated messages are now hidden.
- assert.equal(allHiddenMessageEls.length, allMessageEls.length);
+ const displayedMsgs = dom(element.root).querySelectorAll('gr-message');
+ assert.equal(displayedMsgs.length, 2);
});
- test('autogenerated messages not hidden after comments only toggle',
- () => {
- let allHiddenMessageEls = getHiddenMessages();
-
- element._showAllActivity = false;
- MockInteractions.tap(element.$.showAllActivityToggle);
- allHiddenMessageEls = getHiddenMessages();
-
- // Autogenerated messages are now hidden.
- assert.isFalse(!!allHiddenMessageEls.length);
- });
+ test('unimportant messages shown after toggle', () => {
+ element._showAllActivity = false;
+ MockInteractions.tap(element.$.showAllActivityToggle);
+ flushAsynchronousOperations();
+ const displayedMsgs = dom(element.root).querySelectorAll('gr-message');
+ assert.equal(displayedMsgs.length, 3);
+ });
test('_computeLabelExtremes', () => {
const computeSpy = sandbox.spy(element, '_computeLabelExtremes');
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser.js
index 3d1ce05..224eb5f 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-reviewer-updates-parser.js
@@ -16,6 +16,7 @@
*/
import {util} from '../../../scripts/util.js';
+import {MessageTags} from '../../../constants/constants.js';
/** @constructor */
export function GrReviewerUpdatesParser(change) {
@@ -52,9 +53,7 @@
*/
GrReviewerUpdatesParser.prototype._filterRemovedMessages = function() {
this.result.messages = this.result.messages
- .filter(
- message => message.tag !== 'autogenerated:gerrit:deleteReviewer'
- );
+ .filter(message => message.tag !== MessageTags.TAG_DELETE_REVIEWER);
};
/**
@@ -68,6 +67,7 @@
author: update.updated_by,
date: update.updated,
type: 'REVIEWER_UPDATE',
+ tag: MessageTags.TAG_REVIEWER_UPDATE,
};
};
