From b503c7f9f0cfe6ab2043eeaa22accf82ab7f54c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Wed, 7 Oct 2020 15:47:14 +0000 Subject: [PATCH] [FIX] mail: avoid useless (un)link when using replace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Side effect of this change: dependencies are not longer triggered when replace is not actually doing anything. task-2270780 closes odoo/odoo#59515 X-original-commit: 75a44675873d65fdf17f2205772b05a0f59bb42c Signed-off-by: Alexandre Kühn (aku) --- .../thread_needaction_preview_tests.js | 139 ++++++++++++------ addons/mail/static/src/model/model_field.js | 85 ++++++++--- .../mail/static/src/models/thread/thread.js | 3 + .../src/models/thread_cache/thread_cache.js | 4 + .../mail/static/tests/helpers/mock_server.js | 9 +- 5 files changed, 169 insertions(+), 71 deletions(-) diff --git a/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview_tests.js b/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview_tests.js index ff7d6e4cc08..9e2f19ffbaf 100644 --- a/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview_tests.js +++ b/addons/mail/static/src/components/thread_needaction_preview/thread_needaction_preview_tests.js @@ -30,9 +30,10 @@ QUnit.module('thread_needaction_preview_tests.js', { }; this.start = async params => { - const { env, widget } = await start(Object.assign({}, params, { + const { afterEvent, env, widget } = await start(Object.assign({}, params, { data: this.data, })); + this.afterEvent = afterEvent; this.env = env; this.widget = widget; }; @@ -45,8 +46,22 @@ QUnit.module('thread_needaction_preview_tests.js', { QUnit.test('mark as read', async function (assert) { assert.expect(4); + this.data['mail.message'].records.push({ + id: 21, + model: 'res.partner', + needaction: true, + needaction_partner_ids: [this.data.currentPartnerId], + res_id: 11, + }); + this.data['mail.notification'].records.push({ + mail_message_id: 21, + notification_status: 'sent', + notification_type: 'inbox', + res_partner_id: this.data.currentPartnerId, + }); await this.start({ hasChatWindow: true, + hasMessagingMenu: true, async mockRPC(route, args) { if (route.includes('set_message_done')) { assert.step('set_message_done'); @@ -54,18 +69,14 @@ QUnit.test('mark as read', async function (assert) { return this._super(...arguments); }, }); - const thread = this.env.models['mail.thread'].create({ - id: 11, - model: 'mail.channel', - }); - this.env.models['mail.message'].create({ - id: 21, - isNeedaction: true, - originThread: [['link', thread]], - }); - await this.createThreadNeedactionPreviewComponent({ - threadLocalId: thread.localId, - }); + await afterNextRender(() => this.afterEvent({ + eventName: 'o-thread-cache-loaded-messages', + func: () => document.querySelector('.o_MessagingMenu_toggler').click(), + message: "should wait until inbox loaded initial needaction messages", + predicate: ({ threadCache }) => { + return threadCache.thread.model === 'mail.box' && threadCache.thread.id === 'inbox'; + }, + })); assert.containsOnce( document.body, '.o_ThreadNeedactionPreview_markAsRead', @@ -89,8 +100,22 @@ QUnit.test('mark as read', async function (assert) { QUnit.test('click on preview should mark as read and open the thread', async function (assert) { assert.expect(5); + this.data['mail.message'].records.push({ + id: 21, + model: 'res.partner', + needaction: true, + needaction_partner_ids: [this.data.currentPartnerId], + res_id: 11, + }); + this.data['mail.notification'].records.push({ + mail_message_id: 21, + notification_status: 'sent', + notification_type: 'inbox', + res_partner_id: this.data.currentPartnerId, + }); await this.start({ hasChatWindow: true, + hasMessagingMenu: true, async mockRPC(route, args) { if (route.includes('set_message_done')) { assert.step('set_message_done'); @@ -98,18 +123,14 @@ QUnit.test('click on preview should mark as read and open the thread', async fun return this._super(...arguments); }, }); - const thread = this.env.models['mail.thread'].create({ - id: 11, - model: 'mail.channel', - }); - this.env.models['mail.message'].create({ - id: 21, - isNeedaction: true, - originThread: [['link', thread]], - }); - await this.createThreadNeedactionPreviewComponent({ - threadLocalId: thread.localId, - }); + await afterNextRender(() => this.afterEvent({ + eventName: 'o-thread-cache-loaded-messages', + func: () => document.querySelector('.o_MessagingMenu_toggler').click(), + message: "should wait until inbox loaded initial needaction messages", + predicate: ({ threadCache }) => { + return threadCache.thread.model === 'mail.box' && threadCache.thread.id === 'inbox'; + }, + })); assert.containsOnce( document.body, '.o_ThreadNeedactionPreview', @@ -152,22 +173,32 @@ QUnit.test('click on expand from chat window should close the chat window and op "should redirect to the model of the thread" ); }); + this.data['mail.message'].records.push({ + id: 21, + model: 'res.partner', + needaction: true, + needaction_partner_ids: [this.data.currentPartnerId], + res_id: 11, + }); + this.data['mail.notification'].records.push({ + mail_message_id: 21, + notification_status: 'sent', + notification_type: 'inbox', + res_partner_id: this.data.currentPartnerId, + }); await this.start({ env: { bus }, hasChatWindow: true, + hasMessagingMenu: true, }); - const thread = this.env.models['mail.thread'].create({ - id: 11, - model: 'res.partner', - }); - this.env.models['mail.message'].create({ - id: 21, - isNeedaction: true, - originThread: [['link', thread]], - }); - await this.createThreadNeedactionPreviewComponent({ - threadLocalId: thread.localId, - }); + await afterNextRender(() => this.afterEvent({ + eventName: 'o-thread-cache-loaded-messages', + func: () => document.querySelector('.o_MessagingMenu_toggler').click(), + message: "should wait until inbox loaded initial needaction messages", + predicate: ({ threadCache }) => { + return threadCache.thread.model === 'mail.box' && threadCache.thread.id === 'inbox'; + }, + })); assert.containsOnce( document.body, '.o_ThreadNeedactionPreview', @@ -206,8 +237,22 @@ QUnit.test('[technical] opening a non-channel chat window should not call channe // window, because there is no server sync of fold state for them. assert.expect(3); + this.data['mail.message'].records.push({ + id: 21, + model: 'res.partner', + needaction: true, + needaction_partner_ids: [this.data.currentPartnerId], + res_id: 11, + }); + this.data['mail.notification'].records.push({ + mail_message_id: 21, + notification_status: 'sent', + notification_type: 'inbox', + res_partner_id: this.data.currentPartnerId, + }); await this.start({ hasChatWindow: true, + hasMessagingMenu: true, async mockRPC(route, args) { if (route.includes('channel_fold')) { const message = "should not call channel_fold when opening a non-channel chat window"; @@ -218,18 +263,14 @@ QUnit.test('[technical] opening a non-channel chat window should not call channe return this._super(...arguments); }, }); - const thread = this.env.models['mail.thread'].create({ - id: 11, - model: 'mail.channel', - }); - this.env.models['mail.message'].create({ - id: 21, - isNeedaction: true, - originThread: [['link', thread]], - }); - await this.createThreadNeedactionPreviewComponent({ - threadLocalId: thread.localId, - }); + await afterNextRender(() => this.afterEvent({ + eventName: 'o-thread-cache-loaded-messages', + func: () => document.querySelector('.o_MessagingMenu_toggler').click(), + message: "should wait until inbox loaded initial needaction messages", + predicate: ({ threadCache }) => { + return threadCache.thread.model === 'mail.box' && threadCache.thread.id === 'inbox'; + }, + })); assert.containsOnce( document.body, '.o_ThreadNeedactionPreview', diff --git a/addons/mail/static/src/model/model_field.js b/addons/mail/static/src/model/model_field.js index e4cd3ee878e..55617d5eaa1 100644 --- a/addons/mail/static/src/model/model_field.js +++ b/addons/mail/static/src/model/model_field.js @@ -414,11 +414,7 @@ class ModelField { } break; case 'replace': - // TODO IMP replace should not unlink-all (task-2270780) - if (this._setRelationUnlink(record, currentValue, options)) { - hasChanged = true; - } - if (this._setRelationLink(record, val[1], options)) { + if (this._setRelationReplace(record, val[1], options)) { hasChanged = true; } break; @@ -528,20 +524,9 @@ class ModelField { * @returns {boolean} whether the value changed for the current field */ _setRelationInsertAndReplace(record, data, options) { - // unlink must be done before insert: - // because unlink might trigger delete due to causality and new data - // shouldn't be deleted just after being inserted - // TODO IMP insert-and-replace should not unlink-all (task-2270780) - let hasChanged = false; - if (this._setRelationUnlink(record, this.read(record), options)) { - hasChanged = true; - } const OtherModel = this.env.models[this.to]; - const other = this.env.modelManager._insert(OtherModel, data); - if (this._setRelationLink(record, other, options)) { - hasChanged = true; - } - return hasChanged; + const newValue = this.env.modelManager._insert(OtherModel, data); + return this._setRelationReplace(record, newValue, options); } /** @@ -636,6 +621,70 @@ class ModelField { return true; } + /** + * Set a 'replace' operation on this relational field. + * + * @private + * @param {mail.model} record + * @param {mail.model|mail.model[]} newValue + * @param {Object} [options] + * @returns {boolean} whether the value changed for the current field + */ + _setRelationReplace(record, newValue, options) { + if (['one2one', 'many2one'].includes(this.relationType)) { + // for x2one replace is just link + return this._setRelationLinkX2One(record, newValue, options); + } + + // for x2many: smart process to avoid unnecessary unlink/link + let hasChanged = false; + let hasToReorder = false; + const otherRecordsSet = this.read(record); + const otherRecordsList = [...otherRecordsSet]; + const recordsToReplaceList = [...this._convertX2ManyValue(newValue)]; + const recordsToReplaceSet = new Set(recordsToReplaceList); + + // records to link + const recordsToLink = []; + for (let i = 0; i < recordsToReplaceList.length; i++) { + const recordToReplace = recordsToReplaceList[i]; + if (!otherRecordsSet.has(recordToReplace)) { + recordsToLink.push(recordToReplace); + } + if (otherRecordsList[i] !== recordToReplace) { + hasToReorder = true; + } + } + if (this._setRelationLinkX2Many(record, recordsToLink, options)) { + hasChanged = true; + } + + // records to unlink + const recordsToUnlink = []; + for (let i = 0; i < otherRecordsList.length; i++) { + const otherRecord = otherRecordsList[i]; + if (!recordsToReplaceSet.has(otherRecord)) { + recordsToUnlink.push(otherRecord); + } + if (recordsToReplaceList[i] !== otherRecord) { + hasToReorder = true; + } + } + if (this._setRelationUnlinkX2Many(record, recordsToUnlink, options)) { + hasChanged = true; + } + + // reorder result + if (hasToReorder) { + otherRecordsSet.clear(); + for (const record of recordsToReplaceList) { + otherRecordsSet.add(record); + } + hasChanged = true; + } + return hasChanged; + } + /** * Set an 'unlink' operation on this relational field. * diff --git a/addons/mail/static/src/models/thread/thread.js b/addons/mail/static/src/models/thread/thread.js index 70912503dce..d50fca3e3f4 100644 --- a/addons/mail/static/src/models/thread/thread.js +++ b/addons/mail/static/src/models/thread/thread.js @@ -287,6 +287,9 @@ function factory(dependencies) { } return list; }, []); + if (channelIds.length === 0) { + return; + } const channelPreviews = await this.env.services.rpc({ model: 'mail.channel', method: 'channel_fetch_preview', diff --git a/addons/mail/static/src/models/thread_cache/thread_cache.js b/addons/mail/static/src/models/thread_cache/thread_cache.js index d909fb88984..080040981f2 100644 --- a/addons/mail/static/src/models/thread_cache/thread_cache.js +++ b/addons/mail/static/src/models/thread_cache/thread_cache.js @@ -262,6 +262,10 @@ function factory(dependencies) { if (!extraDomain && messages.length < limit) { this.update({ isAllHistoryLoaded: true }); } + this.env.messagingBus.trigger('o-thread-cache-loaded-messages', { + fetchedMessages: messages, + threadCache: this, + }); return messages; } diff --git a/addons/mail/static/tests/helpers/mock_server.js b/addons/mail/static/tests/helpers/mock_server.js index e2daa7c38d9..6167682c572 100644 --- a/addons/mail/static/tests/helpers/mock_server.js +++ b/addons/mail/static/tests/helpers/mock_server.js @@ -1300,11 +1300,12 @@ 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 busNotifications = [[[false, 'res.partner', this.currentPartnerId], data]]; + this._widget.call('bus_service', 'trigger', 'notification', busNotifications); } - // NOTE: server is also sending channel_ids, not done here for simplicity. - const data = { type: 'mark_as_read', message_ids: ids }; - const busNotifications = [[[false, 'res.partner', this.currentPartnerId], data]]; - this._widget.call('bus_service', 'trigger', 'notification', busNotifications); }, /** * Simulates `toggle_message_starred` on `mail.message`.