From 1c7b149aa7eb509a0b5e06313d75d2c8684f8e11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Thu, 23 Nov 2023 15:41:44 +0100 Subject: [PATCH] [FIX] mail: fix load older/newer init value and preview fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The issue is that "load older" was called every time the thread was made visible, in particular after chat window unfold. The load more button will naturally be visible when first loading a thread, it only makes sense to check its final visibility and not intermediate steps. But there was a bug where the loading mechanism was relying on the initial "load older" to fetch messages, because the messaging menu preview was incorrectly setting the thread as already loaded, when it just fetched the last message. Moreover, the "last message" displayed in the preview was not correctly computed, and it could end up being the last "currently fetched" message and not the last of all messages (when jumping in the past), and it could also be a non-persistent message. runbot-35533 closes odoo/odoo#143129 Signed-off-by: Alexandre Kühn (aku) --- .../static/src/core/common/message_model.js | 12 ++- addons/mail/static/src/core/common/thread.js | 28 ++++-- .../static/src/core/common/thread_model.js | 11 +++ .../static/src/core/common/thread_service.js | 51 +++++----- .../static/src/core/web/messaging_menu.js | 22 ++++- .../static/src/core/web/messaging_menu.xml | 2 +- addons/mail/static/src/utils/common/hooks.js | 49 ++++------ .../tests/chat_window/chat_window_tests.js | 12 +-- .../discuss/core/web/messaging_menu_tests.js | 97 ++++++++++++++++++- .../messaging_menu/messaging_menu_tests.js | 7 +- .../messaging_menu/notification_tests.js | 2 + 11 files changed, 210 insertions(+), 83 deletions(-) diff --git a/addons/mail/static/src/core/common/message_model.js b/addons/mail/static/src/core/common/message_model.js index 7925a447dfb..baa1b586946 100644 --- a/addons/mail/static/src/core/common/message_model.js +++ b/addons/mail/static/src/core/common/message_model.js @@ -109,6 +109,14 @@ export class Message extends Record { linkPreviews = Record.many("LinkPreview", { inverse: "message" }); /** @type {number[]} */ needaction_partner_ids = []; + originThread = Record.one("Thread", { + inverse: "allMessages", + compute() { + if (this.model && this.res_id) { + return { model: this.model, id: this.res_id }; + } + }, + }); /** @type {number[]} */ history_partner_ids = []; parentMessage = Record.one("Message"); @@ -241,10 +249,6 @@ export class Message extends Record { return candidates.has(this.subject?.toLowerCase()); } - get originThread() { - return this._store.Thread.get({ model: this.model, id: this.res_id }); - } - get resUrl() { return `${url("/web")}#model=${this.model}&id=${this.res_id}`; } diff --git a/addons/mail/static/src/core/common/thread.js b/addons/mail/static/src/core/common/thread.js index c8b08c6c118..40e4828f935 100644 --- a/addons/mail/static/src/core/common/thread.js +++ b/addons/mail/static/src/core/common/thread.js @@ -81,16 +81,24 @@ export class Thread extends Component { * scrollable (in other cases). */ this.scrollableRef = this.props.scrollRef ?? useRef("messages"); - this.loadOlderState = useVisible("load-older", () => { - if (this.loadOlderState.isVisible && !this.isJumpingRecent) { - this.threadService.fetchMoreMessages(this.props.thread); - } - }); - this.loadNewerState = useVisible("load-newer", () => { - if (this.loadNewerState.isVisible && !this.isJumpingRecent) { - this.threadService.fetchMoreMessages(this.props.thread, "newer"); - } - }); + this.loadOlderState = useVisible( + "load-older", + () => { + if (this.loadOlderState.isVisible && !this.isJumpingRecent) { + this.threadService.fetchMoreMessages(this.props.thread); + } + }, + { init: null } + ); + this.loadNewerState = useVisible( + "load-newer", + () => { + if (this.loadNewerState.isVisible && !this.isJumpingRecent) { + this.threadService.fetchMoreMessages(this.props.thread, "newer"); + } + }, + { init: null } + ); this.presentThresholdState = useVisible( "present-treshold", () => this.updateShowJumpPresent(), diff --git a/addons/mail/static/src/core/common/thread_model.js b/addons/mail/static/src/core/common/thread_model.js index 2bf712c9214..3e24253c1d9 100644 --- a/addons/mail/static/src/core/common/thread_model.js +++ b/addons/mail/static/src/core/common/thread_model.js @@ -143,6 +143,9 @@ export class Thread extends Record { uuid; /** @type {string} */ model; + allMessages = Record.many("Message", { + inverse: "originThread", + }); /** @type {boolean} */ areAttachmentsLoaded = false; attachments = Record.many("Attachment"); @@ -398,6 +401,14 @@ export class Thread extends Record { return [...this.messages].reverse().find((msg) => Number.isInteger(msg.id)); } + get newestPersistentNotEmptyOfAllMessage() { + const allPersistentMessages = this.allMessages.filter( + (message) => Number.isInteger(message.id) && !message.isEmpty + ); + allPersistentMessages.sort((m1, m2) => m2.id - m1.id); + return allPersistentMessages[0]; + } + get oldestPersistentMessage() { return this.messages.find((msg) => Number.isInteger(msg.id)); } diff --git a/addons/mail/static/src/core/common/thread_service.js b/addons/mail/static/src/core/common/thread_service.js index e91e6c77f00..e37b5637795 100644 --- a/addons/mail/static/src/core/common/thread_service.js +++ b/addons/mail/static/src/core/common/thread_service.js @@ -83,12 +83,13 @@ export class ThreadService { /** * @param {import("models").Thread} thread */ - async markAsRead(thread) { - if (!thread.isLoaded) { - await thread.isLoadedDeferred; - await new Promise(setTimeout); + markAsRead(thread) { + const newestPersistentMessage = thread.newestPersistentNotEmptyOfAllMessage; + if (!newestPersistentMessage && !thread.isLoaded) { + thread.isLoadedDeferred + .then(() => new Promise(setTimeout)) + .then(() => this.markAsRead(thread)); } - const newestPersistentMessage = thread.newestPersistentMessage; thread.seen_message_id = newestPersistentMessage?.id ?? false; if ( thread.message_unread_counter > 0 && @@ -98,13 +99,15 @@ export class ThreadService { this.rpc("/discuss/channel/set_last_seen_message", { channel_id: thread.id, last_message_id: newestPersistentMessage.id, - }).then(() => { - this.updateSeen(thread, newestPersistentMessage.id); - }).catch((e) => { - if (e.code !== 404) { - throw e; - } - }); + }) + .then(() => { + this.updateSeen(thread, newestPersistentMessage.id); + }) + .catch((e) => { + if (e.code !== 404) { + throw e; + } + }); } else if (newestPersistentMessage) { this.updateSeen(thread); } @@ -113,7 +116,7 @@ export class ThreadService { } } - updateSeen(thread, lastSeenId = thread.newestPersistentMessage?.id) { + updateSeen(thread, lastSeenId = thread.newestPersistentNotEmptyOfAllMessage?.id) { const lastReadIndex = thread.messages.findIndex((message) => message.id === lastSeenId); let newNeedactionCounter = 0; let newUnreadCounter = 0; @@ -143,7 +146,7 @@ export class ThreadService { needactionMessages: [], message_unread_counter: 0, message_needaction_counter: 0, - seen_message_id: thread.newestPersistentMessage?.id, + seen_message_id: thread.newestPersistentNotEmptyOfAllMessage?.id, }); } @@ -327,15 +330,9 @@ export class ThreadService { for (const preview of previews) { const thread = this.store.Thread.get({ model: "discuss.channel", id: preview.id }); const message = this.store.Message.insert(preview.last_message, { html: true }); - if (!thread.isLoaded) { - thread.messages.push(message); - if (message.isNeedaction) { - thread.needactionMessages.add(message); - } + if (message.isNeedaction) { + thread.needactionMessages.add(message); } - thread.isLoaded = true; - thread.loadOlder = true; - thread.status = "ready"; } } }); @@ -867,10 +864,16 @@ export class ThreadService { } if (thread?.model === "discuss.channel") { if (persona.type === "partner") { - return url(`/discuss/channel/${thread.id}/partner/${persona.id}/avatar_128`, urlParams); + return url( + `/discuss/channel/${thread.id}/partner/${persona.id}/avatar_128`, + urlParams + ); } if (persona.type === "guest") { - return url(`/discuss/channel/${thread.id}/guest/${persona.id}/avatar_128`, urlParams); + return url( + `/discuss/channel/${thread.id}/guest/${persona.id}/avatar_128`, + urlParams + ); } } if (persona.type === "partner" && persona?.id) { diff --git a/addons/mail/static/src/core/web/messaging_menu.js b/addons/mail/static/src/core/web/messaging_menu.js index 0204e306b07..3039adf2aa8 100644 --- a/addons/mail/static/src/core/web/messaging_menu.js +++ b/addons/mail/static/src/core/web/messaging_menu.js @@ -167,14 +167,28 @@ export class MessagingMenu extends Component { if (b.message_unread_counter > 0 && a.message_unread_counter === 0) { return 1; } - if (!a.newestPersistentMessage?.datetime && b.newestPersistentMessage?.datetime) { + if ( + !a.newestPersistentNotEmptyOfAllMessage?.datetime && + b.newestPersistentNotEmptyOfAllMessage?.datetime + ) { return 1; } - if (!b.newestPersistentMessage?.datetime && a.newestPersistentMessage?.datetime) { + if ( + !b.newestPersistentNotEmptyOfAllMessage?.datetime && + a.newestPersistentNotEmptyOfAllMessage?.datetime + ) { return -1; } - if (a.newestPersistentMessage?.datetime && b.newestPersistentMessage?.datetime) { - return b.newestPersistentMessage.datetime - a.newestPersistentMessage.datetime; + if ( + a.newestPersistentNotEmptyOfAllMessage?.datetime && + b.newestPersistentNotEmptyOfAllMessage?.datetime && + a.newestPersistentNotEmptyOfAllMessage?.datetime !== + b.newestPersistentNotEmptyOfAllMessage?.datetime + ) { + return ( + b.newestPersistentNotEmptyOfAllMessage.datetime - + a.newestPersistentNotEmptyOfAllMessage.datetime + ); } return b.localId > a.localId ? 1 : -1; }); diff --git a/addons/mail/static/src/core/web/messaging_menu.xml b/addons/mail/static/src/core/web/messaging_menu.xml index aa5d108a47f..0ea22574f0e 100644 --- a/addons/mail/static/src/core/web/messaging_menu.xml +++ b/addons/mail/static/src/core/web/messaging_menu.xml @@ -72,7 +72,7 @@ - + true) { export function useVisible(refName, cb, { init = false } = {}) { const ref = useRef(refName); const state = { isVisible: init }; - const observer = new IntersectionObserver((entries) => { - for (const entry of entries) { - const newVal = entry.isIntersecting; - if (state.isVisible !== newVal) { - state.isVisible = newVal; - cb(); - } - } - }); - let el; - onMounted(observe); - onWillUnmount(() => { - if (!el) { - return; - } - observer.unobserve(el); - }); - onPatched(observe); - - function observe() { - if (ref.el !== el) { - if (el) { - observer.unobserve(el); - state.isVisible = false; - } - if (ref.el) { - observer.observe(ref.el); - } - } - el = ref.el; + function setValue(value) { + state.isVisible = value; + cb(); } + const observer = new IntersectionObserver((entries) => { + setValue(entries.at(-1).isIntersecting); + }); + useEffect( + (el) => { + if (el) { + observer.observe(el); + return () => { + setValue(false); + observer.unobserve(el); + }; + } + }, + () => [ref.el] + ); return state; } diff --git a/addons/mail/static/tests/chat_window/chat_window_tests.js b/addons/mail/static/tests/chat_window/chat_window_tests.js index 12ecf28c82f..f47d9446a4e 100644 --- a/addons/mail/static/tests/chat_window/chat_window_tests.js +++ b/addons/mail/static/tests/chat_window/chat_window_tests.js @@ -982,7 +982,7 @@ QUnit.test("chat window: scroll conservation on toggle discuss", async () => { const { openDiscuss, openView } = await start(); await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); await click(".o-mail-NotificationItem"); - await contains(".o-mail-Message", { count: 31 }); + await contains(".o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: "bottom" }); await scroll(".o-mail-ChatWindow .o-mail-Thread", 142); openDiscuss(null); @@ -992,7 +992,7 @@ QUnit.test("chat window: scroll conservation on toggle discuss", async () => { res_model: "discuss.channel", views: [[false, "list"]], }); - await contains(".o-mail-Message", { count: 31 }); + await contains(".o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: 142 }); }); @@ -1011,7 +1011,7 @@ QUnit.test( await start(); await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); await click(".o-mail-NotificationItem"); - await contains(".o-mail-Message", { count: 31 }); + await contains(".o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: "bottom" }); await scroll(".o-mail-ChatWindow .o-mail-Thread", 142); // fold chat window @@ -1020,7 +1020,7 @@ QUnit.test( await contains(".o-mail-ChatWindow .o-mail-Thread", { count: 0 }); // unfold chat window await click(".o-mail-ChatWindow-command[title='Open']"); - await contains(".o-mail-Message", { count: 31 }); + await contains(".o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: 142 }); } ); @@ -1040,7 +1040,7 @@ QUnit.test( const { openDiscuss, openView } = await start(); await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); await click(".o-mail-NotificationItem"); - await contains(".o-mail-Message", { count: 31 }); + await contains(".o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: "bottom" }); await scroll(".o-mail-ChatWindow .o-mail-Thread", 142); // fold chat window @@ -1054,7 +1054,7 @@ QUnit.test( }); // unfold chat window await click(".o-mail-ChatWindow-command[title='Open']"); - await contains(".o-mail-ChatWindow .o-mail-Message", { count: 31 }); + await contains(".o-mail-ChatWindow .o-mail-Message", { count: 30 }); await contains(".o-mail-ChatWindow .o-mail-Thread", { scroll: 142 }); } ); diff --git a/addons/mail/static/tests/discuss/core/web/messaging_menu_tests.js b/addons/mail/static/tests/discuss/core/web/messaging_menu_tests.js index 614462dbf7b..4a49533407e 100644 --- a/addons/mail/static/tests/discuss/core/web/messaging_menu_tests.js +++ b/addons/mail/static/tests/discuss/core/web/messaging_menu_tests.js @@ -5,7 +5,7 @@ import { startServer } from "@bus/../tests/helpers/mock_python_environment"; import { patchUiSize } from "@mail/../tests/helpers/patch_ui_size"; import { start } from "@mail/../tests/helpers/test_utils"; -import { triggerHotkey } from "@web/../tests/helpers/utils"; +import { patchWithCleanup, triggerHotkey } from "@web/../tests/helpers/utils"; import { click, contains, insertText } from "@web/../tests/utils"; QUnit.module("messaging menu"); @@ -58,3 +58,98 @@ QUnit.test("new message [REQUIRE FOCUS]", async () => { await click(".o-mail-MessagingMenu button", { text: "New Message" }); await contains(".o-mail-ChatWindow .o-discuss-ChannelSelector input:focus"); }); + +QUnit.test("channel preview ignores empty message", async () => { + const pyEnv = await startServer(); + const partnerId = pyEnv["res.partner"].create({ name: "Demo" }); + const channelId = pyEnv["discuss.channel"].create({ + name: "General", + }); + pyEnv["mail.message"].create({ + author_id: partnerId, + body: "

