From fca1654dc59cf1085c2d2fbe50bc25724bee17b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 11:13:49 +0200 Subject: [PATCH 01/11] [ADD] test_mass_mailing: add module to hold feature and performance tests for mass mailing Purpose of this commit is to add the skeleton of a module holding models and tests for mass mailing. It is build on top of test_mail in order to use data, models and parameters already defined in it. Moreover having a separate module allow to define test models that will not bloat production databases. It will soon hold unit tests for blacklist mechanism. Future commits will gradually move tests from mass mailing module and improve them. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/test_mass_mailing/__init__.py | 4 +++ addons/test_mass_mailing/__manifest__.py | 20 +++++++++++ addons/test_mass_mailing/models/__init__.py | 4 +++ .../models/mass_mail_test.py | 33 +++++++++++++++++++ .../security/ir.model.access.csv | 5 +++ addons/test_mass_mailing/tests/__init__.py | 2 ++ addons/test_mass_mailing/tests/common.py | 30 +++++++++++++++++ 7 files changed, 98 insertions(+) create mode 100644 addons/test_mass_mailing/__init__.py create mode 100644 addons/test_mass_mailing/__manifest__.py create mode 100644 addons/test_mass_mailing/models/__init__.py create mode 100644 addons/test_mass_mailing/models/mass_mail_test.py create mode 100644 addons/test_mass_mailing/security/ir.model.access.csv create mode 100644 addons/test_mass_mailing/tests/__init__.py create mode 100644 addons/test_mass_mailing/tests/common.py diff --git a/addons/test_mass_mailing/__init__.py b/addons/test_mass_mailing/__init__.py new file mode 100644 index 00000000000..dc5e6b693d1 --- /dev/null +++ b/addons/test_mass_mailing/__init__.py @@ -0,0 +1,4 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from . import models diff --git a/addons/test_mass_mailing/__manifest__.py b/addons/test_mass_mailing/__manifest__.py new file mode 100644 index 00000000000..8bac3f103f5 --- /dev/null +++ b/addons/test_mass_mailing/__manifest__.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- + +{ + 'name': 'Mass Mail Tests', + 'version': '1.0', + 'category': 'Hidden', + 'sequence': 8765, + 'summary': 'Mass Mail Tests: feature and performance tests for mass mailing', + 'description': """This module contains tests related to mass mailing. Those +are contained in a separate module to use specific test models defined in +test_mail. """, + 'depends': ['test_mail', 'mass_mailing'], + 'data': [ + 'security/ir.model.access.csv', + ], + 'demo': [ + ], + 'installable': True, + 'application': False, +} diff --git a/addons/test_mass_mailing/models/__init__.py b/addons/test_mass_mailing/models/__init__.py new file mode 100644 index 00000000000..ab1b44a2dea --- /dev/null +++ b/addons/test_mass_mailing/models/__init__.py @@ -0,0 +1,4 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from . import mass_mail_test diff --git a/addons/test_mass_mailing/models/mass_mail_test.py b/addons/test_mass_mailing/models/mass_mail_test.py new file mode 100644 index 00000000000..87e72497399 --- /dev/null +++ b/addons/test_mass_mailing/models/mass_mail_test.py @@ -0,0 +1,33 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo import api, fields, models + + +class MassMailTest(models.Model): + """ A very simple model only inheriting from mail.thread to test pure mass + mailing features and base performances. """ + _description = 'Simple Mass Mailing Model' + _name = 'mass.mail.test' + _inherit = ['mail.thread'] + + name = fields.Char() + email_from = fields.Char() + + +class MassMailTestBlacklist(models.Model): + """ Model using blacklist mechanism for mass mailing. """ + _description = 'Mass Mailing Model w Blacklist' + _name = 'mass.mail.test.bl' + _inherit = ['mail.thread', 'mail.blacklist.mixin'] + + _primary_email = ['email_from'] # blacklist field to check + + name = fields.Char() + email_from = fields.Char() + user_id = fields.Many2one( + 'res.users', 'Responsible', + track_visibility='onchange') + umbrella_id = fields.Many2one( + 'mail.test', 'Meta Umbrella Record', + track_visibility='onchange') diff --git a/addons/test_mass_mailing/security/ir.model.access.csv b/addons/test_mass_mailing/security/ir.model.access.csv new file mode 100644 index 00000000000..9383184cb02 --- /dev/null +++ b/addons/test_mass_mailing/security/ir.model.access.csv @@ -0,0 +1,5 @@ +id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink +access_mass_mail_test_all,mass.mail.test.all,model_mass_mail_test,,0,0,0,0 +access_mass_mail_test_user,mass.mail.test.user,model_mass_mail_test,base.group_user,1,1,1,1 +access_mass_mail_test_bl_all,mass.mail.test.bl.all,model_mass_mail_test_bl,,0,0,0,0 +access_mass_mail_test_bl_user,mass.mail.test.bl.user,model_mass_mail_test_bl,base.group_user,1,1,1,1 diff --git a/addons/test_mass_mailing/tests/__init__.py b/addons/test_mass_mailing/tests/__init__.py new file mode 100644 index 00000000000..67dee8c60db --- /dev/null +++ b/addons/test_mass_mailing/tests/__init__.py @@ -0,0 +1,2 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. diff --git a/addons/test_mass_mailing/tests/common.py b/addons/test_mass_mailing/tests/common.py new file mode 100644 index 00000000000..88a4ceb0735 --- /dev/null +++ b/addons/test_mass_mailing/tests/common.py @@ -0,0 +1,30 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo.addons.test_mail.tests import common +from odoo.tests import tagged + + +@tagged('mass_mail') +class MassMailingCase(common.MockEmails, common.BaseFunctionalTest): + + @classmethod + def setUpClass(cls): + super(MassMailingCase, cls).setUpClass() + + # be sure for some common data + cls.user_employee.write({ + 'login': 'emp', + }) + + cls.user_marketing = cls.env['res.users'].with_context(cls._quick_create_user_ctx).create({ + 'name': 'Ernest Employee', + 'login': 'marketing', + 'email': 'e.e@example.com', + 'signature': '--\nErnest', + 'notification_type': 'email', + 'groups_id': [ + (6, 0, [cls.env.ref('base.group_user').id]), + (6, 0, [cls.env.ref('mass_mailing.group_mass_mailing_user').id]) + ], + }) From 392283296779d068c4a4bca06f06fec3812fa088 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 13:25:57 +0200 Subject: [PATCH 02/11] [IMP] test_mass_mailing: add blacklist tests Purpose of this commit is to enforce blacklist use and base mechanisms. This commit may break on runbot as fixes will come after having introduced tests. It notably add tests for a8ce59ce364a011e9b677070503e7b1d76b32661. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/test_mass_mailing/tests/__init__.py | 2 + .../test_mass_mailing/tests/test_blacklist.py | 114 ++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 addons/test_mass_mailing/tests/test_blacklist.py diff --git a/addons/test_mass_mailing/tests/__init__.py b/addons/test_mass_mailing/tests/__init__.py index 67dee8c60db..9994621796b 100644 --- a/addons/test_mass_mailing/tests/__init__.py +++ b/addons/test_mass_mailing/tests/__init__.py @@ -1,2 +1,4 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. + +from . import test_blacklist diff --git a/addons/test_mass_mailing/tests/test_blacklist.py b/addons/test_mass_mailing/tests/test_blacklist.py new file mode 100644 index 00000000000..5032bd1e336 --- /dev/null +++ b/addons/test_mass_mailing/tests/test_blacklist.py @@ -0,0 +1,114 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from odoo.tests.common import users +from odoo.addons.test_mass_mailing.tests import common +from odoo.exceptions import AccessError + + +class TestBLAccessRights(common.MassMailingCase): + + @users('emp') + def test_bl_create_employee(self): + with self.assertRaises(AccessError): + self.env['mail.blacklist'].create([{'email': 'Arya.Stark@example.com'}]) + + @users('marketing') + def test_bl_create_marketing(self): + self.env['mail.blacklist'].create([{'email': 'Arya.Stark@example.com'}]) + + +class TestBLConsistency(common.MassMailingCase): + _base_list = ['Arya.Stark@example.com', 'ned.stark@example.com'] + + def setUp(self): + super(TestBLConsistency, self).setUp() + self.bl_rec = self.env['mail.blacklist'].create([ + {'email': 'Not A Stark '}, + ]) + + self.bl_previous = self.env['mail.blacklist'].search([]) + + @users('marketing') + def test_bl_check_case_add(self): + """ Test emails case when adding through _add """ + bl_sudo = self.env['mail.blacklist'].sudo() + existing = bl_sudo.create({ + 'email': 'arya.stark@example.com', + 'active': False, + }) + + added = self.env['mail.blacklist']._add('Arya.Stark@EXAMPLE.com') + self.assertEqual(existing, added) + self.assertTrue(existing.active) + + @users('marketing') + def test_bl_check_case_remove(self): + """ Test emails case when deactivating through _remove """ + bl_sudo = self.env['mail.blacklist'].sudo() + existing = bl_sudo.create({ + 'email': 'arya.stark@example.com', + 'active': True, + }) + + added = self.env['mail.blacklist']._remove('Arya.Stark@EXAMPLE.com') + self.assertEqual(existing, added) + self.assertFalse(existing.active) + + @users('marketing') + def test_bl_create_duplicate(self): + """ Test emails are inserted only once if duplicated """ + bl_sudo = self.env['mail.blacklist'].sudo() + self.env['mail.blacklist'].create([ + {'email': self._base_list[0]}, + {'email': self._base_list[1]}, + {'email': 'Another Ned Stark <%s>' % self._base_list[1]}, + ]) + + new_bl = bl_sudo.search([('id', 'not in', self.bl_previous.ids)]) + + self.assertEqual(len(new_bl), 2) + self.assertEqual( + set(v.lower() for v in self._base_list), + set(v.lower() for v in new_bl.mapped('email')) + ) + + @users('marketing') + def test_bl_create_parsing(self): + """ Test email is correctly extracted from given entries """ + bl_sudo = self.env['mail.blacklist'].sudo() + self.env['mail.blacklist'].create([ + {'email': self._base_list[0]}, + {'email': self._base_list[1]}, + {'email': 'Not Ned Stark '}, + ]) + + new_bl = bl_sudo.search([('id', 'not in', self.bl_previous.ids)]) + + self.assertEqual(len(new_bl), 3) + self.assertEqual( + set(v.lower() for v in self._base_list + ['jaimie.lannister@example.com']), + set(v.lower() for v in new_bl.mapped('email')) + ) + + @users('marketing') + def test_bl_search_exact(self): + search_res = self.env['mail.blacklist'].search([('email', '=', 'john.snow@example.com')]) + self.assertEqual(search_res, self.bl_rec) + + @users('marketing') + def test_bl_search_parsing(self): + search_res = self.env['mail.blacklist'].search([('email', '=', 'Not A Stark ')]) + self.assertEqual(search_res, self.bl_rec) + + @users('marketing') + def test_bl_search_case(self): + search_res = self.env['mail.blacklist'].search([('email', '=', 'john.SNOW@example.COM>')]) + self.assertEqual(search_res, self.bl_rec) + + @users('marketing') + def test_bl_search_partial(self): + search_res = self.env['mail.blacklist'].search([('email', 'ilike', 'John')]) + self.assertEqual(search_res, self.bl_rec) + search_res = self.env['mail.blacklist'].search([('email', 'ilike', 'n.SNOW@example.cO>')]) + self.assertEqual(search_res, self.bl_rec) From 0c37eb272fba1063444f0ec91db1fe543c1beebf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 16:24:33 +0200 Subject: [PATCH 03/11] [IMP] test_mass_mailing: add blacklist mixin tests Purpose of this commit is to add some tests linked to the blacklist mixin itself, notably the computed fields and its search method. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/test_mass_mailing/tests/__init__.py | 1 + .../tests/test_blacklist_mixin.py | 81 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 addons/test_mass_mailing/tests/test_blacklist_mixin.py diff --git a/addons/test_mass_mailing/tests/__init__.py b/addons/test_mass_mailing/tests/__init__.py index 9994621796b..971e2a02ae1 100644 --- a/addons/test_mass_mailing/tests/__init__.py +++ b/addons/test_mass_mailing/tests/__init__.py @@ -2,3 +2,4 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from . import test_blacklist +from . import test_blacklist_mixin diff --git a/addons/test_mass_mailing/tests/test_blacklist_mixin.py b/addons/test_mass_mailing/tests/test_blacklist_mixin.py new file mode 100644 index 00000000000..0760b90f5e2 --- /dev/null +++ b/addons/test_mass_mailing/tests/test_blacklist_mixin.py @@ -0,0 +1,81 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from unittest.mock import patch + +from odoo.tests.common import users +from odoo.addons.test_mass_mailing.tests import common +from odoo.addons.test_mass_mailing.models.mass_mail_test import MassMailTestBlacklist +from odoo.exceptions import AccessError, UserError + + +class TestBLMixin(common.MassMailingCase): + + @classmethod + def setUpClass(cls): + super(TestBLMixin, cls).setUpClass() + + cls.env['mail.blacklist'].create([{ + 'email': 'Arya.Stark@example.com', + 'active': True, + }, { + 'email': 'Sansa.Stark@example.com', + 'active': False, + }]) + + @users('emp') + def test_bl_mixin_primary_field_consistency(self): + MassMailTestBlacklist._primary_email = ['not_a_field'] + with self.assertRaises(UserError): + self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + + MassMailTestBlacklist._primary_email = 'not_a_list' + with self.assertRaises(UserError): + self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + + MassMailTestBlacklist._primary_email = 'email_from' + with self.assertRaises(UserError): + self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + + MassMailTestBlacklist._primary_email = ['email_from', 'name'] + with self.assertRaises(UserError): + self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + + MassMailTestBlacklist._primary_email = ['email_from'] + self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + + @users('emp') + def test_bl_mixin_is_blacklisted(self): + """ Test is_blacklisted field computation """ + record = self.env['mass.mail.test.bl'].create({'email_from': 'arya.stark@example.com'}) + self.assertTrue(record.is_blacklisted) + + record = self.env['mass.mail.test.bl'].create({'email_from': 'not.arya.stark@example.com'}) + self.assertFalse(record.is_blacklisted) + + @users('emp') + def test_bl_mixin_search_blacklisted(self): + """ Test is_blacklisted field search implementation """ + record1 = self.env['mass.mail.test.bl'].create({'email_from': 'arya.stark@example.com'}) + record2 = self.env['mass.mail.test.bl'].create({'email_from': 'not.arya.stark@example.com'}) + + search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', False)]) + self.assertEqual(search_res, record2) + + search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '!=', True)]) + self.assertEqual(search_res, record2) + + search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', True)]) + self.assertEqual(search_res, record1) + + search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '!=', False)]) + self.assertEqual(search_res, record1) + + @users('emp') + def test_bl_mixin_search_blacklisted_format(self): + """ Test is_blacklisted field search using email parsing """ + record1 = self.env['mass.mail.test.bl'].create({'email_from': 'Arya Stark '}) + self.assertTrue(record1.is_blacklisted) + + search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', True)]) + self.assertEqual(search_res, record1) From 2f82b95dcaf8e9773d47c369e9218b9f778642e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 14:39:35 +0200 Subject: [PATCH 04/11] [FIX] mail: insert only sanitized and lower-case emails in blacklist Purpose of this commit is to standardize content of blacklist records and ensure only real email addresses are stored. We introduce a sanitize method to extract email address from string (like Raoul Grosbedon -> raoul@example.com) and return a lower case version of it. Purpose is to gradually get rid of lower() that appear a lot in the code and speedup computation by being sure everything is always lower case. Moreover indexing on blacklist model will be more performant is using directly email field content instead of having to lowerize it. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 39 ++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index 1b3ccb69775..b329d0bbb11 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -1,9 +1,13 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -from odoo import api, fields, models, _ +import logging + +from odoo import api, fields, models, tools, _ from odoo.exceptions import UserError +_logger = logging.getLogger(__name__) + class MailBlackList(models.Model): """ Model of blacklisted email addresses to stop sending emails.""" @@ -22,18 +26,34 @@ class MailBlackList(models.Model): @api.model_create_multi def create(self, values): + # First of all, extract values to ensure emails are really unique (and don't modify values in place) + new_values = [] + for value in values: + email = self._sanitize_email(value.get('email')) + if not email: + _logger.warning('Blacklist: invalid email value %s' % value['email']) + continue + new_value = dict(value, email=email) + new_values.append(new_value) + """ To avoid crash during import due to unique email, return the existing records if any """ - sql = '''SELECT LOWER(email), id FROM mail_blacklist WHERE LOWER(email) = ANY(%s)''' - emails = [(v['email'] or '').lower() for v in values] + sql = '''SELECT email, id FROM mail_blacklist WHERE email = ANY(%s)''' + emails = [v['email'] for v in new_values] self._cr.execute(sql, (emails,)) bl_entries = dict(self._cr.fetchall()) - to_create = [v for v in values - if (v['email'] or '').lower() not in bl_entries] + to_create = [v for v in new_values + if v['email'] not in bl_entries] # TODO DBE Fixme : reorder ids according to incoming ids. results = super(MailBlackList, self).create(to_create) return self.env['mail.blacklist'].browse(bl_entries.values()) | results + @api.multi + def write(self, values): + if 'email' in values: + values['email'] = self._sanitize_email(values['email']) + return super(MailBlackList, self).write(values) + def _add(self, email): record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', email)]) if len(record) > 0: @@ -50,6 +70,15 @@ class MailBlackList(models.Model): record = record.create({'email': email, 'active': False}) return record + def _sanitize_email(self, email): + """ Sanitize and standardize blacklist entries: all emails should be + only real email extracted from strings (A -> a@a) and should be + lower case. """ + emails = tools.email_split(email) + if not emails or len(emails) != 1: + return False + return emails[0].lower() + class MailBlackListMixin(models.AbstractModel): """ Mixin that is inherited by all model with opt out. From 7b0c502d6009bbe351301714551ea098c74e9387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 14:01:27 +0200 Subject: [PATCH 05/11] [FIX] mail: ensure creation of unique emails in blacklist Currently blacklist is configured to have unique emails. Notably when creating new entries it checks for existing records with same email to avoid crashing about duplicate entries. However it does not check if given values use unique emails. When importing a big list of emails you are not sure emails are unique. This commit ensures values given to create are correct to avoid crashing when importing a lot of data. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index b329d0bbb11..312ebaccafb 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -28,11 +28,15 @@ class MailBlackList(models.Model): def create(self, values): # First of all, extract values to ensure emails are really unique (and don't modify values in place) new_values = [] + all_emails = [] for value in values: email = self._sanitize_email(value.get('email')) if not email: _logger.warning('Blacklist: invalid email value %s' % value['email']) continue + if email in all_emails: + continue + all_emails.append(email) new_value = dict(value, email=email) new_values.append(new_value) From 5743122f60ee940f85d03568c83cdd2cc4f5b649 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 14:39:24 +0200 Subject: [PATCH 06/11] [FIX] mail: ensure case insensitive check when (de)activating blacklist records Now that blacklist records are set as lower case be sure all checks in blacklist are done accordingly by using the sanitizer before comparing email adresses. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index 312ebaccafb..a54179d5732 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -59,7 +59,8 @@ class MailBlackList(models.Model): return super(MailBlackList, self).write(values) def _add(self, email): - record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', email)]) + sanitized = self._sanitize_email(email) + record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', sanitized)]) if len(record) > 0: record.write({'active': True}) else: @@ -67,7 +68,8 @@ class MailBlackList(models.Model): return record def _remove(self, email): - record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', email)]) + sanitized = self._sanitize_email(email) + record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', sanitized)]) if len(record) > 0: record.write({'active': False}) else: From bbe469006689c966582e095d3508d0dcecb41dbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 1 Oct 2018 16:57:04 +0200 Subject: [PATCH 07/11] [FIX] mail: assert email field used in blacklist and correctly use it in computed fields This commits fixes the use of email field in blacklist mixin as it was based on 'email' field instead of the one coming from the _primary_email class attribute since commit 2ff9b379ef80ba6a8744fb9702a8088284372ddd. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index a54179d5732..f7793e98b9d 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -101,11 +101,19 @@ class MailBlackListMixin(models.AbstractModel): store=False, search="_search_is_blacklisted", groups="base.group_user", help="If the email address is on the blacklist, the contact won't receive mass mailing anymore, from any list") + def _assert_primary_email(self): + if not hasattr(self, "_primary_email") or \ + not isinstance(self._primary_email, (list, tuple)) or \ + not len(self._primary_email) == 1: + raise UserError(_('Invalid primary email field on model %s') % self._name) + field_name = self._primary_email[0] + if field_name not in self._fields or self._fields[field_name].type != 'char': + raise UserError(_('Invalid primary email field on model %s') % self._name) + @api.model def _search_is_blacklisted(self, operator, value): # Assumes operator is '=' or '!=' and value is True or False - if not hasattr(self.env[self._name], "_primary_email"): - raise UserError(_('Invalid primary email field on model %s') % self._name) + self._assert_primary_email() if operator != '=': if operator == '!=' and isinstance(value, bool): value = not value @@ -117,17 +125,17 @@ class MailBlackListMixin(models.AbstractModel): query = """ SELECT m.id FROM mail_blacklist bl - JOIN %s m on (LOWER(m.email) = LOWER(bl.email) AND bl.active) + JOIN %s m on (LOWER(m.%s) = LOWER(bl.email) AND bl.active) """ else: query = """ SELECT m.id FROM %s m LEFT JOIN mail_blacklist bl - ON (LOWER(m.email) = LOWER(bl.email) AND bl.active) + ON (LOWER(m.%s) = LOWER(bl.email) AND bl.active) WHERE bl.id IS NULL """ - self._cr.execute(query % self._table) + self._cr.execute(query % (self._table, email_field)) res = self._cr.fetchall() if not res: return [(0, '=', 1)] @@ -135,6 +143,7 @@ class MailBlackListMixin(models.AbstractModel): @api.depends(lambda self: self._primary_email) def _compute_is_blacklisted(self): + self._assert_primary_email() [email_field] = self._primary_email # TODO : Should remove the sudo as compute_sudo defined on methods. # But if user doesn't have access to mail.blacklist, doen't work without sudo(). From 2b73bed880ff011332fb11762e87f6eeb5c7bbdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 2 Oct 2018 10:52:02 +0200 Subject: [PATCH 08/11] [FIX] mail: correctly extract email from email field in computed blacklist fields Purpose of this commit is to correctly extract email address from the email field of models inheriting from the blacklist mixin. Indeed email field could contain a formatted address like "Raoul Grosbedon ". Related blacklist entry would be raoul@example.com. We have to extract the email from the email field and lowerize it in the SQL query or the computation in order to have a fully working blacklist mixin. This fixes some bits left at commit 2ff9b379ef80ba6a8744fb9702a8088284372ddd. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index f7793e98b9d..639ac3efa0a 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -124,16 +124,17 @@ class MailBlackListMixin(models.AbstractModel): if value: query = """ SELECT m.id - FROM mail_blacklist bl - JOIN %s m on (LOWER(m.%s) = LOWER(bl.email) AND bl.active) + FROM mail_blacklist bl + JOIN %s m + ON (LOWER(substring(m.%s, '([^ ,;<@]+@[^> ,;]+)')) = bl.email AND bl.active) """ else: query = """ - SELECT m.id + SELECT m.id FROM %s m - LEFT JOIN mail_blacklist bl - ON (LOWER(m.%s) = LOWER(bl.email) AND bl.active) - WHERE bl.id IS NULL + LEFT JOIN mail_blacklist bl + ON (LOWER(substring(m.%s, '([^ ,;<@]+@[^> ,;]+)')) = bl.email AND bl.active) + WHERE bl.id IS NULL """ self._cr.execute(query % (self._table, email_field)) res = self._cr.fetchall() @@ -147,10 +148,7 @@ class MailBlackListMixin(models.AbstractModel): [email_field] = self._primary_email # TODO : Should remove the sudo as compute_sudo defined on methods. # But if user doesn't have access to mail.blacklist, doen't work without sudo(). - BL_sudo = self.env['mail.blacklist'].sudo() - emails_lower = [(email or '').lower() for email in self.mapped(email_field)] - blacklist = set(e.lower() - for e in BL_sudo.search([('email', 'in', emails_lower)]).mapped('email')) + sanitized = [self.env['mail.blacklist']._sanitize_email(email) for email in self.mapped(email_field)] + blacklist = set(self.env['mail.blacklist'].sudo().search([('email', 'in', sanitized)]).mapped('email')) for record in self: - email_lower = (record[email_field] or '').lower() - record.is_blacklisted = email_lower in blacklist + record.is_blacklisted = self.env['mail.blacklist']._sanitize_email(record[email_field]) in blacklist From c7ad31f0a039ebf24ae45517b3363889988216d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 2 Oct 2018 11:55:52 +0200 Subject: [PATCH 09/11] [IMP] blacklist: sanitize search terms on email Purpose of this commit is to make search coherent with sanitize of emails done when storing blacklist entries. We parse search terms in order to find something related to email field and sanitize it to avoid issues linked to case insensitivity or formatted addresses. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/models/mail_blacklist.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index 639ac3efa0a..0a49d2eb172 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -58,6 +58,24 @@ class MailBlackList(models.Model): values['email'] = self._sanitize_email(values['email']) return super(MailBlackList, self).write(values) + def _search(self, args, offset=0, limit=None, order=None, count=False, access_rights_uid=None): + """ Override _search in order to grep search on email field and make it + lower-case and sanitized """ + if args: + new_args = [] + for arg in args: + if isinstance(arg, (list, tuple)) and arg[0] == 'email' and isinstance(arg[2], tools.pycompat.text_type): + sanitized = self.env['mail.blacklist']._sanitize_email(arg[2]) + if sanitized: + new_args.append([arg[0], arg[1], sanitized]) + else: + new_args.append(arg) + else: + new_args.append(arg) + else: + new_args = args + return super(MailBlackList, self)._search(new_args, offset=offset, limit=limit, order=order, count=count, access_rights_uid=access_rights_uid) + def _add(self, email): sanitized = self._sanitize_email(email) record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', sanitized)]) From 331d413f7a2f63734c6c4918ee8a1a91158cf53e Mon Sep 17 00:00:00 2001 From: David Beguin Date: Mon, 1 Oct 2018 15:02:51 +0200 Subject: [PATCH 10/11] [FIX] mail: correctly find and compare blacklist email when sending mass mailing Until now, a recipient with a formatted email like 'John Doe ' could be reached by email even if its email address was blacklisted. Now when sending a mass mail using the composer, the email is sanitized before comparing it to blacklist entries. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mail/wizard/mail_compose_message.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index bac7148ca9c..5422a115fdf 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -282,14 +282,16 @@ class MailComposer(models.TransientModel): blacklisted_rec_ids = [] if mass_mail_mode and hasattr(self.env[self.model], "_primary_email"): BL_sudo = self.env['mail.blacklist'].sudo() - blacklist = set(email.lower() for email in BL_sudo.search([]).mapped('email')) + blacklist = set(BL_sudo.search([]).mapped('email')) if blacklist: [email_field] = self.env[self.model]._primary_email targets = self.env[self.model].browse(res_ids).read([email_field]) - blacklisted_rec_ids = [r['id'] - for r in targets - if r[email_field] - if r[email_field].lower() in blacklist] + # First extract email from recipient before comparing with blacklist + for target in targets: + sanitized_email = self.env['mail.blacklist']._sanitize_email(target.get(email_field)) + if sanitized_email and sanitized_email in blacklist: + blacklisted_rec_ids.append(target['id']) + for res_id in res_ids: # static wizard (mail.message) values mail_values = { From 65ab6f493ab5e3a3492c5822ae3faccc1953d28e Mon Sep 17 00:00:00 2001 From: David Beguin Date: Tue, 2 Oct 2018 14:29:16 +0200 Subject: [PATCH 11/11] [FIX] mass_mailing : sanitize email used for counters and comparison Purpose is to fix the comparison between mass mailing record email field and blacklist entries by sanitizing email to ensure comparison is done on same basis as what is stored in blacklist table. Also fix the action merge where the extraction of email is needed before comparing to the blacklist. This commit is related to task ID 33224 (original blacklist implementation done for v12) and its PR #25966 as well as task ID 1889703 (tests and fixes) and its PR #27330. Done with collaboration of @dbeguin. --- addons/mass_mailing/models/mass_mailing.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/addons/mass_mailing/models/mass_mailing.py b/addons/mass_mailing/models/mass_mailing.py index 29f99cb4f00..fc3cca1816f 100644 --- a/addons/mass_mailing/models/mass_mailing.py +++ b/addons/mass_mailing/models/mass_mailing.py @@ -115,11 +115,11 @@ class MassMailingList(models.Model): left join mail_mass_mailing_contact c on (r.contact_id=c.id) where COALESCE(r.opt_out,FALSE) = FALSE - AND c.email NOT IN (select email from mail_blacklist where active = TRUE) AND substring(c.email, '%s') IS NOT NULL + AND LOWER(substring(c.email, '%s')) NOT IN (select email from mail_blacklist where active = TRUE) group by list_id - ''' % EMAIL_PATTERN) + ''' % (EMAIL_PATTERN, EMAIL_PATTERN)) data = dict(self.env.cr.fetchall()) for mailing_list in self: mailing_list.contact_nbr = data.get(mailing_list.id, 0) @@ -172,7 +172,7 @@ class MassMailingList(models.Model): mail_mass_mailing_list mailing_list WHERE contact.id=contact_list_rel.contact_id AND COALESCE(contact_list_rel.opt_out,FALSE) = FALSE - AND contact.email NOT IN (select email from mail_blacklist where active = TRUE) + AND LOWER(substring(contact.email, %s)) NOT IN (select email from mail_blacklist where active = TRUE) AND mailing_list.id=contact_list_rel.list_id AND mailing_list.id IN %s AND NOT EXISTS @@ -186,7 +186,7 @@ class MassMailingList(models.Model): AND contact_list_rel2.list_id = %s ) ) st - WHERE st.rn = 1;""", (self.id, tuple(src_lists.ids), self.id)) + WHERE st.rn = 1;""", (self.id, EMAIL_PATTERN, tuple(src_lists.ids), self.id)) self.invalidate_cache() if archive: (src_lists - self).write({'active': False})