[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) <tde@openerp.com>

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,
                 **{

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]))

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
 

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

addons/purchase/models/mail_compose_message.py

@@ -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)

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

addons/sale/wizard/mail_compose_message.py

@@ -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)

addons/test_mail/data/mail_template_data.xml

@@ -44,6 +44,19 @@
         </t>
     </template>
 
+    <template id="mail_test_ticket_test_variable_template">
+        <t t-call="web.html_container">
+            <t t-foreach="docs" t-as="ticket">
+                <t t-call="web.external_layout">
+                    <div class="page">
+                        <p>This is a sample of an external report for a ticket for
+                            <span t-out="ticket.count"></span> people.</p>
+                    </div>
+                </t>
+            </t>
+        </t>
+    </template>
+
     <template id="mail_template_simple_test">
         <p>Hello <t t-out="partner.name"/>, this comes from <t t-out="object.name"/>.</p>
     </template>

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': '<p>Common Body</p>',
+            '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': '<p>Common Body</p>',
+            'report_template_ids': [Command.clear()],
+            'subject': 'Common Subject',
+        }
+        same_attachments = {'attachment_ids': template_attachment.ids}
+        diff_body = {'body_html': '<p><t t-esc="object.name"></t></p>'}
+        # 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'<p>{record.name}</p>'
+                    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')

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'})