From 79e9665a495ce92fa177d72e636947194703eccf Mon Sep 17 00:00:00 2001 From: Renaud Thiry Date: Thu, 14 Sep 2023 15:49:37 +0200 Subject: [PATCH] [IMP] mail: improve Composer duplicates check Only duplicate emails used to be checked when sending a mass mail. However it is possible (e.g. using templates) to send a mass mail to the same person containing different information. The existing functions to allow models to specify emails processed in the past by some other means are kept. A new check is added in the processing that checks the full contents of the message, subject and attachment ids. The strings are not hashed as most situations are: - Sending the exact same mail to everyone -> Only need to check against one message -> Same complexity as hashing - Sending all different emails -> Checking inequality of str is usually very fast For attachments, as we cannot compare them easily. They are ignored for the purpose of equating emails whenever there are the same number of attachments in the email values as there are on the composer. This is because each email should receive its own copy of the composer attachments. If they have a different number of attachments, they were generated dynamically through reports and we assume they are all different. We can thus remove the 'document based' information as it is implicitly infered from this check. ------------------------- Test utils are also updated for two purposes: 1. Add optional body and attachment_name discriminents assertMailMail assumed all emails could at least be differentiated by subject. Our test breaks that assumption, so we use body and attachment to find the best-fitting email based on the data passed in. 2. Check email_formatted on recipients When using assertMailMailWEmails, we first find the email using the non-formatted email of the recipient. This does not match assertSentMail which checks against the raw email_to value, which would often be formatted. Task-2826811 closes odoo/odoo#99541 Signed-off-by: Thibault Delavallee (tde) --- addons/account/wizard/account_move_send.py | 3 +- addons/mail/tests/common.py | 30 ++- addons/mail/wizard/mail_compose_message.py | 21 +- addons/purchase/models/__init__.py | 1 - .../purchase/models/mail_compose_message.py | 13 - addons/sale/wizard/__init__.py | 1 - addons/sale/wizard/mail_compose_message.py | 13 - addons/test_mail/data/mail_template_data.xml | 13 + addons/test_mail/tests/test_mail_composer.py | 242 +++++++++++++----- .../test_mass_mailing/tests/test_mailing.py | 5 +- 10 files changed, 230 insertions(+), 112 deletions(-) delete mode 100644 addons/purchase/models/mail_compose_message.py delete mode 100644 addons/sale/wizard/mail_compose_message.py diff --git a/addons/account/wizard/account_move_send.py b/addons/account/wizard/account_move_send.py index 203b34f3727..4649c691b7d 100644 --- a/addons/account/wizard/account_move_send.py +++ b/addons/account/wizard/account_move_send.py @@ -430,8 +430,7 @@ class AccountMoveSend(models.Model): .with_context( no_new_invoice=True, mail_notify_author=self.env.user.partner_id.id in partner_ids, - mailing_document_based=True)\ - .message_post( + ).message_post( message_type='comment', **kwargs, **{ diff --git a/addons/mail/tests/common.py b/addons/mail/tests/common.py index 2692497a85e..cb1b7e148a9 100644 --- a/addons/mail/tests/common.py +++ b/addons/mail/tests/common.py @@ -339,9 +339,9 @@ class MockEmail(common.BaseCase, MockSmtplibCase): ) return mail - def _find_sent_email(self, email_from, emails_to, subject=None): - """ Find an outgoing email based on from / to and optional subject when - having a conflict. + def _find_sent_email(self, email_from, emails_to, subject=None, body=None, attachment_names=None): + """ Find an outgoing email based on from / to and optional subject, body + and attachment names when having conflicts. :return sent_email: an outgoing email generated during the mock; """ @@ -349,9 +349,14 @@ class MockEmail(common.BaseCase, MockSmtplibCase): mail for mail in self._mails if set(mail['email_to']) == set(emails_to) and mail['email_from'] == email_from ] - if len(sent_emails) > 1 and subject: + if len(sent_emails) > 1: # try to better filter - sent_email = next((mail for mail in sent_emails if mail['subject'] == subject), False) + sent_email = next((mail for mail in sent_emails + if (subject is None or mail['subject'] == subject) + and (body is None or mail['body'] == body) + and (attachment_names is None + or set(attachment_names) == set(attachment[0] for attachment in mail['attachments'])) + ), False) else: sent_email = sent_emails[0] if sent_emails else False return sent_email @@ -563,14 +568,14 @@ class MockEmail(common.BaseCase, MockSmtplibCase): """ direct_check = ['body_alternative', 'email_from', 'references', 'reply_to', 'subject'] content_check = ['body_alternative_content', 'body_content', 'references_content'] - list_check = ['email_bcc', 'email_cc', 'email_to'] + email_list_check = ['email_bcc', 'email_cc', 'email_to'] other_check = ['attachments', 'attachments_info', 'body'] expected = {} - for fname in direct_check + content_check + list_check + other_check: + for fname in direct_check + content_check + email_list_check + other_check: if fname in values: expected[fname] = values[fname] - unknown = set(values.keys()) - set(direct_check + content_check + list_check + other_check) + unknown = set(values.keys()) - set(direct_check + content_check + email_list_check + other_check) if unknown: raise NotImplementedError('Unsupported %s' % ', '.join(unknown)) @@ -591,10 +596,15 @@ class MockEmail(common.BaseCase, MockSmtplibCase): expected['email_to'] = email_to_list # fetch mail + attachments = [attachment['name'] + for attachment in values.get('attachments_info', []) + if 'name' in attachment] sent_mail = self._find_sent_email( expected['email_from'], expected['email_to'], - values.get('subject'), + subject=values.get('subject'), + body=values.get('body'), + attachment_names=attachments or None ) debug_info = '' if not sent_mail: @@ -629,7 +639,7 @@ class MockEmail(common.BaseCase, MockSmtplibCase): if 'body' in expected: self.assertHtmlEqual(expected['body'], sent_mail['body'], 'Value for %s: expected %s, received %s' % ('body', expected['body'], sent_mail['body'])) # beware to avoid list ordering differences (but Falsy values -> compare directly) - for val in list_check: + for val in email_list_check: if expected.get(val): self.assertEqual(sorted(expected[val]), sorted(sent_mail[val]), 'Value for %s: expected %s, received %s' % (val, expected[val], sent_mail[val])) diff --git a/addons/mail/wizard/mail_compose_message.py b/addons/mail/wizard/mail_compose_message.py index 6f848b724a8..4657479aa09 100644 --- a/addons/mail/wizard/mail_compose_message.py +++ b/addons/mail/wizard/mail_compose_message.py @@ -1084,8 +1084,7 @@ class MailComposer(models.TransientModel): blacklist_ids = self._get_blacklist_record_ids(mail_values_dict, recipients_info) optout_emails = self._get_optout_emails(mail_values_dict) done_emails = self._get_done_emails(mail_values_dict) - # in case of an invoice e.g. - mailing_document_based = self.env.context.get('mailing_document_based') + sent_emails_mapping = {} # distinct emails sent to each address as {email_address: [mail_values]} for record_id, mail_values in mail_values_dict.items(): recipients = recipients_info[record_id] @@ -1108,7 +1107,7 @@ class MailComposer(models.TransientModel): elif optout_emails and mail_to in optout_emails: mail_values['state'] = 'cancel' mail_values['failure_type'] = 'mail_optout' - elif done_emails and mail_to in done_emails and not mailing_document_based: + elif mail_to in done_emails: mail_values['state'] = 'cancel' mail_values['failure_type'] = 'mail_dup' # void of falsy values -> error @@ -1118,8 +1117,20 @@ class MailComposer(models.TransientModel): elif not mail_to_normalized: mail_values['state'] = 'cancel' mail_values['failure_type'] = 'mail_email_invalid' - elif done_emails is not None and not mailing_document_based: - done_emails.append(mail_to) + elif mail_to in sent_emails_mapping: + # If the number of attachments on the mail exactly matches the number of attachments on the composer + # we assume the attachments are copies of the ones attached to the composer and thus are the same + if len(self.attachment_ids) == len(mail_values.get('attachment_ids', [])) and\ + any(sent_mail.get('subject') == mail_values.get('subject') and + sent_mail.get('body') == mail_values.get('body') for sent_mail in sent_emails_mapping[mail_to]): + mail_values['state'] = 'cancel' + mail_values['failure_type'] = 'mail_dup' + else: + sent_emails_mapping[mail_to].append(mail_values) + else: + sent_emails_mapping[mail_to] = [mail_values] + + done_emails += sent_emails_mapping.keys() return mail_values_dict diff --git a/addons/purchase/models/__init__.py b/addons/purchase/models/__init__.py index e9d1576b95e..f0dd03c884b 100644 --- a/addons/purchase/models/__init__.py +++ b/addons/purchase/models/__init__.py @@ -9,4 +9,3 @@ from . import product from . import res_company from . import res_config_settings from . import res_partner -from . import mail_compose_message diff --git a/addons/purchase/models/mail_compose_message.py b/addons/purchase/models/mail_compose_message.py deleted file mode 100644 index d5d529ed6f2..00000000000 --- a/addons/purchase/models/mail_compose_message.py +++ /dev/null @@ -1,13 +0,0 @@ -# -*- coding: utf-8 -*- -# purches Part of Odoo. See LICENSE file for full copyright and licensing details. - -from odoo import models - - -class MailComposeMessage(models.TransientModel): - _inherit = 'mail.compose.message' - - def _action_send_mail(self, auto_commit=False): - if self.model == 'purchase.order': - self = self.with_context(mailing_document_based=True) - return super(MailComposeMessage, self)._action_send_mail(auto_commit=auto_commit) diff --git a/addons/sale/wizard/__init__.py b/addons/sale/wizard/__init__.py index 9aa43983a08..246f28cce38 100644 --- a/addons/sale/wizard/__init__.py +++ b/addons/sale/wizard/__init__.py @@ -2,7 +2,6 @@ # Part of Odoo. See LICENSE file for full copyright and licensing details. from . import account_payment_register -from . import mail_compose_message from . import payment_link_wizard from . import payment_provider_onboarding_wizard from . import res_config_settings diff --git a/addons/sale/wizard/mail_compose_message.py b/addons/sale/wizard/mail_compose_message.py deleted file mode 100644 index 6e989df230c..00000000000 --- a/addons/sale/wizard/mail_compose_message.py +++ /dev/null @@ -1,13 +0,0 @@ -# -*- coding: utf-8 -*- -# Part of Odoo. See LICENSE file for full copyright and licensing details. - -from odoo import models - - -class MailComposeMessage(models.TransientModel): - _inherit = 'mail.compose.message' - - def _action_send_mail(self, auto_commit=False): - if self.model == 'sale.order': - self = self.with_context(mailing_document_based=True) - return super(MailComposeMessage, self)._action_send_mail(auto_commit=auto_commit) diff --git a/addons/test_mail/data/mail_template_data.xml b/addons/test_mail/data/mail_template_data.xml index f5970d9e211..dcb4712c463 100644 --- a/addons/test_mail/data/mail_template_data.xml +++ b/addons/test_mail/data/mail_template_data.xml @@ -44,6 +44,19 @@ + + diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py index 98f92af1a15..516e6a44d19 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -6,9 +6,10 @@ import base64 from ast import literal_eval from datetime import timedelta from freezegun import freeze_time -from itertools import product +from itertools import chain, product from unittest.mock import DEFAULT, patch +from odoo import Command from odoo.addons.base.tests.test_ir_cron import CronMixinCase from odoo.addons.mail.tests.common import mail_new_test_user, MailCommon from odoo.addons.mail.wizard.mail_compose_message import MailComposer @@ -63,7 +64,7 @@ class TestMailComposer(MailCommon, TestRecipients): additional_values={'user_id': cls.user_employee_2.id}, ) - cls.test_report, cls.test_report_2 = cls.env['ir.actions.report'].create([ + cls.test_report, cls.test_report_2, cls.test_report_3 = cls.env['ir.actions.report'].create([ { 'name': 'Test Report on Mail Test Ticket', 'model': 'mail.test.ticket', @@ -76,6 +77,12 @@ class TestMailComposer(MailCommon, TestRecipients): 'print_report_name': "'TestReport2 for %s' % object.name", 'report_type': 'qweb-pdf', 'report_name': 'test_mail.mail_test_ticket_test_template_2', + }, { + 'name': 'Test Report 3 with variable data on Mail Test Ticket', + 'model': 'mail.test.ticket', + 'print_report_name': "'TestReport3 for %s' % object.name", + 'report_type': 'qweb-pdf', + 'report_name': 'test_mail.mail_test_ticket_test_variable_template', } ]) @@ -1992,6 +1999,174 @@ class TestComposerResultsMass(TestMailComposer): self.assertEqual(len(self._new_msgs), 2, 'Should have created 1 mail.mail per record') self.assertFalse(self._new_msgs.exists(), 'Should have deleted mail.message records') + @users('employee') + @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') + def test_mail_composer_duplicates(self): + """ Ensures emails sent to the same recipient multiple times + are only sent when they are not duplicates + """ + self.template.write({ + 'auto_delete': False, + 'body_html': '

