From ca989b6548e93c25f54684e5569ed3998161acce Mon Sep 17 00:00:00 2001 From: Olivier Dony Date: Fri, 24 Mar 2017 19:01:37 +0100 Subject: [PATCH] [FIX] mass_mailing: prevent duplicates and sending to opted-out The mass-mailing App suffered from severe issues due to its inability to detect and handle duplicates "at the email level", and the absence of any global blacklist system, leading to lack of user trust. Mailing-lists typically include multiple records with the same email, and it is critical to avoid sending them the same email several times. A related problem is the unsubscription of an email that is present in other records (duplicates). Opting out the first email should automatically blacklist it for other records as well. Ideally we should have a global blacklist table in order to share the unsubscription requests globally across models (Leads, Partners, Mailing-list contacts). It would also allow importing it from other blacklist systems. (TODO for master) This commit introduces a partial solution, made of several small changes: - In mail.mail: double-check that an outgoing email has the correct status (`outgoing`) before sending it. This allows adding emails in the queue and cancelling them before they actually get sent. - In crm.lead: force predictable recipients for mass-mailing, by always using the email of the lead rather than the email of the linked partner when there is one. This simplifies the computation of the blacklist and seen list. Other areas in the codebase already assume as much. - In mass.mailing: + Before sending out a mailing-list batch, compute the blacklist (all opted-out emails) and the seen_list (emails who previously received this mail) to make sure we only ever target valid recipients. + While delivering the mass-mailing, any email targeting an address that is in the blacklist or "seen list" is canceled before being sent. The corresponding statistics entry is considered "not delivered". Also updates the "seen list" continuously. + When a mass-mail belongs to a campaign with the "unique AB/B testing" flag, the "seen list" is common to the whole campaign, as an extra safety. + Auto-delete mass-mailing test messages sent with the test wizard, to avoid polluting the mail_mail table Note: this fix uses a simple regex for efficiently extracting the blacklist in pure SQL from different models, and doing so, assumes that each record only holds a single email (no comma-separated adresses). This should be sufficient for most cases. The regex: ([^ ,;<@]+@[^> ,;]+) --- addons/crm/models/crm_lead.py | 9 +++ addons/mail/models/mail_mail.py | 4 ++ addons/mass_mailing/models/mass_mailing.py | 69 ++++++++++++++++++- .../wizard/mail_compose_message.py | 28 ++++++-- addons/mass_mailing/wizard/test_mailing.py | 1 + 5 files changed, 103 insertions(+), 8 deletions(-) diff --git a/addons/crm/models/crm_lead.py b/addons/crm/models/crm_lead.py index 4943c934902..490671d4638 100644 --- a/addons/crm/models/crm_lead.py +++ b/addons/crm/models/crm_lead.py @@ -1044,6 +1044,15 @@ class Lead(models.Model): view_id = super(Lead, self).get_formview_id() return view_id + @api.multi + def message_get_default_recipients(self): + return { + r.id : {'partner_ids': [], + 'email_to': r.email_from, + 'email_cc': False} + for r in self.sudo() + } + @api.multi def message_get_suggested_recipients(self): recipients = super(Lead, self).message_get_suggested_recipients() diff --git a/addons/mail/models/mail_mail.py b/addons/mail/models/mail_mail.py index 6fcae69b21b..ff25c60f04a 100644 --- a/addons/mail/models/mail_mail.py +++ b/addons/mail/models/mail_mail.py @@ -255,6 +255,10 @@ class MailMail(models.Model): for mail_id in self.ids: try: mail = self.browse(mail_id) + if mail.state != 'outgoing': + if mail.state != 'exception' and mail.auto_delete: + mail.sudo().unlink() + continue # TDE note: remove me when model_id field is present on mail.message - done here to avoid doing it multiple times in the sub method if mail.model: model = self.env['ir.model']._get(mail.model)[0] diff --git a/addons/mass_mailing/models/mass_mailing.py b/addons/mass_mailing/models/mass_mailing.py index 2673fcc8106..86ded7246b9 100644 --- a/addons/mass_mailing/models/mass_mailing.py +++ b/addons/mass_mailing/models/mass_mailing.py @@ -4,6 +4,7 @@ import hashlib import hmac from datetime import datetime +import logging import random from odoo import api, fields, models, tools, _ @@ -11,6 +12,7 @@ from odoo.exceptions import UserError from odoo.tools.safe_eval import safe_eval from odoo.tools.translate import html_translate +_logger = logging.getLogger(__name__) class MassMailingTag(models.Model): """Model of categories of mass mailing, i.e. marketing, newsletter, ... """ @@ -567,6 +569,69 @@ class MassMailing(models.Model): # Email Sending #------------------------------------------------------ + def _get_blacklist(self): + """Returns a set of emails opted-out in target model""" + # TODO: implement a global blacklist table, to easily share + # it and update it. + self.ensure_one() + blacklist = {} + target = self.env[self.mailing_model_real] + mail_field = 'email' if 'email' in target._fields else 'email_from' + if 'opt_out' in target._fields: + # avoid loading a large number of records in memory + # + use a basic heuristic for extracting emails + query = """ + SELECT lower(substring(%(mail_field)s, '([^ ,;<@]+@[^> ,;]+)')) + FROM %(target)s + WHERE opt_out AND + substring(%(mail_field)s, '([^ ,;<@]+@[^> ,;]+)') IS NOT NULL; + """ + query = query % {'target': target._table, 'mail_field': mail_field} + self._cr.execute(query) + blacklist = set(m[0] for m in self._cr.fetchall()) + _logger.info( + "Mass-mailing %s targets %s, blacklist: %s emails", + self, target._name, len(blacklist)) + else: + _logger.info("Mass-mailing %s targets %s, no blacklist available", self, target._name) + return blacklist + + def _get_seen_list(self): + """Returns a set of emails already targeted by current mailing/campaign (no duplicates)""" + self.ensure_one() + target = self.env[self.mailing_model_real] + mail_field = 'email' if 'email' in target._fields else 'email_from' + # avoid loading a large number of records in memory + # + use a basic heuristic for extracting emails + query = """ + SELECT lower(substring(%(mail_field)s, '([^ ,;<@]+@[^> ,;]+)')) + FROM mail_mail_statistics s + JOIN %(target)s t ON (s.res_id = t.id) + WHERE substring(%(mail_field)s, '([^ ,;<@]+@[^> ,;]+)') IS NOT NULL + """ + if self.mass_mailing_campaign_id.unique_ab_testing: + query +=""" + AND s.mass_mailing_campaign_id = %%(mailing_campaign_id)s; + """ + else: + query +=""" + AND s.mass_mailing_id = %%(mailing_id)s; + """ + query = query % {'target': target._table, 'mail_field': mail_field} + params = {'mailing_id': self.id, 'mailing_campaign_id': self.mass_mailing_campaign_id.id} + self._cr.execute(query, params) + seen_list = set(m[0] for m in self._cr.fetchall()) + _logger.info( + "Mass-mailing %s has already reached %s %s emails", self, len(seen_list), target._name) + return seen_list + + def _get_mass_mailing_context(self): + """Returns extra context items with pre-filled blacklist and seen list for massmailing""" + return { + 'mass_mailing_blacklist': self._get_blacklist(), + 'mass_mailing_seen_list': self._get_seen_list(), + } + def get_recipients(self): if self.mailing_domain: domain = safe_eval(self.mailing_domain) @@ -625,7 +690,9 @@ class MassMailing(models.Model): composer_values['reply_to'] = mailing.reply_to composer = self.env['mail.compose.message'].with_context(active_ids=res_ids).create(composer_values) - composer.with_context(active_ids=res_ids).send_mail(auto_commit=True) + extra_context = self._get_mass_mailing_context() + composer = composer.with_context(active_ids=res_ids, **extra_context) + composer.send_mail(auto_commit=True) mailing.state = 'done' return True diff --git a/addons/mass_mailing/wizard/mail_compose_message.py b/addons/mass_mailing/wizard/mail_compose_message.py index 82566623d19..c8b1e54f316 100644 --- a/addons/mass_mailing/wizard/mail_compose_message.py +++ b/addons/mass_mailing/wizard/mail_compose_message.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- # Part of Odoo. See LICENSE file for full copyright and licensing details. -from odoo import api, fields, models +from odoo import api, fields, models, tools class MailComposeMessage(models.TransientModel): @@ -40,14 +40,28 @@ class MailComposeMessage(models.TransientModel): 'mailing_model': self.model, 'mailing_domain': self.active_domain, }) + blacklist = self._context.get('mass_mailing_blacklist') + seen_list = self._context.get('mass_mailing_seen_list') for res_id in res_ids: - res[res_id].update({ + mail_values = res[res_id] + recips = tools.email_split(mail_values.get('email_to')) + mail_to = recips[0].lower() if recips else False + if (blacklist and mail_to in blacklist) or (seen_list and mail_to in seen_list): + # prevent sending to blocked addresses that were included by mistake + mail_values['state'] = 'cancel' + elif seen_list is not None: + seen_list.add(mail_to) + stat_vals = { + 'model': self.model, + 'res_id': res_id, + 'mass_mailing_id': mass_mailing.id + } + # propagate exception state to stat when still-born + if mail_values.get('state') == 'cancel': + stat_vals['exception'] = fields.Datetime.now() + mail_values.update({ 'mailing_id': mass_mailing.id, - 'statistics_ids': [(0, 0, { - 'model': self.model, - 'res_id': res_id, - 'mass_mailing_id': mass_mailing.id, - })], + 'statistics_ids': [(0, 0, stat_vals)], # email-mode: keep original message for routing 'notification': mass_mailing.reply_to_mode == 'thread', 'auto_delete': not mass_mailing.keep_archives, diff --git a/addons/mass_mailing/wizard/test_mailing.py b/addons/mass_mailing/wizard/test_mailing.py index 130827e1fe4..24b395c7c53 100644 --- a/addons/mass_mailing/wizard/test_mailing.py +++ b/addons/mass_mailing/wizard/test_mailing.py @@ -30,6 +30,7 @@ class TestMassMailing(models.TransientModel): 'notification': True, 'mailing_id': mailing.id, 'attachment_ids': [(4, attachment.id) for attachment in mailing.attachment_ids], + 'auto_delete': True, } mail = self.env['mail.mail'].create(mail_values) mails |= mail