diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js index 7b8c8a3112..d6a20f0b04 100644 --- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js +++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js @@ -279,9 +279,9 @@ class GrMessagesList extends mixinBehaviors( [ } /** - * Computes message author's file comments for change's message. - * Method uses this.messages to find next message and relies on messages - * to be sorted by date field descending. + * Computes message author's file comments for change's message. The backend + * sets comment.change_message_id for matching, so this computation is fairly + * straightforward. * * @param {!Object} changeComments changeComment object, which includes * a method to get all published comments (including robot comments), @@ -297,39 +297,12 @@ class GrMessagesList extends mixinBehaviors( [ if (message._index === undefined || !comments || !this.messages) { return []; } - const messages = this.messages || []; - const index = message._index; - const authorId = message.author && message.author._account_id; - const mDate = util.parseDate(message.date).getTime(); - // NB: Messages array has oldest messages first. - let nextMDate; - if (index > 0) { - for (let i = index - 1; i >= 0; i--) { - if (messages[i] && messages[i].author && - messages[i].author._account_id === authorId) { - nextMDate = util.parseDate(messages[i].date).getTime(); - break; - } - } - } + + const idFilter = comment => comment.change_message_id === message.id; const msgComments = {}; for (const file in comments) { if (!comments.hasOwnProperty(file)) { continue; } - const fileComments = comments[file]; - for (let i = 0; i < fileComments.length; i++) { - if (fileComments[i].author && - fileComments[i].author._account_id !== authorId) { - continue; - } - const cDate = util.parseDate(fileComments[i].updated).getTime(); - if (cDate <= mDate) { - if (nextMDate && cDate <= nextMDate) { - continue; - } - msgComments[file] = msgComments[file] || []; - msgComments[file].push(fileComments[i]); - } - } + msgComments[file] = comments[file].filter(idFilter); } return msgComments; } diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html index ccbe67c621..79ee96e0f1 100644 --- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html @@ -78,6 +78,10 @@ suite('gr-messages-list tests', () => { return dom(element.root).querySelectorAll('gr-message'); }; + const MESSAGE_ID_0 = '1234ccc949c6d482b061be6a28e10782abf0e7af'; + const MESSAGE_ID_1 = '8c19ccc949c6d482b061be6a28e10782abf0e7af'; + const MESSAGE_ID_2 = 'e7bfdbc842f6b6d8064bc68e0f52b673f40c0ca5'; + const author = { _account_id: 42, name: 'Marvin the Paranoid Android', @@ -88,6 +92,7 @@ suite('gr-messages-list tests', () => { file1: [ { message: 'message text', + change_message_id: MESSAGE_ID_0, updated: '2016-09-27 00:18:03.000000000', in_reply_to: '6505d749_f0bec0aa', line: 62, @@ -100,6 +105,7 @@ suite('gr-messages-list tests', () => { }, { message: 'message text', + change_message_id: MESSAGE_ID_1, updated: '2016-09-27 00:18:03.000000000', in_reply_to: 'c5912363_6b820105', line: 42, @@ -109,6 +115,7 @@ suite('gr-messages-list tests', () => { }, { message: 'message text', + change_message_id: MESSAGE_ID_1, updated: '2016-09-27 00:18:03.000000000', in_reply_to: '6505d749_f0bec0aa', line: 62, @@ -116,10 +123,20 @@ suite('gr-messages-list tests', () => { patch_set: 2, author, }, + { + message: 'message text', + change_message_id: MESSAGE_ID_2, + updated: '2016-09-27 00:18:03.000000000', + line: 64, + id: '34ed05d749_10ed44b2', + patch_set: 2, + author, + }, ], file2: [ { message: 'message text', + change_message_id: MESSAGE_ID_1, updated: '2016-09-27 00:18:03.000000000', in_reply_to: 'c5912363_4b7d450a', line: 132, @@ -389,8 +406,8 @@ suite('gr-messages-list tests', () => { } ); element.messages = messages; - const isAuthor = function(author, message) { - return message.author._account_id === author._account_id; + const isAuthor = function(author, comment) { + return comment.author._account_id === author._account_id; }; const isMarvin = isAuthor.bind(null, author); flushAsynchronousOperations(); @@ -399,10 +416,14 @@ suite('gr-messages-list tests', () => { assert.deepEqual(messageElements[1].message, messages[1]); assert.deepEqual(messageElements[2].message, messages[2]); assert.deepEqual(messageElements[1].comments.file1, - comments.file1.filter(isMarvin)); + comments.file1.filter(isMarvin).filter( + c => c.change_message_id === messages[1].id)); assert.deepEqual(messageElements[1].comments.file2, - comments.file2.filter(isMarvin)); - assert.deepEqual(messageElements[2].comments, {}); + comments.file2.filter(isMarvin).filter( + c => c.change_message_id === messages[1].id)); + assert.deepEqual(messageElements[2].comments.file1, + comments.file1.filter(isMarvin).filter( + c => c.change_message_id === messages[2].id)); }); test('messages without author do not throw', () => {