From b4b31d867ca5802d6a1d621b5a8d427da47631ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 26 Aug 2020 12:45:33 +0000 Subject: [PATCH 1/3] [IMP] test_mail: add some followers test about active flag There is no test ensuring standard message_subscribe API does not subscribe inactive partners. As this is part of this method purpose, let us add a test to avoid any regression. LINKS Task ID-2326281 PR odoo/odoo#56560 X-original-commit: f45d7ace687911f4090c081bdc0e8127c382bd64 --- addons/test_mail/tests/test_mail_followers.py | 38 ++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/addons/test_mail/tests/test_mail_followers.py b/addons/test_mail/tests/test_mail_followers.py index 7669394e875..5bac6d00529 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 From 285aa27da47fe1983993acc0efd91d0ccbf28e99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 26 Aug 2020 12:45:54 +0000 Subject: [PATCH 2/3] [IMP] mail: do not auto-subscribe 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 X-original-commit: 42910ce25f06a31af3cfbc8d5d41e443ee754c34 --- addons/mail/models/mail_followers.py | 15 ++++++---- addons/mail/models/mail_thread.py | 10 +++---- addons/test_mail/tests/test_mail_followers.py | 28 +++++++++++++++++-- 3 files changed, 41 insertions(+), 12 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 24d85f79f70..2decfac6c44 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -1844,8 +1844,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) @@ -2769,11 +2769,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 5bac6d00529..fe8c5f38d96 100644 --- a/addons/test_mail/tests/test_mail_followers.py +++ b/addons/test_mail/tests/test_mail_followers.py @@ -187,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') @@ -221,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', @@ -238,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) From cc5ccf972d0f42ec1bc0d40a364edc6ec4dec70f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 26 Aug 2020 13:54:30 +0000 Subject: [PATCH 3/3] [IMP] mail: subscribe creator of document through mail gateway if not superuser 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. LINKS Task ID-2326281 PR odoo/odoo#56560 Closes odoo/odoo#38383 X-original-commit: fe27f9fe0cf9581c15a2a9e45125abe82dd303ad --- addons/mail/models/mail_thread.py | 6 +- addons/test_mail/tests/test_mail_gateway.py | 67 +++++++++++++++++---- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/addons/mail/models/mail_thread.py b/addons/mail/models/mail_thread.py index 2decfac6c44..0ba6214de90 100644 --- a/addons/mail/models/mail_thread.py +++ b/addons/mail/models/mail_thread.py @@ -1006,6 +1006,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( @@ -1015,7 +1016,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) @@ -1048,6 +1049,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: 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') + """ 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') - # 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)') + 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