From d61946890e1b8f79678146c91fc4f0a15c58af0b Mon Sep 17 00:00:00 2001 From: Antonio Espinosa Date: Fri, 14 Oct 2016 15:42:29 +0200 Subject: [PATCH] [IMP] mail_tracking performance and bounce process (#103) --- mail_tracking/controllers/main.py | 83 +++++++------- mail_tracking/models/mail_message.py | 4 +- mail_tracking/models/mail_tracking_email.py | 118 +++++++++----------- mail_tracking/models/mail_tracking_event.py | 2 +- mail_tracking/models/res_partner.py | 45 ++++---- mail_tracking/tests/test_mail_tracking.py | 68 +++++++---- mail_tracking/views/res_partner_view.xml | 15 +++ 7 files changed, 186 insertions(+), 149 deletions(-) diff --git a/mail_tracking/controllers/main.py b/mail_tracking/controllers/main.py index 0394070f..9be9730c 100644 --- a/mail_tracking/controllers/main.py +++ b/mail_tracking/controllers/main.py @@ -11,17 +11,30 @@ _logger = logging.getLogger(__name__) BLANK = 'R0lGODlhAQABAIAAANvf7wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==' -def _env_get(db): +def _env_get(db, callback, tracking_id, event_type, **kw): + res = 'NOT FOUND' reg = False - try: - reg = registry(db) - except OperationalError: - _logger.warning("Selected BD '%s' not found", db) - except: # pragma: no cover - _logger.warning("Selected BD '%s' connection error", db) - if reg: - return api.Environment(reg.cursor(), SUPERUSER_ID, {}) - return False + current = http.request.db and db == http.request.db + env = current and http.request.env + if not env: + with api.Environment.manage(): + try: + reg = registry(db) + except OperationalError: + _logger.warning("Selected BD '%s' not found", db) + except: # pragma: no cover + _logger.warning("Selected BD '%s' connection error", db) + if reg: + _logger.info("New environment for database '%s'", db) + with reg.cursor() as new_cr: + new_env = api.Environment(new_cr, SUPERUSER_ID, {}) + res = callback(new_env, tracking_id, event_type, **kw) + new_env.cr.commit() + else: + # make sudo when reusing environment + env = env(user=SUPERUSER_ID) + res = callback(env, tracking_id, event_type, **kw) + return res class MailTrackingController(http.Controller): @@ -35,49 +48,37 @@ class MailTrackingController(http.Controller): 'ua_family': request.user_agent.browser or False, } + def _tracking_open(self, env, tracking_id, event_type, **kw): + tracking_email = env['mail.tracking.email'].search([ + ('id', '=', tracking_id), + ]) + if tracking_email: + metadata = self._request_metadata() + tracking_email.event_create('open', metadata) + else: + _logger.warning( + "MailTracking email '%s' not found", tracking_id) + + def _tracking_event(self, env, tracking_id, event_type, **kw): + metadata = self._request_metadata() + return env['mail.tracking.email'].event_process( + http.request, kw, metadata, event_type=event_type) + @http.route('/mail/tracking/all/', type='http', auth='none', csrf=False) def mail_tracking_all(self, db, **kw): - env = _env_get(db) - if not env: - return 'NOT FOUND' - metadata = self._request_metadata() - response = env['mail.tracking.email'].event_process( - http.request, kw, metadata) - env.cr.commit() - env.cr.close() - return response + return _env_get(db, self._tracking_event, None, None, **kw) @http.route('/mail/tracking/event//', type='http', auth='none', csrf=False) def mail_tracking_event(self, db, event_type, **kw): - env = _env_get(db) - if not env: - return 'NOT FOUND' - metadata = self._request_metadata() - response = env['mail.tracking.email'].event_process( - http.request, kw, metadata, event_type=event_type) - env.cr.commit() - env.cr.close() - return response + return _env_get(db, self._tracking_event, None, event_type, **kw) @http.route('/mail/tracking/open/' '//blank.gif', type='http', auth='none') def mail_tracking_open(self, db, tracking_email_id, **kw): - env = _env_get(db) - if env: - tracking_email = env['mail.tracking.email'].search([ - ('id', '=', tracking_email_id), - ]) - if tracking_email: - metadata = self._request_metadata() - tracking_email.event_create('open', metadata) - else: - _logger.warning( - "MailTracking email '%s' not found", tracking_email_id) - env.cr.commit() - env.cr.close() + _env_get(db, self._tracking_open, tracking_email_id, None, **kw) # Always return GIF blank image response = werkzeug.wrappers.Response() diff --git a/mail_tracking/models/mail_message.py b/mail_tracking/models/mail_message.py index 83f39571..5bd499e0 100644 --- a/mail_tracking/models/mail_message.py +++ b/mail_tracking/models/mail_message.py @@ -44,7 +44,7 @@ class MailMessage(models.Model): for tracking in trackings: status = self._partner_tracking_status_get(tracking) recipient = ( - tracking.partner_id.display_name or tracking.recipient) + tracking.partner_id.name or tracking.recipient) partner_trackings.append(( status, tracking.id, recipient, tracking.partner_id.id)) if tracking.partner_id: @@ -59,7 +59,7 @@ class MailMessage(models.Model): for partner in partners: # If there is partners not included, then status is 'unknown' partner_trackings.append(( - 'unknown', False, partner.display_name, partner.id)) + 'unknown', False, partner.name, partner.id)) res[message.id] = partner_trackings return res diff --git a/mail_tracking/models/mail_tracking_email.py b/mail_tracking/models/mail_tracking_email.py index 9dea7b16..81d2835d 100644 --- a/mail_tracking/models/mail_tracking_email.py +++ b/mail_tracking/models/mail_tracking_email.py @@ -23,6 +23,11 @@ class MailTrackingEmail(models.Model): _rec_name = 'display_name' _description = 'MailTracking email' + # This table is going to grow fast and to infinite, so we index: + # - name: Search in tree view + # - time: default order fields + # - recipient_address: Used for email_store calculation (non-store) + # - state: Search and group_by in tree view name = fields.Char(string="Subject", readonly=True, index=True) display_name = fields.Char( string="Display name", readonly=True, store=True, @@ -30,7 +35,7 @@ class MailTrackingEmail(models.Model): timestamp = fields.Float( string='UTC timestamp', readonly=True, digits=dp.get_precision('MailTracking Timestamp')) - time = fields.Datetime(string="Time", readonly=True) + time = fields.Datetime(string="Time", readonly=True, index=True) date = fields.Date( string="Date", readonly=True, compute="_compute_date", store=True) mail_message_id = fields.Many2one( @@ -42,7 +47,7 @@ class MailTrackingEmail(models.Model): recipient = fields.Char(string='Recipient email', readonly=True) recipient_address = fields.Char( string='Recipient email address', readonly=True, store=True, - compute='_compute_recipient_address') + compute='_compute_recipient_address', index=True) sender = fields.Char(string='Sender email', readonly=True) state = fields.Selection([ ('error', 'Error'), @@ -88,67 +93,54 @@ class MailTrackingEmail(models.Model): inverse_name='tracking_email_id', readonly=True) @api.model - def tracking_ids_recalculate(self, model, email_field, tracking_field, - email, new_tracking=None): - objects = self.env[model].search([ - (email_field, '=ilike', email), - ]) - for obj in objects: - trackings = obj[tracking_field] - if new_tracking: - trackings |= new_tracking - trackings = trackings._email_score_tracking_filter() - if set(obj[tracking_field].ids) != set(trackings.ids): - if trackings: - obj.write({ - tracking_field: [(6, False, trackings.ids)] - }) - else: - obj.write({ - tracking_field: [(5, False, False)] - }) - return objects + def _email_score_tracking_filter(self, domain, order='time desc', + limit=10): + """Default tracking search. Ready to be inherited.""" + return self.search(domain, limit=limit, order=order) @api.model - def _tracking_ids_to_write(self, email): - trackings = self.env['mail.tracking.email'].search([ - ('recipient_address', '=ilike', email) - ]) - trackings = trackings._email_score_tracking_filter() - if trackings: - return [(6, False, trackings.ids)] - else: - return [(5, False, False)] - - def _email_score_tracking_filter(self): - """Default email score filter for tracking emails""" - # Consider only last 10 tracking emails - return self.sorted(key=lambda r: r.time, reverse=True)[:10] + def email_is_bounced(self, email): + return len(self._email_score_tracking_filter([ + ('recipient_address', '=ilike', email), + ('state', 'in', ('error', 'rejected', 'spam', 'bounced')), + ])) > 0 @api.model def email_score_from_email(self, email): - trackings = self.env['mail.tracking.email'].search([ + return self._email_score_tracking_filter([ ('recipient_address', '=ilike', email) - ]) - return trackings.email_score() + ]).email_score() + + @api.model + def _email_score_weights(self): + """Default email score weights. Ready to be inherited""" + return { + 'error': -50.0, + 'rejected': -25.0, + 'spam': -25.0, + 'bounced': -25.0, + 'soft-bounced': -10.0, + 'unsub': -10.0, + 'delivered': 1.0, + 'opened': 5.0, + } def email_score(self): - """Default email score algorimth""" + """Default email score algorimth. Ready to be inherited + + Must return a value beetwen 0.0 and 100.0 + - Bad reputation: Value between 0 and 50.0 + - Unknown reputation: Value 50.0 + - Good reputation: Value between 50.0 and 100.0 + """ + weights = self._email_score_weights() score = 50.0 - trackings = self._email_score_tracking_filter() - for tracking in trackings: - if tracking.state in ('error',): - score -= 50.0 - elif tracking.state in ('rejected', 'spam', 'bounced'): - score -= 25.0 - elif tracking.state in ('soft-bounced', 'unsub'): - score -= 10.0 - elif tracking.state in ('delivered',): - score += 5.0 - elif tracking.state in ('opened',): - score += 10.0 + for tracking in self: + score += weights.get(tracking.state, 0.0) if score > 100.0: score = 100.0 + elif score < 0.0: + score = 0.0 return score @api.depends('recipient') @@ -174,14 +166,6 @@ class MailTrackingEmail(models.Model): email.date = fields.Date.to_string( fields.Date.from_string(email.time)) - @api.model - def create(self, vals): - tracking = super(MailTrackingEmail, self).create(vals) - self.tracking_ids_recalculate( - 'res.partner', 'email', 'tracking_email_ids', - tracking.recipient_address, new_tracking=tracking) - return tracking - def _get_mail_tracking_img(self): base_url = self.env['ir.config_parameter'].get_param('web.base.url') path_url = ( @@ -197,6 +181,12 @@ class MailTrackingEmail(models.Model): 'tracking_email_id': self.id, }) + def _partners_email_bounced_set(self, reason): + for tracking_email in self: + self.env['res.partner'].search([ + ('email', '=ilike', tracking_email.recipient_address) + ]).email_bounced_set(tracking_email, reason) + def smtp_error(self, mail_server, smtp_server, exception): self.sudo().write({ 'error_smtp_server': tools.ustr(smtp_server), @@ -204,6 +194,7 @@ class MailTrackingEmail(models.Model): 'error_description': tools.ustr(exception), 'state': 'error', }) + self.sudo()._partners_email_bounced_set('error') return True def tracking_img_add(self, email): @@ -286,13 +277,10 @@ class MailTrackingEmail(models.Model): vals = tracking_email._event_prepare(event_type, metadata) if vals: event_ids += event_ids.sudo().create(vals) - partners = self.tracking_ids_recalculate( - 'res.partner', 'email', 'tracking_email_ids', - tracking_email.recipient_address) - if partners: - partners.email_score_calculate() else: _logger.debug("Concurrent event '%s' discarded", event_type) + if event_type in {'hard_bounce', 'spam', 'reject'}: + self.sudo()._partners_email_bounced_set(event_type) return event_ids @api.model diff --git a/mail_tracking/models/mail_tracking_event.py b/mail_tracking/models/mail_tracking_event.py index c3fb9861..3bd0fda7 100644 --- a/mail_tracking/models/mail_tracking_event.py +++ b/mail_tracking/models/mail_tracking_event.py @@ -23,7 +23,7 @@ class MailTrackingEvent(models.Model): date = fields.Date( string="Date", readonly=True, compute="_compute_date", store=True) tracking_email_id = fields.Many2one( - string='Message', readonly=True, + string='Message', readonly=True, required=True, ondelete='cascade', comodel_name='mail.tracking.email') event_type = fields.Selection(string='Event type', selection=[ ('sent', 'Sent'), diff --git a/mail_tracking/models/res_partner.py b/mail_tracking/models/res_partner.py index 793f7702..8d36c7c4 100644 --- a/mail_tracking/models/res_partner.py +++ b/mail_tracking/models/res_partner.py @@ -8,26 +8,23 @@ from odoo import models, api, fields class ResPartner(models.Model): _inherit = 'res.partner' - tracking_email_ids = fields.Many2many( - string="Tracking emails", comodel_name="mail.tracking.email", - readonly=True) + # tracking_emails_count and email_score are non-store fields in order + # to improve performance + # email_bounced is store=True and index=True field in order to filter + # in tree view for processing bounces easier tracking_emails_count = fields.Integer( - string="Tracking emails count", store=True, readonly=True, - compute="_compute_tracking_emails_count") - email_score = fields.Float( - string="Email score", readonly=True, default=50.0) - - def email_score_calculate(self): - # This is not a compute method because is causing a inter-block - # in mail_tracking_email PostgreSQL table - # We suspect that tracking_email write to state field block that - # table and then inside write ORM try to read from DB - # tracking_email_ids because it's not in cache. - # PostgreSQL blocks read because we have not committed yet the write - for partner in self: - partner.email_score = partner.tracking_email_ids.email_score() + compute='_compute_tracking_emails_count', readonly=True) + email_bounced = fields.Boolean(index=True) + email_score = fields.Float(compute='_compute_email_score', readonly=True) + + @api.depends('email') + def _compute_email_score(self): + for partner in self.filtered('email'): + partner.email_score = self.env['mail.tracking.email'].\ + email_score_from_email(partner.email) - @api.depends('tracking_email_ids') + @api.multi + @api.depends('email') def _compute_tracking_emails_count(self): for partner in self: partner.tracking_emails_count = self.env['mail.tracking.email'].\ @@ -35,10 +32,16 @@ class ResPartner(models.Model): ('recipient_address', '=ilike', partner.email) ]) + @api.multi + def email_bounced_set(self, tracking_email, reason): + """Inherit this method to make any other actions to partners""" + partners = self.filtered(lambda r: not r.email_bounced) + return partners.write({'email_bounced': True}) + def write(self, vals): email = vals.get('email') if email is not None: - m_track = self.env['mail.tracking.email'] - vals['tracking_email_ids'] = m_track._tracking_ids_to_write(email) - vals['email_score'] = m_track.email_score_from_email(email) + vals['email_bounced'] = ( + bool(email) and + self.env['mail.tracking.email'].email_is_bounced(email)) return super(ResPartner, self).write(vals) diff --git a/mail_tracking/tests/test_mail_tracking.py b/mail_tracking/tests/test_mail_tracking.py index 4a1777b8..4e5dea88 100644 --- a/mail_tracking/tests/test_mail_tracking.py +++ b/mail_tracking/tests/test_mail_tracking.py @@ -5,10 +5,10 @@ import mock import base64 import time +from odoo import http from odoo.tests.common import TransactionCase from ..controllers.main import MailTrackingController, BLANK -mock_request = 'odoo.http.request' mock_send_email = ('odoo.addons.base.ir.ir_mail_server.' 'IrMailServer.send_email') @@ -22,11 +22,9 @@ class FakeUserAgent(object): return 'Test suite' -# One test case per method class TestMailTracking(TransactionCase): - # Use case : Prepare some data for current test case - def setUp(self): - super(TestMailTracking, self).setUp() + def setUp(self, *args, **kwargs): + super(TestMailTracking, self).setUp(*args, **kwargs) self.sender = self.env['res.partner'].create({ 'name': 'Test sender', 'email': 'sender@example.com', @@ -37,12 +35,22 @@ class TestMailTracking(TransactionCase): 'email': 'recipient@example.com', 'notify_email': 'always', }) - self.request = { + self.last_request = http.request + http.request = type('obj', (object,), { + 'db': self.env.cr.dbname, + 'env': self.env, + 'endpoint': type('obj', (object,), { + 'routing': [], + }), 'httprequest': type('obj', (object,), { 'remote_addr': '123.123.123.123', 'user_agent': FakeUserAgent(), }), - } + }) + + def tearDown(self, *args, **kwargs): + http.request = self.last_request + return super(TestMailTracking, self).tearDown(*args, **kwargs) def test_message_post(self): # This message will generate a notification for recipient @@ -108,10 +116,15 @@ class TestMailTracking(TransactionCase): mail, tracking = self.mail_send(self.recipient.email) self.assertEqual(mail.email_to, tracking.recipient) self.assertEqual(mail.email_from, tracking.sender) - with mock.patch(mock_request) as mock_func: - mock_func.return_value = type('obj', (object,), self.request) - res = controller.mail_tracking_open(db, tracking.id) - self.assertEqual(image, res.response[0]) + res = controller.mail_tracking_open(db, tracking.id) + self.assertEqual(image, res.response[0]) + # Two events: sent and open + self.assertEqual(2, len(tracking.tracking_event_ids)) + # Fake event: tracking_email_id = False + res = controller.mail_tracking_open(db, False) + self.assertEqual(image, res.response[0]) + # Two events again because no tracking_email_id found for False + self.assertEqual(2, len(tracking.tracking_event_ids)) def test_concurrent_open(self): mail, tracking = self.mail_send(self.recipient.email) @@ -191,31 +204,38 @@ class TestMailTracking(TransactionCase): self.assertEqual('error', tracking.state) self.assertEqual('Warning', tracking.error_type) self.assertEqual('Test error', tracking.error_description) + self.assertTrue(self.recipient.email_bounced) def test_partner_email_change(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('open', {}) orig_score = self.recipient.email_score + orig_count = self.recipient.tracking_emails_count orig_email = self.recipient.email self.recipient.email = orig_email + '2' self.assertEqual(50.0, self.recipient.email_score) + self.assertEqual(0, self.recipient.tracking_emails_count) self.recipient.email = orig_email self.assertEqual(orig_score, self.recipient.email_score) + self.assertEqual(orig_count, self.recipient.tracking_emails_count) def test_process_hard_bounce(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('hard_bounce', {}) self.assertEqual('bounced', tracking.state) + self.assertTrue(self.recipient.email_score < 50.0) def test_process_soft_bounce(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('soft_bounce', {}) self.assertEqual('soft-bounced', tracking.state) + self.assertTrue(self.recipient.email_score < 50.0) def test_process_delivered(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('delivered', {}) self.assertEqual('delivered', tracking.state) + self.assertTrue(self.recipient.email_score > 50.0) def test_process_deferral(self): mail, tracking = self.mail_send(self.recipient.email) @@ -226,35 +246,45 @@ class TestMailTracking(TransactionCase): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('spam', {}) self.assertEqual('spam', tracking.state) + self.assertTrue(self.recipient.email_score < 50.0) def test_process_unsub(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('unsub', {}) self.assertEqual('unsub', tracking.state) + self.assertTrue(self.recipient.email_score < 50.0) def test_process_reject(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('reject', {}) self.assertEqual('rejected', tracking.state) + self.assertTrue(self.recipient.email_score < 50.0) def test_process_open(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('open', {}) self.assertEqual('opened', tracking.state) + self.assertTrue(self.recipient.email_score > 50.0) def test_process_click(self): mail, tracking = self.mail_send(self.recipient.email) tracking.event_create('click', {}) self.assertEqual('opened', tracking.state) + self.assertTrue(self.recipient.email_score > 50.0) + + def test_process_several_bounce(self): + for i in range(1, 10): + mail, tracking = self.mail_send(self.recipient.email) + tracking.event_create('hard_bounce', {}) + self.assertEqual('bounced', tracking.state) + self.assertEqual(0.0, self.recipient.email_score) def test_db(self): db = self.env.cr.dbname controller = MailTrackingController() - with mock.patch(mock_request) as mock_func: - mock_func.return_value = type('obj', (object,), self.request) - not_found = controller.mail_tracking_all('not_found_db') - self.assertEqual('NOT FOUND', not_found.response[0]) - none = controller.mail_tracking_all(db) - self.assertEqual('NONE', none.response[0]) - none = controller.mail_tracking_event(db, 'open') - self.assertEqual('NONE', none.response[0]) + not_found = controller.mail_tracking_all('not_found_db') + self.assertEqual('NOT FOUND', not_found.response[0]) + none = controller.mail_tracking_all(db) + self.assertEqual('NONE', none.response[0]) + none = controller.mail_tracking_event(db, 'open') + self.assertEqual('NONE', none.response[0]) diff --git a/mail_tracking/views/res_partner_view.xml b/mail_tracking/views/res_partner_view.xml index 1df681aa..7708d03b 100644 --- a/mail_tracking/views/res_partner_view.xml +++ b/mail_tracking/views/res_partner_view.xml @@ -24,8 +24,23 @@ + + + Filter bounced partners + res.partner + + + + + + + + +