diff options
| author | Sébastien Theys <seb@odoo.com> | 2020-09-23 10:50:45 +0000 |
|---|---|---|
| committer | Sébastien Theys <seb@odoo.com> | 2020-09-23 17:08:02 +0000 |
| commit | db208c76f7de734fdcaf977412137bd9ea0d82ac (patch) | |
| tree | 7f03fbbabef9abfc043e91c69ed4f640c0d90cae | |
| parent | b9d4ccfc8db1f3425b8ee0e8d6311ebaac0f26b2 (diff) | |
[FIX] mail: prevent unnecessary fold state RPC
When the pending value is the same as the current server value, the pending
value should be ignored and no RPC should be done.
In particular this prevents a non-main tab from calling the RPC when receiving
a bus notification that would lead to closing a chat window, which in turn used
to call the close RPC again before this commit.
This led to inconsistencies especially on the main tab, if the chat window was
requested to be open again after the initial close, but before the "secondary"
close, it would end up closing instead of opening.
The current commit reduces the issue and prevents as many unnecessary RPC as
there are tabs open, which is a good improvement.
But it does not negate it completely. Indeed, when quickly closing then opening
the same chat window, it might still happen for network reasons that the RPC or
the corresponding bus notifications are received in the wrong order. This is
less frequent both functionally (to click so fast) and technically (to have the
network that slow) at the same time, so it is considering acceptable / out of
scope.
The opportunity is taken to clean up the code by removing unnecessary fields and
making use of compute and flux specific RPC instead of `_updateAfter`.
task-2273658
task-2329521
closes odoo/odoo#58367
X-original-commit: cbb1d855c7af5f78819db3cd476dbe0f985bc214
Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>
Signed-off-by: Sébastien Theys (seb) <seb@odoo.com>
7 files changed, 217 insertions, 249 deletions
diff --git a/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js b/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js index 0401cef3809..d29730626b0 100644 --- a/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js +++ b/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js @@ -453,9 +453,8 @@ QUnit.test('chat window: basic rendering', async function (assert) { }); QUnit.test('chat window: fold', async function (assert) { - assert.expect(27); + assert.expect(9); - let foldCall = 0; // channel that is expected to be found in the messaging menu // with random UUID, will be asserted during the test this.data['mail.channel'].records.push({ uuid: 'channel-uuid' }); @@ -463,44 +462,6 @@ QUnit.test('chat window: fold', async function (assert) { mockRPC(route, args) { if (args.method === 'channel_fold') { assert.step(`rpc:${args.method}/${args.kwargs.state}`); - foldCall++; - const kwargsKeys = Object.keys(args.kwargs); - assert.strictEqual( - args.args.length, - 0, - "channel_fold call have no args" - ); - assert.strictEqual( - kwargsKeys.length, - 2, - "channel_fold call have exactly 2 kwargs" - ); - assert.ok( - kwargsKeys.includes('state'), - "channel_fold call have 'state' kwargs" - ); - assert.ok( - kwargsKeys.includes('uuid'), - "channel_fold call have 'uuid' kwargs" - ); - assert.strictEqual( - args.kwargs.uuid, - 'channel-uuid', - "channel_fold call uuid is from channel 20" - ); - if (foldCall % 2 === 0) { - assert.strictEqual( - args.kwargs.state, - 'folded', - "channel_fold call state is 'folded'" - ); - } else { - assert.strictEqual( - args.kwargs.state, - 'open', - "channel_fold call state is 'open'" - ); - } } return this._super(...arguments); }, @@ -515,11 +476,17 @@ QUnit.test('chat window: fold', async function (assert) { '.o_ChatWindow_thread', "chat window should have a thread" ); - assert.verifySteps(['rpc:channel_fold/open']); + assert.verifySteps( + ['rpc:channel_fold/open'], + "should sync fold state 'open' with server after opening chat window" + ); // Fold chat window await afterNextRender(() => document.querySelector(`.o_ChatWindow_header`).click()); - assert.verifySteps(['rpc:channel_fold/folded']); + assert.verifySteps( + ['rpc:channel_fold/folded'], + "should sync fold state 'folded' with server after folding chat window" + ); assert.containsNone( document.body, '.o_ChatWindow_thread', @@ -528,7 +495,10 @@ QUnit.test('chat window: fold', async function (assert) { // Unfold chat window await afterNextRender(() => document.querySelector(`.o_ChatWindow_header`).click()); - assert.verifySteps(['rpc:channel_fold/open']); + assert.verifySteps( + ['rpc:channel_fold/open'], + "should sync fold state 'open' with server after unfolding chat window" + ); assert.containsOnce( document.body, '.o_ChatWindow_thread', @@ -537,9 +507,8 @@ QUnit.test('chat window: fold', async function (assert) { }); QUnit.test('chat window: open / close', async function (assert) { - assert.expect(24); + assert.expect(10); - let foldCall = 0; // channel that is expected to be found in the messaging menu // with random UUID, will be asserted during the test this.data['mail.channel'].records.push({ uuid: 'channel-uuid' }); @@ -547,64 +516,55 @@ QUnit.test('chat window: open / close', async function (assert) { mockRPC(route, args) { if (args.method === 'channel_fold') { assert.step(`rpc:channel_fold/${args.kwargs.state}`); - foldCall++; - const kwargsKeys = Object.keys(args.kwargs); - assert.strictEqual( - args.args.length, - 0, - "channel_fold call have no args" - ); - assert.strictEqual( - kwargsKeys.length, - 2, - "channel_fold call have exactly 2 kwargs" - ); - assert.ok( - kwargsKeys.includes('state'), - "channel_fold call have 'state' kwargs" - ); - assert.ok( - kwargsKeys.includes('uuid'), - "channel_fold call have 'uuid' kwargs" - ); - assert.strictEqual( - args.kwargs.uuid, - 'channel-uuid', - "channel_fold call uuid should be correct" - ); - if (foldCall % 2 === 0) { - assert.strictEqual( - args.kwargs.state, - 'closed', - "channel_fold call state is 'closed'" - ); - } else { - assert.strictEqual( - args.kwargs.state, - 'open', - "channel_fold call state is 'open'" - ); - } } return this._super(...arguments); }, }); + assert.containsNone( + document.body, + '.o_ChatWindow', + "should not have a chat window initially" + ); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_toggler`).click()); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_dropdownMenu .o_NotificationList_preview`).click() ); - assert.verifySteps(['rpc:channel_fold/open']); + assert.containsOnce( + document.body, + '.o_ChatWindow', + "should have a chat window after clicking on thread preview" + ); + assert.verifySteps( + ['rpc:channel_fold/open'], + "should sync fold state 'open' with server after opening chat window" + ); // Close chat window await afterNextRender(() => document.querySelector(`.o_ChatWindowHeader_commandClose`).click()); - assert.verifySteps(['rpc:channel_fold/closed']); + assert.containsNone( + document.body, + '.o_ChatWindow', + "should not have a chat window after closing it" + ); + assert.verifySteps( + ['rpc:channel_fold/closed'], + "should sync fold state 'closed' with server after closing chat window" + ); // Reopen chat window await afterNextRender(() => document.querySelector(`.o_MessagingMenu_toggler`).click()); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_dropdownMenu .o_NotificationList_preview`).click() ); - assert.verifySteps(['rpc:channel_fold/open']); + assert.containsOnce( + document.body, + '.o_ChatWindow', + "should have a chat window again after clicking on thread preview again" + ); + assert.verifySteps( + ['rpc:channel_fold/open'], + "should sync fold state 'open' with server after opening chat window again" + ); }); QUnit.test('chat window: close on ESCAPE', async function (assert) { diff --git a/addons/mail/static/src/components/discuss/discuss.xml b/addons/mail/static/src/components/discuss/discuss.xml index 39f09da0b5b..650c788d7e9 100644 --- a/addons/mail/static/src/components/discuss/discuss.xml +++ b/addons/mail/static/src/components/discuss/discuss.xml @@ -60,9 +60,7 @@ hasSquashCloseMessages="discuss.thread.model !== 'mail.box'" haveMessagesMarkAsReadIcon="discuss.thread === env.messaging.inbox" haveMessagesReplyIcon="discuss.thread === env.messaging.inbox" - selectedMessageLocalId=" - discuss.replyingToMessage and - discuss.replyingToMessage.localId" + selectedMessageLocalId="discuss.replyingToMessage and discuss.replyingToMessage.localId" threadViewLocalId="discuss.threadView.localId" t-on-o-rendered="_onThreadRendered" t-ref="threadView" diff --git a/addons/mail/static/src/components/thread_view/thread_view_tests.js b/addons/mail/static/src/components/thread_view/thread_view_tests.js index d93f911e04a..4aa2d195686 100644 --- a/addons/mail/static/src/components/thread_view/thread_view_tests.js +++ b/addons/mail/static/src/components/thread_view/thread_view_tests.js @@ -242,6 +242,21 @@ QUnit.test('message list asc order', async function (assert) { QUnit.test('mark channel as fetched when a new message is loaded and as seen when focusing composer [REQUIRE FOCUS]', async function (assert) { assert.expect(8); + this.data['res.partner'].records.push({ + email: "fred@example.com", + id: 10, + name: "Fred", + }); + this.data['res.users'].records.push({ + id: 10, + partner_id: 10, + }); + this.data['mail.channel'].records.push({ + channel_type: 'chat', + id: 100, + is_pinned: true, + members: [this.data.currentPartnerId, 10], + }); await this.start({ mockRPC(route, args) { if (args.method === 'channel_fetched') { @@ -272,44 +287,25 @@ QUnit.test('mark channel as fetched when a new message is loaded and as seen whe return this._super(...arguments); } }); - const thread = this.env.models['mail.thread'].create({ + const thread = this.env.models['mail.thread'].findFromIdentifyingData({ id: 100, - isServerPinned: true, // just to avoid joinChannel to be called - members: [['insert', [ - { - email: "john@example.com", - id: this.env.messaging.currentPartner.id, - name: "John", - }, - { - email: "fred@example.com", - id: 10, - name: "Fred", - }, - ]]], model: 'mail.channel', - serverMessageUnreadCounter: 1, // seen would not be called if not > 0 }); - const threadViewer = this.env.models['mail.thread_viewer'].create({ hasThreadView: true, thread: [['link', thread]], }); await this.createThreadViewComponent(threadViewer.threadView, { 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) - ); + await afterNextRender(async () => this.env.services.rpc({ + route: '/mail/chat_post', + params: { + context: { + mockedUserId: 10, + }, + uuid: thread.uuid, + message_content: "new message", + }, + })); assert.verifySteps( ['rpc:channel_fetch'], "Channel should have been fetched but not seen yet" diff --git a/addons/mail/static/src/models/chat_window/chat_window.js b/addons/mail/static/src/models/chat_window/chat_window.js index a197481037c..99745b583f9 100644 --- a/addons/mail/static/src/models/chat_window/chat_window.js +++ b/addons/mail/static/src/models/chat_window/chat_window.js @@ -37,12 +37,18 @@ function factory(dependencies) { /** * Close this chat window. + * + * @param {Object} [param0={}] + * @param {boolean} [param0.notifyServer=true] */ - close() { + close({ notifyServer = true } = {}) { const thread = this.thread; this.delete(); - if (thread) { - thread.update({ pendingFoldState: 'closed' }); + // Flux specific: 'closed' fold state should only be saved on the + // server when manually closing the chat window. Delete at destroy + // or sync from server value for example should not save the value. + if (thread && notifyServer) { + thread.notifyFoldStateToServer('closed'); } } @@ -74,21 +80,28 @@ function factory(dependencies) { } } - fold() { - if (this.thread) { - this.thread.update({ pendingFoldState: 'folded' }); - } else { - this.update({ isFolded: true }); + /** + * @param {Object} [param0={}] + * @param {boolean} [param0.notifyServer=true] + */ + fold({ notifyServer = true } = {}) { + this.update({ isFolded: true }); + // Flux specific: manually folding the chat window should save the + // new state on the server. + if (this.thread && notifyServer) { + this.thread.notifyFoldStateToServer('folded'); } } /** * Makes this chat window active, which consists of making it visible, * unfolding it, and focusing it. + * + * @param {Object} [options] */ - makeActive() { + makeActive(options) { this.makeVisible(); - this.unfold(); + this.unfold(options); this.focus(); } @@ -118,11 +131,16 @@ function factory(dependencies) { this.manager.shiftRight(this); } - unfold() { - if (this.thread) { - this.thread.update({ pendingFoldState: 'open' }); - } else { - this.update({ isFolded: false }); + /** + * @param {Object} [param0={}] + * @param {boolean} [param0.notifyServer=true] + */ + unfold({ notifyServer = true } = {}) { + this.update({ isFolded: false }); + // Flux specific: manually opening the chat window should save the + // new state on the server. + if (this.thread && notifyServer) { + this.thread.notifyFoldStateToServer('open'); } } @@ -362,17 +380,8 @@ function factory(dependencies) { }), /** * Determines whether `this` is folded. - * - * Note: writing this value directly only makes sense when `this.thread` - * is empty. State of chat window of a thread is entirely based on - * `thread.foldState`. */ isFolded: attr({ - compute: '_computeIsFolded', - dependencies: [ - 'thread', - 'threadFoldState', - ], default: false, }), /** @@ -406,13 +415,12 @@ function factory(dependencies) { * Determines the `mail.thread` that should be displayed by `this`. * If no `mail.thread` is linked, `this` is considered "new message". */ - thread: many2one('mail.thread'), + thread: one2one('mail.thread', { + inverse: 'chatWindow', + }), threadDisplayName: attr({ related: 'thread.displayName', }), - threadFoldState: attr({ - related: 'thread.foldState', - }), /** * States the `mail.thread_view` displaying `this.thread`. */ diff --git a/addons/mail/static/src/models/chat_window_manager/chat_window_manager.js b/addons/mail/static/src/models/chat_window_manager/chat_window_manager.js index 45561e6b039..88fb24c1b1c 100644 --- a/addons/mail/static/src/models/chat_window_manager/chat_window_manager.js +++ b/addons/mail/static/src/models/chat_window_manager/chat_window_manager.js @@ -74,11 +74,12 @@ function factory(dependencies) { * Closes all chat windows related to the given thread. * * @param {mail.thread} thread + * @param {Object} [options] */ - closeThread(thread) { + closeThread(thread, options) { for (const chatWindow of this.chatWindows) { if (chatWindow.thread === thread) { - chatWindow.close(); + chatWindow.close(options); } } } @@ -100,28 +101,42 @@ function factory(dependencies) { /** * @param {mail.thread} thread * @param {Object} [param1={}] + * @param {boolean} [param1.isFolded=false] * @param {boolean} [param1.makeActive=false] + * @param {boolean} [param1.notifyServer=true] * @param {boolean} [param1.replaceNewMessage=false] */ - openThread(thread, { makeActive = false, replaceNewMessage = false } = {}) { + openThread(thread, { + isFolded = false, + makeActive = false, + notifyServer = true, + replaceNewMessage = false + } = {}) { let chatWindow = this.chatWindows.find(chatWindow => chatWindow.thread === thread ); if (!chatWindow) { chatWindow = this.env.models['mail.chat_window'].create({ + isFolded, manager: [['link', this]], thread: [['link', thread]], }); - } - if (thread.foldState === 'closed') { - thread.update({ pendingFoldState: 'open' }); + } else { + chatWindow.update({ isFolded }); } if (replaceNewMessage && this.newMessageChatWindow) { this.swap(chatWindow, this.newMessageChatWindow); this.newMessageChatWindow.close(); } if (makeActive) { - chatWindow.makeActive(); + // avoid double notify at this step, it will already be done at + // the end of the current method + chatWindow.makeActive({ notifyServer: false }); + } + // Flux specific: notify server of chat window being opened. + if (notifyServer) { + const foldState = chatWindow.isFolded ? 'folded' : 'open'; + thread.notifyFoldStateToServer(foldState); } } diff --git a/addons/mail/static/src/models/thread/thread.js b/addons/mail/static/src/models/thread/thread.js index 01737253e6c..8e7aee26bc9 100644 --- a/addons/mail/static/src/models/thread/thread.js +++ b/addons/mail/static/src/models/thread/thread.js @@ -287,6 +287,25 @@ function factory(dependencies) { )); } + + /** + * Performs the `channel_fold` RPC on `mail.channel`. + * + * @static + * @param {string} uuid + * @param {string} state + */ + static async performRpcChannelFold(uuid, state) { + return this.env.services.rpc({ + model: 'mail.channel', + method: 'channel_fold', + kwargs: { + state, + uuid, + } + }, { shadow: true }); + } + /** * Performs the `channel_info` RPC on `mail.channel`. * @@ -579,21 +598,13 @@ function factory(dependencies) { } /** - * Notify server the fold state of this thread. Useful for cross-tab - * and cross-device chat window state synchronization. + * Notifies the server of new fold state. Useful for initial, + * cross-tab, and cross-device chat window state synchronization. * - * Only makes sense if pendingFoldState is set to the desired value. + * @param {string} state */ - notifyFoldStateToServer() { - // method is called from _updateAfter so it cannot be async - this.env.services.rpc({ - model: 'mail.channel', - method: 'channel_fold', - kwargs: { - uuid: this.uuid, - state: this.pendingFoldState, - } - }, { shadow: true }); + async notifyFoldStateToServer(state) { + return this.env.models['mail.thread'].performRpcChannelFold(this.uuid, state); } /** @@ -855,10 +866,8 @@ function factory(dependencies) { * Unsubscribe current user from provided channel. */ unsubscribe() { - this.update({ - pendingFoldState: 'closed', - isPendingPinned: false, - }); + this.env.messaging.chatWindowManager.closeThread(this); + this.update({ isPendingPinned: false }); } //---------------------------------------------------------------------- @@ -932,14 +941,6 @@ function factory(dependencies) { /** * @private - * @returns {string} - */ - _computeFoldState() { - return this.pendingFoldState || this.serverFoldState; - } - - /** - * @private */ _computeHasSeenIndicators() { if (this.model !== 'mail.channel') { @@ -1205,6 +1206,29 @@ function factory(dependencies) { } /** + * Handles change of fold state coming from the server. Useful to + * synchronize corresponding chat window. + * + * @private + */ + _onServerFoldStateChanged() { + if (!this.env.messaging.chatWindowManager) { + // avoid crash during destroy + return; + } + if (this.serverFoldState === 'closed') { + this.env.messaging.chatWindowManager.closeThread(this, { + notifyServer: false, + }); + } else { + this.env.messaging.chatWindowManager.openThread(this, { + isFolded: this.serverFoldState === 'folded', + notifyServer: false, + }); + } + } + + /** * @private * @param {Object} [param0={}] * @param {boolean} [param0.mail_invite_follower_channel_only=false] @@ -1240,52 +1264,18 @@ function factory(dependencies) { */ _updateAfter(previous) { if (this.model !== 'mail.channel') { - // fold state only makes sense on channels + // pin state only makes sense on channels return; } if ( - this.pendingFoldState && - previous.pendingFoldState !== this.pendingFoldState - ) { - this.notifyFoldStateToServer(); - } - if ( - this.serverFoldState === this.pendingFoldState - ) { - this.update({ pendingFoldState: clear() }); - } - if ( this.isPendingPinned !== undefined && previous.isPendingPinned !== this.isPendingPinned ) { this.notifyPinStateToServer(); } - if ( - this.isServerPinned === this.isPendingPinned - ) { + if (this.isServerPinned === this.isPendingPinned) { this.update({ isPendingPinned: clear() }); } - - // TODO FIXME prevent to open/close a channel on mobile when you - // open/close it on desktop (task-2267593) - - // chat window - if (previous.foldState === this.foldState) { - // avoid updating chatWindows when not changing foldState - // important to avoid issues when thread is in progress of being - // opened, because the foldState is updated only at the end of - // the process - return; - } - if (!this.env.messaging.chatWindowManager) { - // avoid crash during destroy - return; - } - if (this.foldState !== 'closed') { - this.env.messaging.chatWindowManager.openThread(this); - } else { - this.env.messaging.chatWindowManager.closeThread(this); - } } /** @@ -1293,9 +1283,7 @@ function factory(dependencies) { */ _updateBefore() { return { - foldState: this.foldState, isPendingPinned: this.isPendingPinned, - pendingFoldState: this.pendingFoldState, }; } @@ -1357,6 +1345,20 @@ function factory(dependencies) { isCausal: true, }), channel_type: attr(), + /** + * States the `mail.chat_window` related to `this`. Serves as compute + * dependency. It is computed from the inverse relation and it should + * otherwise be considered read-only. + */ + chatWindow: one2one('mail.chat_window', { + inverse: 'thread', + }), + /** + * Serves as compute dependency. + */ + chatWindowIsFolded: attr({ + related: 'chatWindow.isFolded', + }), composer: one2one('mail.composer', { default: [['create']], inverse: 'thread', @@ -1389,22 +1391,6 @@ function factory(dependencies) { 'name', ], }), - /** - * Determine the fold state of the channel on the web client. - * - * If there is a pending fold state change, it is immediately applied on - * the interface to avoid a feeling of unresponsiveness. Otherwise the - * last known fold state of the server is used. - * - * This field must be considered read only. - */ - foldState: attr({ - compute: '_computeFoldState', - dependencies: [ - 'pendingFoldState', - 'serverFoldState', - ], - }), followersPartner: many2many('mail.partner', { related: 'followers.partner', }), @@ -1577,6 +1563,16 @@ function factory(dependencies) { compute: '_computeNeedactionMessages', dependencies: ['messages'], }), + /** + * Not a real field, used to trigger `_onServerFoldStateChanged` when one of + * the dependencies changes. + */ + onServerFoldStateChanged: attr({ + compute: '_onServerFoldStateChanged', + dependencies: [ + 'serverFoldState', + ], + }), orderedMessages: many2many('mail.message', { compute: '_computeOrderedMessages', dependencies: ['messages'], @@ -1614,15 +1610,6 @@ function factory(dependencies) { isCausal: true, }), /** - * Determine if there is a pending fold state change, which is a change - * of fold state requested by the client but not yet confirmed by the - * server. - * - * This field can be updated to immediately change the fold state on the - * interface and to notify the server of the new state. - */ - pendingFoldState: attr(), - /** * Determine if there is a pending seen message change, which is a change * of seen message requested by the client but not yet confirmed by the * server. diff --git a/addons/mail/static/tests/helpers/mock_server.js b/addons/mail/static/tests/helpers/mock_server.js index 02862b9a5e5..5aaafff59eb 100644 --- a/addons/mail/static/tests/helpers/mock_server.js +++ b/addons/mail/static/tests/helpers/mock_server.js @@ -660,10 +660,13 @@ MockServer.include({ */ _mockMailChannelChannelFold(uuid, state) { const channel = this._getRecords('mail.channel', [['uuid', '=', uuid]])[0]; - this._mockWrite('mail.channel', [channel.id], { - is_minimized: state !== 'closed', - state, - }); + this._mockWrite('mail.channel', [ + [channel.id], + { + is_minimized: state !== 'closed', + state, + } + ]); const notifConfirmFold = [ ["dbName", 'res.partner', this.currentPartnerId], this._mockMailChannelChannelInfo([channel.id])[0] @@ -850,9 +853,10 @@ MockServer.include({ const channels = this._getRecords('mail.channel', [['id', 'in', ids]]); if (commandName === 'leave') { for (const channel of channels) { - this._mockWrite('mail.channel', [channel.id], { - is_pinned: false, - }); + this._mockWrite('mail.channel', [ + [channel.id], + { is_pinned: false }, + ]); const notifConfirmUnpin = [ ["dbName", 'res.partner', this.currentPartnerId], Object.assign({}, channel, { info: 'unsubscribe' }) |
