From 35ccabf051f8c838ea89383dee23dd9252ed055b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 14:41:37 +0000 Subject: [PATCH 01/14] [IMP] digest: add private methods to manage digest members Purpose is to ease digest management and testing. Public methods still work as before this commit, simply calling the private implementation. New private methods allows to better manipulate subscribed users. Those will be used notably as shortcuts in tests. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onbarding and usage improvement) X-original-commit: 3045c49f0e10bd50757fcbaa75f9aaad49a966ce Part-of: odoo/odoo#79877 --- addons/digest/models/digest.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/addons/digest/models/digest.py b/addons/digest/models/digest.py index 483417f8c73..32b372eab37 100644 --- a/addons/digest/models/digest.py +++ b/addons/digest/models/digest.py @@ -87,11 +87,21 @@ class Digest(models.Model): def action_subscribe(self): if self.env.user.has_group('base.group_user') and self.env.user not in self.user_ids: - self.sudo().user_ids |= self.env.user + self._action_subscribe_users(self.env.user) + + def _action_subscribe_users(self, users): + """ Private method to manage subscriptions. Done as sudo() to speedup + computation and avoid ACLs issues. """ + self.sudo().user_ids |= users def action_unsubcribe(self): if self.env.user.has_group('base.group_user') and self.env.user in self.user_ids: - self.sudo().user_ids -= self.env.user + self._action_unsubscribe_users(self.env.user) + + def _action_unsubscribe_users(self, users): + """ Private method to manage subscriptions. Done as sudo() to speedup + computation and avoid ACLs issues. """ + self.sudo().user_ids -= users def action_activate(self): self.state = 'activated' From dad888de50d377be8bdc51da7745117926144e43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 10 Nov 2021 11:36:46 +0000 Subject: [PATCH 02/14] [REF] test_mail_full: cleanup test data, use default one Purpose of this commit is to have as less custom test data as possible. In some sms tests we can re-use existing mailing, leading to more standard tests and expected results. We also improve ``gateway_sms_sent_click`` tool that now creates random IP addresses allowing to have various clicks on a given tracker. Indeed as IP is unique several clicks are currently set into the same click record, which will now not be the case anymore. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onbarding and usage improvement) Task-2686586 (Repair mailing statistics email) X-original-commit: f76b154412a111067203979075fa65f198cc39f4 Part-of: odoo/odoo#79877 --- addons/mass_mailing_sms/tests/common.py | 4 +- addons/test_mail_full/tests/common.py | 6 +- addons/test_mail_full/tests/test_mass_sms.py | 65 ++++++++++---------- 3 files changed, 37 insertions(+), 38 deletions(-) diff --git a/addons/mass_mailing_sms/tests/common.py b/addons/mass_mailing_sms/tests/common.py index e5a26f302ef..4b85306ac01 100644 --- a/addons/mass_mailing_sms/tests/common.py +++ b/addons/mass_mailing_sms/tests/common.py @@ -1,5 +1,7 @@ # -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. +import random import re import werkzeug @@ -147,7 +149,7 @@ class MassSMSCase(SMSCase): self.env['link.tracker.click'].sudo().add_click( code, - ip='100.200.300.400', + ip='100.200.300.%3f' % random.random(), country_code='BE', mailing_trace_id=trace_id ) diff --git a/addons/test_mail_full/tests/common.py b/addons/test_mail_full/tests/common.py index b17f2cd8bb2..71ac889ea95 100644 --- a/addons/test_mail_full/tests/common.py +++ b/addons/test_mail_full/tests/common.py @@ -18,8 +18,8 @@ class TestMailFullCommon(TestMassMailCommon, MassSMSCommon): 'subject': 'Xmas SMS for {object.name}', 'mailing_model_id': cls.env['ir.model']._get('mail.test.sms').id, 'mailing_type': 'sms', - 'mailing_domain': '%s' % repr([('name', 'ilike', 'SMSTest')]), - 'body_plaintext': 'Dear {{ object.display_name }} this is a mass SMS with two links http://www.odoo.com/smstest and http://www.odoo.com/smstest/{{ object.id }}', + 'mailing_domain': '%s' % repr([('name', 'ilike', 'MassSMSTest')]), + 'body_plaintext': 'Dear {{object.display_name}} this is a mass SMS with two links http://www.odoo.com/smstest and http://www.odoo.com/smstest/{{object.id}}', 'sms_force_send': True, 'sms_allow_unsubscribe': True, }) @@ -34,7 +34,7 @@ class TestMailFullCommon(TestMassMailCommon, MassSMSCommon): vals_list = [] for idx in range(count): vals = { - 'name': 'SMSTestRecord_%02d' % idx, + 'name': 'MassSMSTestRecord_%02d' % idx, phone_field: '045600%02d%02d' % (idx, idx) } if partners: diff --git a/addons/test_mail_full/tests/test_mass_sms.py b/addons/test_mail_full/tests/test_mass_sms.py index e4379a6c1f9..9783b5da82d 100644 --- a/addons/test_mail_full/tests/test_mass_sms.py +++ b/addons/test_mail_full/tests/test_mass_sms.py @@ -51,16 +51,6 @@ class TestMassSMSCommon(TestMailFullCommon): for partner in partners ] - cls.mailing = cls.env['mailing.mailing'].with_user(cls.user_marketing).create({ - 'name': 'Xmas Spam', - 'subject': 'Xmas Spam', - 'mailing_model_id': cls.env['ir.model']._get('mail.test.sms').id, - 'mailing_type': 'sms', - 'mailing_domain': '%s' % repr([('name', 'ilike', 'MassSMSTest')]), - 'sms_template_id': cls.sms_template.id, - 'sms_allow_unsubscribe': False, - }) - @tagged('mass_mailing') class TestMassSMSInternals(TestMassSMSCommon): @@ -114,6 +104,7 @@ class TestMassSMSInternals(TestMassSMSCommon): mailing, self.records, ) + @users('user_marketing') def test_mass_sms_internals_errors(self): # same customer, specific different number on record -> should be valid new_record_1 = self.env['mail.test.sms'].create({ @@ -141,7 +132,7 @@ class TestMassSMSInternals(TestMassSMSCommon): 'customer_id': False, 'phone_nbr': '0456110011', }) - self.env['phone.blacklist'].create({'number': '0456110011'}) + self.env['phone.blacklist'].sudo().create({'number': '0456110011'}) # new customer, number already on record -> should be ignored country_be_id = self.env.ref('base.be').id nr2_partner = self.env['res.partner'].create({ @@ -156,81 +147,87 @@ class TestMassSMSInternals(TestMassSMSCommon): }) records_numbers = self.records_numbers + ['+32456999999'] + mailing = self.env['mailing.mailing'].browse(self.mailing_sms.ids) + mailing.write({'sms_force_send': False}) # force outgoing sms, not sent with self.with_user('user_marketing'): with self.mockSMSGateway(): - self.mailing.action_send_sms() + mailing.action_send_sms() self.assertSMSTraces( [{'partner': record.customer_id, 'number': records_numbers[i], - 'content': 'Dear %s this is a mass SMS.' % record.display_name} + 'content': 'Dear %s this is a mass SMS' % record.display_name} for i, record in enumerate(self.records | new_record_1)], - self.mailing, self.records | new_record_1, + mailing, self.records | new_record_1, ) # duplicates self.assertSMSTraces( [{'partner': new_record_2.customer_id, 'number': self.records_numbers[0], - 'content': 'Dear %s this is a mass SMS.' % new_record_2.display_name, 'trace_status': 'cancel', + 'content': 'Dear %s this is a mass SMS' % new_record_2.display_name, 'trace_status': 'cancel', 'failure_type': 'sms_duplicate'}], - self.mailing, new_record_2, + mailing, new_record_2, ) # blacklist self.assertSMSTraces( [{'partner': self.env['res.partner'], 'number': phone_validation.phone_format(bl_record_1.phone_nbr, 'BE', '32', force_format='E164'), - 'content': 'Dear %s this is a mass SMS.' % bl_record_1.display_name, 'trace_status': 'cancel', + 'content': 'Dear %s this is a mass SMS' % bl_record_1.display_name, 'trace_status': 'cancel', 'failure_type': 'sms_blacklist'}], - self.mailing, bl_record_1, + mailing, bl_record_1, ) # missing number self.assertSMSTraces( [{'partner': self.env['res.partner'], 'number': False, - 'content': 'Dear %s this is a mass SMS.' % void_record.display_name, 'trace_status': 'cancel', + 'content': 'Dear %s this is a mass SMS' % void_record.display_name, 'trace_status': 'cancel', 'failure_type': 'sms_number_missing'}], - self.mailing, void_record, + mailing, void_record, ) # wrong values self.assertSMSTraces( [{'partner': self.env['res.partner'], 'number': record.phone_nbr, - 'content': 'Dear %s this is a mass SMS.' % record.display_name, 'trace_status': 'cancel', + 'content': 'Dear %s this is a mass SMS' % record.display_name, 'trace_status': 'cancel', 'failure_type': 'sms_number_format'} for record in falsy_record_1 + falsy_record_2], - self.mailing, falsy_record_1 + falsy_record_2, + mailing, falsy_record_1 + falsy_record_2, ) + @users('user_marketing') def test_mass_sms_internals_done_ids(self): + mailing = self.env['mailing.mailing'].browse(self.mailing_sms.ids) + mailing.write({'sms_force_send': False}) # check with outgoing traces, not already sent + with self.with_user('user_marketing'): with self.mockSMSGateway(): - self.mailing.action_send_sms(res_ids=self.records[:5].ids) + mailing.action_send_sms(res_ids=self.records[:5].ids) - traces = self.env['mailing.trace'].search([('mass_mailing_id', 'in', self.mailing.ids)]) + traces = self.env['mailing.trace'].search([('mass_mailing_id', 'in', mailing.ids)]) self.assertEqual(len(traces), 5) # new traces generated self.assertSMSTraces( [{'partner': record.customer_id, 'number': self.records_numbers[i], - 'content': 'Dear %s this is a mass SMS.' % record.display_name} + 'content': 'Dear %s this is a mass SMS' % record.display_name} for i, record in enumerate(self.records[:5])], - self.mailing, self.records[:5], + mailing, self.records[:5], ) with self.with_user('user_marketing'): with self.mockSMSGateway(): - self.mailing.action_send_sms(res_ids=self.records.ids) + mailing.action_send_sms(res_ids=self.records.ids) # delete old traces (for testing purpose: ease check by deleting old ones) traces.unlink() # new failed traces generated for duplicates self.assertSMSTraces( [{'partner': record.customer_id, 'number': self.records_numbers[i], - 'content': 'Dear %s this is a mass SMS.' % record.display_name, 'trace_status': 'cancel', + 'content': 'Dear %s this is a mass SMS' % record.display_name, 'trace_status': 'cancel', 'failure_type': 'sms_duplicate'} for i, record in enumerate(self.records[:5])], - self.mailing, self.records[:5], + mailing, self.records[:5], ) # new traces generated self.assertSMSTraces( [{'partner': record.customer_id, 'number': self.records_numbers[i+5], - 'content': 'Dear %s this is a mass SMS.' % record.display_name} + 'content': 'Dear %s this is a mass SMS' % record.display_name} for i, record in enumerate(self.records[5:])], - self.mailing, self.records[5:], + mailing, self.records[5:], ) @mute_logger('odoo.addons.mail.models.mail_render_mixin') @@ -268,7 +265,7 @@ class TestMassSMS(TestMassSMSCommon): @users('user_marketing') def test_mass_sms_links(self): - mailing = self.env['mailing.mailing'].browse(self.mailing.ids) + mailing = self.env['mailing.mailing'].browse(self.mailing_sms.ids) mailing.write({ 'body_plaintext': 'Dear {{ object.display_name }} this is a mass SMS with two links http://www.odoo.com/smstest and http://www.odoo.com/smstest/{{ object.name }}', 'sms_template_id': False, @@ -305,7 +302,7 @@ class TestMassSMS(TestMassSMSCommon): }) records = self.env['mail.test.sms.partner'].create([ - {'name': 'SMSTest on %s' % partner.name, + {'name': 'MassSMSTest on %s' % partner.name, 'customer_id': partner.id, } for partner in self.partners ]) @@ -365,7 +362,7 @@ class TestMassSMS(TestMassSMSCommon): }) self.env['mail.test.sms.partner.2many'].create([ - {'name': 'SMSTest on %s' % partner.name, + {'name': 'MassSMSTest on %s' % partner.name, 'customer_ids': [(4, partner.id)], } for partner in self.partners ]) From 581286346aff89cf61a0a747286030a2052a09e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 17:01:06 +0000 Subject: [PATCH 03/14] [FIX] mass_mailing(_sms/_crm/_sale): fix digest layout change breaking mailing statistics Recently some "de-t-rawify" [1] was done through Odoo. A change in digest layout broke mailing statistics. Indeed mailing statistics are build on digest main layout to re-use it. In mailing case a ``kpi_name`` value was missing when rendering statistics columns, leading to a crash. This is now fixed in both mailing and sms marketing. In this commit we also add tests for statistics emails (D+1 KPIs) rendering for both mail and SMS marketing mailings. Email details are also tested in order to better highlight future changes and tweaks in email sending linked to D+1 KPIs emails. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onboarding and usage improvement) Task-2686586 (Repair mailing statistics email) [1] odoo/odoo@c56e8c9f5ae0598c1bb3489917d82a36186c468a X-original-commit: c9905ceb3172d8e80e3aff4ae9bc925dcb6b6519 Part-of: odoo/odoo#79877 --- addons/mass_mailing/models/mailing.py | 2 + .../models/mailing_mailing.py | 1 + .../models/mailing_mailing.py | 1 + .../models/mailing_mailing.py | 1 + addons/test_mail_full/tests/__init__.py | 1 + .../tests/test_mass_mailing_statistics.py | 71 +++++++++++++ addons/test_mass_mailing/tests/__init__.py | 1 + .../tests/test_mailing_statistics.py | 100 ++++++++++++++++++ 8 files changed, 178 insertions(+) create mode 100644 addons/test_mail_full/tests/test_mass_mailing_statistics.py create mode 100644 addons/test_mass_mailing/tests/test_mailing_statistics.py diff --git a/addons/mass_mailing/models/mailing.py b/addons/mass_mailing/models/mailing.py index 9e0f0092325..67cbcfafe95 100644 --- a/addons/mass_mailing/models/mailing.py +++ b/addons/mass_mailing/models/mailing.py @@ -1005,6 +1005,7 @@ class MassMailing(models.Model): 'col_subtitle': _('REPLIED (%i)', self.replied), }, 'kpi_action': None, + 'kpi_name': self.mailing_type, } random_tip = self.env['digest.tip'].search( @@ -1037,6 +1038,7 @@ class MassMailing(models.Model): 'kpi_col1': {}, 'kpi_col2': {}, 'kpi_col3': {}, + 'kpi_name': 'trace', }, ], 'tips': [random_tip] if random_tip else False, diff --git a/addons/mass_mailing_crm/models/mailing_mailing.py b/addons/mass_mailing_crm/models/mailing_mailing.py index f8003d5d0ef..b804327242a 100644 --- a/addons/mass_mailing_crm/models/mailing_mailing.py +++ b/addons/mass_mailing_crm/models/mailing_mailing.py @@ -40,4 +40,5 @@ class MassMailing(models.Model): 'value': tools.format_decimalized_number(self.crm_lead_count, decimal=0), 'col_subtitle': _('LEADS'), } + values['kpi_data'][1]['kpi_name'] = 'lead' return values diff --git a/addons/mass_mailing_sale/models/mailing_mailing.py b/addons/mass_mailing_sale/models/mailing_mailing.py index 292bba3c599..ad4f2f1fc8d 100644 --- a/addons/mass_mailing_sale/models/mailing_mailing.py +++ b/addons/mass_mailing_sale/models/mailing_mailing.py @@ -87,4 +87,5 @@ class MassMailing(models.Model): 'value': formated_amount, 'col_subtitle': _('INVOICED'), } + values['kpi_data'][1]['kpi_name'] = 'sale' return values diff --git a/addons/mass_mailing_sms/models/mailing_mailing.py b/addons/mass_mailing_sms/models/mailing_mailing.py index 43f4cf7be46..43944101226 100644 --- a/addons/mass_mailing_sms/models/mailing_mailing.py +++ b/addons/mass_mailing_sms/models/mailing_mailing.py @@ -291,6 +291,7 @@ class Mailing(models.Model): 'col_subtitle': _('BOUNCED (%i)', self.bounced), }, 'kpi_action': None, + 'kpi_name': self.mailing_type, } return values diff --git a/addons/test_mail_full/tests/__init__.py b/addons/test_mail_full/tests/__init__.py index 9c4bf096766..9b71f41ab53 100644 --- a/addons/test_mail_full/tests/__init__.py +++ b/addons/test_mail_full/tests/__init__.py @@ -4,6 +4,7 @@ from . import common from . import test_odoobot from . import test_phone_blacklist from . import test_mass_mailing +from . import test_mass_mailing_statistics from . import test_mass_sms from . import test_sms_composer from . import test_sms_management diff --git a/addons/test_mail_full/tests/test_mass_mailing_statistics.py b/addons/test_mail_full/tests/test_mass_mailing_statistics.py new file mode 100644 index 00000000000..b9d552a717a --- /dev/null +++ b/addons/test_mail_full/tests/test_mass_mailing_statistics.py @@ -0,0 +1,71 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from lxml import html + +from odoo.addons.mail.tests.common import mail_new_test_user +from odoo.addons.test_mail_full.tests.test_mass_sms import TestMassSMSCommon +from odoo.tests.common import users +from odoo.tests import tagged +from odoo.tools import formataddr, mute_logger + + +@tagged('digest', 'mass_mailing') +class TestMailingStatistics(TestMassSMSCommon): + + @classmethod + def setUpClass(cls): + super(TestMailingStatistics, cls).setUpClass() + + cls.user_marketing_2 = mail_new_test_user( + cls.env, + groups='base.group_user,base.group_partner_manager,mass_mailing.group_mass_mailing_user', + login='user_marketing_2', + name='Marie Marketing', + signature='--\nMarie' + ) + + @users('user_marketing') + @mute_logger('odoo.addons.mass_mailing_sms.models.mailing_mailing', 'odoo.addons.mail.models.mail_mail', 'odoo.addons.mail.models.mail_thread') + def test_mailing_statistics_sms(self): + mailing = self.env['mailing.mailing'].browse(self.mailing_sms.ids) + target_records = self.env['mail.test.sms'].browse(self.records.ids) + mailing.write({'mailing_domain': [('id', 'in', target_records.ids)], 'user_id': self.user_marketing_2.id}) + mailing.action_put_in_queue() + with self.mockSMSGateway(): + mailing.action_send_sms() + + # simulate some replies and clicks + self.gateway_sms_click(mailing, target_records[0]) + self.gateway_sms_click(mailing, target_records[2]) + self.gateway_sms_click(mailing, target_records[3]) + + # check mailing statistics + self.assertEqual(mailing.clicked, 3) + self.assertEqual(mailing.delivered, 10) + self.assertEqual(mailing.opened, 3) + self.assertEqual(mailing.opened_ratio, 30) + self.assertEqual(mailing.sent, 10) + + with self.mock_mail_gateway(mail_unlink_sent=False): + mailing._action_send_statistics() + + self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") + mail = self._new_mails[0] + # test email values + self.assertEqual(mail.author_id, self.user_marketing.partner_id) + self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) + self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) + self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.state, 'sent') + # test body content: KPIs + body_html = html.fromstring(mail.body_html) + kpi_values = body_html.xpath('//div[@data-field="sms"]//*[hasclass("kpi_value")]/text()') + self.assertEqual( + [t.strip().strip('%') for t in kpi_values], + ['100', str(mailing.opened_ratio), str(mailing.replied_ratio)] + ) + # test body content: clicks (a bit hackish but hey we are in stable) + kpi_click_values = body_html.xpath('//div[hasclass("global_layout")]/table//tr[contains(@style,"color: #888888")]/td[contains(@style,"width: 30%")]/text()') + first_link_value = int(kpi_click_values[0].strip().split()[1].strip('()')) + self.assertEqual(first_link_value, mailing.clicked) diff --git a/addons/test_mass_mailing/tests/__init__.py b/addons/test_mass_mailing/tests/__init__.py index c50bc65832e..c5ab39a1c37 100644 --- a/addons/test_mass_mailing/tests/__init__.py +++ b/addons/test_mass_mailing/tests/__init__.py @@ -7,5 +7,6 @@ from . import test_blacklist_mixin from . import test_link_tracker from . import test_mailing from . import test_mailing_server +from . import test_mailing_statistics from . import test_mailing_test from . import test_performance diff --git a/addons/test_mass_mailing/tests/test_mailing_statistics.py b/addons/test_mass_mailing/tests/test_mailing_statistics.py new file mode 100644 index 00000000000..be4b193f09e --- /dev/null +++ b/addons/test_mass_mailing/tests/test_mailing_statistics.py @@ -0,0 +1,100 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + +from lxml import html + +from odoo.addons.mail.tests.common import mail_new_test_user +from odoo.addons.test_mass_mailing.data.mail_test_data import MAIL_TEMPLATE +from odoo.addons.test_mass_mailing.tests.common import TestMassMailCommon +from odoo.tests.common import users +from odoo.tests import tagged +from odoo.tools import formataddr, mute_logger + + +@tagged('digest') +class TestMailingStatistics(TestMassMailCommon): + + @classmethod + def setUpClass(cls): + super(TestMailingStatistics, cls).setUpClass() + + cls.user_marketing_2 = mail_new_test_user( + cls.env, + groups='base.group_user,base.group_partner_manager,mass_mailing.group_mass_mailing_user', + login='user_marketing_2', + name='Marie Marketing', + signature='--\nMarie' + ) + + @users('user_marketing') + @mute_logger('odoo.addons.mass_mailing.models.mailing', 'odoo.addons.mail.models.mail_mail', 'odoo.addons.mail.models.mail_thread') + def test_mailing_statistics(self): + target_records = self._create_mailing_test_records(model='mailing.test.blacklist', count=10) + mailing = self.env['mailing.mailing'].browse(self.mailing_bl.ids) + mailing.write({'mailing_domain': [('id', 'in', target_records.ids)], 'user_id': self.user_marketing_2.id}) + mailing.action_put_in_queue() + with self.mock_mail_gateway(mail_unlink_sent=False): + mailing.action_send_mail() + + # simulate some replies and clicks + self.gateway_mail_reply_wrecord(MAIL_TEMPLATE, target_records[0], use_in_reply_to=True) + self.gateway_mail_reply_wrecord(MAIL_TEMPLATE, target_records[1], use_in_reply_to=True) + self.gateway_mail_reply_wrecord(MAIL_TEMPLATE, target_records[2], use_in_reply_to=True) + self.gateway_mail_click(mailing, target_records[0], 'https://www.odoo.be') + self.gateway_mail_click(mailing, target_records[2], 'https://www.odoo.be') + self.gateway_mail_click(mailing, target_records[3], 'https://www.odoo.be') + + # check mailing statistics + self.assertEqual(mailing.clicked, 3) + self.assertEqual(mailing.delivered, 10) + self.assertEqual(mailing.opened, 4) + self.assertEqual(mailing.opened_ratio, 40) + self.assertEqual(mailing.replied, 3) + self.assertEqual(mailing.replied_ratio, 30) + self.assertEqual(mailing.sent, 10) + + with self.mock_mail_gateway(mail_unlink_sent=False): + mailing._action_send_statistics() + + self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") + mail = self._new_mails[0] + # test email values + self.assertEqual(mail.author_id, self.user_marketing.partner_id) + self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) + self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) + self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.state, 'sent') + # test body content: KPIs + body_html = html.fromstring(mail.body_html) + kpi_values = body_html.xpath('//div[@data-field="mail"]//*[hasclass("kpi_value")]/text()') + self.assertEqual( + [t.strip().strip('%') for t in kpi_values], + ['100', str(mailing.opened_ratio), str(mailing.replied_ratio)] + ) + # test body content: clicks (a bit hackish but hey we are in stable) + kpi_click_values = body_html.xpath('//div[hasclass("global_layout")]/table//tr[contains(@style,"color: #888888")]/td[contains(@style,"width: 30%")]/text()') + first_link_value = int(kpi_click_values[0].strip().split()[1].strip('()')) + self.assertEqual(first_link_value, mailing.clicked) + + @users('user_marketing') + @mute_logger('odoo.addons.mass_mailing.models.mailing', 'odoo.addons.mail.models.mail_mail', 'odoo.addons.mail.models.mail_thread') + def test_mailing_statistics_wo_user(self): + target_records = self._create_mailing_test_records(model='mailing.test.blacklist', count=10) + mailing = self.env['mailing.mailing'].browse(self.mailing_bl.ids) + mailing.write({'mailing_domain': [('id', 'in', target_records.ids)], 'user_id': False}) + mailing.action_put_in_queue() + with self.mock_mail_gateway(mail_unlink_sent=False): + mailing.action_send_mail() + + with self.mock_mail_gateway(mail_unlink_sent=False): + mailing._action_send_statistics() + + self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") + mail = self._new_mails[0] + # test email values + self.assertEqual(mail.author_id, self.user_marketing.partner_id) + self.assertFalse(mail.email_from) + self.assertFalse(mail.email_to) + self.assertTrue(mail.failure_reason) + self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.state, 'exception') From c59039b195c5a9634883ca60ace561c8b818f351 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 16 Nov 2021 13:28:52 +0000 Subject: [PATCH 04/14] [FIX] mass_mailing: avoid clicks overriding reply trace state Currently if someone replies to a mass mailing email, then performs a click on tracked links, final trace status is set to opened. Indeed both reply and click set the trace as opened, as replying or clicking imply opening the mail. Update is the following * user replies -> set status to open, then set status to reply, and update open_datetime and reply_datetime; * user clicks -> set status to open, then update click_datetime; In the end the trace is left as opened while it is replied, with a click datetime attached to the trace. To avoid unnecessary status update, a trace is set to opened only if it is not already opened or replied. Other statuses should not override themselves. Task-2692316 X-original-commit: ce88d364369c8b58256e8d93b7cb506a2c262a34 Part-of: odoo/odoo#79877 --- addons/mass_mailing/models/mailing_trace.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/addons/mass_mailing/models/mailing_trace.py b/addons/mass_mailing/models/mailing_trace.py index 2f12c06c528..ee4d5414f97 100644 --- a/addons/mass_mailing/models/mailing_trace.py +++ b/addons/mass_mailing/models/mailing_trace.py @@ -131,8 +131,11 @@ class MailingTrace(models.Model): return traces def set_opened(self, domain=None): + """ Reply / Open are a bit shared in various processes: reply implies + open, click implies open. Let us avoid status override by skipping traces + that are not already opened or replied. """ traces = self + (self.search(domain) if domain else self.env['mailing.trace']) - traces.write({'trace_status': 'open', 'open_datetime': fields.Datetime.now()}) + traces.filtered(lambda t: t.trace_status not in ('open', 'reply')).write({'trace_status': 'open', 'open_datetime': fields.Datetime.now()}) return traces def set_clicked(self, domain=None): From 8df08eac57a0fa7e6e70293431cdae660d19caca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 17:04:39 +0000 Subject: [PATCH 05/14] [FIX] mass_mailing: use queue to send statistics emails When there is an issue updating mailing state (concurrent access, or some other error that may happen), mailing statistics emails may be sent in loop. As we do not think timing is so important, we now use the email queue to send statistics emails. Task-2686586 (Repair mailing statistics) X-original-commit: f78ac45c2eafc3cf6b213e1f1608e76e243bd8d2 Part-of: odoo/odoo#79877 --- addons/mass_mailing/models/mailing.py | 6 ++++-- .../test_mail_full/tests/test_mass_mailing_statistics.py | 4 ++-- addons/test_mass_mailing/tests/test_mailing_statistics.py | 7 +++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/addons/mass_mailing/models/mailing.py b/addons/mass_mailing/models/mailing.py index 67cbcfafe95..e88fb9e6fad 100644 --- a/addons/mass_mailing/models/mailing.py +++ b/addons/mass_mailing/models/mailing.py @@ -931,6 +931,7 @@ class MassMailing(models.Model): """Send an email to the responsible of each finished mailing with the statistics.""" self.kpi_mail_required = False + mails_sudo = self.env['mail.mail'].sudo() for mailing in self: user = mailing.user_id mailing = mailing.with_context(lang=user.lang or self._context.get('lang')) @@ -973,9 +974,10 @@ class MassMailing(models.Model): 'email_to': user.email_formatted, 'body_html': full_mail, 'auto_delete': True, + 'state': 'outgoing', } - mail = self.env['mail.mail'].sudo().create(mail_values) - mail.send(raise_exception=False) + mails_sudo += self.env['mail.mail'].sudo().create(mail_values) + return mails_sudo def _prepare_statistics_email_values(self): """Return some statistics that will be displayed in the mailing statistics email. diff --git a/addons/test_mail_full/tests/test_mass_mailing_statistics.py b/addons/test_mail_full/tests/test_mass_mailing_statistics.py index b9d552a717a..8a3f67ae0eb 100644 --- a/addons/test_mail_full/tests/test_mass_mailing_statistics.py +++ b/addons/test_mail_full/tests/test_mass_mailing_statistics.py @@ -47,7 +47,7 @@ class TestMailingStatistics(TestMassSMSCommon): self.assertEqual(mailing.opened_ratio, 30) self.assertEqual(mailing.sent, 10) - with self.mock_mail_gateway(mail_unlink_sent=False): + with self.mock_mail_gateway(mail_unlink_sent=True): mailing._action_send_statistics() self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") @@ -57,7 +57,7 @@ class TestMailingStatistics(TestMassSMSCommon): self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) - self.assertEqual(mail.state, 'sent') + self.assertEqual(mail.state, 'outgoing') # test body content: KPIs body_html = html.fromstring(mail.body_html) kpi_values = body_html.xpath('//div[@data-field="sms"]//*[hasclass("kpi_value")]/text()') diff --git a/addons/test_mass_mailing/tests/test_mailing_statistics.py b/addons/test_mass_mailing/tests/test_mailing_statistics.py index be4b193f09e..629b14658ef 100644 --- a/addons/test_mass_mailing/tests/test_mailing_statistics.py +++ b/addons/test_mass_mailing/tests/test_mailing_statistics.py @@ -53,7 +53,7 @@ class TestMailingStatistics(TestMassMailCommon): self.assertEqual(mailing.replied_ratio, 30) self.assertEqual(mailing.sent, 10) - with self.mock_mail_gateway(mail_unlink_sent=False): + with self.mock_mail_gateway(mail_unlink_sent=True): mailing._action_send_statistics() self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") @@ -63,7 +63,7 @@ class TestMailingStatistics(TestMassMailCommon): self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) - self.assertEqual(mail.state, 'sent') + self.assertEqual(mail.state, 'outgoing') # test body content: KPIs body_html = html.fromstring(mail.body_html) kpi_values = body_html.xpath('//div[@data-field="mail"]//*[hasclass("kpi_value")]/text()') @@ -95,6 +95,5 @@ class TestMailingStatistics(TestMassMailCommon): self.assertEqual(mail.author_id, self.user_marketing.partner_id) self.assertFalse(mail.email_from) self.assertFalse(mail.email_to) - self.assertTrue(mail.failure_reason) self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) - self.assertEqual(mail.state, 'exception') + self.assertEqual(mail.state, 'outgoing') From 73672cfc9ff517604238339a5bf80fae98fda14f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 10 Nov 2021 14:20:20 +0000 Subject: [PATCH 06/14] [FIX] mass_mailing: fix mail related values of statistics emails When mailings have no responsible, KPIs emails are broken as there is no from and to on the mail. This leads to mails being invalid and set in exception. Moreover currently author and from / to of those D+1 emails are not coherent as author is current user (generally odoobot as this is generated through a cron) while from and to are based on mailing responsible. In this commit we make statistics emails more coherent * when a responsible is set on the mailing: author, from and to are linked to the responsible; * when there is no responsible, current user is set as it may be sent by regular people; * reply-to is set to company email as it is often the case with 'marketing' emails; Task-2686586 (Repair mailing statistics email) X-original-commit: 95a21ca16f560cf4341c547f6bc909d3261acd1b Part-of: odoo/odoo#79877 --- addons/mass_mailing/models/mailing.py | 24 ++++++++++++------- .../tests/test_mass_mailing_statistics.py | 6 ++--- .../tests/test_mailing_statistics.py | 12 +++++----- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/addons/mass_mailing/models/mailing.py b/addons/mass_mailing/models/mailing.py index e88fb9e6fad..526c52b5a10 100644 --- a/addons/mass_mailing/models/mailing.py +++ b/addons/mass_mailing/models/mailing.py @@ -933,9 +933,13 @@ class MassMailing(models.Model): mails_sudo = self.env['mail.mail'].sudo() for mailing in self: - user = mailing.user_id - mailing = mailing.with_context(lang=user.lang or self._context.get('lang')) + if mailing.user_id: + mailing = mailing.with_user(mailing.user_id).with_context( + lang=mailing.user_id.lang or self._context.get('lang') + ) mailing_type = mailing._get_pretty_mailing_type() + mail_user = mailing.user_id or self.env.user + mail_company = mail_user.company_id link_trackers = self.env['link.tracker'].search( [('mass_mailing_id', '=', mailing.id)] @@ -953,8 +957,8 @@ class MassMailing(models.Model): 'digest.digest_mail_main', { 'body': tools.html_sanitize(link_trackers_body), - 'company': user.company_id, - 'user': user, + 'company': mail_company, + 'user': mail_user, 'display_mobile_banner': True, ** mailing._prepare_statistics_email_values() }, @@ -966,15 +970,17 @@ class MassMailing(models.Model): ) mail_values = { + 'auto_delete': True, + 'author_id': mail_user.partner_id.id, + 'email_from': mail_user.email_formatted, + 'email_to': mail_user.email_formatted, + 'body_html': full_mail, + 'reply_to': mail_company.email_formatted or mail_user.email_formatted, + 'state': 'outgoing', 'subject': _('24H Stats of %(mailing_type)s "%(mailing_name)s"', mailing_type=mailing._get_pretty_mailing_type(), mailing_name=mailing.subject ), - 'email_from': user.email_formatted, - 'email_to': user.email_formatted, - 'body_html': full_mail, - 'auto_delete': True, - 'state': 'outgoing', } mails_sudo += self.env['mail.mail'].sudo().create(mail_values) return mails_sudo diff --git a/addons/test_mail_full/tests/test_mass_mailing_statistics.py b/addons/test_mail_full/tests/test_mass_mailing_statistics.py index 8a3f67ae0eb..45f0dc134bc 100644 --- a/addons/test_mail_full/tests/test_mass_mailing_statistics.py +++ b/addons/test_mail_full/tests/test_mass_mailing_statistics.py @@ -7,7 +7,7 @@ from odoo.addons.mail.tests.common import mail_new_test_user from odoo.addons.test_mail_full.tests.test_mass_sms import TestMassSMSCommon from odoo.tests.common import users from odoo.tests import tagged -from odoo.tools import formataddr, mute_logger +from odoo.tools import mute_logger @tagged('digest', 'mass_mailing') @@ -53,10 +53,10 @@ class TestMailingStatistics(TestMassSMSCommon): self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") mail = self._new_mails[0] # test email values - self.assertEqual(mail.author_id, self.user_marketing.partner_id) + self.assertEqual(mail.author_id, self.user_marketing_2.partner_id) self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) - self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.reply_to, self.company_admin.partner_id.email_formatted) self.assertEqual(mail.state, 'outgoing') # test body content: KPIs body_html = html.fromstring(mail.body_html) diff --git a/addons/test_mass_mailing/tests/test_mailing_statistics.py b/addons/test_mass_mailing/tests/test_mailing_statistics.py index 629b14658ef..fdc5b866b2e 100644 --- a/addons/test_mass_mailing/tests/test_mailing_statistics.py +++ b/addons/test_mass_mailing/tests/test_mailing_statistics.py @@ -8,7 +8,7 @@ from odoo.addons.test_mass_mailing.data.mail_test_data import MAIL_TEMPLATE from odoo.addons.test_mass_mailing.tests.common import TestMassMailCommon from odoo.tests.common import users from odoo.tests import tagged -from odoo.tools import formataddr, mute_logger +from odoo.tools import mute_logger @tagged('digest') @@ -59,10 +59,10 @@ class TestMailingStatistics(TestMassMailCommon): self.assertEqual(len(self._new_mails), 1, "Mailing: a mail should have been created for statistics") mail = self._new_mails[0] # test email values - self.assertEqual(mail.author_id, self.user_marketing.partner_id) + self.assertEqual(mail.author_id, self.user_marketing_2.partner_id) self.assertEqual(mail.email_from, self.user_marketing_2.email_formatted) self.assertEqual(mail.email_to, self.user_marketing_2.email_formatted) - self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.reply_to, self.company_admin.partner_id.email_formatted) self.assertEqual(mail.state, 'outgoing') # test body content: KPIs body_html = html.fromstring(mail.body_html) @@ -93,7 +93,7 @@ class TestMailingStatistics(TestMassMailCommon): mail = self._new_mails[0] # test email values self.assertEqual(mail.author_id, self.user_marketing.partner_id) - self.assertFalse(mail.email_from) - self.assertFalse(mail.email_to) - self.assertEqual(mail.reply_to, formataddr((self.company_admin.name, '%s@%s' % (self.alias_catchall, self.alias_domain)))) + self.assertEqual(mail.email_from, self.user_marketing.email_formatted) + self.assertEqual(mail.email_to, self.user_marketing.email_formatted) + self.assertEqual(mail.reply_to, self.company_admin.partner_id.email_formatted) self.assertEqual(mail.state, 'outgoing') From 54780667a95f33df88da0ce81234d29a74ffb1ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 14:51:29 +0000 Subject: [PATCH 07/14] [FIX] digest: add missing depends on is_subscribed field Currently subscribed field is not computed again if user_ids field value changes on a digest. This has probably few consequences in current code but better be sure this field is correctly updated in a given transaction. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onboarding and usage improvement) X-original-commit: 3a7c1348750d0977a663e50be05bad9f2679f8ff Part-of: odoo/odoo#79877 --- addons/digest/models/digest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/addons/digest/models/digest.py b/addons/digest/models/digest.py index 32b372eab37..628c4dca397 100644 --- a/addons/digest/models/digest.py +++ b/addons/digest/models/digest.py @@ -41,6 +41,7 @@ class Digest(models.Model): kpi_mail_message_total = fields.Boolean('Messages') kpi_mail_message_total_value = fields.Integer(compute='_compute_kpi_mail_message_total_value') + @api.depends('user_ids') def _compute_is_subscribed(self): for digest in self: digest.is_subscribed = self.env.user in digest.user_ids From 9c25a65fa2ab5ba91efc8d286604e49938c1ba3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 15 Sep 2021 14:26:35 +0000 Subject: [PATCH 08/14] [FW][FIX] digest: use email queue to send digest emails (and improve tests) When there is an issue updating digest state (concurrent access, or some other error that may happen), digest emails may be sent in loop. Indeed they are currently sent in the same transaction that the one that updates digest state. This means that emails may be sent even if digest update fails. As we do not think timing is so important, we now use the email queue to send digest emails. That way email creation is rollbacked and they are not sent if digest update fails for some reason. Tests are updated to take into account we now create outgoing mail.mail. Some tests are also added about mail values and subscription, and cleaned in a more general way. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onbarding and usage improvement) X-original-commit: 96a14816cb48d9324482eec3de0ed3fccc0d7ecf Part-of: odoo/odoo#79877 --- addons/digest/models/digest.py | 9 ++- addons/digest/tests/__init__.py | 3 + addons/digest/tests/test_digest.py | 115 +++++++++++++++++++---------- 3 files changed, 84 insertions(+), 43 deletions(-) diff --git a/addons/digest/models/digest.py b/addons/digest/models/digest.py index 628c4dca397..0a9edbc2b6f 100644 --- a/addons/digest/models/digest.py +++ b/addons/digest/models/digest.py @@ -156,14 +156,15 @@ class Digest(models.Model): ) # create a mail_mail based on values, without attachments mail_values = { - 'subject': '%s: %s' % (user.company_id.name, self.name), + 'auto_delete': True, + 'author_id': self.env.user.partner_id.id, 'email_from': self.company_id.partner_id.email_formatted if self.company_id else self.env.user.email_formatted, 'email_to': user.email_formatted, 'body_html': full_mail, - 'auto_delete': True, + 'state': 'outgoing', + 'subject': '%s: %s' % (user.company_id.name, self.name), } - mail = self.env['mail.mail'].sudo().create(mail_values) - mail.send(raise_exception=False) + self.env['mail.mail'].sudo().create(mail_values) return True @api.model diff --git a/addons/digest/tests/__init__.py b/addons/digest/tests/__init__.py index 12994a2edec..04bb83127c5 100644 --- a/addons/digest/tests/__init__.py +++ b/addons/digest/tests/__init__.py @@ -1 +1,4 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + from . import test_digest diff --git a/addons/digest/tests/test_digest.py b/addons/digest/tests/test_digest.py index c493fbcb799..30cf7ea75eb 100644 --- a/addons/digest/tests/test_digest.py +++ b/addons/digest/tests/test_digest.py @@ -1,3 +1,6 @@ +# -*- coding: utf-8 -*- +# Part of Odoo. See LICENSE file for full copyright and licensing details. + import itertools import random @@ -6,41 +9,37 @@ from lxml import html from odoo import fields from odoo.addons.mail.tests import common as mail_test +from odoo.tests.common import users class TestDigest(mail_test.MailCommon): - def test_digest_numbers(self): - self._setup_messages() - digest = self.env['digest.digest'].create({ + @classmethod + def setUpClass(cls): + super(TestDigest, cls).setUpClass() + cls._activate_multi_company() + + # clean messages + cls.env['mail.message'].search([ + ('subtype_id', '=', cls.env.ref('mail.mt_comment').id), + ('message_type', 'in', ['comment', 'email']), + ]).unlink() + cls._setup_messages() + + # clean demo users so that we keep only the test users + cls.env['res.users'].search([('login', 'in', ['demo', 'portal'])]).action_archive() + # clean logs so that town down is activated + cls.env['res.users.log'].search([('create_uid', 'in', (cls.user_admin + cls.user_employee).ids)]).unlink() + + cls.test_digest = cls.env['digest.digest'].create({ + 'kpi_mail_message_total': True, + 'kpi_res_users_connected': True, 'name': "My Digest", - 'kpi_mail_message_total': True + 'periodicity': 'daily', }) - digest_user = digest.with_user(self.user_employee) - # subscribe a user so at least one mail gets sent - digest_user.action_subscribe() - self.assertTrue( - digest_user.is_subscribed, - "check the user was subscribed as action_subscribe will silently " - "ignore subs of non-employees" - ) - - # digest creates its mails in auto_delete mode so we need to capture - # the formatted body during the sending process - with self.mock_mail_gateway(): - digest.action_send() - - self.assertEqual(len(self._mails), 1, "a mail has been created for the digest") - body = self._mails[0]['body'] - - kpi_message_values = html.fromstring(body).xpath('//div[@data-field="kpi_mail_message_total"]//*[hasclass("kpi_value")]/text()') - self.assertEqual( - [t.strip() for t in kpi_message_values], - ['3', '8', '15'] - ) - - def _setup_messages(self): + @classmethod + def _setup_messages(cls): """ Remove all existing messages, then create a bunch of them on random partners with the correct types in correct time-bucket: @@ -50,25 +49,63 @@ class TestDigest(mail_test.MailCommon): based around weeks and months not days), for a total of 15 in the previous month """ - self.env['mail.message'].search([]).unlink() - now = fields.Datetime.now() # regular employee can't necessarily access "private" addresses - partners = self.env['res.partner'].search([('type', '!=', 'private')]) + partners = cls.env['res.partner'].search([('type', '!=', 'private')]) + messages = cls.env['mail.message'] counter = itertools.count() - # pylint: disable=bad-whitespace - for count, (low, high) in [ - (3, (0 * 24, 1 * 24)), - (5, (1 * 24, 7 * 24)), - (7, (7 * 24, 27 * 24)), - ]: + now = fields.Datetime.now() + for count, (low, high) in [(3, (0 * 24, 1 * 24)), + (5, (1 * 24, 7 * 24)), + (7, (7 * 24, 27 * 24)), + ]: for _ in range(count): create_date = now - relativedelta(hours=random.randint(low + 1, high - 1)) - random.choice(partners).message_post( + messages += random.choice(partners).message_post( + author_id=cls.partner_admin.id, body=f"Awesome Partner! ({next(counter)})", + email_from=cls.partner_admin.email_formatted, message_type='comment', subtype_xmlid='mail.mt_comment', # adjust top and bottom by 1h to avoid overlapping with the # range limit and dropping out of the digest's selection thing - create_date=create_date + create_date=create_date, ) + messages.flush() + + @users('admin') + def test_digest_numbers(self): + digest = self.env['digest.digest'].browse(self.test_digest.ids) + digest._action_subscribe_users(self.user_employee) + + # digest creates its mails in auto_delete mode so we need to capture + # the formatted body during the sending process + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + + self.assertEqual(len(self._new_mails), 1, "A new mail.mail should have been created") + mail = self._new_mails[0] + # check mail.mail content + self.assertEqual(mail.author_id, self.partner_admin) + self.assertEqual(mail.email_from, self.company_admin.email_formatted) + self.assertEqual(mail.state, 'outgoing', 'Mail should use the queue') + + kpi_message_values = html.fromstring(mail.body_html).xpath('//div[@data-field="kpi_mail_message_total"]//*[hasclass("kpi_value")]/text()') + self.assertEqual( + [t.strip() for t in kpi_message_values], + ['3', '8', '15'] + ) + + @users('admin') + def test_digest_subscribe(self): + digest_user = self.test_digest.with_user(self.user_employee) + self.assertFalse(digest_user.is_subscribed) + + # subscribe a user so at least one mail gets sent + digest_user.action_subscribe() + self.assertTrue( + digest_user.is_subscribed, + "check the user was subscribed as action_subscribe will silently " + "ignore subs of non-employees" + ) From 4ea673bfd84546b13a831772fd0bd9db66b9eca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 14:38:58 +0000 Subject: [PATCH 09/14] [IMP] digest: add tests about tone down and unsubscribe Purpose is to ensure behavior of digests and prepare future fixes and improvements. Tone down and unsubscribe links are currently not tested. This commit fixes that by adding relevant tests. Task-2641394 (Digest emails sending improvement) Task-2582128 (Digest onbarding and usage improvement) X-original-commit: f340859a3508ebb369b7116f64c81d8f03774e61 Part-of: odoo/odoo#79877 --- addons/digest/tests/test_digest.py | 96 +++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/addons/digest/tests/test_digest.py b/addons/digest/tests/test_digest.py index 30cf7ea75eb..193d0b6c95c 100644 --- a/addons/digest/tests/test_digest.py +++ b/addons/digest/tests/test_digest.py @@ -6,9 +6,12 @@ import random from dateutil.relativedelta import relativedelta from lxml import html +from werkzeug.urls import url_encode -from odoo import fields +from odoo import fields, SUPERUSER_ID +from odoo.addons.base.tests.common import HttpCaseWithUserDemo from odoo.addons.mail.tests import common as mail_test +from odoo.tests import tagged from odoo.tests.common import users @@ -109,3 +112,94 @@ class TestDigest(mail_test.MailCommon): "check the user was subscribed as action_subscribe will silently " "ignore subs of non-employees" ) + + @users('admin') + def test_digest_tone_down(self): + digest = self.env['digest.digest'].browse(self.test_digest.ids) + digest._action_subscribe_users(self.user_employee) + + # initial data + self.assertEqual(digest.periodicity, 'daily') + + # no logs for employee -> should tone down periodicity + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + + self.assertEqual(digest.periodicity, 'weekly') + + @users('admin') + def test_digest_tone_down_wlogs(self): + digest = self.env['digest.digest'].browse(self.test_digest.ids) + digest._action_subscribe_users(self.user_employee) + + # initial data + self.assertEqual(digest.periodicity, 'daily') + + # logs for employee -> should not tone down + self.env['res.users.log'].with_user(SUPERUSER_ID).create({'create_uid': self.user_employee.id}) + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + + self.assertEqual(digest.periodicity, 'daily') + + +@tagged('-at_install', 'post_install') +class TestUnsubscribe(HttpCaseWithUserDemo): + + def setUp(self): + super(TestUnsubscribe, self).setUp() + + self.test_digest = self.env['digest.digest'].create({ + 'kpi_mail_message_total': True, + 'kpi_res_users_connected': True, + 'name': "My Digest", + 'periodicity': 'daily', + 'user_ids': self.user_demo.ids, + }) + self.test_digest._action_subscribe_users(self.user_demo) + self.base_url = self.test_digest.get_base_url() + + @users('demo') + def test_unsubscribe_classic(self): + self.assertIn(self.user_demo, self.test_digest.user_ids) + self.authenticate(self.env.user.login, self.env.user.login) + + response = self._url_unsubscribe() + self.assertEqual(response.status_code, 200) + self.assertNotIn(self.user_demo, self.test_digest.user_ids) + + @users('demo') + def test_unsubscribe_issues(self): + """ Test when not being member """ + self.test_digest.write({'user_ids': [(3, self.user_demo.id)]}) + self.assertNotIn(self.user_demo, self.test_digest.user_ids) + + # unsubscribe + self.authenticate(self.env.user.login, self.env.user.login) + response = self._url_unsubscribe() + self.assertEqual(response.status_code, 200) + self.assertNotIn(self.user_demo, self.test_digest.user_ids) + + def test_unsubscribe_public(self): + """ Check public users are redirected when trying to catch unsubscribe + route. """ + self.authenticate(None, None) + + response = self._url_unsubscribe() + self.assertIn('web/login?redirect', response.url) + + def _url_unsubscribe(self, token=None, user_id=None): + url_params = {} + if token is not None: + url_params['token'] = token + if user_id is not None: + url_params['user_id'] = user_id + + url = "%s/digest/%s/unsubscribe?%s" % ( + self.base_url, + self.test_digest.id, + url_encode(url_params) + ) + return self.url_open(url) From 72637878a1bb3b7edb38d660ead1982821c6eb7e Mon Sep 17 00:00:00 2001 From: Fabio Barbero Date: Mon, 8 Nov 2021 10:00:29 +0000 Subject: [PATCH 10/14] [FIX] digest: display unsubscribe even if no preferences to display A wrongly-placed t-if prevented from displaying unsubscribe link (and the "Sent by Odoo" sentence by the way, even if less annoying). Task-2582128 (Digest onboarding and usage improvement) X-original-commit: 669209b473d1e9581518033b74064fff7f677393 Part-of: odoo/odoo#79877 --- addons/digest/data/digest_data.xml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/addons/digest/data/digest_data.xml b/addons/digest/data/digest_data.xml index 4c9da812c83..927c827c93c 100644 --- a/addons/digest/data/digest_data.xml +++ b/addons/digest/data/digest_data.xml @@ -391,10 +391,11 @@ -
+
-
+
From db5315efad9fa0d4d7a38807a466f8f2bcfe19f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Tue, 9 Nov 2021 16:46:24 +0000 Subject: [PATCH 11/14] [FIX] digest, mass_mailing: fix crash about impossible 'unsubscribe' Mailing statistics emails are build on digest layout. This layout contains a reference to the digest unsubscribe mechanism, which obviously does not work with mailing statistics emails as they currently have no opt-out mechanism. This commit fixes that part of the template so that unsubscribe is displayed only with digests. Task-2582128 (Digest onboarding and usage improvement) Task-2686586 (Repair mailing statistics email) X-original-commit: febe4506cf14ba9308d85538515eb61c4638b18d Part-of: odoo/odoo#79877 --- addons/digest/data/digest_data.xml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/addons/digest/data/digest_data.xml b/addons/digest/data/digest_data.xml index 927c827c93c..5a026dac495 100644 --- a/addons/digest/data/digest_data.xml +++ b/addons/digest/data/digest_data.xml @@ -399,8 +399,14 @@
- Sent by Odoo – - Unsubscribe + Sent by Odoo + + – + + Unsubscribe + +
From d27a6f7276f0cf92a748b2b9471cdf30b51a211f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Mon, 8 Nov 2021 17:16:12 +0000 Subject: [PATCH 12/14] [FIX] digest, mass_mailing: correctly set body before odoo mentions Body is used in statistics as an additional content to digest layout. However it is strangely located after Odoo mentions (Send by, ...). Currently link trackers statistics are therefore at the end of the email. They belong to the "core" section of the emails. Task-2686586 (Repair mailing statistics email) X-original-commit: c9a503eec9a40809b6a88d56f84c06a66ad6dbeb Part-of: odoo/odoo#79877 --- addons/digest/data/digest_data.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/addons/digest/data/digest_data.xml b/addons/digest/data/digest_data.xml index 5a026dac495..6d8161a3c1c 100644 --- a/addons/digest/data/digest_data.xml +++ b/addons/digest/data/digest_data.xml @@ -391,6 +391,8 @@
+ +
@@ -412,7 +414,6 @@
-
Sent by Odoo - + + – + + Unsubscribe + + + – diff --git a/addons/digest/models/digest.py b/addons/digest/models/digest.py index 0a9edbc2b6f..658e00e4eac 100644 --- a/addons/digest/models/digest.py +++ b/addons/digest/models/digest.py @@ -7,7 +7,6 @@ import pytz from datetime import datetime, date from dateutil.relativedelta import relativedelta from markupsafe import Markup -from werkzeug.urls import url_join from odoo import api, fields, models, tools, _ from odoo.addons.base.models.ir_mail_server import MailDeliveryException @@ -137,6 +136,7 @@ class Digest(models.Model): 'top_button_url': self.get_base_url(), 'company': user.company_id, 'user': user, + 'unsubscribe_token': self._get_unsubscribe_token(user.id), 'tips_count': tips_count, 'formatted_date': datetime.today().strftime('%B %d, %Y'), 'display_mobile_banner': True, @@ -176,6 +176,14 @@ class Digest(models.Model): except MailDeliveryException as e: _logger.warning('MailDeliveryException while sending digest %d. Digest is now scheduled for next cron update.', digest.id) + def _get_unsubscribe_token(self, user_id): + """Generate a secure hash for this digest and user. It allows to + unsubscribe from a digest while keeping some security in that process. + + :param int user_id: ID of the user to unsubscribe + """ + return tools.hmac(self.env(su=True), 'digest-unsubscribe', (self.id, user_id)) + # ------------------------------------------------------------ # KPIS # ------------------------------------------------------------ diff --git a/addons/digest/tests/test_digest.py b/addons/digest/tests/test_digest.py index 193d0b6c95c..1c403433da4 100644 --- a/addons/digest/tests/test_digest.py +++ b/addons/digest/tests/test_digest.py @@ -160,6 +160,7 @@ class TestUnsubscribe(HttpCaseWithUserDemo): }) self.test_digest._action_subscribe_users(self.user_demo) self.base_url = self.test_digest.get_base_url() + self.user_demo_unsubscribe_token = self.test_digest._get_unsubscribe_token(self.user_demo.id) @users('demo') def test_unsubscribe_classic(self): @@ -182,13 +183,21 @@ class TestUnsubscribe(HttpCaseWithUserDemo): self.assertEqual(response.status_code, 200) self.assertNotIn(self.user_demo, self.test_digest.user_ids) + def test_unsubscribe_token(self): + self.assertIn(self.user_demo, self.test_digest.user_ids) + self.authenticate(None, None) + response = self._url_unsubscribe(token=self.user_demo_unsubscribe_token, user_id=self.user_demo.id) + self.assertEqual(response.status_code, 200) + self.test_digest.invalidate_cache() + self.assertNotIn(self.user_demo, self.test_digest.user_ids) + def test_unsubscribe_public(self): """ Check public users are redirected when trying to catch unsubscribe route. """ self.authenticate(None, None) response = self._url_unsubscribe() - self.assertIn('web/login?redirect', response.url) + self.assertEqual(response.status_code, 404) def _url_unsubscribe(self, token=None, user_id=None): url_params = {} From 49085a122816fd889c34ba1e96dfad78cdff7059 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= Date: Wed, 10 Nov 2021 15:18:30 +0000 Subject: [PATCH 14/14] [FIX] digest: improve tone down Currently digest sending is toned down when being sent if subscribed users did not connect since more than 3 days. It allows to avoid spamming people not being active or present anymore. This is currently limited to daily digests that are set to weekly. However it may continues to send digests each week until the digest is de-activated. In this commit we tone down the digest from daily to quarterly depending on users activity. It is still based on overall activity and not individual users to avoid complex computation on heavy digests. This will be done in further cleaning of digests. Task-2688856 (Digest tone down improvement) X-original-commit: f916354baec5bb69511069a829ab1648a5ebaccd Part-of: odoo/odoo#79877 --- addons/digest/models/digest.py | 31 +++++++++++++++++++---- addons/digest/tests/test_digest.py | 40 ++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/addons/digest/models/digest.py b/addons/digest/models/digest.py index 658e00e4eac..f03c1d2dac0 100644 --- a/addons/digest/models/digest.py +++ b/addons/digest/models/digest.py @@ -121,7 +121,7 @@ class Digest(models.Model): lang=user.lang )._action_send_to_user(user, tips_count=1) if digest in to_slowdown: - digest.write({'periodicity': 'weekly'}) + digest.write({'periodicity': self._get_next_periodicity()[0]}) digest.next_run_date = digest._get_next_run_date() def _action_send_to_user(self, user, tips_count=1, consum_tips=True): @@ -276,7 +276,11 @@ class Digest(models.Model): """ preferences = [] if self._context.get('digest_slowdown'): - preferences.append(_("We have noticed you did not connect these last few days so we've automatically switched your preference to weekly Digests.")) + _dummy, new_perioridicy_str = self._get_next_periodicity() + preferences.append( + _("We have noticed you did not connect these last few days. We have automatically switched your preference to %(new_perioridicy_str)s Digests.", + new_perioridicy_str=new_perioridicy_str) + ) elif self.periodicity == 'daily' and user.has_group('base.group_erp_manager'): preferences.append(Markup('

