[FIX] mail: make Inbox counters more reliable in mark as read flows

- Send the new Inbox counter when marking as read, to guarantee the JS has the
  correct value (instead of having to guess it which was impossible when an
  arbitrary domain was given to the method).

  NOTE: Manual increment is kept on receiving a new Inbox message because that
  one was reliable and it would have been more tricky to compute the new counter
  for each target partner (mark as read is one user doing an action for himself,
  whereas new message is from another user to potentially many different users).
  Generic counter for threads is also not sent on every change for similar
  reasons.

- Refresh Inbox automatically when some messages are marked as read and there
  are more message on the server than currently loaded.

- Remove confusing (and impossible to maintain) counter on non-origin threads
  (in particular channel followers). This behavior in JS was not consistent with
  the server which always counted only origin thread (model/res_id).

task-2446302

closes odoo/odoo#65161

Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>

5 files changed, 91 insertions, 61 deletions

diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index 2eee9623fb2..5103a2a2f79 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -704,17 +704,17 @@ class Message(models.Model):
         notif_domain = [
             ('res_partner_id', '=', partner_id),
             ('is_read', '=', False)]
-
         if domain:
-            messages_ids = self.search(domain).ids  # need sudo?
-            notif_domain = expression.AND([notif_domain, [('mail_message_id', 'in', messages_ids)]])
+            messages = self.search(domain)
+            messages.set_message_done()
+            return messages.ids
 
         notifications = self.env['mail.notification'].sudo().search(notif_domain)
         notifications.write({'is_read': True})
 
         ids = [n['mail_message_id'] for n in notifications.read(['mail_message_id'])]
 
-        notification = {'type': 'mark_as_read', 'message_ids': [id[0] for id in ids]}
+        notification = {'type': 'mark_as_read', 'message_ids': [id[0] for id in ids], 'needaction_inbox_counter': self.env.user.partner_id.get_needaction_count()}
         self.env['bus.bus'].sendone((self._cr.dbname, 'res.partner', partner_id), notification)
 
         return ids
@@ -752,7 +752,8 @@ class Message(models.Model):
         notifications.write({'is_read': True})
 
         for (msg_ids, channel_ids) in groups:
-            notification = {'type': 'mark_as_read', 'message_ids': msg_ids, 'channel_ids': [c.id for c in channel_ids]}
+            # channel_ids in result is deprecated and will be removed in a future version
+            notification = {'type': 'mark_as_read', 'message_ids': msg_ids, 'channel_ids': [c.id for c in channel_ids], 'needaction_inbox_counter': self.env.user.partner_id.get_needaction_count()}
             self.env['bus.bus'].sendone((self._cr.dbname, 'res.partner', partner_id.id), notification)
 
     @api.model
diff --git a/addons/mail/static/src/components/discuss/tests/discuss_tests.js b/addons/mail/static/src/components/discuss/tests/discuss_tests.js
index 59a2b8cc239..dc2005e5ac7 100644
--- a/addons/mail/static/src/components/discuss/tests/discuss_tests.js
+++ b/addons/mail/static/src/components/discuss/tests/discuss_tests.js
@@ -2574,18 +2574,22 @@ QUnit.test('inbox: mark all messages as read', async function (assert) {
             body: "not empty",
             channel_ids: [20], // link message to channel
             id: 100, // random unique id, useful to link notification
+            model: 'mail.channel',
             // needaction needs to be set here for message_fetch domain, because
             // mocked models don't have computed fields
             needaction: true,
+            res_id: 20,
         },
         // second expected message
         {
             body: "not empty",
             channel_ids: [20], // link message to channel
             id: 101, // random unique id, useful to link notification
+            model: 'mail.channel',
             // needaction needs to be set here for message_fetch domain, because
             // mocked models don't have computed fields
             needaction: true,
+            res_id: 20,
         }
     );
     this.data['mail.notification'].records.push(
diff --git a/addons/mail/static/src/model/model_field_command.js b/addons/mail/static/src/model/model_field_command.js
index 89f42651ad9..a6dadb7b983 100644
--- a/addons/mail/static/src/model/model_field_command.js
+++ b/addons/mail/static/src/model/model_field_command.js
@@ -35,11 +35,37 @@ function clear() {
     );
 }
 