Common Body

', + 'email_to': '', + 'partner_to': '{{ object.customer_id.id if object.customer_id else "" }}', + 'subject': 'Common Subject', + }) + + # Guarantee points of variation for test_report_3 + self.test_records[0].write({'count': 1, 'name': 'A'}) + self.test_records[1].write({'count': 2, 'name': 'B'}) + + template_attachment, composer_attachment = self.env['ir.attachment'].create([{ + 'datas': base64.b64encode(b'ExtraData'), + 'mimetype': 'text/plain', + 'name': f'{record._name}_Common_Attachment.txt', + 'res_id': record.id if record else False, + 'res_model': record._name, + } for record in (self.template, self.env['mail.compose.message'])]) + + customer_ids = self.partners[:2] + customer_ids.write({ + 'email': 'test@test.lan' + }) + + def _instanciate_composer(composer_attachments=False): + composer_form = Form(self.env['mail.compose.message'].with_context( + self._get_web_context(self.test_records[:2], add_web=True, + default_template_id=self.template.id) + )) + composer = composer_form.save() + if composer_attachments: + composer_attachments.res_id = composer.id + composer.attachment_ids += composer_attachments + return composer + + base_customer_values = [{'email': 'test@test.lan'}] + # different recipients should always receive emails + customer_diff_emails = [{'email': f'difftest{n}@test.lan'} for n in range(2)] + base_template_values = { + 'attachment_ids': [Command.clear()], + 'body_html': '

