diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index 1b3ccb69775..0a49d2eb172 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,20 +26,59 @@ 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 = [] + 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) + """ 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 _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): - 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: @@ -43,13 +86,23 @@ 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: 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. @@ -66,11 +119,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 @@ -81,18 +142,19 @@ class MailBlackListMixin(models.AbstractModel): if value: query = """ SELECT m.id - FROM mail_blacklist bl - JOIN %s m on (LOWER(m.email) = 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.email) = 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) + self._cr.execute(query % (self._table, email_field)) res = self._cr.fetchall() if not res: return [(0, '=', 1)] @@ -100,13 +162,11 @@ 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(). - 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 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 = { 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}) 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..971e2a02ae1 --- /dev/null +++ b/addons/test_mass_mailing/tests/__init__.py @@ -0,0 +1,5 @@ +# -*- coding: utf-8 -*- +# 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/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]) + ], + }) 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) 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)