+/**
+ * Returns a decrement command to give to the model manager at create/update.
+ *
+ * @param {number} [amount=1]
+ */
+function decrement(amount = 1) {
+    return new FieldCommand((field, record, options) => {
+        const oldValue = field.get(record);
+        field.set(record, oldValue - amount, options);
+    });
+}
+
+/**
+ * Returns a increment command to give to the model manager at create/update.
+ *
+ * @param {number} [amount=1]
+ */
+function increment(amount = 1) {
+    return new FieldCommand((field, record, options) => {
+        const oldValue = field.get(record);
+        field.set(record, oldValue + amount, options);
+    });
+}
+
 return {
     // class
     FieldCommand,
     // shortcuts
     clear,
+    decrement,
+    increment,
 };
 
 });
diff --git a/addons/mail/static/src/models/messaging_notification_handler/messaging_notification_handler.js b/addons/mail/static/src/models/messaging_notification_handler/messaging_notification_handler.js
index f497fa420e6..a63acfb82fe 100644
--- a/addons/mail/static/src/models/messaging_notification_handler/messaging_notification_handler.js
+++ b/addons/mail/static/src/models/messaging_notification_handler/messaging_notification_handler.js
@@ -3,6 +3,7 @@ odoo.define('mail/static/src/models/messaging_notification_handler/messaging_not
 
 const { registerNewModel } = require('mail/static/src/model/model_core.js');
 const { one2one } = require('mail/static/src/model/model_field.js');
+const { decrement, increment } = require('mail/static/src/model/model_field_command.js');
 const { htmlToTextContentInline } = require('mail.utils');
 
 const PREVIEW_MSG_MAX_SIZE = 350; // optimal for native English speakers
@@ -222,7 +223,7 @@ function factory(dependencies) {
                     oldMessageWasModeratedByCurrentPartner
                 ) {
                     const moderation = this.env.messaging.moderation;
-                    moderation.update({ counter: moderation.counter - 1 });
+                    moderation.update({ counter: decrement() });
                 }
                 return;
             }