%s
%s

') % ( _('Prefer a broader overview ?'), @@ -338,17 +342,34 @@ class Digest(models.Model): return margin def _check_daily_logs(self): - three_days_ago = datetime.now().replace(hour=0, minute=0, second=0, microsecond=0) - relativedelta(days=3) + """ Badly named method that checks user logs and slowdown the sending + of digest emails based on recipients being away. """ + today = datetime.now().replace(hour=0, minute=0, second=0, microsecond=0) to_slowdown = self.env['digest.digest'] - for digest in self.filtered(lambda digest: digest.periodicity == 'daily'): + for digest in self: + if digest.periodicity == 'daily': # 3 days ago + limit_dt = today - relativedelta(days=3) + elif digest.periodicity == 'weekly': # 2 weeks ago + limit_dt = today - relativedelta(days=14) + elif digest.periodicity == 'monthly': # 1 month ago + limit_dt = today - relativedelta(months=1) + elif digest.periodicity == 'quarterly': # 3 month ago + limit_dt = today - relativedelta(months=3) users_logs = self.env['res.users.log'].sudo().search_count([ ('create_uid', 'in', digest.user_ids.ids), - ('create_date', '>=', three_days_ago) + ('create_date', '>=', limit_dt) ]) if not users_logs: to_slowdown += digest return to_slowdown + def _get_next_periodicity(self): + if self.periodicity == 'weekly': + return 'monthly', _('monthly') + if self.periodicity == 'monthly': + return 'quarterly', _('quarterly') + return 'weekly', _('weekly') + def _format_currency_amount(self, amount, currency_id): pre = currency_id.position == 'before' symbol = u'{symbol}'.format(symbol=currency_id.symbol or '') diff --git a/addons/digest/tests/test_digest.py b/addons/digest/tests/test_digest.py index 1c403433da4..196550b1bd1 100644 --- a/addons/digest/tests/test_digest.py +++ b/addons/digest/tests/test_digest.py @@ -128,6 +128,20 @@ class TestDigest(mail_test.MailCommon): self.assertEqual(digest.periodicity, 'weekly') + # no logs for employee -> should tone down periodicity + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + + self.assertEqual(digest.periodicity, 'monthly') + + # no logs for employee -> should tone down periodicity + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + + self.assertEqual(digest.periodicity, 'quarterly') + @users('admin') def test_digest_tone_down_wlogs(self): digest = self.env['digest.digest'].browse(self.test_digest.ids) @@ -137,12 +151,34 @@ class TestDigest(mail_test.MailCommon): self.assertEqual(digest.periodicity, 'daily') # logs for employee -> should not tone down - self.env['res.users.log'].with_user(SUPERUSER_ID).create({'create_uid': self.user_employee.id}) + logs = self.env['res.users.log'].with_user(SUPERUSER_ID).create({'create_uid': self.user_employee.id}) digest.flush() with self.mock_mail_gateway(): digest.action_send() - self.assertEqual(digest.periodicity, 'daily') + logs.unlink() + logs = self.env['res.users.log'].with_user(SUPERUSER_ID).create({ + 'create_uid': self.user_employee.id, + 'create_date': fields.Datetime.now() - relativedelta(days=20), + }) + + # logs for employee are more than 3 days old -> should tone down + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + self.assertEqual(digest.periodicity, 'weekly') + + # logs for employee are more than 2 weeks old -> should tone down + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + self.assertEqual(digest.periodicity, 'monthly') + + # logs for employee are less than 1 month old -> should not tone down + digest.flush() + with self.mock_mail_gateway(): + digest.action_send() + self.assertEqual(digest.periodicity, 'monthly') @tagged('-at_install', 'post_install')