From 4ca549253be2c099294f05d8ba57384e1b7bc0d6 Mon Sep 17 00:00:00 2001
From: Xavier Dubuc <xdu@odoo.com>
Date: Thu, 11 Jun 2020 15:19:26 +0000
Subject: [PATCH] [FIX] mail: mark message as seen only when thread composer is
 focused
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

task-2272510

closes odoo/odoo#53088

X-original-commit: edf356c2022b3039e53e82a092fc64533f59df27
Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>
---
 .../src/components/composer/composer.js       | 24 -----
 .../composer_text_input.js                    | 14 +++
 .../composer_text_input.xml                   |  2 +-
 .../static/src/components/discuss/discuss.xml |  1 -
 .../src/components/discuss/discuss_tests.js   | 85 +++++++++--------
 .../components/message_list/message_list.js   | 26 ++---
 .../thread_viewer/thread_viewer.xml           |  1 -
 .../thread_viewer/thread_viewer_tests.js      | 95 ++++++++++++++++---
 .../static/src/models/composer/composer.js    |  3 +
 .../mail/static/src/models/thread/thread.js   | 18 +++-
 .../src/models/thread_viewer/thread_viewer.js | 56 ++++++++++-
 11 files changed, 228 insertions(+), 97 deletions(-)

diff --git a/addons/mail/static/src/components/composer/composer.js b/addons/mail/static/src/components/composer/composer.js
index f2135835a4d..63d85a0fb95 100644
--- a/addons/mail/static/src/components/composer/composer.js
+++ b/addons/mail/static/src/components/composer/composer.js
@@ -48,35 +48,13 @@ class Composer extends Component {
          * Reference of the text input component.
          */
         this._textInputRef = useRef('textInput');
-        /**
-         * Tracked focus counter from props. Useful to determine whether it
-         * should auto focus this composer when patched.
-         */
-        this._focusCount = 0;
-        // to focus if the prop changes
-        this._lastComposer = undefined;
         this._onClickCaptureGlobal = this._onClickCaptureGlobal.bind(this);
     }
 
     mounted() {
-        this._lastComposer = this.composer;
-        if (this.props.isFocusOnMount) {
-            this.focus();
-        }
         document.addEventListener('click', this._onClickCaptureGlobal, true);
     }
 
-    patched() {
-        // focus when changing composer
-        if (
-            this.props.isFocusOnMount &&
-            this._lastComposer !== this.composer
-        ) {
-            this.focus();
-        }
-        this._lastComposer = this.composer;
-    }
-
     willUnmount() {
         document.removeEventListener('click', this._onClickCaptureGlobal, true);
     }
