[REF] mail: improve message_post and message_notify API

Message post should always be called on a record (ensure_one). That way we
ensure posting a message is always done in a record's context with right
values computed (reply_to, followers, ...)

Message_notify can be called on record or on mail_thread and must have
partner_ids. It is based on the recently modified user_notification mechanism
and allow to notify a partner on a record or just to push him a message
(aka, not linked to a record).

Small performance improvement

  * browse recipients instead of search in _notify_email_recipients;
  * todo in future optimizations: mayybe be improve by searching on ids
    and is_blacklist immediately;

Related to task 1943901
Linked to PR #32404

5 files changed, 163 insertions, 124 deletions

diff --git a/addons/mail/models/mail_activity.py b/addons/mail/models/mail_activity.py
index ab098ed52a1..6b9ce7aacf2 100644
--- a/addons/mail/models/mail_activity.py
+++ b/addons/mail/models/mail_activity.py
@@ -387,14 +387,15 @@ class MailActivity(models.Model):
                 minimal_qcontext=True
             )
             record = self.env[activity.res_model].browse(activity.res_id)
-            record.message_notify(
-                partner_ids=activity.user_id.partner_id.ids,
-                body=body,
-                subject=_('%s: %s assigned to you') % (activity.res_name, activity.summary or activity.activity_type_id.name),
-                record_name=activity.res_name,
-                model_description=model_description,
-                notif_layout='mail.mail_notification_light',
-            )
+            if activity.user_id:
+                record.message_notify(
+                    partner_ids=activity.user_id.partner_id.ids,
+                    body=body,
+                    subject=_('%s: %s assigned to you') % (activity.res_name, activity.summary or activity.activity_type_id.name),
+                    record_name=activity.res_name,
+                    model_description=model_description,
+                    notif_layout='mail.mail_notification_light',
+                )
 
     @api.multi
     def action_done(self):
diff --git a/addons/mail/models/mail_channel.py b/addons/mail/models/mail_channel.py
index 6c1856154e1..2c57c93d00b 100644
--- a/addons/mail/models/mail_channel.py
+++ b/addons/mail/models/mail_channel.py
@@ -348,7 +348,7 @@ class Channel(models.Model):
     @api.multi
     def _notify_email_recipients(self, recipient_ids):
         # Excluded Blacklisted
