[MERGE] mail, various: improve follower subscription in gateway

SPECIFICATIONS: MAIL GATEWAY

Document creation or update is still done without auto subscribe. Indeed user
running mailgateway or owning alias is not necessarily linked to the email
author. That way we avoid auto subscription of irrelevant people.

Posting message based on incoming email is now allowing auto subscription if
there is an author found during email parsing. We also ensure this author is
not root, to be sure he is not added in followers of documents.

SPECIFICATIONS: INACTIVE PARTNERS

Tested flows

  * posting a message through an inactive partner (like automated actions
    posting a message on behalf of an inactive partner);
  * automatic subscription based on parent record (like an archived user and
    partner following a project that should not be added as follower of sub
    tasks);
  * automatic subscription based on responsible field: this is already fixed
    as user has to be active to receive a notification and be added in
    followers;

LINKS

Task ID-2326281
PR odoo/odoo#56560
Closes to odoo/odoo#38383

Signed-off-by: Thibault Delavallee (tde) <tde@openerp.com>

4 files changed, 135 insertions, 31 deletions

diff --git a/addons/mail/models/mail_followers.py b/addons/mail/models/mail_followers.py
index cdbdca86066..6c526375584 100644
--- a/addons/mail/models/mail_followers.py
+++ b/addons/mail/models/mail_followers.py
@@ -193,7 +193,7 @@ FROM mail_channel channel WHERE channel.id IN %s """
             res = []
         return res
 
-    def _get_subscription_data(self, doc_data, pids, cids, include_pshare=False):
+    def _get_subscription_data(self, doc_data, pids, cids, include_pshare=False, include_active=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.
 
@@ -202,6 +202,7 @@ FROM mail_channel channel WHERE channel.id IN %s """
         :param pids: optional partner to filter; if None take all, otherwise limitate to pids
         :param cids: optional channel to filter; if None take all, otherwise limitate to cids
         :param include_pshare: optional join in partner to fetch their share status
+        :param include_active: optional join in partner to fetch their active flag
 
         :return: list of followers data which is a list of tuples containing
           follower ID,
@@ -210,6 +211,7 @@ FROM mail_channel channel WHERE channel.id IN %s """
           channel ID (void if partner_id),
           followed subtype IDs,
           share status of partner (void id channel_id, returned only if include_pshare is True)
+          active flag status of partner (void id channel_id, returned only if include_active is True)
         """
         # base query: fetch followers of given documents
         where_clause = ' OR '.join(['fol.res_model = %s AND fol.res_id IN %s'] * len(doc_data))
@@ -231,17 +233,20 @@ FROM mail_channel channel WHERE channel.id IN %s """
             where_clause += "AND (%s)" % " OR ".join(sub_where)
 
         query = """
-SELECT fol.id, fol.res_id, fol.partner_id, fol.channel_id, array_agg(subtype.id)%s
+SELECT fol.id, fol.res_id, fol.partner_id, fol.channel_id, array_agg(subtype.id)%s%s
 FROM mail_followers fol
 %s
 LEFT JOIN mail_followers_mail_message_subtype_rel fol_rel ON fol_rel.mail_followers_id = fol.id
 LEFT JOIN mail_message_subtype subtype ON subtype.id = fol_rel.mail_message_subtype_id
 WHERE %s
-GROUP BY fol.id%s""" % (
+GROUP BY fol.id%s%s""" % (
             ', partner.partner_share' if include_pshare else '',
-            'LEFT JOIN res_partner partner ON partner.id = fol.partner_id' if include_pshare else '',
+            ', partner.active' if include_active else '',
+            'LEFT JOIN res_partner partner ON partner.id = fol.partner_id' if (include_pshare or include_active) else '',
             where_clause,
-            ', partner.partner_share' if include_pshare else '')
+            ', partner.partner_share' if include_pshare else '',
+            ', partner.active' if include_active else ''
+        )
         self.env.cr.execute(query, tuple(where_params))
         return self.env.cr.fetchall()
 
diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py
index 96a78aa6079..24adb15b450 100644
--- a/addons/mail/models/mail_thread.py
+++ b/addons/mail/models/mail_thread.py
@@ -1027,6 +1027,7 @@ class MailThread(models.AbstractModel):
         thread_id = False
         for model, thread_id, custom_values, user_id, alias in routes or ():
             subtype_id = False