@@ -338,7 +316,6 @@ Object.assign(Composer, {
         isCompact: true,
         isDiscardOnClickAway: false,
         isExpandable: false,
-        isFocusOnMount: false,
         isLog: false,
     },
     props: {
@@ -382,7 +359,6 @@ Object.assign(Composer, {
         isCompact: Boolean,
         isDiscardOnClickAway: Boolean,
         isExpandable: Boolean,
-        isFocusOnMount: Boolean,
         isLog: Boolean,
     },
     template: 'mail.Composer',
diff --git a/addons/mail/static/src/components/composer_text_input/composer_text_input.js b/addons/mail/static/src/components/composer_text_input/composer_text_input.js
index 198f563d1c7..1be0e77a98b 100644
--- a/addons/mail/static/src/components/composer_text_input/composer_text_input.js
+++ b/addons/mail/static/src/components/composer_text_input/composer_text_input.js
@@ -158,6 +158,20 @@ class ComposerTextInput extends Component {
     // Handlers
     //--------------------------------------------------------------------------
 
+    /**
+     * @private
+     */
+    _onFocusinTextarea() {
+        this.composer.update({ hasFocus: true });
+    }
+
+    /**
+     * @private
+     */
+    _onFocusoutTextarea() {
+        this.composer.update({ hasFocus: false });
+    }
+
     /**
      * @private
      */
diff --git a/addons/mail/static/src/components/composer_text_input/composer_text_input.xml b/addons/mail/static/src/components/composer_text_input/composer_text_input.xml
index f68b295cb8d..949f1065a56 100644
--- a/addons/mail/static/src/components/composer_text_input/composer_text_input.xml
+++ b/addons/mail/static/src/components/composer_text_input/composer_text_input.xml
@@ -36,7 +36,7 @@
                         </div>
                     </div>
                 </t>
-                <textarea class="o_ComposerTextInput_textarea" t-esc="composer.textInputContent" placeholder="Write something..." t-on-keydown="_onKeydownTextarea" t-on-keyup="_onKeyupTextarea" t-on-input="_onInputTextarea" t-ref="textarea"/>
+                <textarea class="o_ComposerTextInput_textarea" t-esc="composer.textInputContent" placeholder="Write something..." t-on-focusin="_onFocusinTextarea" t-on-focusout="_onFocusoutTextarea" t-on-keydown="_onKeydownTextarea" t-on-keyup="_onKeyupTextarea" t-on-input="_onInputTextarea" t-ref="textarea"/>
             </t>
         </div>
     </t>
diff --git a/addons/mail/static/src/components/discuss/discuss.xml b/addons/mail/static/src/components/discuss/discuss.xml
index 789c7a465c8..f20f08b409b 100644
--- a/addons/mail/static/src/components/discuss/discuss.xml
+++ b/addons/mail/static/src/components/discuss/discuss.xml
@@ -97,7 +97,6 @@
                 hasDiscardButton="true"
                 hasThreadName="true"
                 isDiscardOnClickAway="true"
-                isFocusOnMount="true"
                 t-on-o-message-posted="_onReplyingToMessageMessagePosted"
             />
         </t>
diff --git a/addons/mail/static/src/components/discuss/discuss_tests.js b/addons/mail/static/src/components/discuss/discuss_tests.js
index 7a37349041f..5fe7f90c142 100644
--- a/addons/mail/static/src/components/discuss/discuss_tests.js
+++ b/addons/mail/static/src/components/discuss/discuss_tests.js
@@ -1951,7 +1951,7 @@ QUnit.test('new messages separator', async function (assert) {
     // visible. This is necessary in order to display 'new messages' and not
     // remove from DOM right away from seeing last message.
     // AKU TODO: thread specific test
-    assert.expect(5);
+    assert.expect(6);
 
     let step = 0;
     Object.assign(this.data.initMessaging, {
@@ -2000,26 +2000,26 @@ QUnit.test('new messages separator', async function (assert) {
             return this._super(...arguments);
         },
     });
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_Discuss_thread .o_ThreadViewer_messageList .o_MessageList_message
-        `).length,
+    assert.containsN(
+        document.body,
+        '.o_MessageList_message',
         25,
         "should have 25 messages"
     );
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_Discuss_thread .o_ThreadViewer_messageList .o_MessageList_separatorNewMessages
-        `).length,
-        0,
+    assert.containsNone(
+        document.body,
+        '.o_MessageList_separatorNewMessages',
         "should not display 'new messages' separator"
     );
 
+    // scroll to top
     document.querySelector(`.o_Discuss_thread .o_ThreadViewer_messageList`).scrollTop = 0;
+    // composer is focused by default, we remove that focus
+    document.querySelector('.o_ComposerTextInput_textarea').blur();
     // simulate receiving a new message
     const data = {
         author_id: [36, "User26"],
-        body: "<p>boddy26</p>",
+        body: "<p>body26</p>",
         channel_ids: [20],
         date: "2019-04-20 10:00:00",
         id: 126,
@@ -2030,33 +2030,36 @@ QUnit.test('new messages separator', async function (assert) {
     };
     await afterNextRender(() => {
         const notifications = [[['my-db', 'mail.channel', 20], data]];
-        this.widget.call('bus_service', 'trigger', 'notification', notifications);
+        this.widget.call('bus_service', 'trigger', 'notification', notifications)
     });
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_Discuss_thread .o_ThreadViewer_messageList .o_MessageList_message
-        `).length,
+    assert.containsN(
+        document.body,
+        '.o_MessageList_message',
         26,
         "should have 26 messages"
     );
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_Discuss_thread .o_ThreadViewer_messageList .o_MessageList_separatorNewMessages
-        `).length,
-        1,
+    assert.containsOnce(
+        document.body,
+        '.o_MessageList_separatorNewMessages',
         "should display 'new messages' separator"
     );
 
-    // scroll to bottom
     await afterNextRender(() => {
         document.querySelector(`.o_Discuss_thread .o_ThreadViewer_messageList`).scrollTop =
             document.querySelector(`.o_Discuss_thread .o_ThreadViewer_messageList`).scrollHeight;
     });
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_Discuss_thread .o_ThreadViewer_messageList .o_MessageList_separatorNewMessages
-        `).length,
-        0,
+
+    assert.containsOnce(
+        document.body,
+        '.o_MessageList_separatorNewMessages',
+        "should still display 'new messages' separator as composer is not focused"
+    );
+    await afterNextRender(() =>
+        document.querySelector('.o_ComposerTextInput_textarea').focus()
+    );
+    assert.containsNone(
+        document.body,
+        '.o_MessageList_separatorNewMessages',
         "should no longer display 'new messages' separator (message seen)"
     );
 });
@@ -3491,19 +3494,17 @@ QUnit.test('mark channel as seen on last message visible', async function (asser
             return this._super(...arguments);
         },
     });
-    assert.strictEqual(
-        document.querySelectorAll(`
-            .o_DiscussSidebar_item[data-thread-local-id="${
-                this.env.models['mail.thread'].find(thread =>
-                    thread.id === 10 &&
-                    thread.model === 'mail.channel'
-                ).localId
-            }"]
-        `).length,
-        1,
+    assert.containsOnce(
+        document.body,
+        `.o_DiscussSidebar_item[data-thread-local-id="${
+            this.env.models['mail.thread'].find(thread =>
+                thread.id === 10 &&
+                thread.model === 'mail.channel'
+            ).localId
+        }"]`,
         "should have discuss sidebar item with the channel"
     );
-    assert.ok(
+    assert.hasClass(
         document.querySelector(`
             .o_DiscussSidebar_item[data-thread-local-id="${
                 this.env.models['mail.thread'].find(thread =>
@@ -3511,7 +3512,8 @@ QUnit.test('mark channel as seen on last message visible', async function (asser
                     thread.model === 'mail.channel'
                 ).localId
             }"]
-        `).classList.contains('o-unread'),
+        `),
+        'o-unread',
         "sidebar item of channel ID 10 should be unread"
     );
 
@@ -3525,7 +3527,7 @@ QUnit.test('mark channel as seen on last message visible', async function (asser
             }"]
         `).click()
     );
-    assert.notOk(
+    assert.doesNotHaveClass(
         document.querySelector(`
             .o_DiscussSidebar_item[data-thread-local-id="${
                 this.env.models['mail.thread'].find(thread =>
@@ -3533,7 +3535,8 @@ QUnit.test('mark channel as seen on last message visible', async function (asser
                     thread.model === 'mail.channel'
                 ).localId
             }"]
-        `).classList.contains('o-unread'),
+        `),
+        'o-unread',
         "sidebar item of channel ID 10 should not longer be unread"
     );
 });
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 c948689c6d1..f9106fd73c1 100644
--- a/addons/mail/static/src/components/message_list/message_list.js
+++ b/addons/mail/static/src/components/message_list/message_list.js
@@ -374,23 +374,17 @@ class MessageList extends Component {
     /**
      * @private
      */
-    _checkThreadMarkAsRead() {
+    _checkMostRecentMessageIsVisible() {
         const thread = this.threadViewer.thread;
         const threadCache = this.threadViewer.threadCache;
-        if (!threadCache) {
-            return;
-        }
-        if (threadCache.messages.length === 0) {
-            return;
-        }
-        if (!this.mostRecentMessageRef) {
-            return;
-        }
-        if (
+        const lastMessageIsVisible =
+            threadCache &&
+            threadCache.messages.length > 0 &&
+            this.mostRecentMessageRef &&
             threadCache === thread.mainCache &&
-            this.mostRecentMessageRef.isPartiallyVisible()
-        ) {
-            thread.markAsSeen();
+            this.mostRecentMessageRef.isPartiallyVisible();
+        if (lastMessageIsVisible) {
+            this.threadViewer.handleVisibleMessage(this.mostRecentMessageRef.message);
         }
     }
 
@@ -460,7 +454,7 @@ class MessageList extends Component {
      * @private
      */
     _update() {
-        this._checkThreadMarkAsRead();
+        this._checkMostRecentMessageIsVisible();
     }
 
     //--------------------------------------------------------------------------
@@ -492,7 +486,7 @@ class MessageList extends Component {
         if (this._isLoadMoreVisible()) {
             this._loadMore();
         }
-        this._checkThreadMarkAsRead();
+        this._checkMostRecentMessageIsVisible();
     }
 
 }
diff --git a/addons/mail/static/src/components/thread_viewer/thread_viewer.xml b/addons/mail/static/src/components/thread_viewer/thread_viewer.xml
index e292ac4e4ec..331d61c86b0 100644
--- a/addons/mail/static/src/components/thread_viewer/thread_viewer.xml
+++ b/addons/mail/static/src/components/thread_viewer/thread_viewer.xml
@@ -31,7 +31,6 @@
                         hasCurrentPartnerAvatar="props.hasComposerCurrentPartnerAvatar"
                         hasSendButton="props.hasComposerSendButton"
                         hasThreadTyping="props.hasComposerThreadTyping"
-                        isFocusOnMount="!env.messaging.device.isMobile"
                         showAttachmentsExtensions="props.showComposerAttachmentsExtensions"
                         showAttachmentsFilenames="props.showComposerAttachmentsFilenames"
                         t-ref="composer"
diff --git a/addons/mail/static/src/components/thread_viewer/thread_viewer_tests.js b/addons/mail/static/src/components/thread_viewer/thread_viewer_tests.js
index 1c7d24a86e1..259e877cb34 100644
--- a/addons/mail/static/src/components/thread_viewer/thread_viewer_tests.js
+++ b/addons/mail/static/src/components/thread_viewer/thread_viewer_tests.js
@@ -286,8 +286,8 @@ QUnit.test('message list asc order', async function (assert) {
     assert.verifySteps(['message_fetch_1', 'message_fetch_2']);
 });
 
-QUnit.test('mark channel as fetched when a new message is loaded and as seen when message is visible', async function (assert) {
-    assert.expect(7);
+QUnit.test('mark channel as fetched when a new message is loaded and as seen when focusing composer', async function (assert) {
+    assert.expect(8);
 
     await this.start({
         mockRPC(route, args) {
@@ -302,7 +302,7 @@ QUnit.test('mark channel as fetched when a new message is loaded and as seen whe
                     'mail.channel',
                     'channel_fetched is called on the right channel model'
                 );
-                assert.step('rpc_channel_fetched:called');
+                assert.step('rpc:channel_fetch');
             } else if (args.method === 'channel_seen') {
                 assert.strictEqual(
                     args.args[0][0],
@@ -314,7 +314,79 @@ QUnit.test('mark channel as fetched when a new message is loaded and as seen whe
                     'mail.channel',
                     'channel_seeb is called on the right channel model'
                 );
-                assert.step('rpc_channel_seen:called');
+                assert.step('rpc:channel_seen');
+            }
+            return this._super(...arguments);
+        }
+    });
+    const thread = this.env.models['mail.thread'].create({
+        // FIXME should not be needed, see task-2277537
+        composer: [['create']], // avoid initial focus
+        id: 100,
+        isPinned: true, // just to avoid joinChannel to be called
+        members: [['insert', [
+            {
+                email: "john@example.com",
+                id: this.env.session.partner_id,
+                name: "John",
+            },
+            {
+                email: "fred@example.com",
+                id: 10,
+                name: "Fred",
+            },
+        ]]],
+        message_unread_counter: 1, // seen would not be called if not > 0
+        model: 'mail.channel',
+    });
+
+    const threadViewer = this.env.models['mail.thread_viewer'].create({ thread: [['link', thread]] });
+    await this.createThreadViewerComponent(threadViewer, { hasComposer: true });
+    const notifications = [
+        [['myDB', 'mail.channel', 100], {
+            channelId: 100,
+            id: 1,
+            body: "<p>fdsfsd</p>",
+            author_id: [10, "Fred"],
+            model: "mail.channel",
+            channel_ids: [100],
+        }]
+    ];
+
+    await afterNextRender(() =>
+        this.widget.call('bus_service', 'trigger', 'notification', notifications)
+    );
+    assert.verifySteps(
+        ['rpc:channel_fetch'],
+        "Channel should have been fetched but not seen yet"
+    );
+
+    await afterNextRender(() => thread.composer.focus());
+    assert.verifySteps(
+        ['rpc:channel_seen'],
+        "Channel should have been marked as seen after threadViewer got the focus"
+    );
+});
+
+QUnit.test('mark channel as fetched and seen when a new message is loaded if composer is focused', async function (assert) {
+    assert.expect(4);
+
+    await this.start({
+        mockRPC(route, args) {
+            if (args.method === 'channel_fetched' && args.args[0] === 100) {
+                throw new Error("'channel_fetched' RPC must not be called for created channel as message is directly seen");
+            } else if (args.method === 'channel_seen') {
+                assert.strictEqual(
+                    args.args[0][0],
+                    100,
+                    'channel_seen is called on the right channel id'
+                );
+                assert.strictEqual(
+                    args.model,
+                    'mail.channel',
+                    'channel_seen is called on the right channel model'
+                );
+                assert.step('rpc:channel_seen');
             }
             return this._super(...arguments);
         }
@@ -334,10 +406,12 @@ QUnit.test('mark channel as fetched when a new message is loaded and as seen whe
                 name: "Fred",
             },
         ]]],
+        message_unread_counter: 1, // seen would not be called if not > 0
         model: 'mail.channel',
     });
+
     const threadViewer = this.env.models['mail.thread_viewer'].create({ thread: [['link', thread]] });
-    await this.createThreadViewerComponent(threadViewer);
+    await this.createThreadViewerComponent(threadViewer, { hasComposer: true });
     const notifications = [
         [['myDB', 'mail.channel', 100], {
             channelId: 100,
@@ -348,15 +422,14 @@ QUnit.test('mark channel as fetched when a new message is loaded and as seen whe
             channel_ids: [100],
         }]
     ];
-    await afterNextRender(() => {
-        this.widget.call('bus_service', 'trigger', 'notification', notifications);
-    });
+    await afterNextRender(() =>
+        this.widget.call('bus_service', 'trigger', 'notification', notifications)
+    );
     assert.verifySteps(
-        ['rpc_channel_fetched:called', 'rpc_channel_seen:called'],
-        "Channel should have been fetched before having been seen"
+        ['rpc:channel_seen'],
+        "Channel should have been mark as seen directly"
     );
 });
-
 });
 });
 });
diff --git a/addons/mail/static/src/models/composer/composer.js b/addons/mail/static/src/models/composer/composer.js
index 343e0d372c0..fa6e2698eb0 100644
--- a/addons/mail/static/src/models/composer/composer.js
+++ b/addons/mail/static/src/models/composer/composer.js
@@ -554,6 +554,9 @@ function factory(dependencies) {
                 'mainSuggestedPartners',
             ],
         }),
+        hasFocus: attr({
+            default: false,
+        }),
         hasSuggestedPartners: attr({
             compute: '_computeHasSuggestedPartners',
             dependencies: [
diff --git a/addons/mail/static/src/models/thread/thread.js b/addons/mail/static/src/models/thread/thread.js
index 16f52ab861f..e1fd74a5d77 100644
--- a/addons/mail/static/src/models/thread/thread.js
+++ b/addons/mail/static/src/models/thread/thread.js
@@ -10,6 +10,23 @@ function factory(dependencies) {
 
     class Thread extends dependencies['mail.model'] {
 
+        /**
+         * FIXME With this, whenever client is aware of new thread, this will
+         * (almost) always focus its composer when displayed. This shouldn't be
+         * the case, instead auto-focus of composer is flow-specific.
+         * See task-2277537
+         *
+         * @override
+         */
+        static create(data) {
+            if (!data.composer) {
+                data.composer = [['create', {
+                    isDoFocus: true,
+                }]];
+            }
+            return super.create(data);
+        }
+
         /**
          * @override
          */
@@ -1140,7 +1157,6 @@ function factory(dependencies) {
             dependencies: ['viewersChatWindow'],
         }),
         composer: one2one('mail.composer', {
-            autocreate: true,
             inverse: 'thread',
             isCausal: true,
         }),
diff --git a/addons/mail/static/src/models/thread_viewer/thread_viewer.js b/addons/mail/static/src/models/thread_viewer/thread_viewer.js
index 4da88fd9513..834c07d878a 100644
--- a/addons/mail/static/src/models/thread_viewer/thread_viewer.js
+++ b/addons/mail/static/src/models/thread_viewer/thread_viewer.js
@@ -67,6 +67,15 @@ function factory(dependencies) {
             });
         }
 
+        /**
+         * @param {mail.message} message
+         */
+        handleVisibleMessage(message) {
+            if (!this.lastVisibleMessage || this.lastVisibleMessage.id < message.id) {
+                this.update({ lastVisibleMessage: [['replace', message]] });
+            }
+        }
+
         /**
          * @param {string} scrollTop
          */
@@ -96,6 +105,24 @@ function factory(dependencies) {
             return [['replace', this.thread.cache(this.stringifiedDomain)]];
         }
 
+        /**
+         * Not a real field, used to trigger `thread.markAsSeen` when one of
+         * the dependencies changes.
+         *
+         * @private
+         * @returns {boolean}
+         */
+        _computeThreadShouldBeSetAsSeen() {
+            // FIXME condition should not be on "composer is focused" but "thread viewer is active"
+            // See task-2277543
+            const lastMessageIsVisible = this.lastVisibleMessage &&
+                this.lastVisibleMessage === this.lastMessage;
+            if (lastMessageIsVisible && this.hasComposerFocus && this.thread) {
+                this.thread.markAsSeen();
+            }
+            return true;
+        }
+
         /**
          * @private
          * @returns {integer|undefined}
@@ -171,7 +198,7 @@ function factory(dependencies) {
             }
 
             if (
-                this.thread && this.threadCache.isLoading &&
+                this.thread && this.threadCache && this.threadCache.isLoading &&
                 !this.isShowingLoading && !this._isPreparingLoading
             ) {
                 this._prepareLoading();
@@ -218,6 +245,20 @@ function factory(dependencies) {
         componentHintList: attr({
             default: [],
         }),
+        composer: many2one('mail.composer', {
+            related: 'thread.composer',
+        }),
+        hasComposerFocus: attr({
+            related: 'composer.hasFocus',
+        }),
+        lastMessage: many2one('mail.message', {
+            related: 'threadCache.lastMessage',
+        }),
+        /**
+         * Most recent message in the current thread viewer that has been shown
+         * to the current partner.
+         */
+        lastVisibleMessage: many2one('mail.message'),
         messages: many2many('mail.message', {
             related: 'threadCache.messages',
         }),
@@ -255,6 +296,19 @@ function factory(dependencies) {
         threadCaches: many2many('mail.thread_cache', {
             related: 'thread.caches',
         }),
+        /**
+         * Not a real field, used to trigger `thread.markAsSeen` when one of
+         * the dependencies changes.
+         */
+        threadShouldBeSetAsSeen: attr({
+            compute: '_computeThreadShouldBeSetAsSeen',
+            dependencies: [
+                'hasComposerFocus',
+                'lastMessage',
+                'lastVisibleMessage',
+                'threadCache',
+            ],
+        }),
         uncheckedMessages: many2many('mail.message', {
             related: 'threadCache.uncheckedMessages',
         }),