before last

", + model: "discuss.channel", + res_id: channelId, + }); + pyEnv["mail.message"].create({ + author_id: partnerId, + body: "

", + model: "discuss.channel", + res_id: channelId, + }); + const { openDiscuss } = await start(); + await openDiscuss(channelId); + await contains(".o-mail-Message", { text: "before last" }); + await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); + await contains(".o-mail-NotificationItem-text", { text: "Demo: before last" }); +}); + +QUnit.test("channel preview ignores transient message", async () => { + const pyEnv = await startServer(); + const partnerId = pyEnv["res.partner"].create({ name: "Demo" }); + const channelId = pyEnv["discuss.channel"].create({ + name: "General", + }); + pyEnv["mail.message"].create({ + author_id: partnerId, + body: "

test

", + model: "discuss.channel", + res_id: channelId, + }); + const { openDiscuss } = await start(); + await openDiscuss(channelId); + await insertText(".o-mail-Composer-input", "/who"); + await click(".o-mail-Composer-send:enabled"); + await contains(".o_mail_notification", { text: "You are alone in this channel." }); + await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); + await contains(".o-mail-NotificationItem-text", { text: "Demo: test" }); +}); + +QUnit.test("channel preview ignores messages from the past", async () => { + // make scroll behavior instantaneous. + patchWithCleanup(Element.prototype, { + scrollIntoView() { + return super.scrollIntoView(true); + }, + }); + const pyEnv = await startServer(); + const channelId = pyEnv["discuss.channel"].create({ name: "General" }); + const messageId = pyEnv["mail.message"].create({ + body: "first message", + message_type: "comment", + model: "discuss.channel", + res_id: channelId, + }); + for (let i = 0; i < 100; i++) { + pyEnv["mail.message"].create({ + body: `message ${i}`, + message_type: "comment", + model: "discuss.channel", + res_id: channelId, + }); + } + pyEnv["mail.message"].create({ + body: "last message", + message_type: "comment", + model: "discuss.channel", + parent_id: messageId, + res_id: channelId, + }); + const { env, openDiscuss } = await start(); + await openDiscuss(channelId); + await contains(".o-mail-Message", { count: 30 }); + await contains(".o-mail-Message", { text: "last message" }); + await contains(".o-mail-Thread", { scroll: "bottom" }); + await click(".o-mail-MessageInReply-content", { text: "first message" }); + await contains(".o-mail-Message", { text: "last message", count: 0 }); + await click(".o_menu_systray .dropdown-toggle:has(i[aria-label='Messages'])"); + await contains(".o-mail-NotificationItem-text", { text: "You: last message" }); + pyEnv.withUser(pyEnv.currentUserId, () => + env.services.rpc("/mail/message/post", { + post_data: { body: "new message", message_type: "comment" }, + thread_id: channelId, + thread_model: "discuss.channel", + }) + ); + await contains(".o-mail-NotificationItem-text", { text: "You: new message" }); +}); diff --git a/addons/mail/static/tests/messaging_menu/messaging_menu_tests.js b/addons/mail/static/tests/messaging_menu/messaging_menu_tests.js index e8cea29a527..fc6c3c6972a 100644 --- a/addons/mail/static/tests/messaging_menu/messaging_menu_tests.js +++ b/addons/mail/static/tests/messaging_menu/messaging_menu_tests.js @@ -427,10 +427,11 @@ QUnit.test("mark unread channel as read", async (assert) => { Command.create({ message_unread_counter: 1, partner_id: pyEnv.currentPartnerId }), Command.create({ partner_id: partnerId }), ], + name: "My Channel", }); const [messagId_1] = pyEnv["mail.message"].create([ - { author_id: partnerId, model: "discuss.channel", res_id: channelId }, - { author_id: partnerId, model: "discuss.channel", res_id: channelId }, + { author_id: partnerId, body: "not empty", model: "discuss.channel", res_id: channelId }, + { author_id: partnerId, body: "not empty", model: "discuss.channel", res_id: channelId }, ]); const [currentMemberId] = pyEnv["discuss.channel.member"].search([ ["channel_id", "=", channelId], @@ -447,8 +448,8 @@ QUnit.test("mark unread channel as read", async (assert) => { await click(".o_menu_systray i[aria-label='Messages']"); await triggerEvents(".o-mail-NotificationItem", ["mouseenter"]); await click(".o-mail-NotificationItem [title='Mark As Read']"); - assert.verifySteps(["set_last_seen_message"]); await contains(".o-mail-NotificationItem.text-muted"); + assert.verifySteps(["set_last_seen_message"]); await triggerEvents(".o-mail-NotificationItem", ["mouseenter"]); await contains(".o-mail-NotificationItem [title='Mark As Read']", { count: 0 }); await contains(".o-mail-ChatWindow", { count: 0 }); diff --git a/addons/mail/static/tests/messaging_menu/notification_tests.js b/addons/mail/static/tests/messaging_menu/notification_tests.js index 4426fc76dbf..3e6a68d5be4 100644 --- a/addons/mail/static/tests/messaging_menu/notification_tests.js +++ b/addons/mail/static/tests/messaging_menu/notification_tests.js @@ -248,11 +248,13 @@ QUnit.test("marked as read thread notifications are ordered by last message date ]); pyEnv["mail.message"].create([ { + body: "not empty", date: "2019-01-01 00:00:00", model: "discuss.channel", res_id: channelId_1, }, { + body: "not empty", date: "2020-01-01 00:00:00", model: "discuss.channel", res_id: channelId_2,