From 0fe50a01d2d4969f2a4cf85c36c2884407ccc682 Mon Sep 17 00:00:00 2001 From: David Beguin Date: Thu, 25 Oct 2018 15:55:02 +0000 Subject: [PATCH] [IMP] mail : apply mail.address.mixin on blacklist Apply the mail.address.mixin on blacklist mixin. So that every model inheriting the blacklist.mixin will also inherit the address.mixin. That simplify also the comparaison to - and the searches on - the blacklist. Task ID 1896677 --- addons/mail/models/mail_blacklist.py | 57 +++++++------------ addons/mail/wizard/mail_compose_message.py | 11 ++-- addons/mass_mailing/controllers/main.py | 5 +- addons/mass_mailing/models/mass_mailing.py | 18 +++--- .../models/mass_mail_test.py | 3 +- .../tests/test_performance.py | 2 +- 6 files changed, 40 insertions(+), 56 deletions(-) diff --git a/addons/mail/models/mail_blacklist.py b/addons/mail/models/mail_blacklist.py index 10df7379fa0..d9f7b95a0a0 100644 --- a/addons/mail/models/mail_blacklist.py +++ b/addons/mail/models/mail_blacklist.py @@ -30,7 +30,7 @@ class MailBlackList(models.Model): new_values = [] all_emails = [] for value in values: - email = self._sanitize_email(value.get('email')) + email = self._normalize_email(value.get('email')) if not email: raise UserError(_('Invalid email address %r') % value['email']) if email in all_emails: @@ -44,8 +44,7 @@ class MailBlackList(models.Model): 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 new_values - if v['email'] 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) @@ -54,7 +53,7 @@ class MailBlackList(models.Model): @api.multi def write(self, values): if 'email' in values: - values['email'] = self._sanitize_email(values['email']) + values['email'] = self._normalize_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): @@ -64,9 +63,9 @@ class MailBlackList(models.Model): 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]) + normalized = self._normalize_email(arg[2]) + if normalized: + new_args.append([arg[0], arg[1], normalized]) else: new_args.append(arg) else: @@ -76,8 +75,8 @@ class MailBlackList(models.Model): 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)]) + normalized = self._normalize_email(email) + record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', normalized)]) if len(record) > 0: record.write({'active': True}) else: @@ -85,16 +84,16 @@ class MailBlackList(models.Model): return record def _remove(self, email): - sanitized = self._sanitize_email(email) - record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', sanitized)]) + normalized = self._normalize_email(email) + record = self.env["mail.blacklist"].with_context(active_test=False).search([('email', '=', normalized)]) 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 + def _normalize_email(self, email): + """ Sanitize and standardize email address entries: all emails should be only real email extracted from strings (A -> a@a) and should be lower case. """ emails = tools.email_split(email) @@ -105,12 +104,12 @@ class MailBlackList(models.Model): class MailBlackListMixin(models.AbstractModel): """ Mixin that is inherited by all model with opt out. - USAGE : the field '_primary_email' must be overridden in the model that inherit the mixin - and must contain the email field of the model. + This mixin is inheriting of another mixin, mail.address.mixin which defines the _primary_email variable + and the email_normalized field that are mandatory to use the blacklist mixin. """ _name = 'mail.blacklist.mixin' _description = 'Mail Blacklist mixin' - _primary_email = ['email'] + _inherit = ['mail.address.mixin'] # Note : is_blacklisted sould only be used for display. As the compute is not depending on the blacklist, # once read, it won't be re-computed again if the blacklist is modified in the same request. @@ -118,15 +117,6 @@ 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 @@ -137,35 +127,32 @@ class MailBlackListMixin(models.AbstractModel): else: raise NotImplementedError() - [email_field] = self._primary_email if value: query = """ SELECT m.id FROM mail_blacklist bl JOIN %s m - ON (LOWER(substring(m.%s, '([^ ,;<@]+@[^> ,;]+)')) = bl.email AND bl.active) + ON m.email_normalized = bl.email AND bl.active """ else: query = """ SELECT m.id FROM %s m LEFT JOIN mail_blacklist bl - ON (LOWER(substring(m.%s, '([^ ,;<@]+@[^> ,;]+)')) = bl.email AND bl.active) + ON m.email_normalized = bl.email AND bl.active WHERE bl.id IS NULL """ - self._cr.execute(query % (self._table, email_field)) + self._cr.execute(query % self._table) res = self._cr.fetchall() if not res: return [(0, '=', 1)] return [('id', 'in', [r[0] for r in res])] - @api.depends(lambda self: self._primary_email) + @api.depends('email_normalized') 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(). - 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')) + blacklist = set(self.env['mail.blacklist'].sudo().search([ + ('email', 'in', self.mapped('email_normalized'))]).mapped('email')) for record in self: - record.is_blacklisted = self.env['mail.blacklist']._sanitize_email(record[email_field]) in blacklist + record.is_blacklisted = record.email_normalized in blacklist diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index 5422a115fdf..b9a5845896c 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -280,17 +280,14 @@ class MailComposer(models.TransientModel): reply_to_value = self.env['mail.thread']._notify_get_reply_to_on_records(default=self.email_from, records=records) blacklisted_rec_ids = [] - if mass_mail_mode and hasattr(self.env[self.model], "_primary_email"): + if mass_mail_mode and issubclass(type(self.env[self.model]), self.pool['mail.blacklist.mixin']): BL_sudo = self.env['mail.blacklist'].sudo() 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]) + targets = self.env[self.model].browse(res_ids).read(['email_normalized']) # 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']) + blacklisted_rec_ids.extend([target['id'] for target in targets + if target['email_normalized'] and target['email_normalized'] in blacklist]) for res_id in res_ids: # static wizard (mail.message) values diff --git a/addons/mass_mailing/controllers/main.py b/addons/mass_mailing/controllers/main.py index 7d43038a3a6..66d17709b46 100644 --- a/addons/mass_mailing/controllers/main.py +++ b/addons/mass_mailing/controllers/main.py @@ -39,7 +39,7 @@ class MassMailController(http.Controller): # Unsubscribe directly + Let the user choose his subscriptions mailing.update_opt_out(email, mailing.contact_list_ids.ids, True) - contacts = request.env['mail.mass_mailing.contact'].sudo().search([('email', '=', email)]) + contacts = request.env['mail.mass_mailing.contact'].sudo().search([('email_normalized', '=ilike', email)]) subscription_list_ids = contacts.mapped('subscription_list_ids') # In many user are found : if user is opt_out on the list with contact_id 1 but not with contact_id 2, # assume that the user is not opt_out on both @@ -152,8 +152,7 @@ class MassMailController(http.Controller): if not self._valid_unsubscribe_token(mailing_id, res_id, email, token): return 'unauthorized' model = request.env[mailing.mailing_model_real] - [email_field] = model._primary_email - records = model.sudo().search([(email_field, '=ilike', email)]) + records = model.sudo().search([('email_normalized', '=ilike', email)]) for record in records: record.sudo().message_post(body=_("Feedback from %s: %s" % (email, feedback))) return bool(records) diff --git a/addons/mass_mailing/models/mass_mailing.py b/addons/mass_mailing/models/mass_mailing.py index 48d5c6704d9..33c202a396b 100644 --- a/addons/mass_mailing/models/mass_mailing.py +++ b/addons/mass_mailing/models/mass_mailing.py @@ -25,7 +25,6 @@ MASS_MAILING_BUSINESS_MODELS = [ 'mail.mass_mailing.list', 'mail.mass_mailing.contact' ] -EMAIL_PATTERN = '([^ ,;<@]+@[^> ,;]+)' class MassMailingTag(models.Model): @@ -115,16 +114,16 @@ class MassMailingList(models.Model): from mail_mass_mailing_contact_list_rel r left join mail_mass_mailing_contact c on (r.contact_id=c.id) - left join mail_blacklist bl on (LOWER(substring(c.email, %s)) = bl.email and bl.active) + left join mail_blacklist bl on c.email_normalized = bl.email and bl.active where list_id in %s - COALESCE(r.opt_out,FALSE) = FALSE - AND c.email IS NOT NULL + AND COALESCE(r.opt_out,FALSE) = FALSE + AND c.email_normalized IS NOT NULL AND bl.id IS NULL group by list_id - ''') - data = dict(self.env.cr.fetchall(), [EMAIL_PATTERN, tuple(self.ids)]) + ''', (tuple(self.ids), )) + data = dict(self.env.cr.fetchall()) for mailing_list in self: mailing_list.contact_nbr = data.get(mailing_list.id, 0) @@ -176,7 +175,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 LOWER(substring(contact.email, %s)) NOT IN (select email from mail_blacklist where active = TRUE) + AND contact.email_normalized 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 @@ -190,7 +189,7 @@ class MassMailingList(models.Model): AND contact_list_rel2.list_id = %s ) ) st - WHERE st.rn = 1;""", (self.id, EMAIL_PATTERN, tuple(src_lists.ids), self.id)) + WHERE st.rn = 1;""", (self.id, tuple(src_lists.ids), self.id)) self.invalidate_cache() if archive: (src_lists - self).write({'active': False}) @@ -231,7 +230,8 @@ class MassMailingContact(models.Model): @api.depends('email') def _compute_is_email_valid(self): for record in self: - record.is_email_valid = re.match(EMAIL_PATTERN, record.email) + normalized = self._normalize_email(record.email) + record.is_email_valid = normalized if not normalized else True @api.model def _search_opt_out(self, operator, value): diff --git a/addons/test_mass_mailing/models/mass_mail_test.py b/addons/test_mass_mailing/models/mass_mail_test.py index 87e72497399..ac9d3eeb65a 100644 --- a/addons/test_mass_mailing/models/mass_mail_test.py +++ b/addons/test_mass_mailing/models/mass_mail_test.py @@ -9,7 +9,8 @@ class MassMailTest(models.Model): mailing features and base performances. """ _description = 'Simple Mass Mailing Model' _name = 'mass.mail.test' - _inherit = ['mail.thread'] + _inherit = ['mail.thread', 'mail.address.mixin'] + _primary_email = ['email_from'] name = fields.Char() email_from = fields.Char() diff --git a/addons/test_mass_mailing/tests/test_performance.py b/addons/test_mass_mailing/tests/test_performance.py index a25c7164202..8fc27fe8cfb 100644 --- a/addons/test_mass_mailing/tests/test_performance.py +++ b/addons/test_mass_mailing/tests/test_performance.py @@ -49,7 +49,7 @@ class TestMassMailPerformance(TestMassMailPerformanceBase): 'mailing_domain': [('id', 'in', self.mm_recs.ids)], }) - with self.assertQueryCount(__system__=2432, marketing=3088): + with self.assertQueryCount(__system__=2433, marketing=3089): mailing.send_mail() self.assertEqual(mailing.sent, 50)