From 6de76cc4f1a44462ae2d600887078d70df73520a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 4 Apr 2018 14:54:52 +0200 Subject: [PATCH 1/4] [REF] mail: optimize notification recipients data fetch through SQL query In this commit we update _notify method of mail.message to fetch most data related to notification process in a single query. Indeed posting a message can be a costly process, especially when involving several partners, channels, depending on their notification status, user groups, ... Most of the required data can be fetched in a single hand-tuned query. Purpose is to lessen query count. Side effect of this commit is that code is less recordset based and therefore more low-level. It is also harder to customize the behavior through inheritance as it implies rewriting a bit more of code. This query is located in a private method on follower model. It takes parameters allowing to either fetch data related to followers of a given subtype, either related to some partners and channels. First use case is related to the process of posting a message with a subtype that triggers notification of followers. Second use case is related to notifying some specific partners or channels of a given message. It takes notably into consideration internal subtypes and active field of partners, allowing to correctly handle corner case for deactivated followers and portal / share partners. Notification process is divided into two steps. First one is to compute recipient data. Second one is to apply the result of this computation and call the various notification mechanisms accordingly. Doing so allows to easily re-call notification process or compute some data differently if necessary. API of notification methods is updated to allow passing notably the values used to create the newly-posted message. It allows in some simple case to avoid browsing message and partners when no notification is necessary. It saves some queries in simple case like posting messages with specific subtypes not often followed or logging notes without mentioning people. This commit allows to save a few queries on several performance tests as indicated by updated performance counters. Future commits will gradually propagate the use of this data to further lessen query count. This commit is linked to task ID 47934 and PR #24033. No functional change should occur with this commit. --- addons/mail/models/mail_followers.py | 89 ++++++++++++++ addons/mail/models/mail_message.py | 135 ++++++++++++++------- addons/mail/models/mail_thread.py | 18 ++- addons/test_mail/tests/test_performance.py | 28 ++--- 4 files changed, 208 insertions(+), 62 deletions(-) diff --git a/addons/mail/models/mail_followers.py b/addons/mail/models/mail_followers.py index 55b155d85f5..ccb245e18d3 100644 --- a/addons/mail/models/mail_followers.py +++ b/addons/mail/models/mail_followers.py @@ -76,6 +76,95 @@ class Followers(models.Model): # Private tools methods to fetch followers data # -------------------------------------------------- + def _get_recipient_data(self, records, subtype_id, pids=None, cids=None): + """ Private method allowing to fetch recipients data based on a subtype. + Purpose of this method is to fetch all data necessary to notify recipients + in a single query. It fetches data from + + * followers (partners and channels) of records that follow the given + subtype if records and subtype are set; + * partners if pids is given; + * channels if cids is given; + + :param records: fetch data from followers of records that follow subtype_id; + :param subtype_id: mail.message.subtype to check against followers; + :param pids: additional set of partner IDs from which to fetch recipient data; + :param cids: additional set of channel IDs from which to fetch recipient data; + + :return: list of recipient data which is a tuple containing + partner ID (void if channel ID), + channel ID (void if partner ID), + active value (always True for channels), + share status of partner (void as irrelevant if channel ID), + notification status of partner or channel (email or inbox), + user groups of partner (void as irrelevant if channel ID), + """ + if records and subtype_id: + query = """ +WITH sub_followers AS ( + SELECT fol.id, fol.partner_id, fol.channel_id, subtype.internal + FROM mail_followers fol + RIGHT JOIN mail_followers_mail_message_subtype_rel subrel + ON subrel.mail_followers_id = fol.id + RIGHT JOIN mail_message_subtype subtype + ON subtype.id = subrel.mail_message_subtype_id + WHERE subrel.mail_message_subtype_id = %%s AND fol.res_model = %%s AND fol.res_id IN %%s +) +SELECT partner.id as pid, NULL AS cid, + partner.active as active, partner.partner_share as pshare, NULL as ctype, + users.notification_type AS notif, array_agg(groups.id) AS groups + FROM res_partner partner + LEFT JOIN res_users users ON users.partner_id = partner.id + LEFT JOIN res_groups_users_rel groups_rel ON groups_rel.uid = users.id + LEFT JOIN res_groups groups ON groups.id = groups_rel.gid + WHERE EXISTS ( + SELECT partner_id FROM sub_followers + WHERE sub_followers.channel_id IS NULL + AND sub_followers.partner_id = partner.id + AND (sub_followers.internal <> TRUE OR partner.partner_share <> TRUE) + ) %s + GROUP BY partner.id, users.notification_type +UNION +SELECT NULL AS pid, channel.id AS cid, + TRUE as active, NULL AS pshare, channel.channel_type AS ctype, + CASE WHEN channel.email_send = TRUE THEN 'email' ELSE 'inbox' END AS notif, NULL AS groups + FROM mail_channel channel + WHERE EXISTS ( + SELECT channel_id FROM sub_followers WHERE partner_id IS NULL AND sub_followers.channel_id = channel.id + ) %s +""" % ('OR partner.id IN %s' if pids else '', 'OR channel.id IN %s' if cids else '') + params = [subtype_id, records._name, tuple(records.ids)] + if pids: + params.append(tuple(pids)) + if cids: + params.append(tuple(cids)) + self.env.cr.execute(query, tuple(params)) + res = self.env.cr.fetchall() + elif pids or cids: + params, query_pid, query_cid = [], '', '' + if pids: + query_pid = """ +SELECT partner.id as pid, NULL AS cid, + partner.active as active, partner.partner_share as pshare, NULL as ctype, + users.notification_type AS notif, NULL AS groups +FROM res_partner partner +LEFT JOIN res_users users ON users.partner_id = partner.id +WHERE partner.id IN %s""" + params.append(tuple(pids)) + if cids: + query_cid = """ +SELECT NULL AS pid, channel.id AS cid, + TRUE as active, NULL AS pshare, channel.channel_type AS ctype, + CASE when channel.email_send = TRUE then 'email' else 'inbox' end AS notif, NULL AS groups +FROM mail_channel channel WHERE channel.id IN %s """ + params.append(tuple(cids)) + query = ' UNION'.join(x for x in [query_pid, query_cid] if x) + self.env.cr.execute(query, tuple(params)) + res = self.env.cr.fetchall() + else: + res = [] + return res + def _get_subscription_data(self, doc_data, pids, cids, include_pshare=False): """ Private method allowing to fetch follower data from several documents of a given model. Followers can be filtered given partner IDs and channel IDs. diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 816f15a5b24..fed4ad94c88 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -976,46 +976,92 @@ class Message(models.Model): #------------------------------------------------------ @api.multi - def _notify(self, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): - """ Compute recipients to notify based on specified recipients and document - followers. Delegate notification to partners to send emails and bus notifications - and to channels to broadcast messages on channels """ - group_user = self.env.ref('base.group_user') - # have a sudoed copy to manipulate partners (public can go here with website modules like forum / blog / ... ) - self_sudo = self.sudo() + def _notify(self, record, msg_vals, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): + """ Main notification method. This method basically does two things + * call ``_notify_compute_recipients`` that computes recipients to + notify based on message record or message creation values if given + (to optimize performance if we already have data computed); + * call ``_notify_recipients`` that performs the notification process; + + :param record: record on which the message is posted, if any; + :param msg_vals: dictionary of values used to create the message. If given + it is used instead of accessing ``self`` to lesen query count in some + simple cases where no notification is actually required; + :param force_send: tells whether to send notification emails within the + current transaction or to use the email queue; + :param send_after_commit: if force_send, tells whether to send emails after + the transaction has been committed using a post-commit hook; + :param model_description: optional data used in notification process (see + notification templates); + :param mail_auto_delete: delete notification emails once sent; + """ + msg_vals = msg_vals if msg_vals else {} + rdata = self._notify_compute_recipients(record, msg_vals) + return self._notify_recipients( + rdata, record, msg_vals, + force_send=force_send, send_after_commit=send_after_commit, + model_description=model_description, mail_auto_delete=mail_auto_delete) + + @api.multi + def _notify_compute_recipients(self, record, msg_vals): + """ Compute recipients to notify based on subtype and followers. This + method returns data structured as expected for ``_notify_recipients``. """ + msg_sudo = self.sudo() + + pids = [x[1] for x in msg_vals.get('partner_ids')] if 'partner_ids' in msg_vals else msg_sudo.partner_ids.ids + cids = [x[1] for x in msg_vals.get('channel_ids')] if 'channel_ids' in msg_vals else msg_sudo.channel_ids.ids + subtype_id = msg_vals.get('subtype_id') if 'subtype_id' in msg_vals else msg_sudo.subtype_id.id + + recipient_data = { + 'partners': [], + 'channels': [], + } + res = self.env['mail.followers']._get_recipient_data(record, subtype_id, pids, cids) + author_id = msg_vals.get('author_id') or self.author_id.id if res else False + for pid, cid, active, pshare, ctype, notif, groups in res: + if pid and pid == author_id: # do not notify the author of its own messages + continue + if pid: + pdata = {'id': pid, 'active': active, 'share': pshare, 'groups': groups} + if notif == 'inbox': + recipient_data['partners'].append(dict(pdata, notif=notif, type='user')) + else: + if not pshare and notif: # has an user and is not shared, is therefore user + recipient_data['partners'].append(dict(pdata, notif='email', type='user')) + elif pshare and notif: # has an user but is shared, is therefore portal + recipient_data['partners'].append(dict(pdata, notif='email', type='portal')) + else: # has no user, is therefore customer + recipient_data['partners'].append(dict(pdata, notif='email', type='customer')) + elif cid: + recipient_data['channels'].append({'id': cid, 'notif': notif, 'type': ctype}) + return recipient_data + + @api.multi + def _notify_recipients(self, rdata, record, msg_vals, + force_send=False, send_after_commit=True, + model_description=False, mail_auto_delete=True): + """ Main method implementing the notification process. + + :param rdata: dict containing recipients data: { + 'partners': list of dict containing partner data: id, share status (boolean), + notification type ('email' or 'inbox'), type (main classification group used + in notification process), groups (user groups) + 'channels': list of dict containing channel data: id, notification type + ('email' for mailing list otherwise 'inbox'), type (channel_type) + } + """ self.ensure_one() - partners_sudo = self_sudo.partner_ids - channels_sudo = self_sudo.channel_ids - # all followers of the mail.message document have to be added as partners and notified - # and filter to employees only if the subtype is internal - if self_sudo.subtype_id and self.model and self.res_id: - followers = self_sudo.env['mail.followers'].search([ - ('res_model', '=', self.model), - ('res_id', '=', self.res_id), - ('subtype_ids', 'in', self_sudo.subtype_id.id), - ]) - if self_sudo.subtype_id.internal: - followers = followers.filtered(lambda fol: fol.channel_id or (fol.partner_id.user_ids and group_user in fol.partner_id.user_ids[0].mapped('groups_id'))) - channels_sudo |= followers.mapped('channel_id') - partners_sudo |= followers.mapped('partner_id') + email_cids = [r['id'] for r in rdata['channels'] if r['notif'] == 'email'] + email_pids = [r['id'] for r in rdata['partners'] if r['notif'] == 'email'] + inbox_pids = [r['id'] for r in rdata['partners'] if r['notif'] == 'inbox'] - # remove author from notified partners - if not self._context.get('mail_notify_author', False) and self_sudo.author_id: - partners_sudo = partners_sudo - self_sudo.author_id - - # list channels and partner by notification type - email_channels = channels_sudo.filtered(lambda channel: channel.email_send) - notif_partners = partners_sudo.filtered(lambda partner: 'inbox' in partner.mapped('user_ids.notification_type')) - email_partner = partners_sudo - notif_partners - - #update message, with maybe custom values message_values = {} - if channels_sudo: - message_values['channel_ids'] = [(6, 0, channels_sudo.ids)] - if partners_sudo: - message_values['needaction_partner_ids'] = [(6, 0, partners_sudo.ids)] + if rdata['channels']: + message_values['channel_ids'] = [(6, 0, [r['id'] for r in rdata['channels']])] + if rdata['partners']: + message_values['needaction_partner_ids'] = [(6, 0, [r['id'] for r in rdata['partners']])] if self.model and self.res_id and hasattr(self.env[self.model], 'message_get_message_notify_values'): message_values.update(self.env[self.model].browse(self.res_id).message_get_message_notify_values(self, message_values)) if message_values: @@ -1024,16 +1070,18 @@ class Message(models.Model): # notify partners and channels # those methods are called as SUPERUSER because portal users posting messages # have no access to partner model. Maybe propagating a real uid could be necessary. - if email_channels or email_partner: - partners_sudo.search([ + if email_cids or email_pids: + self.env['res.partner'].sudo().search([ '|', - ('id', 'in', (email_partner).ids), - ('channel_ids', 'in', email_channels.ids), - ('email', '!=', self_sudo.author_id.email or self_sudo.email_from), + ('id', 'in', email_pids), + ('channel_ids', 'in', email_cids), + ('email', '!=', self.author_id.email or self.email_from), ])._notify(self, force_send=force_send, send_after_commit=send_after_commit, model_description=model_description, mail_auto_delete=mail_auto_delete) - notif_partners._notify_by_chat(self) - channels_sudo._notify(self) + self.env['res.partner'].sudo().browse(inbox_pids)._notify_by_chat(self) + + if rdata['channels']: + self.env['mail.channel'].sudo().browse([r['id'] for r in rdata['channels']])._notify(self) return True @@ -1098,7 +1146,8 @@ class Message(models.Model): }) # proceed with notification process to send notification emails and Inbox messages for message in self: - message._notify() + record = self.env[message.model].browse(message.res_id) if message.model and message.res_id else None + message._notify(record, {}) @api.multi def _moderate_send_reject_email(self, subject, comment): diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 55e28e5c196..1fe466d164a 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -2014,6 +2014,7 @@ class MailThread(models.AbstractModel): 'parent_id': parent_id, 'subtype_id': subtype_id, 'partner_ids': [(4, pid) for pid in partner_ids], + 'channel_ids': kwargs.get('channel_ids', []), 'add_sign': add_sign }) if notif_layout: @@ -2033,16 +2034,23 @@ class MailThread(models.AbstractModel): self._message_post_after_hook(new_message, values, model_description=model_description, mail_auto_delete=mail_auto_delete) return new_message - def _message_post_after_hook(self, message, values, model_description=False, mail_auto_delete=True): + def _message_post_after_hook(self, message, msg_vals, model_description=False, mail_auto_delete=True): """ Hook to add custom behavior after having posted the message. Both message and computed value are given, to try to lessen query count by using already-computed values instead of having to rebrowse things. """ # Notify recipients of the newly-created message (Inbox / Email + channels) - if values.get('moderation_status') != 'pending_moderation': - message._notify(force_send=self.env.context.get('mail_notify_force_send', True), model_description=model_description, mail_auto_delete=mail_auto_delete) + if msg_vals.get('moderation_status') != 'pending_moderation': + message._notify( + self, msg_vals, + force_send=self.env.context.get('mail_notify_force_send', True), + send_after_commit=True, + model_description=model_description, + mail_auto_delete=mail_auto_delete, + ) + # Post-process: subscribe author - if values['author_id'] and values['model'] and self.ids and values['message_type'] != 'notification' and not self._context.get('mail_create_nosubscribe'): - self._message_subscribe([values['author_id']]) + if msg_vals['author_id'] and msg_vals['model'] and self.ids and msg_vals['message_type'] != 'notification' and not self._context.get('mail_create_nosubscribe'): + self._message_subscribe([msg_vals['author_id']]) else: message._notify_pending_by_chat() diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index ae6c7f263a7..38fcba3be8f 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -173,7 +173,7 @@ class TestAdvMailPerformance(TransactionCase): 'activity_type_id': self.env.ref('mail.mail_activity_data_todo').id, }) - with self.assertQueryCount(margin=1, admin=37, emp=58): # test_mail only: 37 - 58 + with self.assertQueryCount(margin=1, admin=27, emp=48): # com runbot: 27 - 47 // test_mail only: 27 - 48 activity.action_feedback(feedback='Zizisse Done !') @users('admin', 'emp') @@ -187,7 +187,7 @@ class TestAdvMailPerformance(TransactionCase): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(margin=1, admin=39, emp=60): # test_mail only: 39 - 60 + with self.assertQueryCount(margin=1, admin=29, emp=50): # com runbot: 27 - 49 // test_mail only: 29 - 50 record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=64, emp=82): # com runbot: 63 - 81 // test_mail only: 64 - 82 + with self.assertQueryCount(margin=1, admin=60, emp=78): # com runbot: 59 - 77 // test_mail only: 60 - 78 record.write({ 'user_id': self.user_test.id, }) @@ -209,7 +209,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_assignation_inbox(self): record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=43, emp=54): # test_mail only: 43 - 54 + with self.assertQueryCount(margin=1, admin=37, emp=48): # test_mail only: 37 - 48 record.write({ 'user_id': self.user_test.id, }) @@ -229,7 +229,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_log_with_post(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(admin=15, emp=22): # com runbot: 13 - 20 // test_mail only: 15 - 22 + with self.assertQueryCount(admin=11, emp=18): # com runbot: 7 - 13 // test_mail only: 11 - 18 record.message_post( body='