Common Body

', + 'report_template_ids': [Command.clear()], + 'subject': 'Common Subject', + } + same_attachments = {'attachment_ids': template_attachment.ids} + diff_body = {'body_html': '

'} + # regardless of whether they have different bodies or not, they are considered duplicates + diff_attachment_same_content = {'report_template_ids': [self.test_report_2.id]} + diff_attachment_diff_content = {'report_template_ids': [self.test_report_3.id]} + diff_subject = {'subject': '{{ object.name }}'} + + # all template variations using same recipients + diff_combinations = product( + [[diff_body], [diff_subject], + [diff_attachment_same_content], [diff_attachment_diff_content], + [diff_attachment_same_content, same_attachments], + [diff_body, diff_attachment_diff_content, diff_subject]], + [base_customer_values]) + # no template variations using different recipients + diff_combinations = chain(diff_combinations, [[[], customer_diff_emails]]) + # expect all sent + for template_changes, customer_changes in diff_combinations: + test_template_values = dict(base_template_values) + for change in template_changes: + test_template_values.update(change) + self.template.write(test_template_values) + for customer, base_vals, update_vals in zip(customer_ids, base_customer_values, customer_changes): + customer.write({**base_vals, **update_vals}) + with self.subTest(template_values=test_template_values, customer_changes=customer_changes): + composer = _instanciate_composer() + with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): + composer._action_send_mail() + self.assertEqual(len(self._new_mails), 2) + # check emails + for record in self.test_records[:2]: + # email_to will be normalized and formatted, even if already formatted + expected_subject = base_template_values['subject'] + if test_template_values['subject'] != base_template_values['subject']: + expected_subject = record.name + expected_body = base_template_values['body_html'] + if test_template_values['body_html'] != base_template_values['body_html']: + expected_body = f'