+            related_user = self.env['res.users'].browse(user_id)
             Model = self.env[model].with_context(mail_create_nosubscribe=True, mail_create_nolog=True)
             if not (thread_id and hasattr(Model, 'message_update') or hasattr(Model, 'message_new')):
                 raise ValueError(
@@ -1036,7 +1037,7 @@ class MailThread(models.AbstractModel):
 
             # disabled subscriptions during message_new/update to avoid having the system user running the
             # email gateway become a follower of all inbound messages
-            ModelCtx = Model.with_user(user_id).sudo()
+            ModelCtx = Model.with_user(related_user).sudo()
             if thread_id and hasattr(ModelCtx, 'message_update'):
                 thread = ModelCtx.browse(thread_id)
                 thread.message_update(message_dict)
@@ -1069,6 +1070,9 @@ class MailThread(models.AbstractModel):
             if thread._name == 'mail.thread':  # message with parent_id not linked to record
                 new_msg = thread.message_notify(**post_params)
             else:
+                # parsing should find an author independently of user running mail gateway, and ensure it is not odoobot
+                partner_from_found = message_dict.get('author_id') and message_dict['author_id'] != self.env['ir.model.data'].xmlid_to_res_id('base.partner_root')
+                thread = thread.with_context(mail_create_nosubscribe=not partner_from_found)
                 new_msg = thread.message_post(**post_params)
 
             if new_msg and original_partner_ids:
@@ -1870,8 +1874,8 @@ class MailThread(models.AbstractModel):
         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']])
+            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, **notif_kwargs)
@@ -2801,11 +2805,11 @@ class MailThread(models.AbstractModel):
 
         if udpated_fields:
             doc_data = [(model, [updated_values[fname] for fname in fnames]) for model, fnames in updated_relation.items()]
-            res = self.env['mail.followers']._get_subscription_data(doc_data, None, None, include_pshare=True)
-            for fid, rid, pid, cid, subtype_ids, pshare in res:
+            res = self.env['mail.followers']._get_subscription_data(doc_data, None, None, include_pshare=True, include_active=True)
+            for fid, rid, pid, cid, subtype_ids, pshare, active in res:
                 sids = [parent[sid] for sid in subtype_ids if parent.get(sid)]
                 sids += [sid for sid in subtype_ids if sid not in parent and sid in def_ids or sid in int_ids]
-                if pid:
+                if pid and active:  # auto subscribe only active partners
                     new_partners[pid] = (set(sids) & set(all_ids)) - set(int_ids) if pshare else set(sids) & set(all_ids)
                 if cid:
                     new_channels[cid] = (set(sids) & set(all_ids)) - set(int_ids)
diff --git a/addons/test_mail/tests/test_mail_followers.py b/addons/test_mail/tests/test_mail_followers.py
index 7669394e875..fe8c5f38d96 100644
--- a/addons/test_mail/tests/test_mail_followers.py
+++ b/addons/test_mail/tests/test_mail_followers.py
@@ -3,7 +3,7 @@
 
 from psycopg2 import IntegrityError
 
-from odoo.tests import tagged
+from odoo.tests import tagged, users
 from odoo.addons.test_mail.tests.common import TestMailCommon
 from odoo.tools.misc import mute_logger
 
@@ -17,13 +17,20 @@ class BaseFollowersTest(TestMailCommon):
         cls._create_portal_user()
         cls._create_channel_listener()
 
+        # allow employee to update partners
+        cls.user_employee.write({'groups_id': [(4, cls.env.ref('base.group_partner_manager').id)]})
+
         Subtype = cls.env['mail.message.subtype']
-        cls.mt_mg_def = Subtype.create({'name': 'mt_mg_def', 'default': True, 'res_model': 'mail.test.simple'})
-        cls.mt_cl_def = Subtype.create({'name': 'mt_cl_def', 'default': True, 'res_model': 'mail.test.container'})
+        # global
         cls.mt_al_def = Subtype.create({'name': 'mt_al_def', 'default': True, 'res_model': False})
-        cls.mt_mg_nodef = Subtype.create({'name': 'mt_mg_nodef', 'default': False, 'res_model': 'mail.test.simple'})
         cls.mt_al_nodef = Subtype.create({'name': 'mt_al_nodef', 'default': False, 'res_model': False})