Test message_post as log

', subtype='mail.mt_note', @@ -240,7 +240,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_no_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(admin=15, emp=22): # com runbot: 13 - 20 // test_mail only: 15 - 22 + with self.assertQueryCount(admin=11, emp=18): # com runbot: 7 - 13 // test_mail only: 11 - 18 record.message_post( body='

Test Post Performances basic

', partner_ids=[], @@ -253,7 +253,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=61, emp=79): # com runbot: 54 - 72 // test_mail only: 61 - 79 + with self.assertQueryCount(margin=1, admin=55, emp=74): # com runbot: 48 - 67 // test_mail only: 55 - 74 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -265,7 +265,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_inbox_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=39, emp=48): # com runbot 37 - 46 // test_mail only: 39 - 48 + with self.assertQueryCount(margin=1, admin=32, emp=45): # com runbot 30 - 41 // test_mail only: 32 - 45 record.message_post( body='

Test Post Performances with an inbox ping

', partner_ids=self.user_test.partner_id.ids, @@ -389,7 +389,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=97, emp=120): # com runbot 90 - 113 // test_mail only: 97 - 120 + with self.assertQueryCount(admin=90, emp=112): # com runbot 83 - 105 // test_mail only: 90 - 112 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -406,7 +406,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=116, emp=151): # com runbot 109 - 144 // test_mail only: 116 - 151 + with self.assertQueryCount(admin=109, emp=143): # com runbot 102 - 136 // test_mail only: 109 - 143 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -476,7 +476,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=66, emp=84): # com runbot: 65 - 83 // test_mail only: 66 - 84 + with self.assertQueryCount(admin=62, emp=80): # com runbot: 61 - 79 // test_mail only: 62 - 80 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -499,7 +499,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(margin=1, admin=175, emp=211): # com runbot: 173 - 209 // test_mail only: 175 - 211 + with self.assertQueryCount(margin=1, admin=165, emp=200): # com runbot: 163 - 198 // test_mail only: 165 - 200 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -528,7 +528,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=105, emp=124): # com runbot: 104 - 123 // test_mail only: 105 - 124 + with self.assertQueryCount(margin=1, admin=100, emp=118): # com runbot: 99 - 117 // test_mail only: 100 - 118 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -566,7 +566,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=111, emp=134): # test_mail only: 111 - 134 + with self.assertQueryCount(margin=1, admin=106, emp=127): # test_mail only: 106 - 127 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, From df8130b2fa3a829f798e3aef648f75b5cd90f1f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 4 May 2018 11:55:41 +0200 Subject: [PATCH 2/4] [REF] test_mail: update counters due to enterprise mail_push update This commit is related to enterprise commit that updated mail_push. It is about [REF] mail_push: optimize mail.message notification process override It updates code of mail_push to lessen its impact on mail-related code. It allows to change a few counters in test_mail. Indeed counters are impacted by code located in enterprise because those are "maximum queries" counters. As mail_push is an auto-install module enterprise runbot would be broken if the counters weren't updated. This commit is linked to task ID 47934 and PR #24033. No functional change should occur with this commit. --- addons/test_mail/tests/test_performance.py | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 38fcba3be8f..43ecaa3f236 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -173,7 +173,7 @@ class TestAdvMailPerformance(TransactionCase): 'activity_type_id': self.env.ref('mail.mail_activity_data_todo').id, }) - with self.assertQueryCount(margin=1, admin=27, emp=48): # com runbot: 27 - 47 // test_mail only: 27 - 48 + with self.assertQueryCount(margin=1, admin=27, emp=47): # test_mail only: 27 - 47 activity.action_feedback(feedback='Zizisse Done !') @users('admin', 'emp') @@ -187,7 +187,7 @@ class TestAdvMailPerformance(TransactionCase): record.write({'name': 'Dupe write'}) - with self.assertQueryCount(margin=1, admin=29, emp=50): # com runbot: 27 - 49 // test_mail only: 29 - 50 + with self.assertQueryCount(margin=1, admin=29, emp=49): # test_mail only: 29 - 49 record.action_close('Dupe feedback') self.assertEqual(record.activity_ids, self.env['mail.activity']) @@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=60, emp=78): # com runbot: 59 - 77 // test_mail only: 60 - 78 + with self.assertQueryCount(margin=1, admin=59, emp=77): # com runbot: 59 - 77 // test_mail only: 59 - 77 record.write({ 'user_id': self.user_test.id, }) @@ -229,7 +229,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_log_with_post(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(admin=11, emp=18): # com runbot: 7 - 13 // test_mail only: 11 - 18 + with self.assertQueryCount(admin=7, emp=13): # test_mail only: 7 - 13 record.message_post( body='