@@ -389,15 +390,10 @@ function factory(dependencies) {
             const message = this.env.models['mail.message'].insert(
                 this.env.models['mail.message'].convertData(data)
             );
-            const inboxMailbox = this.env.messaging.inbox;
-            inboxMailbox.update({ counter: inboxMailbox.counter + 1 });
-            for (const thread of message.threads) {
-                if (
-                    thread.channel_type === 'channel' &&
-                    message.isNeedaction
-                ) {
-                    thread.update({ message_needaction_counter: thread.message_needaction_counter + 1 });
-                }
+            this.env.messaging.inbox.update({ counter: increment() });
+            const originThread = message.originThread;
+            if (originThread && message.isNeedaction) {
+                originThread.update({ message_needaction_counter: increment() });
             }
             // manually force recompute of counter
             this.messaging.messagingMenu.update();
@@ -522,7 +518,7 @@ function factory(dependencies) {
                         message.moderation_status === 'pending_moderation' &&
                         message.originThread.isModeratedByCurrentPartner
                     ) {
-                        moderationMailbox.update({ counter: moderationMailbox.counter - 1 });
+                        moderationMailbox.update({ counter: decrement() });
                     }
                     message.delete();
                 }
@@ -558,11 +554,9 @@ function factory(dependencies) {
          * @param {Object} param0
          * @param {integer[]} [param0.channel_ids
          * @param {integer[]} [param0.message_ids=[]]
+         * @param {integer} [param0.needaction_inbox_counter]
          */
-        _handleNotificationPartnerMarkAsRead({ channel_ids, message_ids = [] }) {
-            const inboxMailbox = this.env.messaging.inbox;
-
-            // 1. move messages from inbox to history
+        _handleNotificationPartnerMarkAsRead({ channel_ids, message_ids = [], needaction_inbox_counter }) {
             for (const message_id of message_ids) {
                 // We need to ignore all not yet known messages because we don't want them
                 // to be shown partially as they would be linked directly to mainCache
@@ -570,34 +564,33 @@ function factory(dependencies) {
                 // but something like last read message_id or something like that.
                 // (just imagine you mark 1000 messages as read ... )
                 const message = this.env.models['mail.message'].findFromIdentifyingData({ id: message_id });
-                if (message) {
-                    message.update({
-                        isNeedaction: false,
-                        isHistory: true,
-                    });
+                if (!message) {
+                    continue;
                 }
+                // update thread counter
+                const originThread = message.originThread;
+                if (originThread && message.isNeedaction) {
+                    originThread.update({ message_needaction_counter: decrement() });
+                }
+                // move messages from Inbox to history
+                message.update({
+                    isHistory: true,
+                    isNeedaction: false,
+                });
             }
-
-            // 2. remove "needaction" from channels
-            let channels;
-            if (channel_ids) {
-                channels = channel_ids
-                    .map(id => this.env.models['mail.thread'].findFromIdentifyingData({
-                        id,
-                        model: 'mail.channel',
-                    }))
-                    .filter(thread => !!thread);
+            const inbox = this.env.messaging.inbox;
+            if (needaction_inbox_counter !== undefined) {
+                inbox.update({ counter: needaction_inbox_counter });
             } else {
-                // flux specific: channel_ids unset means "mark all as read"
-                channels = this.env.models['mail.thread'].all(thread =>
-                    thread.model === 'mail.channel'
-                );
+                // kept for compatibility in stable
+                inbox.update({ counter: decrement(message_ids.length) });
             }
-            for (const channel of channels) {
-                channel.update({ message_needaction_counter: 0 });
+            if (inbox.counter > inbox.mainCache.fetchedMessages.length) {
+                // Force refresh Inbox because depending on what was marked as
+                // read the cache might become empty even though there are more
+                // messages on the server.
+                inbox.mainCache.update({ hasToLoadMessages: true });
             }
-            inboxMailbox.update({ counter: inboxMailbox.counter - message_ids.length });
-
             // manually force recompute of counter
             this.messaging.messagingMenu.update();
         }
@@ -613,7 +606,7 @@ function factory(dependencies) {
             );
             const moderationMailbox = this.env.messaging.moderation;
             if (moderationMailbox) {
-                moderationMailbox.update({ counter: moderationMailbox.counter + 1 });
+                moderationMailbox.update({ counter: increment() });
             }
             // manually force recompute of counter
             this.messaging.messagingMenu.update();
@@ -636,9 +629,7 @@ function factory(dependencies) {
                 }
                 message.update({ isStarred: starred });
                 starredMailbox.update({
-                    counter: starred
-                        ? starredMailbox.counter + 1
-                        : starredMailbox.counter - 1,
+                    counter: starred ? increment() : decrement(),
                 });
             }
         }
@@ -758,7 +749,7 @@ function factory(dependencies) {
             }
             const notificationContent = htmlToTextContentInline(message.body).substr(0, PREVIEW_MSG_MAX_SIZE);
             this.env.services['bus_service'].sendNotification(notificationTitle, notificationContent);
-            messaging.update({ outOfFocusUnreadMessageCounter: messaging.outOfFocusUnreadMessageCounter + 1 });
+            messaging.update({ outOfFocusUnreadMessageCounter: increment() });
             const titlePattern = messaging.outOfFocusUnreadMessageCounter === 1
                 ? this.env._t("%d Message")
                 : this.env._t("%d Messages");
diff --git a/addons/mail/static/tests/helpers/mock_server.js b/addons/mail/static/tests/helpers/mock_server.js
index e78ce39fd31..dc027c679f5 100644
--- a/addons/mail/static/tests/helpers/mock_server.js
+++ b/addons/mail/static/tests/helpers/mock_server.js
@@ -223,7 +223,8 @@ MockServer.include({
             return this._mockMailMessageModerate(args);
         }
         if (args.model === 'mail.message' && args.method === 'set_message_done') {
-            return this._mockMailMessageSetMessageDone(args);
+            const ids = args.args[0];
+            return this._mockMailMessageSetMessageDone(ids);
         }
         if (args.model === 'mail.message' && args.method === 'toggle_message_starred') {
             const ids = args.args[0];
@@ -389,10 +390,7 @@ MockServer.include({
         const currentPartner = this._getRecords('res.partner', [['id', '=', this.currentPartnerId]])[0];
         const currentPartnerFormat = this._mockResPartnerMailPartnerFormat(currentPartner.id);
 
-        const needaction_inbox_counter = this._getRecords('mail.notification', [
-            ['res_partner_id', '=', this.currentPartnerId],
-            ['is_read', '=', false],
-        ]).length;
+        const needaction_inbox_counter = this._mockResPartnerGetNeedactionCount();
 
         const mailFailures = this._mockMailMessageMessageFetchFailed();
 
@@ -1076,9 +1074,9 @@ MockServer.include({
         ];
         if (domain) {
             const messages = this._getRecords('mail.message', domain);
-            notifDomain.push(
-                ['mail_message_id', 'in', messages.map(messages => messages.id)]
-            );
+            const ids = messages.map(messages => messages.id);
+            this._mockMailMessageSetMessageDone(ids);
+            return ids;
         }
         const notifications = this._getRecords('mail.notification', notifDomain);
         this._mockWrite('mail.notification', [
@@ -1104,7 +1102,7 @@ MockServer.include({
                 },
             ]);
         }
-        const notificationData = { type: 'mark_as_read', message_ids: messageIds };
+        const notificationData = { type: 'mark_as_read', message_ids: messageIds, needaction_inbox_counter: this._mockResPartnerGetNeedactionCount() };
         const notification = [[false, 'res.partner', this.currentPartnerId], notificationData];
         this._widget.call('bus_service', 'trigger', 'notification', [notification]);
         return messageIds;
@@ -1307,10 +1305,9 @@ MockServer.include({
      * messages have been marked as read, so that UI is updated.
      *
      * @private
-     * @param {Object} args
+     * @param {integer[]} ids
      */
-    _mockMailMessageSetMessageDone(args) {
-        const ids = args.args[0];
+    _mockMailMessageSetMessageDone(ids) {
         const messages = this._getRecords('mail.message', [['id', 'in', ids]]);
 
         const notifications = this._getRecords('mail.notification', [
@@ -1335,7 +1332,7 @@ MockServer.include({
             ]);
             // NOTE server is sending grouped notifications per channel_ids but
             // this optimization is not needed here.
-            const data = { type: 'mark_as_read', message_ids: [message.id], channel_ids: message.channel_ids };
+            const data = { type: 'mark_as_read', message_ids: [message.id], channel_ids: message.channel_ids, needaction_inbox_counter: this._mockResPartnerGetNeedactionCount() };
             const busNotifications = [[[false, 'res.partner', this.currentPartnerId], data]];
             this._widget.call('bus_service', 'trigger', 'notification', busNotifications);
         }
@@ -1730,6 +1727,17 @@ MockServer.include({
         return [mainMatchingPartners, extraMatchingPartners];
     },
     /**
+     * Simulates `get_needaction_count` on `res.partner`.
+     *
+     * @private
+     */
+    _mockResPartnerGetNeedactionCount() {
+        return this._getRecords('mail.notification', [
+            ['res_partner_id', '=', this.currentPartnerId],
+            ['is_read', '=', false],
+        ]).length;
+    },
+    /**
      * Simulates `im_search` on `res.partner`.
      *
      * @private