From 40f2c764b0f98a06b8d18e3b5b056a8f74fa235a Mon Sep 17 00:00:00 2001 From: "Didier (did)" Date: Tue, 27 Apr 2021 11:59:41 +0000 Subject: [PATCH] [IMP] mail, sms, snailmail: remove default date on message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should not assume that the default date of the message is "now". A message should only have a date when it's coming from the database that supply it. closes odoo/odoo#70069 Signed-off-by: Sébastien Theys (seb) --- .../static/src/components/message/message.xml | 16 ++-- .../src/components/message/message_tests.js | 1 + .../components/message_list/message_list.js | 10 ++- .../notification_group/notification_group.xml | 8 +- .../notification_list/notification_list.js | 9 +- ...ification_list_notification_group_tests.js | 5 +- .../thread_needaction_preview.xml | 2 +- .../thread_preview/thread_preview.xml | 2 +- .../mail/static/src/models/message/message.js | 7 +- .../notification_group/notification_group.js | 90 ++++++++++++++++--- .../notification_group_manager.js | 19 ---- .../mail/static/tests/helpers/mock_models.js | 2 +- ...ification_list_notification_group_tests.js | 8 +- ...ification_list_notification_group_tests.js | 8 +- 14 files changed, 125 insertions(+), 62 deletions(-) diff --git a/addons/mail/static/src/components/message/message.xml b/addons/mail/static/src/components/message/message.xml index c8476f5a109..3e3887b1d42 100644 --- a/addons/mail/static/src/components/message/message.xml +++ b/addons/mail/static/src/components/message/message.xml @@ -37,9 +37,11 @@ -
- -
+ +
+ +
+
-
- - -
+ +
+ - +
+ diff --git a/addons/mail/static/src/components/message/message_tests.js b/addons/mail/static/src/components/message/message_tests.js index 48c96905601..1d01ca545e6 100644 --- a/addons/mail/static/src/components/message/message_tests.js +++ b/addons/mail/static/src/components/message/message_tests.js @@ -56,6 +56,7 @@ QUnit.test('basic rendering', async function (assert) { const message = this.env.models['mail.message'].create({ author: insert({ id: 7, display_name: "Demo User" }), body: "

Test

", + date: moment(), id: 100, }); await this.createMessageComponent(message); diff --git a/addons/mail/static/src/components/message_list/message_list.js b/addons/mail/static/src/components/message_list/message_list.js index c24f109a018..626bd8199e6 100644 --- a/addons/mail/static/src/components/message_list/message_list.js +++ b/addons/mail/static/src/components/message_list/message_list.js @@ -172,6 +172,11 @@ class MessageList extends Component { * @returns {string} */ getDateDay(message) { + if (!message.date) { + // Without a date, we assume that it's a today message. This is + // mainly done to avoid flicker inside the UI. + return this.env._t("Today"); + } const date = message.date.format('YYYY-MM-DD'); if (date === moment().format('YYYY-MM-DD')) { return this.env._t("Today"); @@ -293,7 +298,10 @@ class MessageList extends Component { if (!this.props.hasSquashCloseMessages) { return false; } - if (Math.abs(message.date.diff(prevMessage.date)) > 60000) { + if (!prevMessage.date && message.date) { + return false; + } + if (message.date && prevMessage.date && Math.abs(message.date.diff(prevMessage.date)) > 60000) { // more than 1 min. elasped return false; } diff --git a/addons/mail/static/src/components/notification_group/notification_group.xml b/addons/mail/static/src/components/notification_group/notification_group.xml index c2f3dcebda1..c9f41c79734 100644 --- a/addons/mail/static/src/components/notification_group/notification_group.xml +++ b/addons/mail/static/src/components/notification_group/notification_group.xml @@ -18,9 +18,11 @@ () - - - + + + + +
diff --git a/addons/mail/static/src/components/notification_list/notification_list.js b/addons/mail/static/src/components/notification_list/notification_list.js index ff423175240..7980b52b433 100644 --- a/addons/mail/static/src/components/notification_list/notification_list.js +++ b/addons/mail/static/src/components/notification_list/notification_list.js @@ -84,7 +84,7 @@ class NotificationList extends Component { return 1; } if (t1.lastNeedactionMessageAsOriginThread && t2.lastNeedactionMessageAsOriginThread) { - return t1.lastNeedactionMessageAsOriginThread.date.isBefore(t2.lastNeedactionMessageAsOriginThread.date) ? 1 : -1; + return t1.lastNeedactionMessageAsOriginThread.id < t2.lastNeedactionMessageAsOriginThread.id ? 1 : -1; } if (t1.lastNeedactionMessageAsOriginThread) { return -1; @@ -112,7 +112,7 @@ class NotificationList extends Component { return 1; } if (t1.lastMessage && t2.lastMessage) { - return t1.lastMessage.date.isBefore(t2.lastMessage.date) ? 1 : -1; + return t1.lastMessage.id < t2.lastMessage.id ? 1 : -1; } if (t1.lastMessage) { return -1; @@ -133,9 +133,8 @@ class NotificationList extends Component { if (props.filter === 'all') { const notificationGroups = this.env.messaging.notificationGroupManager.groups; notifications = Object.values(notificationGroups) - .sort((group1, group2) => - group1.date.isAfter(group2.date) ? -1 : 1 - ).map(notificationGroup => { + .sort((group1, group2) => group1.sequence - group2.sequence) + .map(notificationGroup => { return { notificationGroup, uniqueId: notificationGroup.localId, diff --git a/addons/mail/static/src/components/notification_list/notification_list_notification_group_tests.js b/addons/mail/static/src/components/notification_list/notification_list_notification_group_tests.js index 5198ed809c6..6b898613663 100644 --- a/addons/mail/static/src/components/notification_list/notification_list_notification_group_tests.js +++ b/addons/mail/static/src/components/notification_list/notification_list_notification_group_tests.js @@ -419,13 +419,12 @@ QUnit.test('different mail.channel are not grouped', async function (assert) { ); }); -QUnit.test('multiple grouped notifications by document model, sorted by date desc', async function (assert) { +QUnit.test('multiple grouped notifications by document model, sorted by the most recent message of each group', async function (assert) { assert.expect(9); this.data['mail.message'].records.push( // first message that is expected to have a failure { - date: moment.utc().format("YYYY-MM-DD HH:mm:ss"), // random date id: 11, // random unique id, will be used to link failure to message message_type: 'email', // message must be email (goal of the test) model: 'res.partner', // different model from second message @@ -434,8 +433,6 @@ QUnit.test('multiple grouped notifications by document model, sorted by date des }, // second message that is expected to have a failure { - // random date, later than first message - date: moment.utc().add(1, 'days').format("YYYY-MM-DD HH:mm:ss"), id: 12, // random unique id, will be used to link failure to message message_type: 'email', // message must be email (goal of the test) model: 'res.company', // different model from first message diff --git a/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview.xml b/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview.xml index 3fd33224d59..ea8c1bcf1d1 100644 --- a/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview.xml +++ b/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview.xml @@ -31,7 +31,7 @@ () - + diff --git a/addons/mail/static/src/components/thread_preview/thread_preview.xml b/addons/mail/static/src/components/thread_preview/thread_preview.xml index 8a4baf3dfeb..1ca993574f0 100644 --- a/addons/mail/static/src/components/thread_preview/thread_preview.xml +++ b/addons/mail/static/src/components/thread_preview/thread_preview.xml @@ -34,7 +34,7 @@ - + diff --git a/addons/mail/static/src/models/message/message.js b/addons/mail/static/src/models/message/message.js index 9dcae068f70..d6839349297 100644 --- a/addons/mail/static/src/models/message/message.js +++ b/addons/mail/static/src/models/message/message.js @@ -586,9 +586,10 @@ function factory(dependencies) { inverse: 'checkedMessages', readonly: true, }), - date: attr({ - default: moment(), - }), + /** + * Determines the date of the message as a moment object. + */ + date: attr(), /** * States the time elapsed since date up to now. */ diff --git a/addons/mail/static/src/models/notification_group/notification_group.js b/addons/mail/static/src/models/notification_group/notification_group.js index 46bb0bafa2a..e16942efca2 100644 --- a/addons/mail/static/src/models/notification_group/notification_group.js +++ b/addons/mail/static/src/models/notification_group/notification_group.js @@ -2,7 +2,7 @@ import { registerNewModel } from '@mail/model/model_core'; import { attr, many2one, one2many } from '@mail/model/model_field'; -import { insert, unlink } from '@mail/model/model_field_command'; +import { clear, insert, unlink } from '@mail/model/model_field_command'; function factory(dependencies) { @@ -51,13 +51,17 @@ function factory(dependencies) { * @returns {mail.thread|undefined} */ _computeThread() { - if (this.res_id) { - return insert({ - id: this.res_id, - model: this.res_model, - }); + const notificationsThreadIds = this.notifications + .filter(notification => notification.message && notification.message.originThread) + .map(notification => notification.message.originThread.id); + const threadIds = new Set(notificationsThreadIds); + if (threadIds.size !== 1) { + return unlink(); } - return unlink(); + return insert({ + id: notificationsThreadIds[0], + model: this.res_model, + }); } /** @@ -67,6 +71,32 @@ function factory(dependencies) { return `${this.modelName}_${data.id}`; } + /** + * Compute the most recent date inside the notification messages. + * + * @private + * @returns {moment|undefined} + */ + _computeDate() { + const dates = this.notifications + .filter(notification => notification.message && notification.message.date) + .map(notification => notification.message.date); + if (dates.length === 0) { + return clear(); + } + return moment.max(dates); + } + + /** + * Compute the position of the group inside the notification list. + * + * @private + * @returns {number} + */ + _computeSequence() { + return -Math.max(...this.notifications.map(notification => notification.message.id)); + } + /** * Opens the view that displays all the records of the group. * @@ -97,23 +127,63 @@ function factory(dependencies) { } NotificationGroup.fields = { - date: attr(), + /** + * States the most recent date of all the notification message. + */ + date: attr({ + compute: '_computeDate', + dependencies: [ + 'notifications', + 'notificationsMessage', + 'notificationsMessageDate', + ], + }), id: attr({ required: true, }), notification_type: attr(), notifications: one2many('mail.notification'), - res_id: attr(), + /** + * Serves as compute dependency. + */ + notificationsMessage: one2many('mail.message', { + related: 'notifications.message', + }), + /** + * Serves as compute dependency. + */ + notificationsMessageDate: attr({ + related: 'notificationsMessage.date', + }), + /** + * Serves as compute dependency. + */ + notificationsMessageOriginThread: one2many('mail.thread', { + related: 'notificationsMessage.originThread', + }), res_model: attr(), res_model_name: attr(), + /** + * States the position of the group inside the notification list. + */ + sequence: attr({ + compute: '_computeSequence', + default: 0, + dependencies: [ + 'notifications', + 'notificationsMessage', + ], + }), /** * Related thread when the notification group concerns a single thread. */ thread: many2one('mail.thread', { compute: '_computeThread', dependencies: [ - 'res_id', 'res_model', + 'notifications', + 'notificationsMessage', + 'notificationsMessageOriginThread', ], }) }; diff --git a/addons/mail/static/src/models/notification_group_manager/notification_group_manager.js b/addons/mail/static/src/models/notification_group_manager/notification_group_manager.js index 2989a230fc8..f8419a99d8a 100644 --- a/addons/mail/static/src/models/notification_group_manager/notification_group_manager.js +++ b/addons/mail/static/src/models/notification_group_manager/notification_group_manager.js @@ -33,25 +33,6 @@ function factory(dependencies) { res_model_name: thread.model_name, }); group.update({ notifications: link(notification) }); - // keep res_id only if all notifications are for the same record - // set null if multiple records are present in the group - let res_id = group.res_id; - if (group.res_id === undefined) { - res_id = thread.id; - } else if (group.res_id !== thread.id) { - res_id = null; - } - // keep only the most recent date from all notification messages - let date = group.date; - if (!date) { - date = notification.message.date; - } else { - date = moment.max(group.date, notification.message.date); - } - group.update({ - date, - res_id, - }); // avoid linking the same group twice when adding a notification // to an existing group if (!groups.includes(group)) { diff --git a/addons/mail/static/tests/helpers/mock_models.js b/addons/mail/static/tests/helpers/mock_models.js index b820e62793f..28f9b30f6ce 100644 --- a/addons/mail/static/tests/helpers/mock_models.js +++ b/addons/mail/static/tests/helpers/mock_models.js @@ -124,7 +124,7 @@ class MockModels { attachment_ids: { string: "Attachments", type: 'many2many', relation: 'ir.attachment', default: [] }, author_id: { string: "Author", type: 'many2one', relation: 'res.partner', default() { return this.currentPartnerId; } }, body: { string: "Contents", type: 'html', default: "

" }, - date: { string: "Date", type: 'datetime' }, + date: { string: "Date", type: 'datetime', default() { return moment.utc().format("YYYY-MM-DD HH:mm:ss"); } }, email_from: { string: "From", type: 'char' }, history_partner_ids: { string: "Partners with History", type: 'many2many', relation: 'res.partner' }, id: { string: "Id", type: 'integer' }, diff --git a/addons/sms/static/src/components/notification_list/notification_list_notification_group_tests.js b/addons/sms/static/src/components/notification_list/notification_list_notification_group_tests.js index 75b5c21ecc8..fbaf258a150 100644 --- a/addons/sms/static/src/components/notification_list/notification_list_notification_group_tests.js +++ b/addons/sms/static/src/components/notification_list/notification_list_notification_group_tests.js @@ -108,7 +108,7 @@ QUnit.test('notifications grouped by notification_type', async function (assert) // first message that is expected to have a failure { id: 11, // random unique id, will be used to link failure to message - message_type: 'email', // different type from second message + message_type: 'sms', // different type from second message model: 'res.partner', // same model as second message (and not `mail.channel`) res_id: 31, // same res_id as second message res_model_name: "Partner", // random related model name @@ -116,7 +116,7 @@ QUnit.test('notifications grouped by notification_type', async function (assert) // second message that is expected to have a failure { id: 12, // random unique id, will be used to link failure to message - message_type: 'sms', // different type from first message + message_type: 'email', // different type from first message model: 'res.partner', // same model as first message (and not `mail.channel`) res_id: 31, // same res_id as first message res_model_name: "Partner", // same related model name for consistency @@ -127,13 +127,13 @@ QUnit.test('notifications grouped by notification_type', async function (assert) { mail_message_id: 11, // id of the related first message notification_status: 'exception', // necessary value to have a failure - notification_type: 'email', // different type from second failure + notification_type: 'sms', // different type from second failure }, // second failure that is expected to be used in the test { mail_message_id: 12, // id of the related second message notification_status: 'exception', // necessary value to have a failure - notification_type: 'sms', // different type from first failure + notification_type: 'email', // different type from first failure } ); await this.start(); diff --git a/addons/snailmail/static/src/components/notification_list/notification_list_notification_group_tests.js b/addons/snailmail/static/src/components/notification_list/notification_list_notification_group_tests.js index 7c700c86109..8b562bfedd7 100644 --- a/addons/snailmail/static/src/components/notification_list/notification_list_notification_group_tests.js +++ b/addons/snailmail/static/src/components/notification_list/notification_list_notification_group_tests.js @@ -103,7 +103,7 @@ QUnit.test('notifications grouped by notification_type', async function (assert) // first message that is expected to have a failure { id: 11, // random unique id, will be used to link failure to message - message_type: 'email', // different type from second message + message_type: 'snailmail', // different type from second message model: 'res.partner', // same model as second message (and not `mail.channel`) res_id: 31, // same res_id as second message res_model_name: "Partner", // random related model name @@ -111,7 +111,7 @@ QUnit.test('notifications grouped by notification_type', async function (assert) // second message that is expected to have a failure { id: 12, // random unique id, will be used to link failure to message - message_type: 'snailmail', // different type from first message + message_type: 'email', // different type from first message model: 'res.partner', // same model as first message (and not `mail.channel`) res_id: 31, // same res_id as first message res_model_name: "Partner", // same related model name for consistency @@ -122,13 +122,13 @@ QUnit.test('notifications grouped by notification_type', async function (assert) { mail_message_id: 11, // id of the related first message notification_status: 'exception', // necessary value to have a failure - notification_type: 'email', // different type from second failure + notification_type: 'snail', // different type from second failure }, // second failure that is expected to be used in the test { mail_message_id: 12, // id of the related second message notification_status: 'exception', // necessary value to have a failure - notification_type: 'snail', // different type from first failure + notification_type: 'email', // different type from first failure } ); await this.start();