Test message_post as log

', subtype='mail.mt_note', @@ -240,7 +240,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_no_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(admin=11, emp=18): # com runbot: 7 - 13 // test_mail only: 11 - 18 + with self.assertQueryCount(admin=7, emp=13): # test_mail only: 7 - 13 record.message_post( body='

Test Post Performances basic

', partner_ids=[], @@ -253,7 +253,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=55, emp=74): # com runbot: 48 - 67 // test_mail only: 55 - 74 + with self.assertQueryCount(margin=1, admin=50, emp=69): # com runbot: 48 - 67 // test_mail only: 50 - 69 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -265,7 +265,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_inbox_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=32, emp=45): # com runbot 30 - 41 // test_mail only: 32 - 45 + with self.assertQueryCount(margin=1, admin=31, emp=42): # com runbot 30 - 41 // test_mail only: 31 - 42 record.message_post( body='

Test Post Performances with an inbox ping

', partner_ids=self.user_test.partner_id.ids, @@ -389,7 +389,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=90, emp=112): # com runbot 83 - 105 // test_mail only: 90 - 112 + with self.assertQueryCount(admin=85, emp=107): # com runbot 83 - 105 // test_mail only: 85 - 107 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -406,7 +406,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=109, emp=143): # com runbot 102 - 136 // test_mail only: 109 - 143 + with self.assertQueryCount(admin=104, emp=138): # com runbot 102 - 136 // test_mail only: 104 - 138 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -476,7 +476,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=62, emp=80): # com runbot: 61 - 79 // test_mail only: 62 - 80 + with self.assertQueryCount(admin=61, emp=79): # com runbot: 61 - 79 // test_mail only: 61 - 79 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -499,7 +499,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(margin=1, admin=165, emp=200): # com runbot: 163 - 198 // test_mail only: 165 - 200 + with self.assertQueryCount(margin=1, admin=163, emp=198): # com runbot: 163 - 198 // test_mail only: 163 - 198 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -528,7 +528,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=100, emp=118): # com runbot: 99 - 117 // test_mail only: 100 - 118 + with self.assertQueryCount(margin=1, admin=99, emp=117): # com runbot: 99 - 117 // test_mail only: 99 - 117 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, From 89051058f4ab7069024f7f6cf2caac05bd68b499 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 27 Apr 2018 17:21:18 +0200 Subject: [PATCH 3/4] [REF] mail: clean code related to notification email generation This commit cleans code about sending email notification to recipients. Purpose of this code is to prepare notification emails, group recipients and finally send emails. In this commit we clean and optimize a bit by * propagating some additional parameter to simplify code, like record on which the notification process runs; * inlining some code currently located in sub-methods. It eases code understanding as there are less code jumps. As those methods are not inherited inlining them can be done; * simplifying a bit notification emails preparation and computation; It allows to save a few query by avoiding to fetch and browse again some data that were already computed. This commit is linked to task ID 47934 and PR #24033. No functional change should occur with this commit. --- addons/mail/models/mail_message.py | 9 +- addons/mail/models/res_partner.py | 171 ++++++++------------- addons/mail/wizard/invite.py | 2 +- addons/mail/wizard/mail_resend_message.py | 5 +- addons/test_mail/tests/test_performance.py | 18 +-- 5 files changed, 82 insertions(+), 123 deletions(-) diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index fed4ad94c88..78487f35af7 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -1062,8 +1062,8 @@ class Message(models.Model): message_values['channel_ids'] = [(6, 0, [r['id'] for r in rdata['channels']])] if rdata['partners']: message_values['needaction_partner_ids'] = [(6, 0, [r['id'] for r in rdata['partners']])] - if self.model and self.res_id and hasattr(self.env[self.model], 'message_get_message_notify_values'): - message_values.update(self.env[self.model].browse(self.res_id).message_get_message_notify_values(self, message_values)) + if message_values and record and hasattr(record, 'message_get_message_notify_values'): + message_values.update(record.message_get_message_notify_values(self, message_values)) if message_values: self.write(message_values) @@ -1076,9 +1076,10 @@ class Message(models.Model): ('id', 'in', email_pids), ('channel_ids', 'in', email_cids), ('email', '!=', self.author_id.email or self.email_from), - ])._notify(self, force_send=force_send, send_after_commit=send_after_commit, model_description=model_description, mail_auto_delete=mail_auto_delete) + ])._notify(self, record, force_send=force_send, send_after_commit=send_after_commit, model_description=model_description, mail_auto_delete=mail_auto_delete) - self.env['res.partner'].sudo().browse(inbox_pids)._notify_by_chat(self) + if inbox_pids: + self.env['res.partner'].sudo().browse(inbox_pids)._notify_by_chat(self) if rdata['channels']: self.env['mail.channel'].sudo().browse([r['id'] for r in rdata['channels']])._notify(self) diff --git a/addons/mail/models/res_partner.py b/addons/mail/models/res_partner.py index a8df8efce74..aa52ca8d339 100644 --- a/addons/mail/models/res_partner.py +++ b/addons/mail/models/res_partner.py @@ -37,32 +37,27 @@ class Partner(models.Model): return dict((res_id, {'partner_ids': [res_id], 'email_to': False, 'email_cc': False}) for res_id in self.ids) @api.model - def _notify_prepare_template_context(self, message, model_description=False, mail_auto_delete=True): - # compute signature - if not message.add_sign: - signature = False - elif message.author_id and message.author_id.user_ids and message.author_id.user_ids[0].signature: - signature = message.author_id.user_ids[0].signature - elif message.author_id: - signature = "

