[MERGE][FIX] mail, mass_mailing: fix blacklist behavior and its use in mass mailing

This merge fixes blacklist mechanism, mixin and its use in mass mailing. Some fixes are related to https://github.com/odoo/odoo/commit/2ff9b379ef80ba6a8744fb9702a8088284372ddd. Tests are added in a new module that will gradually hold more complex tests for mass mailing. 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 closes PR #27330.
2018-10-02 16:58:09 +02:00
parent 4b3884b0d7 65ab6f493a
commit ed4beb0303
12 changed files with 389 additions and 31 deletions
@@ -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> -> 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
@@ -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 = {
@@ -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})
@@ -0,0 +1,4 @@
+# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+from . import models
@@ -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,
+}
@@ -0,0 +1,4 @@
+# -*- coding: utf-8 -*-
+# Part of Odoo. See LICENSE file for full copyright and licensing details.
+
+from . import mass_mail_test
@@ -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')
@@ -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
@@ -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
@@ -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])
+            ],
+        })
@@ -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 <john.snow@example.com>'},
+        ])
+
+        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 <jaimie.lannister@example.com>'},
+        ])
+
+        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 <john.snow@example.com>')])
+        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)
@@ -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 <arya.stark@example.com>'})
+        self.assertTrue(record1.is_blacklisted)
+
+        search_res = self.env['mass.mail.test.bl'].search([('is_blacklisted', '=', True)])
+        self.assertEqual(search_res, record1)