-        whitelist = self.env['res.partner'].sudo().search([('id', 'in', recipient_ids)]).filtered(lambda p: not p.is_blacklisted)
+        whitelist = self.env['res.partner'].sudo().browse(recipient_ids).filtered(lambda p: not p.is_blacklisted)
         # real mailing list: multiple recipients (hidden by X-Forge-To)
         if self.alias_domain and self.alias_name:
             return {
@@ -525,6 +525,18 @@ class Channel(models.Model):
         notifications = self._channel_message_notifications(message)
         self.env['bus.bus'].sendmany(notifications)
 
+    def _notify_thread(self, message, msg_vals, model_description=False, mail_auto_delete=True):
+        # When posting a message on a mail channel, manage moderation and postpone notify users
+        if msg_vals.get('moderation_status') != 'pending_moderation':
+            super(Channel, self)._notify_thread(
+                message,
+                msg_vals,
+                model_description=model_description,
+                mail_auto_delete=mail_auto_delete,
+            )
+        else:
+            message._notify_pending_by_chat()
+
     @api.multi
     def _channel_message_notifications(self, message):
         """ Generate the bus notifications for the given message
diff --git a/addons/mail/models/mail_message.py b/addons/mail/models/mail_message.py
index d974f772b0c..515fc5dcfbe 100644
--- a/addons/mail/models/mail_message.py
+++ b/addons/mail/models/mail_message.py
@@ -1106,18 +1106,17 @@ class Message(models.Model):
         """ 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
-
+        # get values from msg_vals or from message if msg_vals doen't exists
+        pids = msg_vals.get('partner_ids', []) if msg_vals else msg_sudo.partner_ids.ids
+        cids = msg_vals.get('channel_ids', []) if msg_vals else msg_sudo.channel_ids.ids
+        subtype_id = msg_vals.get('subtype_id') if msg_vals else msg_sudo.subtype_id.id
+        # is it possible to have record but no subtype_id ?
         recipient_data = {
             'partners': [],
             'channels': [],
         }
         res = []
-        user_notification = msg_vals.get('message_type') == 'user_notification'
-        res = self.env['mail.followers']._get_recipient_data(not user_notification and record, subtype_id, pids, cids)
+        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 and not self.env.context.get('mail_notify_author'):  # do not notify the author of its own messages
@@ -1169,13 +1168,18 @@ class Message(models.Model):
 
         # notify partners and channels
         if email_cids:
+            author_id = msg_vals.get('author_id') or self.author_id.id
+            email_from = msg_vals.get('email_from') or self.email_from
+            exept_partner = [r['id'] for r in rdata['partners']]
+            if author_id:
+                exept_partner.append(author_id)
             new_pids = self.env['res.partner'].sudo().search([
-                ('id', 'not in', [r['id'] for r in rdata['partners']]),
+                ('id', 'not in', exept_partner),
                 ('channel_ids', 'in', email_cids),
-                ('email', 'not in', [self.author_id.email, self.email_from]),
+                ('email', 'not in', [email_from]),
             ])
             for partner in new_pids:
-                rdata['partners'].append({'id': partner.id, 'share': True, 'notif': 'email', 'type': 'customer', 'groups': []})
+                rdata['partners'].append({'id': partner.id, 'share': True, 'active': True, 'notif': 'email', 'type': 'customer', 'groups': []})
 
         partner_email_rdata = [r for r in rdata['partners'] if r['notif'] == 'email']
         if partner_email_rdata:
@@ -1250,8 +1254,9 @@ class Message(models.Model):
         })
         # proceed with notification process to send notification emails and Inbox messages
         for message in self:
-            record = self.env[message.model].browse(message.res_id) if message.is_thread_message() else None
-            message._notify(record, {})
+            if message.is_thread_message(): # note, since we will only intercept _notify_thread for message posted on channel, 
+                # message will always be a thread_message. This check should always be true.
+                self.env[message.model].browse(message.res_id)._notify_thread(message, {})
 
     @api.multi
     def _moderate_send_reject_email(self, subject, comment):