{record.name}

' + expected_attachment_info = [] + if self.template.attachment_ids: + expected_attachment_info.append({'name': 'mail.template_Common_Attachment.txt', 'type': 'text/plain'}) + if self.template.report_template_ids: + test_report_name = 'TestReport2' if self.template.report_template_ids == self.test_report_2 else 'TestReport3' + expected_attachment_info.append( + {'name': f'{test_report_name} for {record.name}.html', 'type': 'text/plain'}, + ) + self.assertMailMailWEmails( + [record.customer_id.email], + 'sent', + author=self.env.user.partner_id, + content=expected_body, + mail_message=record.message_ids[0], + email_to_recipients=[[record.customer_id.email_formatted]], + email_values={ + 'attachments_info': expected_attachment_info, + 'body': expected_body, + 'email_from': record.user_id.email_formatted, + 'subject': expected_subject, + }, + ) + + # expect duplicates + for composer_attachment, template_changes in zip([False, composer_attachment, [], composer_attachment], [[], [], [same_attachments], [same_attachments]]): + test_template_values = dict(base_template_values) + for change in template_changes: + test_template_values.update(change) + self.template.write(test_template_values) + # reset customers to have the same email + for customer, base_vals in zip(customer_ids, base_customer_values): + customer.write(base_vals) + with self.subTest(composer_attachments=composer_attachment, template_values=test_template_values): + composer = _instanciate_composer(composer_attachments=composer_attachment) + with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): + composer._action_send_mail() + self.assertEqual(len(self._new_mails), 2) + self.assertEqual(len(self._mails), 1) + # check email + expected_attachment_info = [] + if template_changes: + expected_attachment_info.append({ + 'name': f'{self.template._name}_Common_Attachment.txt', + 'raw': b'ExtraData', + 'type': 'text/plain', + }) + if composer_attachment: + expected_attachment_info.append({ + 'name': f'{composer._name}_Common_Attachment.txt', + 'raw': b'ExtraData', + 'type': 'text/plain', + }) + self.assertMailMailWRecord( + self.test_records[0], + [self.test_records[0].customer_id], + 'sent', + author=self.env.user.partner_id, + content='Common Body', + email_values={ + 'attachments_info': expected_attachment_info, + 'body_content': 'Common Body', + 'email_from': record.user_id.email_formatted, + 'subject': 'Common Subject', + }, + ) + self. assertMailMailWRecord( + self.test_records[1], + [self.test_records[1].customer_id], + 'cancel', + author=self.env.user.partner_id, + content='Common Body', + email_values={ + 'attachments_info': expected_attachment_info, + 'body_content': 'Common Body', + 'email_from': record.user_id.email_formatted, + 'subject': 'Common Subject', + }, + ) + @users('employee') @mute_logger('odoo.models.unlink', 'odoo.addons.mail.models.mail_mail') def test_mail_composer_wtpl(self): @@ -2727,66 +2902,3 @@ class TestComposerResultsMassStatus(TestMailComposer): } ) self.assertEqual(len(self._mails), 2, 'Should have sent 2 emails, even to excluded email.') - - @users('employee') - @mute_logger('odoo.tests', 'odoo.addons.mail.models.mail_mail', 'odoo.models.unlink') - def test_mailing_duplicates_document_based(self): - """ Tests a document-based mass mailing with the same address mails - This should be allowed and not considered as duplicate in this context - """ - test_records = self.test_records.with_env(self.env) - composer_form = Form(self.env['mail.compose.message'].with_context( - self._get_web_context(test_records, add_web=True, - default_template_id=self.template.id) - )) - composer = composer_form.save() - - # by default duplicates are canceled - with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): - composer._action_send_mail() - - for record, expected_state, expected_ft in zip( - test_records, - ['cancel', 'sent', 'sent', 'sent', 'cancel', 'sent', 'cancel'], - ['mail_bl', False, False, False, 'mail_dup', False, 'mail_dup'] - ): - with self.subTest(record=record, expected_state=expected_state, expected_ft=expected_ft): - self.assertMailMailWRecord( - record, record.customer_id, expected_state, - # author is current user, email_from is coming from template (user_id of record) - author=self.user_employee.partner_id, - fields_values={ - 'email_from': self.user_employee_2.email_formatted, - 'failure_reason': False, - 'failure_type': expected_ft, - }, - email_values={ - 'email_from': self.user_employee_2.email_formatted, - } - ) - self.assertEqual(len(self._mails), 4, 'Should have sent 4 emails, and skipped an excluded and 2 duplicate emails.') - - # magic context key allowing to send duplicates when necessary - with self.mock_mail_gateway(mail_unlink_sent=False), self.mock_mail_app(): - composer.with_context(mailing_document_based=True)._action_send_mail() - - for record, expected_state, expected_ft in zip( - test_records, - ['cancel', 'sent', 'sent', 'sent', 'sent', 'sent', 'sent'], - ['mail_bl', False, False, False, False, False, False] - ): - with self.subTest(record=record, expected_state=expected_state, expected_ft=expected_ft): - self.assertMailMailWRecord( - record, record.customer_id, expected_state, - # author is current user, email_from is coming from template (user_id of record) - author=self.user_employee.partner_id, - fields_values={ - 'email_from': self.user_employee_2.email_formatted, - 'failure_reason': False, - 'failure_type': expected_ft, - }, - email_values={ - 'email_from': self.user_employee_2.email_formatted, - } - ) - self.assertEqual(len(self._mails), 6, 'Should have sent 6 emails, and skipped an excluded email') diff --git a/addons/test_mass_mailing/tests/test_mailing.py b/addons/test_mass_mailing/tests/test_mailing.py index d4206fc1347..dc969fbf92a 100644 --- a/addons/test_mass_mailing/tests/test_mailing.py +++ b/addons/test_mass_mailing/tests/test_mailing.py @@ -538,8 +538,9 @@ class TestMassMailing(TestMassMailCommon): @mute_logger('odoo.addons.mail.models.mail_mail') def test_mailing_mailing_list_optout(self): """ Test mailing list model specific optout behavior """ - mailing_contact_1 = self.env['mailing.contact'].create({'name': 'test 1A', 'email': 'test@test.example.com'}) - mailing_contact_2 = self.env['mailing.contact'].create({'name': 'test 1B', 'email': 'test@test.example.com'}) + # as duplication checks body and subject, we create 2 exact copies to make sure only 1 is sent + mailing_contact_1 = self.env['mailing.contact'].create({'name': 'test 1', 'email': 'test@test.example.com'}) + mailing_contact_2 = self.env['mailing.contact'].create({'name': 'test 1', 'email': 'test@test.example.com'}) mailing_contact_3 = self.env['mailing.contact'].create({'name': 'test 3', 'email': 'test3@test.example.com'}) mailing_contact_4 = self.env['mailing.contact'].create({'name': 'test 4', 'email': 'test4@test.example.com'}) mailing_contact_5 = self.env['mailing.contact'].create({'name': 'test 5', 'email': 'test5@test.example.com'})