[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

@@ -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)

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 = "<p>-- <br/>%s</p>" % 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 = "<p>-- <br/>%s</p>" % 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

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'}

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'}
 

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='<p>Test Post Performances with an email ping</p>',
                 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='<p>Test Post Performances</p>',
                 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, '<p>Adding stuff on %s</p>' % 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,