@@ -1348,7 +1353,7 @@ class Message(models.Model):
         MailThread = self.env['mail.thread'].with_context(mail_notify_author=True)
         for moderator in moderators_to_notify:
             MailThread.message_notify(
-                moderator.partner_id.ids,
+                partner_ids=moderator.partner_id.ids,
                 subject=_('Message are pending moderation'),  # tocheck: target language
                 body=template.render({'record': moderator.partner_id}, engine='ir.qweb', minimal_qcontext=True),
                 email_from=moderator.company_id.catchall or moderator.company_id.email,
diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py
index 07fcca7cb70..9a7b157260c 100644
--- a/addons/mail/models/mail_thread.py
+++ b/addons/mail/models/mail_thread.py
@@ -1344,15 +1344,17 @@ class MailThread(models.AbstractModel):
                     subtype_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note')
                     if message_dict.get('parent_id'):
                         parent_message = self.env['mail.message'].sudo().browse(message_dict['parent_id'])
-                        if parent_message.author_id:
+                        if parent_message and parent_message.author_id:
                             partner_ids = [parent_message.author_id.id]
                 else:
                     subtype_id = self.env['ir.model.data'].xmlid_to_res_id('mail.mt_comment')
 
             post_params = dict(subtype_id=subtype_id, partner_ids=partner_ids, **message_dict)
-            if not hasattr(thread, 'message_post'):
-                post_params['model'] = model
-            new_msg = thread.message_post(**post_params)
+            new_msg = False
+            if thread._name == 'mail.thread':  # message with parent_id not linked to record
+                new_msg = thread.message_notify(**post_params)
+            else:
+                new_msg = thread.message_post(**post_params)
 
             if new_msg and original_partner_ids:
                 # postponed after message_post, because this is an external message and we don't want to create
@@ -1918,10 +1920,10 @@ class MailThread(models.AbstractModel):
 
     @api.multi
     @api.returns('mail.message', lambda value: value.id)
-    def message_post(self, body='', subject=None,
-                     message_type='notification', subtype=None,
-                     parent_id=False, attachments=None,
-                     notif_layout=False, add_sign=True, model_description=False,
+    def message_post(self, body='', subject=None, subtype_id=False,
+                     message_type='notification', subtype=None, author_id=None, partner_ids=None, channel_ids=None,
+                     parent_id=False, attachments=None, attachment_ids=None, model=False,
+                     add_sign=True, model_description=False,
                      mail_auto_delete=True, **kwargs):
         """ Post a new message in an existing thread, returning the new
             mail.message ID.
@@ -1940,52 +1942,33 @@ class MailThread(models.AbstractModel):
                     to the related document. Should only be set by Chatter.
             :return int: ID of newly created mail.message
         """
-        if attachments is None:
-            attachments = {}
-        if self.ids and not self.ensure_one():
-            raise exceptions.Warning(_('Invalid record set: should be called as model (without records) or on single-record recordset'))
+        self.ensure_one()  # should always be posted on a record, use message_notify if no record
 
-        # if we're processing a message directly coming from the gateway, the destination model was
-        # set in the context.
-        model = False
-        if self.ids:
-            self.ensure_one()  # not so usefull
-            model = kwargs.get('model', False) if self._name == 'mail.thread' else self._name
-            if model and model != self._name and hasattr(self.env[model], 'message_post'):
-                return self.env[model].browse(self.ids).message_post(
-                    body=body, subject=subject, message_type=message_type,
-                    subtype=subtype, parent_id=parent_id, attachments=attachments,
-                    notif_layout=notif_layout, add_sign=add_sign,
-                    mail_auto_delete=mail_auto_delete, model_description=model_description, **kwargs)
-
-        # 0: Find the message's author, because we need it for private discussion
-        author_id = kwargs.get('author_id')
+        if self._name == 'mail.thread' or not self.id or message_type == 'user_notification':
+            raise ValueError('message_post should only be call to post message on record. Use message_notify instead')
+
+        if 'model' in kwargs or 'res_id' in kwargs:
+            raise ValueError("message_post doesn't support model and res_id parameters anymore. Please call message_post on record")
+
+        # Find the message's author, because we need it for private discussion
         if author_id is None:  # keep False values
-            author_id = self.env['mail.message']._get_default_author().id
+            author_id = self.env['mail.message']._get_default_author().id # self.env.user.partner_id
 
-        partner_ids = set(kwargs.pop('partner_ids', []))
-        channel_ids = set(kwargs.pop('channel_ids', []))
+        partner_ids = set(partner_ids or [])
+        channel_ids = set(channel_ids or [])
 
-        if parent_id and not model:
-            parent_message = self.env['mail.message'].browse(parent_id)
-            private_followers = set([partner.id for partner in parent_message.partner_ids])
-            if parent_message.author_id:
-                private_followers.add(parent_message.author_id.id)
-            private_followers -= set([author_id])
-            partner_ids |= private_followers
+        for pc_id in partner_ids | channel_ids:
+            if not isinstance(pc_id, int):
+                raise ValueError('message_post partner_ids and channel_ids must be integer list, not commands')
 
-        # 4: mail.message.subtype
-        subtype_id = kwargs.get('subtype_id', False)
         if not subtype_id:
             subtype = subtype or 'mt_note'
             if '.' not in subtype:
                 subtype = 'mail.%s' % subtype
             subtype_id = self.env['ir.model.data'].xmlid_to_res_id(subtype)
 
-        user_notification = message_type == 'user_notification'
-
         # automatically subscribe recipients if asked to
-        if self._context.get('mail_post_autofollow') and self.ids and not user_notification and partner_ids:
+        if self._context.get('mail_post_autofollow') and partner_ids:
             partner_to_subscribe = partner_ids
             if self._context.get('mail_post_autofollow_partner_ids'):
                 partner_to_subscribe = [p for p in partner_ids if p in self._context.get('mail_post_autofollow_partner_ids')]
@@ -1993,8 +1976,8 @@ class MailThread(models.AbstractModel):
 
         # _mail_flat_thread: automatically set free messages to the first posted message
         MailMessage = self.env['mail.message']
-        if self._mail_flat_thread and model and not parent_id and self.ids and not user_notification:
-            messages = MailMessage.search(['&', ('res_id', '=', self.ids[0]), ('model', '=', model), ('message_type', '!=', 'user_notification')], order="id ASC", limit=1)
+        if self._mail_flat_thread and not parent_id:
+            messages = MailMessage.search(['&', ('res_id', '=', self.ids[0]), ('model', '=', self._name), ('message_type', '!=', 'user_notification')], order="id ASC", limit=1)
             parent_id = messages.ids and messages.ids[0] or False
         # we want to set a parent: force to set the parent_id to the oldest ancestor, to avoid having more than 1 level of thread
         elif parent_id:
@@ -2011,65 +1994,57 @@ class MailThread(models.AbstractModel):
         values = kwargs
         values.update({
             'author_id': author_id,
-            'model': model,
-            'res_id': model and self.ids[0] or False,
+            'model': self._name,
+            'res_id': self._name and self.id or False,
             'body': body,
             'subject': subject or False,
             'message_type': message_type,
             'parent_id': parent_id,
             'subtype_id': subtype_id,
-            'partner_ids': [(4, pid) for pid in partner_ids],
-            'channel_ids': [(4, cid) for cid in channel_ids],
-            'add_sign': add_sign
+            'partner_ids': partner_ids,
+            'channel_ids': channel_ids,
+            'add_sign': add_sign,
         })
-        if notif_layout:
-            values['layout'] = notif_layout
 
         # 3. Attachments
         #   - HACK TDE FIXME: Chatter: attachments linked to the document (not done JS-side), load the message
-        attachment_ids = self._message_post_process_attachments(attachments, kwargs.pop('attachment_ids', []), values)
-        values['attachment_ids'] = attachment_ids
 
-        # Avoid warnings about non-existing fields
-        for x in ('from', 'to', 'cc'):
-            values.pop(x, None)
-
-        # Post the message
-        # canned_response_ids are added by js to be used by other computations (odoobot)
-        # we need to pop it from values since it is not stored on mail.message
-        canned_response_ids = values.pop('canned_response_ids', False)
-        new_message = MailMessage.create(values)
-        values['canned_response_ids'] = canned_response_ids
-        record = self.env['mail.thread'] if user_notification else self
-        record._message_post_after_hook(new_message, values, model_description, mail_auto_delete)
+        values['attachment_ids'] = self._message_post_process_attachments(attachments or [], attachment_ids or [], values)
+        new_message = self._message_create(values)
+        # Set main attachment field if necessary
+        self._message_set_main_attachment_id(values['attachment_ids'])
+
+        if values['author_id'] and values['message_type'] != 'notification' and not self._context.get('mail_create_nosubscribe'):
+            #if self.env['res.partner'].browse(values['author_id']).active:  # we dont want to add odoobot/inactive as a follower
+            self._message_subscribe([values['author_id']])
+
+        self._message_post_after_hook(new_message, values)
+        self._notify_thread(new_message, values, model_description=model_description, mail_auto_delete=mail_auto_delete)
         return new_message
 
-    def _message_post_after_hook(self, message, msg_vals, model_description, mail_auto_delete):
-        """ 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. """
-        # Set main attachment field if necessary
-        attachment_ids = msg_vals['attachment_ids']
-        if not self._abstract and attachment_ids and self.ids and not self.message_main_attachment_id:
+    def _message_set_main_attachment_id(self, attachment_ids):  # todo move this out of mail.thread
+        if not self._abstract and attachment_ids and not self.message_main_attachment_id:
             all_attachments = self.env['ir.attachment'].browse([attachment_tuple[1] for attachment_tuple in attachment_ids])
             prioritary_attachments = all_attachments.filtered(lambda x: x.mimetype.endswith('pdf')) \
                                      or all_attachments.filtered(lambda x: x.mimetype.startswith('image')) \
                                      or all_attachments
             self.sudo().write({'message_main_attachment_id': prioritary_attachments[0].id})
-        # Notify recipients of the newly-created message (Inbox / Email + channels)
-        if msg_vals.get('moderation_status') != 'pending_moderation':
-            message._notify(
-                self, msg_vals,
-                force_send=self.env.context.get('mail_notify_force_send', True),
-                model_description=model_description,
-                mail_auto_delete=mail_auto_delete,
-            )
 
-            # Post-process: subscribe author
-            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()
+    def _message_post_after_hook(self, message, msg_vals):
+        """ 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. """
+        pass
+
+    @api.multi
+    def _notify_thread(self, message, msg_vals, model_description=False, mail_auto_delete=True):
+        # Can be overide to intercept and postpone notification mecanism
+        message._notify(
+            self, msg_vals,
+            force_send=self.env.context.get('mail_notify_force_send', True),
+            model_description=model_description,
+            mail_auto_delete=mail_auto_delete,
+        )
 
     @api.multi
     def message_post_with_view(self, views_or_xmlid, **kwargs):
@@ -2129,35 +2104,65 @@ class MailThread(models.AbstractModel):
             composer.write(update_values)
         return composer.send_mail()
 
-    def message_notify(self, partner_ids, body='', subject=False, **kwargs):
+    def message_notify(self, partner_ids=False, parent_id=False, model=False, res_id=False,
+                       author_id=False, body='', subject=False, model_description=False,
+                       mail_auto_delete=True, **kwargs):
         """ Shortcut allowing to notify partners of messages that shouldn't be 
         displayed on a document. It pushes notifications on inbox or by email depending
         on the user configuration, like other notifications. """
-        kw_author = kwargs.pop('author_id', False)
-        if kw_author:
-            author = self.env['res.partner'].sudo().browse(kw_author)
+
+        # should we handle channel_ids here?
+        if self:
+            self.ensure_one()
+
+        if author_id:
+            author = self.env['res.partner'].sudo().browse(author_id)
         else:
             author = self.env.user.partner_id
+
         if not author.email:
             raise exceptions.UserError(_("Unable to notify message, please configure the sender's email address."))
         email_from = formataddr((author.name, author.email))
 
-        msg_values = {
+        partner_ids = partner_ids or set()
+        if parent_id:  # looks like no test case are going throug this condition. Linked to private discussion. This may be removed soon.
+            parent_message = self.env['mail.message'].browse(parent_id)
+            private_followers = set([partner.id for partner in parent_message.partner_ids])
+            if parent_message.author_id:
+                private_followers.add(parent_message.author.id)
+            private_followers -= set([author.id])
+            partner_ids |= private_followers
+
+        if not partner_ids:
+            _logger.warning('Message notify called without recipient_ids, skipping')
+            return self.env['mail_message']
+
+        if not (model and res_id):  # both value should be set or none should be set (record)
+            model = False
+            res_id = False
+
+        MailThread = self.env['mail.thread']
+        values = {
+            'parent_id': parent_id,
+            'model': self._name if self else False,
+            'res_id': self.id if self else False,
+            'message_type': 'user_notification',
             'subject': subject,
             'body': body,
             'author_id': author.id,
             'email_from': email_from,
-            'message_type': 'user_notification',
             'partner_ids': partner_ids,
             'subtype_id': self.env['ir.model.data'].xmlid_to_res_id('mail.mt_note'),
             'record_name': False,
-            'reply_to': self.env['mail.thread']._notify_get_reply_to(default=email_from, records=None)[False],
+            'reply_to': MailThread._notify_get_reply_to(default=email_from, records=None)[False],
             'message_id': tools.generate_tracking_message_id('message-notify'),
         }
-        msg_values.update(kwargs)
-        return self.message_post(**msg_values)
+        values.update(kwargs)
+        new_message = MailThread._message_create(values)
+        MailThread._notify_thread(new_message, values, model_description=model_description, mail_auto_delete=mail_auto_delete)
+        return new_message
 
-    def _message_log(self, body='', subject=False, message_type='notification', **kwargs):
+    def _message_log(self, body='', author_id=None, subject=False, message_type='notification', **kwargs):
         """ Shortcut allowing to post note on a document. It does not perform
         any notification and pre-computes some values to have a short code
         as optimized as possible. This method is private as it does not check
@@ -2167,14 +2172,15 @@ class MailThread(models.AbstractModel):
         if len(self.ids) > 1:
             raise exceptions.Warning(_('Invalid record set: should be called as model (without records) or on single-record recordset'))
 
-        kw_author = kwargs.pop('author_id', False)
-        if kw_author:
-            author = self.env['res.partner'].sudo().browse(kw_author)
+        if author_id:
+            author = self.env['res.partner'].sudo().browse(author_id)
         else:
             author = self.env.user.partner_id
+            author_id = author.id
+
         if not author.email:
             raise exceptions.UserError(_("Unable to log message, please configure the sender's email address."))
-        email_from = formataddr((author.name, author.email))
+        email_from = author.email_formatted
 
         message_values = {
             'subject': subject,
@@ -2193,6 +2199,17 @@ class MailThread(models.AbstractModel):
         message = self.env['mail.message'].sudo().create(message_values)
         return message
 
+    def _message_create(self, values):
+        create_values = dict(values)
+        if 'notif_layout' in create_values:
+            create_values['layout'] = create_values.pop('notif_layout')
+        # Avoid warnings about non-existing fields
+        for x in ('from', 'to', 'cc', 'canned_response_ids'):
+            create_values.pop(x, None)
+        create_values['partner_ids'] = [(4, pid) for pid in create_values.get('partner_ids', [])]
+        create_values['channel_ids'] = [(4, cid) for cid in create_values.get('channel_ids', [])]
+        return self.env['mail.message'].create(create_values)
+
     # ------------------------------------------------------
     # Followers API
     # ------------------------------------------------------
diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py
index fe09f0ff18c..8053dfb4484 100644
--- a/addons/mail/wizard/mail_compose_message.py
+++ b/addons/mail/wizard/mail_compose_message.py
@@ -279,9 +279,13 @@ class MailComposer(models.TransientModel):
                             mail_auto_delete=wizard.template_id.auto_delete if wizard.template_id else False,
                             model_description=model_description,
                             **mail_values)
-                        if ActiveModel._name == 'mail.thread' and wizard.model:
-                            post_params['model'] = wizard.model
-                        ActiveModel.browse(res_id).message_post(**post_params)
+                        if ActiveModel._name == 'mail.thread':
+                            if wizard.model:
+                                post_params['model'] = wizard.model
+                                post_params['res_id'] = res_id
+                            ActiveModel.message_notify(**post_params)
+                        else:
+                            ActiveModel.browse(res_id).message_post(**post_params)
 
                 if wizard.composition_mode == 'mass_mail':
                     batch_mails.send(auto_commit=auto_commit)