From 8f38b5130cacf72bbc65f88e37759ff0b02caab5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Mon, 12 Jun 2023 14:19:12 +0000 Subject: [PATCH] [FIX] bus, mail: message notif should not increment counter twice Before this commit, a new unread message could contribute to unread counter of conversation twice. Steps to reproduce: 1. Log in as Demo and start a conversation with Mitchell Admin 2. Log in as Admin: if this is the first message in the conversation, counter is correct and shows (1) 3. Log out as Admin 4. Send another message to Admin 5. Log in as Admin and notice that the counter is doubled 6. If the conversation was still open, counter is correct! This happens because some notification are pending dispatch before fetching messaging state initially. The counter at init_messaging fetch takes into account the notification. However, when handling the message notifcation, it's wrongly considered as a new notification, thus incrementing the counter when it shouldn't. This commit fixes the issue by fetching the last bus notification when fetching init_messaging. Doing so allow to determine whether a notification is newer or older than init_messaging state. This helps deciding whether the counter should be incremented or not. Task-3357960 closes odoo/odoo#124767 X-original-commit: 081ab772dece86f1b5a5d4b3f6227b4b311d9206 Signed-off-by: Stockbauer Matthieu (tsm) --- addons/bus/models/bus.py | 4 ++++ addons/bus/static/src/services/bus_service.js | 1 + addons/bus/static/tests/bus_tests.js | 2 +- addons/bus/static/tests/helpers/mock_server.js | 4 ++-- addons/mail/models/discuss/mail_guest.py | 1 + addons/mail/models/res_users.py | 1 + .../static/src/core/common/messaging_service.js | 13 ++++++++++--- addons/mail/static/src/core/common/store_service.js | 10 ++++++++++ .../tests/helpers/mock_server/models/res_users.js | 1 + addons/test_discuss_full/tests/test_performance.py | 1 + 10 files changed, 32 insertions(+), 6 deletions(-) diff --git a/addons/bus/models/bus.py b/addons/bus/models/bus.py index 9ce962fdcaf..9c262ae3e17 100644 --- a/addons/bus/models/bus.py +++ b/addons/bus/models/bus.py @@ -103,6 +103,10 @@ class ImBus(models.Model): }) return result + def _bus_last_id(self): + last = self.env['bus.bus'].search([], order='id desc', limit=1) + return last.id if last else 0 + #---------------------------------------------------------- # Dispatcher diff --git a/addons/bus/static/src/services/bus_service.js b/addons/bus/static/src/services/bus_service.js index 33f24d0b180..076d00038e9 100644 --- a/addons/bus/static/src/services/bus_service.js +++ b/addons/bus/static/src/services/bus_service.js @@ -64,6 +64,7 @@ export const busService = { const { type } = messageEv.data; let { data } = messageEv.data; if (type === "notification") { + data.forEach((d) => (d.message.id = d.id)); // put notification id in notif message multiTab.setSharedValue("last_notification_id", data[data.length - 1].id); data = data.map((notification) => notification.message); for (const { type, payload } of data) { diff --git a/addons/bus/static/tests/bus_tests.js b/addons/bus/static/tests/bus_tests.js index 13d5e231b0e..8bf403d4e8e 100644 --- a/addons/bus/static/tests/bus_tests.js +++ b/addons/bus/static/tests/bus_tests.js @@ -413,7 +413,7 @@ QUnit.module( await env2.services["bus_service"].start(); await updateLastNotificationDeferred; // Second bus service sends the last known notification id. - assert.verifySteps([`initialize_connection - 1`]); + assert.verifySteps([`initialize_connection - 2`]); } ); diff --git a/addons/bus/static/tests/helpers/mock_server.js b/addons/bus/static/tests/helpers/mock_server.js index d3c78186223..bd665b069fb 100644 --- a/addons/bus/static/tests/helpers/mock_server.js +++ b/addons/bus/static/tests/helpers/mock_server.js @@ -59,7 +59,7 @@ patch(MockServer.prototype, "bus", { const values = []; for (const notification of notifications) { const [type, payload] = notification.slice(1, notification.length); - values.push({ id: this.lastBusNotificationId++, message: { payload, type } }); + values.push({ id: ++this.lastBusNotificationId, message: { payload, type } }); if (this.debug) { console.log("%c[bus]", "color: #c6e; font-weight: bold;", type, payload); } @@ -70,7 +70,7 @@ patch(MockServer.prototype, "bus", { * Simulate the lost of the connection by simulating a closeEvent on * the worker websocket. * - * @param {number} clodeCode the code to close the connection with. + * @param {number} closeCode the code to close the connection with. */ _simulateConnectionLost(closeCode) { this.websocketWorker.websocket.close(closeCode); diff --git a/addons/mail/models/discuss/mail_guest.py b/addons/mail/models/discuss/mail_guest.py index f5465b2f225..f688e9c0f15 100644 --- a/addons/mail/models/discuss/mail_guest.py +++ b/addons/mail/models/discuss/mail_guest.py @@ -113,6 +113,7 @@ class MailGuest(models.Model): 'current_user_id': False, 'current_user_settings': False, 'hasLinkPreviewFeature': self.env['mail.link.preview']._is_link_preview_enabled(), + 'initBusId': self.env['bus.bus'].sudo()._bus_last_id(), 'menu_id': False, 'needaction_inbox_counter': False, 'odoobot': { diff --git a/addons/mail/models/res_users.py b/addons/mail/models/res_users.py index c7b15cc097b..00914aa494c 100644 --- a/addons/mail/models/res_users.py +++ b/addons/mail/models/res_users.py @@ -267,6 +267,7 @@ class Users(models.Model): 'current_user_id': self.id, 'current_user_settings': self.env['res.users.settings']._find_or_create_for_user(self)._res_users_settings_format(), 'hasLinkPreviewFeature': self.env['mail.link.preview']._is_link_preview_enabled(), + 'initBusId': self.env['bus.bus'].sudo()._bus_last_id(), 'internalUserGroupId': self.env.ref('base.group_user').id, 'menu_id': self.env['ir.model.data']._xmlid_to_res_id('mail.menu_root_discuss'), 'needaction_inbox_counter': self.partner_id._get_needaction_count(), diff --git a/addons/mail/static/src/core/common/messaging_service.js b/addons/mail/static/src/core/common/messaging_service.js index 17ff46fbb96..9edb6ddfc08 100644 --- a/addons/mail/static/src/core/common/messaging_service.js +++ b/addons/mail/static/src/core/common/messaging_service.js @@ -121,6 +121,7 @@ export class Messaging { this.insertCannedResponse(code); }); this.store.hasLinkPreviewFeature = data.hasLinkPreviewFeature; + this.store.initBusId = data.initBusId; this.isReady.resolve(); this.store.isMessagingReady = true; } @@ -482,16 +483,22 @@ export class Messaging { if (message.isSelfAuthored) { channel.seen_message_id = message.id; } else { - channel.message_unread_counter++; + if (notif.id > this.store.initBusId) { + channel.message_unread_counter++; + } if (message.isNeedaction) { const inbox = this.store.discuss.inbox; if (!inbox.messages.includes(message)) { inbox.messages.push(message); - inbox.counter++; + if (notif.id > this.store.initBusId) { + inbox.counter++; + } } if (!channel.needactionMessages.includes(message)) { channel.needactionMessages.push(message); - channel.message_needaction_counter++; + if (notif.id > this.store.initBusId) { + channel.message_needaction_counter++; + } } } } diff --git a/addons/mail/static/src/core/common/store_service.js b/addons/mail/static/src/core/common/store_service.js index 3410743775d..200986b209c 100644 --- a/addons/mail/static/src/core/common/store_service.js +++ b/addons/mail/static/src/core/common/store_service.js @@ -54,6 +54,16 @@ export class Store { */ guest = null; + /** + * The last id of bus notification at the time for fetch init_messaging. + * When receiving a notification: + * - if id greater than this value: the notification is newer than init_messaging state. + * - if same id or lower: the notification is older than init_messaging state. + * This is useful to determine whether we should increment or decrement a counter based + * on init_messaging state. + */ + initBusId = 0; + /** * Indicates whether the current user is using the application through the * public page. diff --git a/addons/mail/static/tests/helpers/mock_server/models/res_users.js b/addons/mail/static/tests/helpers/mock_server/models/res_users.js index 084d686665a..78035cf5412 100644 --- a/addons/mail/static/tests/helpers/mock_server/models/res_users.js +++ b/addons/mail/static/tests/helpers/mock_server/models/res_users.js @@ -34,6 +34,7 @@ patch(MockServer.prototype, "mail/models/res_users", { current_user_settings: this._mockResUsersSettings_ResUsersSettingsFormat( userSettings.id ), + initBusId: this.lastBusNotificationId, menu_id: false, // not useful in QUnit tests needaction_inbox_counter: this._mockResPartner_GetNeedactionCount(user.partner_id), odoobot: this._mockResPartnerMailPartnerFormat(this.odoobotId).get(this.odoobotId), diff --git a/addons/test_discuss_full/tests/test_performance.py b/addons/test_discuss_full/tests/test_performance.py index f04422f20b4..349e1bd7a18 100644 --- a/addons/test_discuss_full/tests/test_performance.py +++ b/addons/test_discuss_full/tests/test_performance.py @@ -108,6 +108,7 @@ class TestDiscussFullPerformance(TransactionCase): The point of having a separate getter is to allow it to be overriden. """ return { + 'initBusId': self.env['bus.bus'].sudo()._bus_last_id(), 'hasLinkPreviewFeature': True, 'needaction_inbox_counter': 1, 'starred_counter': 1,