From b3e450abcac1b0457eae69d35c1d929ffd626a21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Wed, 7 Nov 2018 09:35:51 +0000 Subject: [PATCH 1/2] [FIX] mail: open inbox on preview click not linked to any document Before this commit, when the user clicked on a preview in the systray messaging menu that is not linked to any document, there was a traceback. Steps to reproduce: - Admin sets notification management to "Handle with Odoo" - Demo user assigns Admin to a task - Admin clicks on preview from being assigned to a task Those are previews from notifications received from the inbox, which are not related to any document. It attempts to open a document that does not exists (i.e. no model/id). With this commit, when such a preview is not linked to any document, it now opens Discuss with Inbox. Task-ID 1891350 --- .../src/js/systray/systray_messaging_menu.js | 6 +++- .../systray/systray_messaging_menu_tests.js | 33 ++++++++----------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/addons/mail/static/src/js/systray/systray_messaging_menu.js b/addons/mail/static/src/js/systray/systray_messaging_menu.js index 5a687131504..a6c425232b6 100644 --- a/addons/mail/static/src/js/systray/systray_messaging_menu.js +++ b/addons/mail/static/src/js/systray/systray_messaging_menu.js @@ -282,7 +282,11 @@ var MessagingMenu = Widget.extend({ // e.g. needaction message of channel var documentID = $target.data('document-id'); var documentModel = $target.data('document-model'); - this._openDocument(documentModel, documentID); + if (!documentModel) { + this._openDiscuss('mailbox_inbox'); + } else { + this._openDocument(documentModel, documentID); + } } else { // preview of thread this.call('mail_service', 'openThread', previewID); diff --git a/addons/mail/static/tests/systray/systray_messaging_menu_tests.js b/addons/mail/static/tests/systray/systray_messaging_menu_tests.js index 02c7ec86e6b..c01e8dbf0ae 100644 --- a/addons/mail/static/tests/systray/systray_messaging_menu_tests.js +++ b/addons/mail/static/tests/systray/systray_messaging_menu_tests.js @@ -146,8 +146,8 @@ QUnit.test('messaging menu widget: messaging menu with 1 record', function (asse messagingMenu.destroy(); }); -QUnit.test('messaging menu widget: no crash when clicking on inbox notification not associated to a document', function (assert) { - assert.expect(3); +QUnit.test('messaging menu widget: open inbox for needaction not linked to any document', function (assert) { + assert.expect(4); var messagingMenu = new MessagingMenu(); testUtils.addMockEnvironment(messagingMenu, { @@ -156,17 +156,6 @@ QUnit.test('messaging menu widget: no crash when clicking on inbox notification session: { partner_id: 1, }, - intercepts: { - /** - * Simulate action 'mail.action_discuss' successfully performed. - * - * @param {OdooEvent} ev - * @param {function} ev.data.on_success called when success action performed - */ - do_action: function (ev) { - ev.data.on_success(); - }, - }, }); messagingMenu.appendTo($('#qunit-fixture')); @@ -195,12 +184,18 @@ QUnit.test('messaging menu widget: no crash when clicking on inbox notification assert.strictEqual($firstChannelPreview.data('preview-id'), 'mailbox_inbox', "should be a preview from channel inbox"); - try { - $firstChannelPreview.click(); - assert.ok(true, "should not have crashed when clicking on needaction preview message"); - } finally { - messagingMenu.destroy(); - } + + testUtils.intercept(messagingMenu, 'do_action', function (ev) { + if (ev.data.action === 'mail.action_discuss') { + assert.step('do_action:' + ev.data.action + ':' + ev.data.options.active_id); + } + }, true); + $firstChannelPreview.click(); + assert.verifySteps( + ['do_action:mail.action_discuss:mailbox_inbox'], + "should open Discuss with Inbox"); + + messagingMenu.destroy(); }); QUnit.test("messaging menu widget: mark as read on thread preview", function ( assert ) { From 8af392865deac628b1ff57627fa116131547bea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Wed, 7 Nov 2018 10:20:48 +0000 Subject: [PATCH 2/2] [FIX] mail: mark as read per preview Before this commit, when there was several previews on the messaging menu that were not linked to any document, marking one of them as read had the unintended behaviour to mark all of them as read. Steps to reproduce: - Admin with notification management as 'Handle with Odoo' - Demo assign Admin to two different tasks - Admin mark one preview as read > Both previews notifying assignment to a task have been marked as read This is caused by the fact that messages of those previews are not related to any document. As a result, when marking one preview as read, it maps related messages to mark as read, which was taking into account all messages that are not related to any document. With this commit, previews now tracks message IDs, so that marking one preview as read only mark the related messages as read. Task-ID 1907159 --- .../static/src/js/models/messages/message.js | 1 + .../static/src/js/models/threads/mailbox.js | 33 ++----- .../src/js/systray/systray_messaging_menu.js | 12 +-- addons/mail/static/src/xml/discuss.xml | 3 +- .../systray/systray_messaging_menu_tests.js | 96 +++++++++++++++++++ 5 files changed, 111 insertions(+), 34 deletions(-) diff --git a/addons/mail/static/src/js/models/messages/message.js b/addons/mail/static/src/js/models/messages/message.js index c733b74f347..1176c0f8c48 100644 --- a/addons/mail/static/src/js/models/messages/message.js +++ b/addons/mail/static/src/js/models/messages/message.js @@ -262,6 +262,7 @@ var Message = AbstractMessage.extend(Mixins.EventDispatcherMixin, ServicesMixin documentID: this.getDocumentID(), id: id, imageSRC: this._getModuleIcon() || this.getAvatarSource(), + messageID: this.getID(), status: this.status, title: title, }; diff --git a/addons/mail/static/src/js/models/threads/mailbox.js b/addons/mail/static/src/js/models/threads/mailbox.js index 28dbc4b7163..8d49d84d777 100644 --- a/addons/mail/static/src/js/models/threads/mailbox.js +++ b/addons/mail/static/src/js/models/threads/mailbox.js @@ -45,28 +45,6 @@ var Mailbox = SearchableThread.extend({ num = _.isNumber(num) ? num : 1; this._mailboxCounter = Math.max(this._mailboxCounter - num, 0); }, - /** - * Override so that there are options to filter messages based on document - * model and ID. - * - * @override - * @param {Object} [options] - * @param {string} [options.documentModel] model of the document that the - * local messages of inbox must be linked to. - * @param {integer} [options.documentID] ID of the document that the local - * messages of inbox must be linked to. - * @returns {mail.model.Message[]} - */ - getMessages: function (options) { - var messages = this._super.apply(this, arguments); - if (options.documentModel && options.documentID) { - return _.filter(messages, function (message) { - return message.getDocumentModel() === options.documentModel && - message.getDocumentID() === options.documentID; - }); - } - return messages; - }, /** * Get the mailbox counter of this mailbox. * @@ -89,31 +67,40 @@ var Mailbox = SearchableThread.extend({ return this.fetchMessages().then(function (messages) { // pick only last message of chatter // items = list of objects - // { unreadCounter: integer, message: mail.model.Message } + // { + // unreadCounter: {integer}, + // message: {mail.model.Message}, + // messageIDs: {integer[]}, + // } var items = []; _.each(messages, function (message) { var unreadCounter = 1; + var messageIDs = [message.getID()]; var similarItem = _.find(items, function (item) { return self._areMessagesFromSameDocumentThread(item.message, message) || self._areMessagesFromSameChannel(item.message, message); }); if (similarItem) { unreadCounter = similarItem.unreadCounter + 1; + messageIDs = similarItem.messagesIDs.concat(messageIDs); var index = _.findIndex(items, similarItem); items[index] = { unreadCounter: unreadCounter, message: message, + messageIDs: messageIDs }; } else { items.push({ unreadCounter: unreadCounter, message: message, + messageIDs: messageIDs, }); } }); return _.map(items, function (item) { return _.extend(item.message.getPreview(), { unreadCounter: item.unreadCounter, + messageIDs: item.messageIDs, }); }); }); diff --git a/addons/mail/static/src/js/systray/systray_messaging_menu.js b/addons/mail/static/src/js/systray/systray_messaging_menu.js index a6c425232b6..d86e9e3913b 100644 --- a/addons/mail/static/src/js/systray/systray_messaging_menu.js +++ b/addons/mail/static/src/js/systray/systray_messaging_menu.js @@ -304,19 +304,11 @@ var MessagingMenu = Widget.extend({ var thread; var $preview = $(ev.currentTarget).closest('.o_mail_preview'); var previewID = $preview.data('preview-id'); - var documentModel = $preview.data('document-model'); if (previewID === 'mailbox_inbox') { - var documentID = $preview.data('document-id'); - var inbox = this.call('mail_service', 'getMailbox', 'inbox'); - var messages = inbox.getMessages({ - documentModel: documentModel, - documentID: documentID, - }); - var messageIDs = _.map(messages, function (message) { - return message.getID(); - }); + var messageIDs = [].concat($preview.data('message-ids')); this.call('mail_service', 'markMessagesAsRead', messageIDs); } else if (previewID === 'mail_failure') { + var documentModel = $preview.data('document-model'); var unreadCounter = $preview.data('unread-counter'); this.do_action('mail.mail_resend_cancel_action', { additional_context: { diff --git a/addons/mail/static/src/xml/discuss.xml b/addons/mail/static/src/xml/discuss.xml index a09b7a6f754..9d97de471ce 100644 --- a/addons/mail/static/src/xml/discuss.xml +++ b/addons/mail/static/src/xml/discuss.xml @@ -267,10 +267,11 @@ @param {string} [preview.title] @param {string} [preview.status] @param {integer} [preview.unreadCounter] + @param {integer[]} [preview.messageIDs] -->
+ t-att-data-preview-id="preview.id" t-att-data-document-id="preview.documentID" t-att-data-document-model="preview.documentModel" t-att-data-unread-counter="preview.unreadCounter" t-att-data-message-ids="preview.messageIDs">
Preview diff --git a/addons/mail/static/tests/systray/systray_messaging_menu_tests.js b/addons/mail/static/tests/systray/systray_messaging_menu_tests.js index c01e8dbf0ae..30447b714b1 100644 --- a/addons/mail/static/tests/systray/systray_messaging_menu_tests.js +++ b/addons/mail/static/tests/systray/systray_messaging_menu_tests.js @@ -427,5 +427,101 @@ QUnit.test('update messaging preview on receiving a new message in channel previ messagingMenu.destroy(); }); +QUnit.test('preview of inbox message not linked to document + mark as read', function (assert) { + assert.expect(17); + + this.data.initMessaging = { + needaction_inbox_counter: 2, + }; + + var needactionMessages = [{ + author_id: [1, "Demo"], + body: "

*Message1*

", + id: 689, + needaction: true, + needaction_partner_ids: [44], + }, { + author_id: [1, "Demo"], + body: "

*Message2*

", + id: 690, + needaction: true, + needaction_partner_ids: [44], + }]; + this.data['mail.message'].records = + this.data['mail.message'].records.concat(needactionMessages); + + var messagingMenu = new MessagingMenu(); + testUtils.addMockEnvironment(messagingMenu, { + services: this.services, + data: this.data, + session: { + partner_id: 44, + }, + mockRPC: function (route, args) { + if (args.method === 'set_message_done') { + assert.step({ + method: 'set_message_done', + messageIDs: args.args[0], + }); + } + return this._super.apply(this, arguments); + }, + }); + messagingMenu.appendTo($('#qunit-fixture')); + assert.strictEqual(messagingMenu.$('.o_notification_counter').text(), '2', + "should display a counter of 2 on the messaging menu icon"); + + messagingMenu.$('.dropdown-toggle').click(); + + assert.strictEqual(messagingMenu.$('.o_mail_preview').length, 2, + "should display two previews"); + + var $preview1 = messagingMenu.$('.o_mail_preview').eq(0); + var $preview2 = messagingMenu.$('.o_mail_preview').eq(1); + + assert.strictEqual($preview1.data('preview-id'), + "mailbox_inbox", + "1st preview should be from the mailbox inbox"); + assert.strictEqual($preview2.data('preview-id'), + "mailbox_inbox", + "2nd preview should also be from the mailbox inbox"); + assert.ok($preview1.hasClass('o_preview_unread'), + "1st preview should be marked as unread"); + assert.ok($preview2.hasClass('o_preview_unread'), + "2nd preview should also be marked as unread"); + assert.strictEqual($preview1.find('.o_last_message_preview').text().replace(/\s/g, ''), + "Demo:*Message1*", "should correctly display the 1st preview"); + assert.strictEqual($preview2.find('.o_last_message_preview').text().replace(/\s/g, ''), + "Demo:*Message2*", "should correctly display the 2nd preview"); + + $preview1.find('.o_mail_preview_mark_as_read').click(); + assert.verifySteps([{ + method: 'set_message_done', + messageIDs: [689], + }], "should mark 1st preview as read"); + assert.strictEqual(messagingMenu.$('.o_notification_counter').text(), '1', + "should display a counter of 1 on the messaging menu icon after marking one preview as read"); + assert.strictEqual(messagingMenu.$('.o_mail_preview').length, 1, + "should display a single preview remaining"); + assert.strictEqual(messagingMenu.$('.o_mail_preview .o_last_message_preview').text().replace(/\s/g, ''), + "Demo:*Message2*", "preview 2 should be the remaining one"); + + $preview2 = messagingMenu.$('.o_mail_preview'); + $preview2.find('.o_mail_preview_mark_as_read').click(); + assert.verifySteps([{ + method: 'set_message_done', + messageIDs: [689], + }, { + method: 'set_message_done', + messageIDs: [690], + }], "should mark 2nd preview as read"); + assert.strictEqual(messagingMenu.$('.o_notification_counter').text(), '0', + "should display a counter of 0 on the messaging menu icon after marking both previews as read"); + assert.strictEqual(messagingMenu.$('.o_mail_preview').length, 0, + "should display no preview remaining"); + + messagingMenu.destroy(); +}); + }); });