-        cls.mt_mg_def_int = cls.env['mail.message.subtype'].create({'name': 'mt_mg_def', 'default': True, 'res_model': 'mail.test.simple', 'internal': True})
+        # mail.test.simple
+        cls.mt_mg_def = Subtype.create({'name': 'mt_mg_def', 'default': True, 'res_model': 'mail.test.simple'})
+        cls.mt_mg_nodef = Subtype.create({'name': 'mt_mg_nodef', 'default': False, 'res_model': 'mail.test.simple'})
+        cls.mt_mg_def_int = Subtype.create({'name': 'mt_mg_def', 'default': True, 'res_model': 'mail.test.simple', 'internal': True})
+        # mail.test.container
+        cls.mt_cl_def = Subtype.create({'name': 'mt_cl_def', 'default': True, 'res_model': 'mail.test.container'})
+
         cls.default_group_subtypes = Subtype.search([('default', '=', True), '|', ('res_model', '=', 'mail.test.simple'), ('res_model', '=', False)])
         cls.default_group_subtypes_portal = Subtype.search([('internal', '=', False), ('default', '=', True), '|', ('res_model', '=', 'mail.test.simple'), ('res_model', '=', False)])
 
@@ -127,6 +134,27 @@ class BaseFollowersTest(TestMailCommon):
             channel_ids=[self.channel_listen.id]
         )
 
+    @users('employee')
+    def test_followers_inactive(self):
+        """ Test standard API does not subscribe inactive partners """
+        customer = self.env['res.partner'].create({
+            'name': 'Valid Lelitre',
+            'email': 'valid.lelitre@agrolait.com',
+            'country_id': self.env.ref('base.be').id,
+            'mobile': '0456001122',
+            'active': False,
+        })
+        document = self.env['mail.test.simple'].browse(self.test_record.id)
+        self.assertEqual(document.message_partner_ids, self.env['res.partner'])
+        document.message_subscribe(partner_ids=(self.partner_portal | customer).ids)
+        self.assertEqual(document.message_partner_ids, self.partner_portal)
+        self.assertEqual(document.message_follower_ids.partner_id, self.partner_portal)
+
+        # works through low-level API
+        document._message_subscribe(partner_ids=(self.partner_portal | customer).ids)
+        self.assertEqual(document.message_partner_ids, self.partner_portal, 'No active test: customer not visible')
+        self.assertEqual(document.message_follower_ids.partner_id, self.partner_portal | customer)
+
 
 class AdvancedFollowersTest(TestMailCommon):
     @classmethod
@@ -159,6 +187,22 @@ class AdvancedFollowersTest(TestMailCommon):
         """ Creator of records are automatically added as followers """
         self.assertEqual(self.test_track.message_partner_ids, self.user_employee.partner_id)
 
+    def test_auto_subscribe_inactive(self):
+        """ Test inactive are not added as followers in automated subscription """
+        self.test_track.user_id = False
+        self.user_admin.active = False
+        self.user_admin.flush()
+        self.partner_admin.active = False
+        self.partner_admin.flush()
+
+        self.test_track.with_user(self.user_admin).message_post(body='Coucou hibou', message_type='comment')
+        self.assertEqual(self.test_track.message_partner_ids, self.user_employee.partner_id)
+        self.assertEqual(self.test_track.message_follower_ids.partner_id, self.user_employee.partner_id)
+
+        self.test_track.write({'user_id': self.user_admin.id})
+        self.assertEqual(self.test_track.message_partner_ids, self.user_employee.partner_id)
+        self.assertEqual(self.test_track.message_follower_ids.partner_id, self.user_employee.partner_id)
+
     def test_auto_subscribe_post(self):
         """ People posting a message are automatically added as followers """
         self.test_track.with_user(self.user_admin).message_post(body='Coucou hibou', message_type='comment')
@@ -193,13 +237,20 @@ class AdvancedFollowersTest(TestMailCommon):
            automatically create subscription with matching subtypes
          * subscribing to a sub-record as creator applies default subtype values
          * portal user should not have access to internal subtypes
