From 9eff26367fdc7f57bea5dca10c3e5d7b3a858f5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 15 Nov 2018 20:37:21 +0000 Subject: [PATCH 1/3] [FIX] mail: easily mention followers and employees in chatter Revision on https://github.com/odoo/odoo/commit/cd34f6de727d5b3858420cff3c9d3c5995c4e75c Resulting of the mail JS refactoring, the list of prefetched suggestions has been changed as follow: - before: list of list of mention suggestions `{Array}` - after: list of mention suggestions `{Object[]}` The change resulted in wrongly thinking that wrapping the list of suggestions in a list was pointless, thus simplifying it as a simple list of mention suggestions. However, because of this change, it has become much harder to mention employees and followers of documents from the chatter: it now always fetches all partner suggestions at all time, whereas before it was suggesting followers, then employees, and finally all partners based on the input of the user. In fact, there was a purpose of using a list of list of prefetched mention suggestions, which is to group suggestions by "priority": followers first, then employees, and all partners as a last resort. This commit re-introduces prefetched mention suggestions as a list of list of partners, so that it re-becomes easier to mention followers and partners of a document from the chatter. Task-ID 1910111 --- .../static/src/js/composers/basic_composer.js | 25 ++-- .../static/src/js/models/threads/channel.js | 5 +- .../static/src/js/models/threads/livechat.js | 5 +- .../static/src/js/models/threads/thread.js | 2 +- .../static/src/js/services/mail_manager.js | 6 +- addons/mail/static/tests/chatter_tests.js | 118 ++++++++++++++++++ 6 files changed, 142 insertions(+), 19 deletions(-) diff --git a/addons/mail/static/src/js/composers/basic_composer.js b/addons/mail/static/src/js/composers/basic_composer.js index a6cdb39e2de..8a533cd4515 100644 --- a/addons/mail/static/src/js/composers/basic_composer.js +++ b/addons/mail/static/src/js/composers/basic_composer.js @@ -211,7 +211,8 @@ var BasicComposer = Widget.extend({ * displayed to the user. If none of them match, then it will fetch for more * partner suggestions (@see _mentionFetchPartners). * - * @param {$.Deferred} prefetchedPartners + * @param {$.Deferred} prefetchedPartners list of list of + * prefetched partners. */ mentionSetPrefetchedPartners: function (prefetchedPartners) { this._mentionPrefetchedPartners = prefetchedPartners; @@ -302,21 +303,23 @@ var BasicComposer = Widget.extend({ */ _mentionFetchPartners: function (search) { var self = this; - return $.when(this._mentionPrefetchedPartners).then(function (partners) { + return $.when(this._mentionPrefetchedPartners).then(function (prefetchedPartners) { // filter prefetched partners with the given search string var suggestions = []; var limit = self.options.mentionFetchLimit; var searchRegexp = new RegExp(_.str.escapeRegExp(mailUtils.unaccent(search)), 'i'); - if (limit > 0) { - var filteredPartners = _.filter(partners, function (partner) { - return partner.email && searchRegexp.test(partner.email) || - partner.name && searchRegexp.test(mailUtils.unaccent(partner.name)); - }); - if (filteredPartners.length) { - suggestions.push(filteredPartners.slice(0, limit)); - limit -= filteredPartners.length; + _.each(prefetchedPartners, function (partners) { + if (limit > 0) { + var filteredPartners = _.filter(partners, function (partner) { + return partner.email && searchRegexp.test(partner.email) || + partner.name && searchRegexp.test(mailUtils.unaccent(partner.name)); + }); + if (filteredPartners.length) { + suggestions.push(filteredPartners.slice(0, limit)); + limit -= filteredPartners.length; + } } - } + }); if (!suggestions.length && !self.options.mentionPartnersRestricted) { // no result found among prefetched partners, fetch other suggestions suggestions = self._mentionFetchThrottled( diff --git a/addons/mail/static/src/js/models/threads/channel.js b/addons/mail/static/src/js/models/threads/channel.js index 695275e799e..808e8b7fbca 100644 --- a/addons/mail/static/src/js/models/threads/channel.js +++ b/addons/mail/static/src/js/models/threads/channel.js @@ -179,7 +179,8 @@ var Channel = SearchableThread.extend(ThreadTypingMixin, { /** * Get listeners of a channel * - * @returns {$.Promise} resolved with list of channel listeners + * @returns {$.Promise>} resolved with list of list of + * channel listeners. */ getMentionPartnerSuggestions: function () { var self = this; @@ -193,7 +194,7 @@ var Channel = SearchableThread.extend(ThreadTypingMixin, { }) .then(function (members) { self._members = members; - return members; + return [members]; }); } return this._membersDef; diff --git a/addons/mail/static/src/js/models/threads/livechat.js b/addons/mail/static/src/js/models/threads/livechat.js index b7235e7bc0e..f4cb58ebad3 100644 --- a/addons/mail/static/src/js/models/threads/livechat.js +++ b/addons/mail/static/src/js/models/threads/livechat.js @@ -37,7 +37,8 @@ var Livechat = TwoUserChannel.extend({ * display of a user that is typing. * * @override - * @returns {$.Promise} resolved with list of livechat members + * @returns {$.Promise>} resolved with list of list of + * livechat members. */ getMentionPartnerSuggestions: function () { var self = this; @@ -49,7 +50,7 @@ var Livechat = TwoUserChannel.extend({ name: self._WEBSITE_USER_NAME, }); } - return self._members; + return [self._members]; }); }, /** diff --git a/addons/mail/static/src/js/models/threads/thread.js b/addons/mail/static/src/js/models/threads/thread.js index c9dda28dd93..b15ceb8563c 100644 --- a/addons/mail/static/src/js/models/threads/thread.js +++ b/addons/mail/static/src/js/models/threads/thread.js @@ -111,7 +111,7 @@ var Thread = AbstractThread.extend(ServicesMixin, { * By default, a thread has not listener. * * @abstract - * @returns {$.Promise} + * @returns {$.Promise>} */ getMentionPartnerSuggestions: function () { return $.when([]); diff --git a/addons/mail/static/src/js/services/mail_manager.js b/addons/mail/static/src/js/services/mail_manager.js index 734f12f0cf3..c21089b7095 100644 --- a/addons/mail/static/src/js/services/mail_manager.js +++ b/addons/mail/static/src/js/services/mail_manager.js @@ -199,7 +199,7 @@ var MailManager = AbstractService.extend({ * Get partners as mentions from a chatter * Typically all employees as partner suggestions. * - * @returns {Array} + * @returns {Array>} */ getMentionPartnerSuggestions: function () { return this._mentionPartnerSuggestions; @@ -1195,8 +1195,8 @@ var MailManager = AbstractService.extend({ * * @private * @param {Object} result data from server on mail/init_messaging rpc - * @param {Object[]} result.mention_partner_suggestions list of suggestions - * with all the employees + * @param {Array} result.mention_partner_suggestions list of + * suggestions. * @param {integer} result.menu_id the menu ID of discuss app */ _updateInternalStateFromServer: function (result) { diff --git a/addons/mail/static/tests/chatter_tests.js b/addons/mail/static/tests/chatter_tests.js index b379bc9050b..4bcce4ab352 100644 --- a/addons/mail/static/tests/chatter_tests.js +++ b/addons/mail/static/tests/chatter_tests.js @@ -2450,6 +2450,124 @@ QUnit.test('chatter: suggested partner auto-follow on message post', function (a form.destroy(); }); +QUnit.test('chatter: mention prefetched partners (followers & employees)', function (assert) { + // Note: employees are in prefeteched partner for mentions in chatter when + // the module hr is installed. + assert.expect(10); + + var followerSuggestions = [{ + id: 1, + name: 'FollowerUser1', + email: 'follower-user1@example.com', + }, { + id: 2, + name: 'FollowerUser2', + email: 'follower-user2@example.com', + }]; + + var nonFollowerSuggestions = [{ + id: 3, + name: 'NonFollowerUser1', + email: 'non-follower-user1@example.com', + }, { + id: 4, + name: 'NonFollowerUser2', + email: 'non-follower-user2@example.com', + }]; + + // link followers + this.data.partner.records[0].message_follower_ids = [10, 20]; + + // prefetched partners + this.data.initMessaging = { + mention_partner_suggestions: [followerSuggestions.concat(nonFollowerSuggestions)], + }; + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + services: this.services, + arch: '
' + + '' + + '' + + '' + + '
' + + '' + + '' + + '
' + + '
', + res_id: 2, + mockRPC: function (route, args) { + if (route === '/mail/read_followers') { + return $.when({ + followers: [{ + id: 10, + name: 'FollowerUser1', + email: 'follower-user1@example.com', + res_model: 'res.partner', + res_id: 1, + }, { + id: 20, + name: 'FollowerUser2', + email: 'follower-user2@example.com', + res_model: 'res.partner', + res_id: 2, + }], + subtypes: [], + }); + } + if (args.method === 'message_get_suggested_recipients') { + return $.when({2: []}); + } + if (args.method === 'get_mention_suggestions') { + throw new Error('should not fetch partners for mentions'); + } + return this._super(route, args); + }, + session: {}, + }); + + assert.strictEqual(form.$('.o_followers_count').text(), '2', + "should have two followers of this document"); + assert.strictEqual(form.$('.o_followers_list > .o_partner').text().replace(/\s+/g, ''), + 'FollowerUser1FollowerUser2', + "should have correct follower names"); + assert.strictEqual(form.$('.o_composer_mention_dropdown').length, 0, + "should not show the mention suggestion dropdown"); + + form.$('.o_chatter_button_new_message').click(); + var $input = form.$('.oe_chatter .o_composer_text_field:first()'); + $input.val('@'); + // the cursor position must be set for the mention manager to detect that we are mentionning + $input[0].selectionStart = 1; + $input[0].selectionEnd = 1; + $input.trigger('keyup'); + + assert.strictEqual(form.$('.o_composer_mention_dropdown').length, 1, + "should show the mention suggestion dropdown"); + + assert.strictEqual(form.$('.o_mention_proposition').length, 4, + "should show 4 mention suggestions"); + assert.strictEqual(form.$('.o_mention_proposition').eq(0).text().replace(/\s+/g, ''), + "FollowerUser1(follower-user1@example.com)", + "should display correct 1st mention suggestion"); + assert.strictEqual(form.$('.o_mention_proposition').eq(1).text().replace(/\s+/g, ''), + "FollowerUser2(follower-user2@example.com)", + "should display correct 2nd mention suggestion"); + assert.ok(form.$('.o_mention_proposition').eq(1).next().hasClass('dropdown-divider'), + "should have a mention separator after last follower mention suggestion"); + assert.strictEqual(form.$('.o_mention_proposition').eq(2).text().replace(/\s+/g, ''), + "NonFollowerUser1(non-follower-user1@example.com)", + "should display correct 3rd mention suggestion"); + assert.strictEqual(form.$('.o_mention_proposition').eq(3).text().replace(/\s+/g, ''), + "NonFollowerUser2(non-follower-user2@example.com)", + "should display correct 4th mention suggestion"); + + //cleanup + form.destroy(); +}); + QUnit.module('FieldMany2ManyTagsEmail', { beforeEach: function () { this.data = { From 7613db4178c8de6c054876e43e2680ef8672d8c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 15 Nov 2018 21:16:32 +0000 Subject: [PATCH 2/3] [FIX] mail: show message document link in chat window Revision on https://github.com/odoo/odoo/commit/cd34f6de727d5b3858420cff3c9d3c5995c4e75c The commit above consisted of refactoring the JS mail module. A small regression was introduced on messages linked to a document from a chat window: it was no longer displaying the link to redirect to the document that this message originates from. This issue was only isolated to chat windows: it was working fine in the Discuss app. This commit re-introduces the displaying of the document link on messages from chat windows. Task-ID 1910119 --- .../thread_windows/abstract_thread_window.js | 1 - .../basic_thread_window_tests.js | 75 +++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/addons/mail/static/src/js/thread_windows/abstract_thread_window.js b/addons/mail/static/src/js/thread_windows/abstract_thread_window.js index f6ba0ccecca..716b86400be 100644 --- a/addons/mail/static/src/js/thread_windows/abstract_thread_window.js +++ b/addons/mail/static/src/js/thread_windows/abstract_thread_window.js @@ -79,7 +79,6 @@ var AbstractThreadWindow = Widget.extend({ this.$header = this.$('.o_thread_window_header'); this._threadWidget = new ThreadWidget(this, { - displayDocumentLinks: false, displayMarkAsRead: false, displayStars: this.options.displayStars, }); diff --git a/addons/mail/static/tests/thread_window/basic_thread_window_tests.js b/addons/mail/static/tests/thread_window/basic_thread_window_tests.js index f05fd367697..4b8935e81cf 100644 --- a/addons/mail/static/tests/thread_window/basic_thread_window_tests.js +++ b/addons/mail/static/tests/thread_window/basic_thread_window_tests.js @@ -353,6 +353,81 @@ QUnit.test('do not mark as read the newly open thread window from received messa parent.destroy(); }); +QUnit.test('show document link of message linked to a document', function (assert) { + assert.expect(6); + + this.data['mail.channel'] = { + fields: { + name: { + string: "Name", + type: "char", + required: true, + }, + channel_type: { + string: "Channel Type", + type: "selection", + }, + channel_message_ids: { + string: "Messages", + type: "many2many", + relation: 'mail.message' + }, + message_unread_counter: { + string: "Amount of Unread Messages", + type: "integer" + }, + }, + records: [{ + id: 2, + name: "R&D Tasks", + channel_type: "channel", + }], + }; + this.data['mail.message'].records.push({ + author_id: [5, "Someone else"], + body: "

Test message

", + id: 40, + model: 'some.document', + record_name: 'Some Document', + res_id: 10, + channel_ids: [2], + }); + + this.data.initMessaging.channel_slots.channel_channel.push({ + id: 2, + name: "R&D Tasks", + channel_type: "public", + }); + + var parent = this.createParent({ + data: this.data, + services: this.services, + session: { partner_id: 3 }, + }); + + assert.strictEqual($('.o_thread_window').length, 0, + "no thread window should be open initially"); + + // get channel instance to link to thread window + var channel = parent.call('mail_service', 'getChannel', 2); + channel.detach(); + + var $threadWindow = $('.o_thread_window'); + assert.strictEqual($threadWindow.length, 1, + "a thread window should be open"); + assert.strictEqual($threadWindow.find('.o_thread_window_title').text().trim(), + "#R&D Tasks", + "should be thread window of correct channel"); + assert.strictEqual($threadWindow.find('.o_thread_message').length, 1, + "should contain a single message in thread window"); + assert.ok($threadWindow.find('.o_mail_info').text().replace(/\s/g, "").indexOf('Someoneelse') !== -1, + "message should be from 'Someone else' user"); + assert.ok($threadWindow.find('.o_mail_info').text().replace(/\s/g, "").indexOf('onSomeDocument') !== -1, + "message should link to 'Some Document'"); + + parent.destroy(); +}); + }); }); }); From e3f36b10ac6c47954dd29b003161a289f7baaeb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexandre=20K=C3=BChn?= Date: Thu, 15 Nov 2018 23:43:23 +0000 Subject: [PATCH 3/3] [FIX] mail: no crash when searching messages Revision on https://github.com/odoo/odoo/commit/299ebb2cdf63de2e3967da0d2f3e7effd239812b The commit above added the notion of moderation, which requires re-rendering the thread when a message has its moderation changed (i.e. the message is deleted on reject or is no longer marked as 'pending moderation'). However, this condition was triggered even when the moderation status on the message has not changed. This introduced a crash when using the search in discuss: - Access a channel with more than 30 messages, so that not all messages have been fetched. - Search messages in this thread so that it displays more than 30 messages, and the last message of those has already been fetched. > It crashes with "Cannot read property 'getID' of undefined". The bug comes from re-fetching messages: it behaves as a "load more" fetch, although no message has been stored yet. A "load more" fetch requires at least one message in order to determine the minimum message ID to fetch, thus the crash due to having no message to get such an ID. Here's the explanation why it fetches with load more: when performing the search and fetching the same last message, it will detect that it has already been fetched. Its moderation status is 'accepted', and it will assume a change of moderation status. This will trigger a fetch and render of the thread. At this moment, if the search exceeds the fetch limit, not all messages will be fetched, so that this fetch is considered as a "load more" fetch. This commit fixes the issue by only re-rendering the thread when there is a change of the moderation status on an already fetched message. Since changes of the moderation status are handled by means of the longpolling, it should be rare to run into the case of having a change of moderation status on a message while using the search on a thread. Task-ID 1910180 --- .../static/src/js/models/messages/message.js | 3 + .../static/src/js/models/threads/thread.js | 5 +- .../static/tests/discuss_moderation_tests.js | 80 +++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/addons/mail/static/src/js/models/messages/message.js b/addons/mail/static/src/js/models/messages/message.js index 1176c0f8c48..4342b1e244a 100644 --- a/addons/mail/static/src/js/models/messages/message.js +++ b/addons/mail/static/src/js/models/messages/message.js @@ -458,6 +458,9 @@ var Message = AbstractMessage.extend(Mixins.EventDispatcherMixin, ServicesMixin */ setModerationStatus: function (newModerationStatus, options) { var self = this; + if (newModerationStatus === this._moderationStatus) { + return; + } this._moderationStatus = newModerationStatus; if (newModerationStatus === 'accepted' && options) { _.each(options.additionalThreadIDs, function (threadID) { diff --git a/addons/mail/static/src/js/models/threads/thread.js b/addons/mail/static/src/js/models/threads/thread.js index b15ceb8563c..8c2beb8bbfa 100644 --- a/addons/mail/static/src/js/models/threads/thread.js +++ b/addons/mail/static/src/js/models/threads/thread.js @@ -15,7 +15,8 @@ var ServicesMixin = require('web.ServicesMixin'); * In particular, channels and mailboxes are two different kinds of threads. */ var Thread = AbstractThread.extend(ServicesMixin, { - + // max number of fetched messages from the server + _FETCH_LIMIT: 30, /** * @override * @param {Object} params @@ -33,8 +34,6 @@ var Thread = AbstractThread.extend(ServicesMixin, { // means that there is no message in this channel. this._previewed = false; this._type = params.data.type || params.data.channel_type; - // max number of fetched messages from the server - this._FETCH_LIMIT = 30; }, //-------------------------------------------------------------------------- diff --git a/addons/mail/static/tests/discuss_moderation_tests.js b/addons/mail/static/tests/discuss_moderation_tests.js index 51fb7b55c61..4d8e48b4dcb 100644 --- a/addons/mail/static/tests/discuss_moderation_tests.js +++ b/addons/mail/static/tests/discuss_moderation_tests.js @@ -1,6 +1,7 @@ odoo.define('mail.discuss_moderation_tests', function (require) { "use strict"; +var Thread = require('mail.model.Thread'); var mailTestUtils = require('mail.testUtils'); var createDiscuss = mailTestUtils.createDiscuss; @@ -760,5 +761,84 @@ QUnit.test('author: sent message rejected in moderated channel', function (asser }); }); +QUnit.test('no crash when load-more fetching "accepted" message twice', function (assert) { + // This tests requires discuss not loading more messages due to having less + // messages to fetch than available height. This justifies we simply do not + // patch FETCH_LIMIT to 1, as it would detect that more messages could fit + // the empty space (it behaviour is linked to "auto load more"). + var done = assert.async(); + assert.expect(2); + + var FETCH_LIMIT = Thread.prototype._FETCH_LIMIT; + // FETCH LIMIT + 30 should be enough to cover the whole available space in + // the thread of discuss app. + var messageData = []; + _.each(_.range(1, FETCH_LIMIT+31), function (num) { + messageData.push({ + id: num, + body: "

test" + num + "

", + author_id: [100, "Someone"], + channel_ids: [1], + model: 'mail.channel', + res_id: 1, + moderation_status: 'accepted', + } + ); + }); + + this.data['mail.message'].records = messageData; + + this.data.initMessaging = { + channel_slots: { + channel_channel: [{ + id: 1, + channel_type: "channel", + name: "general", + }], + }, + }; + var count = 0; + + createDiscuss({ + id: 1, + context: {}, + params: {}, + data: this.data, + services: this.services, + session: { partner_id: 3 }, + mockRPC: function (route, args) { + if (args.method === 'message_fetch') { + count++; + if (count === 1) { + // inbox message_fetch + return $.when([]); + } + // general message_fetch + return $.when(messageData); + } + return this._super.apply(this, arguments); + }, + }) + .then(function (discuss) { + var $general = discuss.$('.o_mail_discuss_sidebar') + .find('.o_mail_discuss_item[data-thread-id=1]'); + assert.strictEqual($general.length, 1, + "should have the channel item with id 1"); + assert.strictEqual($general.attr('title'), 'general', + "should have the title 'general'"); + + // click on general + $general.click(); + + // simulate search + discuss.trigger_up('search', { + domains: [['author_id', '=', 100]], + }); + + discuss.destroy(); + done(); + }); +}); + }); });