--
%s

" % message.author_id.name - else: - signature = "" - - # compute Sent by + def _notify_prepare_template_context(self, message, record, model_description=False, mail_auto_delete=True): + # compute send user and its related signature + signature = '' if message.author_id and message.author_id.user_ids: user = message.author_id.user_ids[0] + if message.add_sign: + signature = user.signature else: user = self.env.user - if user.company_id.website: - website_url = 'http://%s' % user.company_id.website if not user.company_id.website.lower().startswith(('http:', 'https:')) else user.company_id.website + if message.add_sign: + signature = "

--
%s

" % message.author_id.name + + company = record.company_id if record and 'company_id' in record else user.company_id + if company.website: + website_url = 'http://%s' % company.website if not company.website.lower().startswith(('http:', 'https:')) else company.website else: website_url = False if not model_description and message.model: model_description = self.env['ir.model']._get(message.model).display_name - record_name = message.record_name - tracking = [] for tracking_value in self.env['mail.tracking.value'].sudo().search([('mail_message_id', '=', message.id)]): tracking.append((tracking_value.field_desc, @@ -71,14 +66,6 @@ class Partner(models.Model): is_discussion = message.subtype_id.id == self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment') - record = False - if message.res_id and message.model in self.env: - record = self.env[message.model].browse(message.res_id) - - company = user.company_id - if record and hasattr(record, 'company_id'): - company = record.company_id - return { 'message': message, 'signature': signature, @@ -86,79 +73,26 @@ class Partner(models.Model): 'company': company, 'model_description': model_description, 'record': record, - 'record_name': record_name, + 'record_name': message.record_name, 'tracking_values': tracking, 'is_discussion': is_discussion, 'subtype': message.subtype_id, } - @api.model - def _notify_prepare_email_values(self, message, mail_auto_delete=True): - # compute email references - references = message.parent_id.message_id if message.parent_id else False - - # custom values - custom_values = dict() - if message.res_id and message.model: - custom_values = self.env['mail.thread']._notify_specific_email_values_on_records(message, records=self.env[message.model].browse(message.res_id)) - - mail_values = { - 'mail_message_id': message.id, - 'mail_server_id': message.mail_server_id.id, - 'auto_delete': mail_auto_delete, - 'references': references, - } - mail_values.update(custom_values) - return mail_values - - @api.model - def _notify_send(self, body, subject, recipients, **mail_values): - emails = self.env['mail.mail'] - recipients_nbr = len(recipients) - for email_chunk in split_every(50, recipients.ids): - # TDE FIXME: missing message parameter. So we will find mail_message_id - # in the mail_values and browse it. It should already be in the - # cache so should not impact performances. - mail_message_id = mail_values.get('mail_message_id') - message = self.env['mail.message'].browse(mail_message_id) if mail_message_id else None - tig = self.env[message.model].browse(message.res_id) if message and message.model and message.res_id else False - recipient_values = self.env['mail.thread']._notify_email_recipients_on_records(message, email_chunk, records=tig) - create_values = { - 'body_html': body, - 'subject': subject, - } - create_values.update(mail_values) - create_values.update(recipient_values) - emails |= self.env['mail.mail'].create(create_values) - return emails, recipients_nbr - - @api.model - def _notify_udpate_notifications(self, emails): - for email in emails: - notifications = self.env['mail.notification'].sudo().search([ - ('mail_message_id', '=', email.mail_message_id.id), - ('res_partner_id', 'in', email.recipient_ids.ids)]) - notifications.write({ - 'mail_id': email.id, - 'is_email': True, - 'is_read': True, # handle by email discards Inbox notification - 'email_status': 'ready', - }) - @api.multi - def _notify(self, message, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): + def _notify(self, message, record, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): """ Method to send email linked to notified messages. The recipients are the recordset on which this method is called. - :param boolean force_send: send notification emails now instead of letting the scheduler handle the email queue - :param boolean send_after_commit: send notification emails after the transaction end instead of durign the - transaction; this option is used only if force_send is True - :param dict values: values used to compute the notification process, containing - - * add_sign: add user signature to notification email, default is True - * mail_auto_delete: auto delete send emails, default is True - * other values are given to the context used to render the notification template, allowing customization - + :param message: mail.message record to notify; + :param record: optional record on which the message was posted; + :param force_send: tells whether to send notification emails within the + current transaction or to use the email queue; + :param send_after_commit: if force_send, tells whether to send emails after + the transaction has been committed using a post-commit hook; + :param model_description: optional data used in notification process (see + notification templates); + :param mail_auto_delete: delete notification emails once sent; """ if not self.ids: return True @@ -170,31 +104,52 @@ class Partner(models.Model): _logger.warning('QWeb template %s not found when sending notification emails. Sending without layouting.' % (template_xmlid)) base_template = False - base_template_ctx = self._notify_prepare_template_context(message, model_description=model_description) - base_mail_values = self._notify_prepare_email_values(message, mail_auto_delete=mail_auto_delete) + base_template_ctx = self._notify_prepare_template_context(message, record, model_description=model_description) + + # prepare notification mail values + base_mail_values = { + 'mail_message_id': message.id, + 'mail_server_id': message.mail_server_id.id, + 'auto_delete': mail_auto_delete, + 'references': message.parent_id.message_id if message.parent_id else False + } + if record: + base_mail_values.update(self.env['mail.thread']._notify_specific_email_values_on_records(message, records=record)) # classify recipients: actions / no action - tig = self.env[message.model].browse(message.res_id) if message.model and message.res_id else False - recipients = self.env['mail.thread']._notify_classify_recipients_on_records(message, self, records=tig) + recipients = self.env['mail.thread']._notify_classify_recipients_on_records(message, self, records=record) emails = self.env['mail.mail'] recipients_nbr, recipients_max = 0, 50 - for email_type, recipient_template_values in recipients.items(): - if recipient_template_values['recipients']: - # generate notification email content - template_ctx = {**base_template_ctx, **recipient_template_values} # fixme: set button_unfollow to none - fol_values = { - 'subject': message.subject or (message.record_name and 'Re: %s' % message.record_name), - 'body': base_template.render(template_ctx, engine='ir.qweb', minimal_qcontext=True) if base_template else message.body, - } - fol_values['body'] = self.env['mail.thread']._replace_local_links(fol_values['body']) - # send email - new_emails, new_recipients_nbr = self._notify_send(fol_values['body'], fol_values['subject'], recipient_template_values['recipients'], **base_mail_values) - # update notifications - self._notify_udpate_notifications(new_emails) + for group_tpl_values in [group for group in recipients.values() if group['recipients']]: + # generate notification email content + template_ctx = {**base_template_ctx, **group_tpl_values} + mail_body = base_template.render(template_ctx, engine='ir.qweb', minimal_qcontext=True) + mail_body = self.env['mail.thread']._replace_local_links(mail_body) + mail_subject = message.subject or (message.record_name and 'Re: %s' % message.record_name) - emails |= new_emails - recipients_nbr += new_recipients_nbr + # send email + for email_chunk in split_every(50, group_tpl_values['recipients'].ids): + recipient_values = self.env['mail.thread']._notify_email_recipients_on_records(message, email_chunk, records=record) + create_values = { + 'body_html': mail_body, + 'subject': mail_subject, + } + create_values.update(base_mail_values) + create_values.update(recipient_values) + emails |= self.env['mail.mail'].create(create_values) + + # update notifications + for email in emails: + notifications = self.env['mail.notification'].sudo().search([ + ('mail_message_id', '=', email.mail_message_id.id), + ('res_partner_id', 'in', email.recipient_ids.ids)]) + notifications.write({ + 'mail_id': email.id, + 'is_email': True, + 'is_read': True, # handle by email discards Inbox notification + 'email_status': 'ready', + }) # NOTE: # 1. for more than 50 followers, use the queue system @@ -202,7 +157,7 @@ class Partner(models.Model): # to prevent sending email during a simple update of the database # using the command-line. test_mode = getattr(threading.currentThread(), 'testing', False) - if force_send and recipients_nbr < recipients_max and \ + if force_send and len(emails) < recipients_max and \ (not self.pool._init or test_mode): email_ids = emails.ids dbname = self.env.cr.dbname diff --git a/addons/mail/wizard/invite.py b/addons/mail/wizard/invite.py index 6aa42e05f3f..541d3c1ea4f 100644 --- a/addons/mail/wizard/invite.py +++ b/addons/mail/wizard/invite.py @@ -60,6 +60,6 @@ class Invite(models.TransientModel): 'no_auto_thread': True, 'add_sign': True, }) - new_partners.with_context(auto_delete=True)._notify(message, force_send=True, send_after_commit=False) + new_partners.with_context(auto_delete=True)._notify(message, document, force_send=True, send_after_commit=False) message.unlink() return {'type': 'ir.actions.act_window_close'} diff --git a/addons/mail/wizard/mail_resend_message.py b/addons/mail/wizard/mail_resend_message.py index 38815c1a64d..bf56128ad61 100644 --- a/addons/mail/wizard/mail_resend_message.py +++ b/addons/mail/wizard/mail_resend_message.py @@ -57,7 +57,10 @@ class MailResendMessage(models.TransientModel): to_send = wizard.partner_ids.filtered(lambda p: p.resend).mapped("partner_id") notif_to_cancel = wizard.notification_ids.filtered(lambda notif: notif.res_partner_id in to_cancel and notif.email_status in ('exception', 'bounce')) notif_to_cancel.sudo().write({'email_status': 'canceled'}) - to_send.sudo()._notify(self.mail_message_id, force_send=True, send_after_commit=False) + if to_send: + message = wizard.mail_message_id + record = self.env[message.model].browse(message.res_id) if message.model and message.res_id else None + to_send.sudo()._notify(self.mail_message_id, record, force_send=True, send_after_commit=False) self.mail_message_id._notify_failure_update() return {'type': 'ir.actions.act_window_close'} diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 43ecaa3f236..95e950fa97c 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=59, emp=77): # com runbot: 59 - 77 // test_mail only: 59 - 77 + with self.assertQueryCount(margin=1, admin=59, emp=76): # com runbot: 59 - 76 // test_mail only: 59 - 76 record.write({ 'user_id': self.user_test.id, }) @@ -209,7 +209,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_assignation_inbox(self): record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=37, emp=48): # test_mail only: 37 - 48 + with self.assertQueryCount(margin=1, admin=36, emp=46): # test_mail only: 36 - 46 record.write({ 'user_id': self.user_test.id, }) @@ -253,7 +253,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=50, emp=69): # com runbot: 48 - 67 // test_mail only: 50 - 69 + with self.assertQueryCount(margin=1, admin=50, emp=68): # com runbot: 48 - 66 // test_mail only: 50 - 68 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -389,7 +389,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=85, emp=107): # com runbot 83 - 105 // test_mail only: 85 - 107 + with self.assertQueryCount(admin=82, emp=103): # com runbot 80 - 101 // test_mail only: 82 - 103 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -406,7 +406,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=104, emp=138): # com runbot 102 - 136 // test_mail only: 104 - 138 + with self.assertQueryCount(admin=101, emp=134): # com runbot 99 - 132 // test_mail only: 101 - 134 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -476,7 +476,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=61, emp=79): # com runbot: 61 - 79 // test_mail only: 61 - 79 + with self.assertQueryCount(admin=61, emp=78): # com runbot: 61 - 78 // test_mail only: 61 - 78 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -499,7 +499,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(margin=1, admin=163, emp=198): # com runbot: 163 - 198 // test_mail only: 163 - 198 + with self.assertQueryCount(margin=1, admin=160, emp=193): # com runbot: 160 - 193 // test_mail only: 160 - 193 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -528,7 +528,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=99, emp=117): # com runbot: 99 - 117 // test_mail only: 99 - 117 + with self.assertQueryCount(margin=1, admin=96, emp=112): # com runbot: 96 - 112 // test_mail only: 96 - 112 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -566,7 +566,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=106, emp=127): # test_mail only: 106 - 127 + with self.assertQueryCount(margin=1, admin=103, emp=122): # test_mail only: 103 - 122 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, From 689a496f1c60eb662dec7bbf7079f33b070a2cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Fri, 27 Apr 2018 17:44:28 +0200 Subject: [PATCH 4/4] [REF] mail: propagate recipient data through notification process Purpose of this commit is to avoid browsing and prefetching data about recipients when notifying a message to partners and channels. Mail message _notify computes all necessary data in a single query. This commit allow to re-use this data by propagating it through the call chain. Addons inheriting from classification methods used when sending notification emails are updated accordingly to the API and data update. This commit is linked to task ID 47934 and PR #24033. No functional change should occur with this commit. --- addons/crm/models/crm_lead.py | 3 +- addons/hr_holidays/models/hr_leave.py | 3 +- .../hr_holidays/models/hr_leave_allocation.py | 3 +- addons/mail/models/mail_message.py | 26 +++++------ addons/mail/models/mail_thread.py | 22 +++++----- addons/mail/models/res_partner.py | 43 +++++++++++-------- addons/mail/wizard/invite.py | 7 ++- addons/mail/wizard/mail_resend_message.py | 21 ++++++++- addons/portal/models/portal_mixin.py | 2 +- addons/project/models/project.py | 3 +- addons/test_mail/tests/test_performance.py | 16 +++---- addons/website_blog/models/website_blog.py | 9 ++-- addons/website_forum/models/forum.py | 10 ++--- 13 files changed, 101 insertions(+), 67 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 35806bddb6f..929dda2f379 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -1116,8 +1116,9 @@ class Lead(models.Model): if self.team_id: salesman_actions.append({'url': self._notify_get_action_link('view', res_id=self.team_id.id, model=self.team_id._name), 'title': _('Sales Team Settings')}) + salesman_group_id = self.env.ref('sales_team.group_sale_salesman').id new_group = ( - 'group_sale_salesman', lambda partner: bool(partner.user_ids) and any(user.has_group('sales_team.group_sale_salesman') for user in partner.user_ids), { + 'group_sale_salesman', lambda pdata: pdata['type'] == 'user' and salesman_group_id in pdata['groups'], { 'actions': salesman_actions, }) diff --git a/addons/hr_holidays/models/hr_leave.py b/addons/hr_holidays/models/hr_leave.py index 201c62eb8f0..8a6474ba841 100644 --- a/addons/hr_holidays/models/hr_leave.py +++ b/addons/hr_holidays/models/hr_leave.py @@ -664,8 +664,9 @@ class HolidaysRequest(models.Model): ref_action = self._notify_get_action_link('controller', controller='/leave/refuse') hr_actions += [{'url': ref_action, 'title': _('Refuse')}] + holiday_user_group_id = self.env.ref('hr_holidays.group_hr_holidays_user').id new_group = ( - 'group_hr_holidays_user', lambda partner: bool(partner.user_ids) and any(user.has_group('hr_holidays.group_hr_holidays_user') for user in partner.user_ids), { + 'group_hr_holidays_user', lambda pdata: pdata['type'] == 'user' and holiday_user_group_id in pdata['groups'], { 'actions': hr_actions, }) diff --git a/addons/hr_holidays/models/hr_leave_allocation.py b/addons/hr_holidays/models/hr_leave_allocation.py index 7e952ebe42d..1417150f150 100644 --- a/addons/hr_holidays/models/hr_leave_allocation.py +++ b/addons/hr_holidays/models/hr_leave_allocation.py @@ -509,8 +509,9 @@ class HolidaysAllocation(models.Model): ref_action = self._notify_get_action_link('controller', controller='/allocation/refuse') hr_actions += [{'url': ref_action, 'title': _('Refuse')}] + holiday_user_group_id = self.env.ref('hr_holidays.group_hr_holidays_user').id new_group = ( - 'group_hr_holidays_user', lambda partner: bool(partner.user_ids) and any(user.has_group('hr_holidays.group_hr_holidays_user') for user in partner.user_ids), { + 'group_hr_holidays_user', lambda pdata: pdata['type'] == 'user' and holiday_user_group_id in pdata['groups'], { 'actions': hr_actions, }) diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py index 78487f35af7..07ec041050b 100644 --- a/addons/mail/models/mail_message.py +++ b/addons/mail/models/mail_message.py @@ -1054,7 +1054,6 @@ class Message(models.Model): self.ensure_one() email_cids = [r['id'] for r in rdata['channels'] if r['notif'] == 'email'] - email_pids = [r['id'] for r in rdata['partners'] if r['notif'] == 'email'] inbox_pids = [r['id'] for r in rdata['partners'] if r['notif'] == 'inbox'] message_values = {} @@ -1062,24 +1061,27 @@ class Message(models.Model): message_values['channel_ids'] = [(6, 0, [r['id'] for r in rdata['channels']])] if rdata['partners']: message_values['needaction_partner_ids'] = [(6, 0, [r['id'] for r in rdata['partners']])] - if message_values and record and hasattr(record, 'message_get_message_notify_values'): - message_values.update(record.message_get_message_notify_values(self, message_values)) + if message_values and record and hasattr(record, '_notify_customize_recipients'): + message_values.update(record._notify_customize_recipients(self, message_values, rdata)) if message_values: self.write(message_values) # notify partners and channels - # those methods are called as SUPERUSER because portal users posting messages - # have no access to partner model. Maybe propagating a real uid could be necessary. - if email_cids or email_pids: - self.env['res.partner'].sudo().search([ - '|', - ('id', 'in', email_pids), + if email_cids: + new_pids = self.env['res.partner'].sudo().search([ + ('id', 'not in', [r['id'] for r in rdata['partners']]), ('channel_ids', 'in', email_cids), - ('email', '!=', self.author_id.email or self.email_from), - ])._notify(self, record, force_send=force_send, send_after_commit=send_after_commit, model_description=model_description, mail_auto_delete=mail_auto_delete) + ('email', 'not in', [self.author_id.email, self.email_from]), + ]) + for partner in new_pids: + rdata['partners'].append({'id': partner.id, 'share': True, 'notif': 'email', 'type': 'customer', 'groups': []}) + + partner_email_rdata = [r for r in rdata['partners'] if r['notif'] == 'email'] + if partner_email_rdata: + self.env['res.partner']._notify(self, partner_email_rdata, record, force_send=force_send, send_after_commit=send_after_commit, model_description=model_description, mail_auto_delete=mail_auto_delete) if inbox_pids: - self.env['res.partner'].sudo().browse(inbox_pids)._notify_by_chat(self) + self.env['res.partner'].browse(inbox_pids)._notify_by_chat(self) if rdata['channels']: self.env['mail.channel'].sudo().browse([r['id'] for r in rdata['channels']])._notify(self) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 1fe466d164a..fbdfcbd42e9 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -707,7 +707,7 @@ class MailThread(models.AbstractModel): return groups @api.multi - def _notify_classify_recipients(self, message, recipients): + def _notify_classify_recipients(self, message, recipient_data): """ Classify recipients to be notified of a message in groups to have specific rendering depending on their group. For example users could have access to buttons customers should not have in their emails. @@ -716,7 +716,7 @@ class MailThread(models.AbstractModel): method defined here-under. :param message: mail.message record about to be notified - :param recipients: res.partner recordset to notify + :param recipients: res.partner recordset to notify UPDATE ME """ result = {} @@ -729,11 +729,11 @@ class MailThread(models.AbstractModel): view_title = _('View') default_groups = [ - ('user', lambda partner: bool(partner.user_ids) and not any(user.share for user in partner.user_ids), {}), - ('portal', lambda partner: bool(partner.user_ids) and all(user.share for user in partner.user_ids), { + ('user', lambda pdata: pdata['type'] == 'user', {}), + ('portal', lambda pdata: pdata['type'] == 'portal', { 'has_button_access': False, }), - ('customer', lambda partner: True, { + ('customer', lambda pdata: True, { 'has_button_access': False, }) ] @@ -746,12 +746,12 @@ class MailThread(models.AbstractModel): 'url': access_link, 'title': view_title}) group_data.setdefault('actions', list()) - group_data.setdefault('recipients', self.env['res.partner']) + group_data.setdefault('recipients', list()) - for recipient in recipients: + for recipient in recipient_data: for group_name, group_func, group_data in groups: if group_func(recipient): - group_data['recipients'] |= recipient + group_data['recipients'].append(recipient['id']) break for group_name, group_method, group_data in groups: @@ -759,13 +759,13 @@ class MailThread(models.AbstractModel): return result - def _notify_classify_recipients_on_records(self, message, recipients, records=None): + def _notify_classify_recipients_on_records(self, message, recipient_data, records=None): """ Generic wrapper on ``_notify_classify_recipients`` checking mail.thread inheritance and allowing to call model-specific implementation in a one liner. This method should not be overridden. """ if records and hasattr(records, '_notify_classify_recipients'): - return records._notify_classify_recipients(message, recipients) - return self._notify_classify_recipients(message, recipients) + return records._notify_classify_recipients(message, recipient_data) + return self._notify_classify_recipients(message, recipient_data) @api.multi def _notify_get_reply_to(self, default=None, records=None, company=None, doc_names=None): diff --git a/addons/mail/models/res_partner.py b/addons/mail/models/res_partner.py index aa52ca8d339..b0bdf1e09b2 100644 --- a/addons/mail/models/res_partner.py +++ b/addons/mail/models/res_partner.py @@ -79,12 +79,13 @@ class Partner(models.Model): 'subtype': message.subtype_id, } - @api.multi - def _notify(self, message, record, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): + @api.model + def _notify(self, message, rdata, record, force_send=False, send_after_commit=True, model_description=False, mail_auto_delete=True): """ Method to send email linked to notified messages. The recipients are the recordset on which this method is called. :param message: mail.message record to notify; + :param rdata: recipient data (see mail.message _notify); :param record: optional record on which the message was posted; :param force_send: tells whether to send notification emails within the current transaction or to use the email queue; @@ -94,7 +95,7 @@ class Partner(models.Model): notification templates); :param mail_auto_delete: delete notification emails once sent; """ - if not self.ids: + if not rdata: return True template_xmlid = message.layout if message.layout else 'mail.message_notification_email' @@ -117,9 +118,11 @@ class Partner(models.Model): base_mail_values.update(self.env['mail.thread']._notify_specific_email_values_on_records(message, records=record)) # classify recipients: actions / no action - recipients = self.env['mail.thread']._notify_classify_recipients_on_records(message, self, records=record) + recipients = self.env['mail.thread']._notify_classify_recipients_on_records(message, rdata, records=record) - emails = self.env['mail.mail'] + Mail = self.env['mail.mail'].sudo() + emails = self.env['mail.mail'].sudo() + email_pids = set() recipients_nbr, recipients_max = 0, 50 for group_tpl_values in [group for group in recipients.values() if group['recipients']]: # generate notification email content @@ -129,7 +132,7 @@ class Partner(models.Model): mail_subject = message.subject or (message.record_name and 'Re: %s' % message.record_name) # send email - for email_chunk in split_every(50, group_tpl_values['recipients'].ids): + for email_chunk in split_every(50, group_tpl_values['recipients']): recipient_values = self.env['mail.thread']._notify_email_recipients_on_records(message, email_chunk, records=record) create_values = { 'body_html': mail_body, @@ -137,19 +140,23 @@ class Partner(models.Model): } create_values.update(base_mail_values) create_values.update(recipient_values) - emails |= self.env['mail.mail'].create(create_values) + recipient_ids = [r[1] for r in create_values.get('recipient_ids', [])] + email = Mail.create(create_values) - # update notifications - for email in emails: - notifications = self.env['mail.notification'].sudo().search([ - ('mail_message_id', '=', email.mail_message_id.id), - ('res_partner_id', 'in', email.recipient_ids.ids)]) - notifications.write({ - 'mail_id': email.id, - 'is_email': True, - 'is_read': True, # handle by email discards Inbox notification - 'email_status': 'ready', - }) + if email and recipient_ids: + notifications = self.env['mail.notification'].sudo().search([ + ('mail_message_id', '=', email.mail_message_id.id), + ('res_partner_id', 'in', list(recipient_ids)) + ]) + notifications.write({ + 'is_email': True, + 'mail_id': email.id, + 'is_read': True, # handle by email discards Inbox notification + 'email_status': 'ready', + }) + + emails |= email + email_pids.update(recipient_ids) # NOTE: # 1. for more than 50 followers, use the queue system diff --git a/addons/mail/wizard/invite.py b/addons/mail/wizard/invite.py index 541d3c1ea4f..39d6c0bdeeb 100644 --- a/addons/mail/wizard/invite.py +++ b/addons/mail/wizard/invite.py @@ -60,6 +60,11 @@ class Invite(models.TransientModel): 'no_auto_thread': True, 'add_sign': True, }) - new_partners.with_context(auto_delete=True)._notify(message, document, force_send=True, send_after_commit=False) + self.env['res.partner'].with_context(auto_delete=True)._notify( + message, + [{'id': pid, 'share': True, 'notif': 'email', 'type': 'customer', 'groups': []} for pid in new_partners.ids], + document, + force_send=True, + send_after_commit=False) message.unlink() return {'type': 'ir.actions.act_window_close'} diff --git a/addons/mail/wizard/mail_resend_message.py b/addons/mail/wizard/mail_resend_message.py index bf56128ad61..77b1a2a102d 100644 --- a/addons/mail/wizard/mail_resend_message.py +++ b/addons/mail/wizard/mail_resend_message.py @@ -60,7 +60,26 @@ class MailResendMessage(models.TransientModel): if to_send: message = wizard.mail_message_id record = self.env[message.model].browse(message.res_id) if message.model and message.res_id else None - to_send.sudo()._notify(self.mail_message_id, record, force_send=True, send_after_commit=False) + + rdata = [] + for pid, cid, active, pshare, ctype, notif, groups in self.env['mail.followers']._get_recipient_data(None, False, pids=to_send.ids): + if pid and notif == 'email' or not notif: + pdata = {'id': pid, 'share': pshare, 'active': active, 'notif': 'email', 'groups': groups} + if not pshare and notif: # has an user and is not shared, is therefore user + rdata.append(dict(pdata, type='user')) + elif pshare and notif: # has an user and is shared, is therefore portal + rdata.append(dict(pdata, type='portal')) + else: # has no user, is therefore customer + rdata.append(dict(pdata, type='customer')) + + self.env['res.partner']._notify( + message, + rdata, + record, + force_send=True, + send_after_commit=False + ) + self.mail_message_id._notify_failure_update() return {'type': 'ir.actions.act_window_close'} diff --git a/addons/portal/models/portal_mixin.py b/addons/portal/models/portal_mixin.py index 2a0c0d2a5a4..a03794b5dae 100644 --- a/addons/portal/models/portal_mixin.py +++ b/addons/portal/models/portal_mixin.py @@ -73,7 +73,7 @@ class PortalMixin(models.AbstractModel): access_link = self._notify_get_action_link('view', **additional_params) new_group = [ - ('portal_customer', lambda partner: partner.id == customer.id, { + ('portal_customer', lambda pdata: pdata['id'] == customer.id, { 'has_button_access': False, 'button_access': { 'url': access_link, diff --git a/addons/project/models/project.py b/addons/project/models/project.py index 373e6e0eee7..c5c5370d78a 100644 --- a/addons/project/models/project.py +++ b/addons/project/models/project.py @@ -829,8 +829,9 @@ class Task(models.Model): if not self.user_id and not self.stage_id.fold: take_action = self._notify_get_action_link('assign') project_actions = [{'url': take_action, 'title': _('I take it')}] + project_user_group_id = self.env.ref('project.group_project_user').id new_group = ( - 'group_project_user', lambda partner: bool(partner.user_ids) and any(user.has_group('project.group_project_user') for user in partner.user_ids), { + 'group_project_user', lambda pdata: pdata['type'] == 'user' and project_user_group_id in pdata['groups'], { 'actions': project_actions, }) groups = [new_group] + groups diff --git a/addons/test_mail/tests/test_performance.py b/addons/test_mail/tests/test_performance.py index 95e950fa97c..94f5fcd7817 100644 --- a/addons/test_mail/tests/test_performance.py +++ b/addons/test_mail/tests/test_performance.py @@ -199,7 +199,7 @@ class TestAdvMailPerformance(TransactionCase): self.user_test.write({'notification_type': 'email'}) record = self.env['mail.test.track'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=59, emp=76): # com runbot: 59 - 76 // test_mail only: 59 - 76 + with self.assertQueryCount(margin=1, admin=56, emp=71): # com runbot: 56 - 71 // test_mail only: 56 - 71 record.write({ 'user_id': self.user_test.id, }) @@ -253,7 +253,7 @@ class TestAdvMailPerformance(TransactionCase): def test_message_post_one_email_notification(self): record = self.env['mail.test.simple'].create({'name': 'Test'}) - with self.assertQueryCount(margin=1, admin=50, emp=68): # com runbot: 48 - 66 // test_mail only: 50 - 68 + with self.assertQueryCount(margin=1, admin=47, emp=64): # test_mail only: 47 - 64 record.message_post( body='

Test Post Performances with an email ping

', partner_ids=self.customer.ids, @@ -389,7 +389,7 @@ class TestHeavyMailPerformance(TransactionCase): self.umbrella.message_subscribe(self.user_portal.partner_id.ids) record = self.umbrella.sudo(self.env.user) - with self.assertQueryCount(admin=82, emp=103): # com runbot 80 - 101 // test_mail only: 82 - 103 + with self.assertQueryCount(admin=79, emp=99): # com runbot 77 - 97 // test_mail only: 79 - 99 record.message_post( body='

Test Post Performances

', message_type='comment', @@ -406,7 +406,7 @@ class TestHeavyMailPerformance(TransactionCase): record = self.umbrella.sudo(self.env.user) template_id = self.env.ref('test_mail.mail_test_tpl').id - with self.assertQueryCount(admin=101, emp=134): # com runbot 99 - 132 // test_mail only: 101 - 134 + with self.assertQueryCount(admin=98, emp=130): # com runbot 96 - 128 // test_mail only: 98 - 130 record.message_post_with_template(template_id, message_type='comment', composition_mode='comment') self.assertEqual(record.message_ids[0].body, '

Adding stuff on %s

' % record.name) @@ -476,7 +476,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id) - with self.assertQueryCount(admin=61, emp=78): # com runbot: 61 - 78 // test_mail only: 61 - 78 + with self.assertQueryCount(admin=58, emp=73): # com runbot: 58 - 73 // test_mail only: 58 - 73 rec.write({'user_id': self.user_portal.id}) self.assertEqual(rec.message_partner_ids, self.partners | self.env.user.partner_id | self.user_portal.partner_id) @@ -499,7 +499,7 @@ class TestHeavyMailPerformance(TransactionCase): customer_id = self.customer.id user_id = self.user_portal.id - with self.assertQueryCount(margin=1, admin=160, emp=193): # com runbot: 160 - 193 // test_mail only: 160 - 193 + with self.assertQueryCount(margin=1, admin=155, emp=184): # com runbot: 154 - 183 // test_mail only: 155 - 184 rec = self.env['mail.test.full'].create({ 'name': 'Test', 'umbrella_id': umbrella_id, @@ -528,7 +528,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=96, emp=112): # com runbot: 96 - 112 // test_mail only: 96 - 112 + with self.assertQueryCount(margin=1, admin=94, emp=109): # com runbot: 94 - 109 // test_mail only: 94 - 109 rec.write({ 'name': 'Test2', 'umbrella_id': self.umbrella.id, @@ -566,7 +566,7 @@ class TestHeavyMailPerformance(TransactionCase): }) self.assertEqual(rec.message_partner_ids, self.user_portal.partner_id | self.env.user.partner_id) - with self.assertQueryCount(margin=1, admin=103, emp=122): # test_mail only: 103 - 122 + with self.assertQueryCount(margin=1, admin=100, emp=119): # test_mail only: 100 - 119 rec.write({ 'name': 'Test2', 'umbrella_id': umbrella_id, diff --git a/addons/website_blog/models/website_blog.py b/addons/website_blog/models/website_blog.py index 6ff422579fd..b1e0253b385 100644 --- a/addons/website_blog/models/website_blog.py +++ b/addons/website_blog/models/website_blog.py @@ -256,12 +256,11 @@ class BlogPost(models.Model): return groups @api.multi - def message_get_message_notify_values(self, message, message_values): + def _notify_customize_recipients(self, message, msg_vals, recipients_vals): """ Override to avoid keeping all notified recipients of a comment. We avoid tracking needaction on post comments. Only emails should be sufficient. """ - if message.message_type == 'comment': - return { - 'needaction_partner_ids': [], - } + msg_type = msg_vals.get('message_type') or message.message_type + if msg_type == 'comment': + return {'needaction_partner_ids': []} return {} diff --git a/addons/website_forum/models/forum.py b/addons/website_forum/models/forum.py index 74af33b462c..b9368c8d4d8 100644 --- a/addons/website_forum/models/forum.py +++ b/addons/website_forum/models/forum.py @@ -833,15 +833,13 @@ class Post(models.Model): return super(Post, self).message_post(message_type=message_type, **kwargs) @api.multi - def message_get_message_notify_values(self, message, message_values): + def _notify_customize_recipients(self, message, msg_vals, recipients_vals): """ Override to avoid keeping all notified recipients of a comment. We avoid tracking needaction on post comments. Only emails should be sufficient. """ - if message.message_type == 'comment': - return { - 'needaction_partner_ids': [], - 'partner_ids': [], - } + msg_type = msg_vals.get('message_type') or message.message_type + if msg_type == 'comment': + return {'needaction_partner_ids': [], 'partner_ids': []} return {}