+
+        Inactive partners should not be auto subscribed.
         """
         container = self.env['mail.test.container'].with_context(self._test_context).create({
             'name': 'Project-Like',
         })
 
-        container.message_subscribe(partner_ids=[self.partner_portal.id])
-        self.assertEqual(container.message_partner_ids, self.partner_portal)
+        container.message_subscribe(partner_ids=(self.partner_portal | self.partner_admin).ids)
+        self.assertEqual(container.message_partner_ids, self.partner_portal | self.partner_admin)
+
+        self.user_admin.active = False
+        self.user_admin.flush()
+        self.partner_admin.active = False
+        self.partner_admin.flush()
 
         sub1 = self.env['mail.test.track'].with_user(self.user_employee).create({
             'name': 'Task-Like Test',
@@ -210,6 +261,7 @@ class AdvancedFollowersTest(TestMailCommon):
         external_defaults = all_defaults.filtered(lambda subtype: not subtype.internal)
 
         self.assertEqual(sub1.message_partner_ids, self.partner_portal | self.user_employee.partner_id)
+        self.assertEqual(sub1.message_follower_ids.partner_id, self.partner_portal | self.user_employee.partner_id)
         self.assertEqual(
             sub1.message_follower_ids.filtered(lambda fol: fol.partner_id == self.partner_portal).subtype_ids,
             external_defaults | self.sub_umb1)
diff --git a/addons/test_mail/tests/test_mail_gateway.py b/addons/test_mail/tests/test_mail_gateway.py
index b615ceb5ff5..630ccf47ffa 100644
--- a/addons/test_mail/tests/test_mail_gateway.py
+++ b/addons/test_mail/tests/test_mail_gateway.py
@@ -223,19 +223,62 @@ class TestMailgateway(TestMailCommon):
             set(['rosaçée.gif', 'verte!µ.gif', 'orangée.gif']))
 
     def test_message_process_followers(self):
-        pass
-        # TODO : the author of a message post should be added as follower
-        # currently it is not the case as otherwise Administrator would be follower of a lot of stuff
-        # this is a bug with mail_create_nosubscribe -> should be changed in master
-        # self.assertEqual(record.message_partner_ids, self.partner_1,
-        #                  'message_process: recognized email -> added as follower')
-
-        # TODO : the author of a message post on mail.test should not be added as follower
-        # Test: author (and not recipient) added as follower
-        # self.assertEqual(self.test_public.message_partner_ids, self.partner_1 | self.partner_2,
-        #                  'message_process: after reply, group should have 2 followers')
-        # self.assertEqual(self.test_public.message_channel_ids, self.env['mail.test.container'],
-        #                  'message_process: after reply, group should have 2 followers (0 channels)')
+        """ Incoming email: recognized author not archived and not odoobot: added as follower """
+        with self.mock_mail_gateway():
+            record = self.format_and_process(MAIL_TEMPLATE, self.partner_1.email_formatted, 'groups@test.com')
+
+        self.assertEqual(record.message_ids[0].author_id, self.partner_1,
+                         'message_process: recognized email -> author_id')
+        self.assertEqual(record.message_ids[0].email_from, self.partner_1.email_formatted)
+        self.assertEqual(record.message_follower_ids.partner_id, self.partner_1,
+                         'message_process: recognized email -> added as follower')
+        self.assertEqual(record.message_partner_ids, self.partner_1,
+                         'message_process: recognized email -> added as follower')
+
+        # just an email -> no follower
+        with self.mock_mail_gateway():
+            record2 = self.format_and_process(
+                MAIL_TEMPLATE, self.email_from, 'groups@test.com',
+                subject='Another Email')
+
+        self.assertEqual(record2.message_ids[0].author_id, self.env['res.partner'])
+        self.assertEqual(record2.message_ids[0].email_from, self.email_from)
+        self.assertEqual(record2.message_follower_ids.partner_id, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
+        self.assertEqual(record2.message_partner_ids, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
+
+        # archived partner -> no follower
+        self.partner_1.active = False
+        self.partner_1.flush()
+        with self.mock_mail_gateway():
+            record3 = self.format_and_process(
+                MAIL_TEMPLATE, self.partner_1.email_formatted, 'groups@test.com',
+                subject='Yet Another Email')
+
+        self.assertEqual(record3.message_ids[0].author_id, self.env['res.partner'])
+        self.assertEqual(record3.message_ids[0].email_from, self.partner_1.email_formatted)
+        self.assertEqual(record3.message_follower_ids.partner_id, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
+        self.assertEqual(record3.message_partner_ids, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
+
+
+        # partner_root -> never again
+        odoobot = self.env.ref('base.partner_root')
+        odoobot.active = True
+        odoobot.email = 'odoobot@example.com'
+        with self.mock_mail_gateway():
+            record4 = self.format_and_process(
+                MAIL_TEMPLATE, odoobot.email_formatted, 'groups@test.com',
+                subject='Odoobot Automatic Answer')
+
+        self.assertEqual(record4.message_ids[0].author_id, odoobot)
+        self.assertEqual(record4.message_ids[0].email_from, odoobot.email_formatted)
+        self.assertEqual(record4.message_follower_ids.partner_id, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
+        self.assertEqual(record4.message_partner_ids, self.env['res.partner'],
+                         'message_process: unrecognized email -> no follower')
 
     # --------------------------------------------------
     # Author recognition