From 4f0b262dbeda8f8ebb8483f9dac090bac55c041d Mon Sep 17 00:00:00 2001 From: tsm-odoo Date: Thu, 15 Feb 2024 14:48:29 +0100 Subject: [PATCH] [FIX] bus, mail: fix redundant bus subscriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 16.3, opening the web client on the Odoo backends results in two subscribe events being sent through the bus websocket. Discuss listens to the thread changes to know when the subscription should be refreshed. In order to do so, the last subscription that was made is kept. This issue is that this subscription is empty in the first place so the first evaluation always considers the subscription should be made. In order to fix this issue, this PR refine the condition to determine if the subscription should be renewed: - The last subscription is different from the last one - The user joined a channel after the bus initialization - The user left a channel after the bus initialization In order to test those scenario in a reliable way, this PR also backports odoo#147455 closes odoo/odoo#154170 X-original-commit: 109b45f Signed-off-by: Sébastien Theys (seb) Signed-off-by: Matthieu Stockbauer (tsm) --- addons/bus/static/src/services/bus_service.js | 5 +- .../tests/test_get_discuss_channel.py | 12 +++- .../models/discuss/discuss_channel_member.py | 5 +- .../src/core/common/channel_member_model.js | 7 +++ .../static/src/core/common/store_service.js | 36 ++++++++---- .../static/src/core/common/thread_model.js | 8 ++- .../common/discuss_core_common_service.js | 1 + .../tests/discuss/core/discuss_tests.js | 46 ++++++++++++++- .../models/discuss_channel_member.js | 1 + .../tests/test_performance.py | 57 +++++++++++++++++++ 10 files changed, 156 insertions(+), 22 deletions(-) diff --git a/addons/bus/static/src/services/bus_service.js b/addons/bus/static/src/services/bus_service.js index 2986f424c70..2e2fa9dcaec 100644 --- a/addons/bus/static/src/services/bus_service.js +++ b/addons/bus/static/src/services/bus_service.js @@ -29,7 +29,7 @@ export const busService = { let isActive = false; let isInitialized = false; let isUsingSharedWorker = browser.SharedWorker && !isIosApp(); - const startTs = new Date().getTime(); + const startedAt = luxon.DateTime.now().set({ milliseconds: 0 }); const connectionInitializedDeferred = new Deferred(); /** @@ -101,7 +101,7 @@ export const busService = { debug: odoo.debug, lastNotificationId: multiTab.getSharedValue("last_notification_id", 0), uid, - startTs, + startTs: startedAt.valueOf(), }); } @@ -201,6 +201,7 @@ export const busService = { callback(detail) ); }, + startedAt, }; }, }; diff --git a/addons/im_livechat/tests/test_get_discuss_channel.py b/addons/im_livechat/tests/test_get_discuss_channel.py index 1cc1aefce6a..7e82be4788a 100644 --- a/addons/im_livechat/tests/test_get_discuss_channel.py +++ b/addons/im_livechat/tests/test_get_discuss_channel.py @@ -70,10 +70,13 @@ class TestGetDiscussChannel(TestImLivechatCommon, MailCommon): }) self.assertFalse(channel_info['anonymous_name']) self.assertEqual(channel_info['anonymous_country'], {'code': 'BE', 'id': belgium.id, 'name': 'Belgium'}) + operator_member = self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', operator.partner_id.id)]) + visitor_member = self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', test_user.partner_id.id)]) self.assertEqual(channel_info['channelMembers'], [['ADD', [ { 'thread': {'id': channel_info['id'], 'model': "discuss.channel"}, - 'id': self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', operator.partner_id.id)]).id, + 'create_date': fields.Datetime.to_string(operator_member.create_date), + 'id': operator_member.id, 'persona': { 'active': True, 'country': False, @@ -86,7 +89,8 @@ class TestGetDiscussChannel(TestImLivechatCommon, MailCommon): }, { 'thread': {'id': channel_info['id'], 'model': "discuss.channel"}, - 'id': self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', test_user.partner_id.id)]).id, + 'create_date': fields.Datetime.to_string(visitor_member.create_date), + 'id': visitor_member.id, 'persona': { 'active': True, 'country': { @@ -112,13 +116,15 @@ class TestGetDiscussChannel(TestImLivechatCommon, MailCommon): 'user_id': operator.id, 'channel_id': self.livechat_channel.id, }) + operator_member = self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', operator.partner_id.id)]) self.assertEqual(channel_info['operator_pid'], [operator.partner_id.id, "Michel Operator"]) self.assertFalse(channel_info['anonymous_name']) self.assertEqual(channel_info['anonymous_country'], False) self.assertEqual(channel_info['channelMembers'], [['ADD', [ { 'thread': {'id': channel_info['id'], 'model': "discuss.channel"}, - 'id': self.env['discuss.channel.member'].search([('channel_id', '=', channel_info['id']), ('partner_id', '=', operator.partner_id.id)]).id, + 'create_date': fields.Datetime.to_string(operator_member.create_date), + 'id': operator_member.id, 'persona': { 'active': True, 'country': False, diff --git a/addons/mail/models/discuss/discuss_channel_member.py b/addons/mail/models/discuss/discuss_channel_member.py index 7baafc8f375..8e65a1fd974 100644 --- a/addons/mail/models/discuss/discuss_channel_member.py +++ b/addons/mail/models/discuss/discuss_channel_member.py @@ -3,6 +3,7 @@ import logging import requests +import odoo from odoo import api, fields, models, _ from odoo.exceptions import AccessError, UserError, ValidationError from odoo.osv import expression @@ -180,7 +181,7 @@ class ChannelMember(models.Model): def _discuss_channel_member_format(self, fields=None): if not fields: - fields = {'id': True, 'channel': {}, 'persona': {}} + fields = {'id': True, 'channel': {}, 'persona': {}, 'create_date': True} members_formatted_data = {} for member in self: data = {} @@ -201,6 +202,8 @@ class ChannelMember(models.Model): data['custom_notifications'] = member.custom_notifications if 'mute_until_dt' in fields: data['mute_until_dt'] = member.mute_until_dt + if 'create_date' in fields: + data['create_date'] = odoo.fields.Datetime.to_string(member.create_date) members_formatted_data[member] = data return members_formatted_data diff --git a/addons/mail/static/src/core/common/channel_member_model.js b/addons/mail/static/src/core/common/channel_member_model.js index e73fd670ef0..e91e2b8980d 100644 --- a/addons/mail/static/src/core/common/channel_member_model.js +++ b/addons/mail/static/src/core/common/channel_member_model.js @@ -1,6 +1,7 @@ /* @odoo-module */ import { Record } from "@mail/core/common/record"; +import { deserializeDateTime } from "@web/core/l10n/dates"; /** * @class ChannelMember @@ -22,6 +23,8 @@ export class ChannelMember extends Record { return super.insert(...arguments); } + /** @type {string} */ + create_date; /** @type {number} */ id; persona = Record.one("Persona", { inverse: "channelMembers" }); @@ -36,6 +39,10 @@ export class ChannelMember extends Record { getLangName() { return this.persona.lang_name; } + + get memberSince() { + return deserializeDateTime(this.create_date); + } } ChannelMember.register(); diff --git a/addons/mail/static/src/core/common/store_service.js b/addons/mail/static/src/core/common/store_service.js index bc311f2550e..b2b0588279e 100644 --- a/addons/mail/static/src/core/common/store_service.js +++ b/addons/mail/static/src/core/common/store_service.js @@ -49,7 +49,7 @@ export class Store extends BaseStore { return this.imStatusTrackedPersonas.map((persona) => persona.id); } - lastChannelSubscription = ""; + knownChannelIds = new Set(); /** This is the current logged partner */ user = Record.one("Persona"); /** This is the current logged guest */ @@ -203,22 +203,34 @@ export class Store extends BaseStore { } updateBusSubscription() { - const channelIds = []; - const ids = Object.keys(this.Thread.records).sort(); // Ensure channels processed in same order. - for (const id of ids) { - const thread = this.Thread.records[id]; - if (thread.model === "discuss.channel" && !thread.isTransient) { - channelIds.push(id); - if (!thread.hasSelfAsMember) { - this.env.services["bus_service"].addChannel(`discuss.channel_${thread.id}`); + if (!this.isMessagingReady) { + return; + } + const allSelfChannelIds = new Set(); + for (const thread of Object.values(this.Thread.records)) { + if (thread.model === "discuss.channel" && thread.hasSelfAsMember) { + if (thread.selfMember.memberSince < this.env.services["bus_service"].startedAt) { + this.knownChannelIds.add(thread.id); } + allSelfChannelIds.add(thread.id); } } - const channels = JSON.stringify(channelIds); - if (this.isMessagingReady && this.lastChannelSubscription !== channels) { + let shouldUpdateChannels = false; + for (const id of allSelfChannelIds) { + if (!this.knownChannelIds.has(id)) { + shouldUpdateChannels = true; + this.knownChannelIds.add(id); + } + } + for (const id of this.knownChannelIds) { + if (!allSelfChannelIds.has(id)) { + shouldUpdateChannels = true; + this.knownChannelIds.delete(id); + } + } + if (shouldUpdateChannels) { this.env.services["bus_service"].forceUpdateChannels(); } - this.lastChannelSubscription = channels; } } Store.register(); diff --git a/addons/mail/static/src/core/common/thread_model.js b/addons/mail/static/src/core/common/thread_model.js index 91890ad1c50..71bc432f481 100644 --- a/addons/mail/static/src/core/common/thread_model.js +++ b/addons/mail/static/src/core/common/thread_model.js @@ -437,9 +437,11 @@ export class Thread extends Record { } get hasSelfAsMember() { - return this.channelMembers.some((channelMember) => - channelMember.persona?.eq(this._store.self) - ); + return Boolean(this.selfMember); + } + + get selfMember() { + return this.channelMembers.find((member) => member.persona.eq(this._store.self)); } get invitationLink() { diff --git a/addons/mail/static/src/discuss/core/common/discuss_core_common_service.js b/addons/mail/static/src/discuss/core/common/discuss_core_common_service.js index ae85a224cf0..ec5f760d89b 100644 --- a/addons/mail/static/src/discuss/core/common/discuss_core_common_service.js +++ b/addons/mail/static/src/discuss/core/common/discuss_core_common_service.js @@ -31,6 +31,7 @@ export class DiscussCoreCommon { setup() { this.messagingService.isReady.then((data) => { + this.store.updateBusSubscription(); Record.MAKE_UPDATE(() => { for (const channelData of data.channels) { this.insertInitChannel(channelData); diff --git a/addons/mail/static/tests/discuss/core/discuss_tests.js b/addons/mail/static/tests/discuss/core/discuss_tests.js index 7532ac5a56c..17203b2e3ce 100644 --- a/addons/mail/static/tests/discuss/core/discuss_tests.js +++ b/addons/mail/static/tests/discuss/core/discuss_tests.js @@ -4,7 +4,9 @@ import { startServer } from "@bus/../tests/helpers/mock_python_environment"; import { start } from "@mail/../tests/helpers/test_utils"; -import { click, contains } from "@web/../tests/utils"; +import { assertSteps, click, contains, insertText, step } from "@web/../tests/utils"; +import { patchWebsocketWorkerWithCleanup } from "@bus/../tests/helpers/mock_websocket"; +import { patchDate } from "@web/../tests/helpers/utils"; QUnit.module("discuss"); @@ -19,3 +21,45 @@ QUnit.test("Member list and settings menu are exclusive", async () => { await contains(".o-discuss-CallSettings"); await contains(".o-discuss-ChannelMemberList", { count: 0 }); }); + +QUnit.test("bus subscription is refreshed when channel is joined", async () => { + const pyEnv = await startServer(); + pyEnv["discuss.channel"].create([{ name: "General" }, { name: "Sales" }]); + patchWebsocketWorkerWithCleanup({ + _sendToServer({ event_name, data }) { + if (event_name === "subscribe") { + step(`subscribe - ${JSON.stringify(data.channels)}`); + } + }, + }); + const later = luxon.DateTime.now().plus({ seconds: 2 }); + patchDate(later.year, later.month, later.day, later.hour, later.minute, later.second); + const { openDiscuss } = await start(); + await assertSteps(["subscribe - []"]); + await openDiscuss(); + await assertSteps([]); + await click(".o-mail-DiscussSidebar i[title='Add or join a channel']"); + await insertText(".o-discuss-ChannelSelector input", "new channel"); + await click(".o-discuss-ChannelSelector-suggestion"); + await assertSteps(["subscribe - []"]); +}); + +QUnit.test("bus subscription is refreshed when channel is left", async () => { + const pyEnv = await startServer(); + pyEnv["discuss.channel"].create({ name: "General" }); + patchWebsocketWorkerWithCleanup({ + _sendToServer({ event_name, data }) { + if (event_name === "subscribe") { + step(`subscribe - ${JSON.stringify(data.channels)}`); + } + }, + }); + const later = luxon.DateTime.now().plus({ seconds: 2 }); + patchDate(later.year, later.month, later.day, later.hour, later.minute, later.second); + const { openDiscuss } = await start(); + await assertSteps(["subscribe - []"]); + await openDiscuss(); + await assertSteps([]); + await click("[title='Leave this channel']"); + await assertSteps(["subscribe - []"]); +}); diff --git a/addons/mail/static/tests/helpers/mock_server/models/discuss_channel_member.js b/addons/mail/static/tests/helpers/mock_server/models/discuss_channel_member.js index 3a13da45e3f..9f672ae623b 100644 --- a/addons/mail/static/tests/helpers/mock_server/models/discuss_channel_member.js +++ b/addons/mail/static/tests/helpers/mock_server/models/discuss_channel_member.js @@ -64,6 +64,7 @@ patch(MockServer.prototype, { thread: { id: member.channel_id, model: "discuss.channel" }, id: member.id, persona, + create_date: member.create_date, }; dataList.push(data); } diff --git a/addons/test_discuss_full/tests/test_performance.py b/addons/test_discuss_full/tests/test_performance.py index 8d814df13ed..b94622e0f9a 100644 --- a/addons/test_discuss_full/tests/test_performance.py +++ b/addons/test_discuss_full/tests/test_performance.py @@ -158,6 +158,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_general.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_general.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_general.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -210,6 +213,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_channel_public_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_channel_public_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_channel_public_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -262,6 +268,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_channel_public_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_channel_public_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_channel_public_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -314,6 +323,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_channel_group_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -366,6 +378,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_channel_group_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_channel_group_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_channel_group_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -419,6 +434,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_group_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -441,6 +459,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_group_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[12].partner_id).create_date + ), 'id': self.channel_group_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[12].partner_id).id, 'persona': { 'active': True, @@ -509,6 +530,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_chat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -531,6 +555,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[14].partner_id).create_date + ), 'id': self.channel_chat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[14].partner_id).id, 'persona': { 'active': True, @@ -599,6 +626,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_chat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -621,6 +651,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[15].partner_id).create_date + ), 'id': self.channel_chat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[15].partner_id).id, 'persona': { 'active': True, @@ -689,6 +722,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_3.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_3.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_chat_3.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -711,6 +747,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_3.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_3.channel_member_ids.filtered(lambda m: m.partner_id == self.users[2].partner_id).create_date + ), 'id': self.channel_chat_3.channel_member_ids.filtered(lambda m: m.partner_id == self.users[2].partner_id).id, 'persona': { 'active': True, @@ -779,6 +818,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_4.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_4.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_chat_4.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -801,6 +843,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_chat_4.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_chat_4.channel_member_ids.filtered(lambda m: m.partner_id == self.users[3].partner_id).create_date + ), 'id': self.channel_chat_4.channel_member_ids.filtered(lambda m: m.partner_id == self.users[3].partner_id).id, 'persona': { 'active': True, @@ -873,6 +918,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_livechat_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_livechat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_livechat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -889,6 +937,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_livechat_1.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_livechat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[1].partner_id).create_date + ), 'id': self.channel_livechat_1.channel_member_ids.filtered(lambda m: m.partner_id == self.users[1].partner_id).id, 'persona': { 'active': True, @@ -960,6 +1011,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_livechat_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_livechat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).create_date + ), 'id': self.channel_livechat_2.channel_member_ids.filtered(lambda m: m.partner_id == self.users[0].partner_id).id, 'persona': { 'active': True, @@ -976,6 +1030,9 @@ class TestDiscussFullPerformance(HttpCase): 'id': self.channel_livechat_2.id, 'model': "discuss.channel", }, + 'create_date': fields.Datetime.to_string( + self.channel_livechat_2.channel_member_ids.filtered(lambda m: m.guest_id).create_date + ), 'id': self.channel_livechat_2.channel_member_ids.filtered(lambda m: m.guest_id).id, 'persona': { 'id': self.channel_livechat_2.channel_member_ids.filtered(lambda m: m.guest_id).guest_id.id,