From e1e383cabef6a2c1a2e88c5b9654fdfa71854760 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= <tde@odoo.com>
Date: Tue, 19 Apr 2022 14:46:19 +0200
Subject: [PATCH] [IMP] tools, base, mail: use first found email in
'email_normalized'
PURPOSE
Be defensive when dealing with email fields, notably when having multi-emails
or email field containing an already-formatted email.
SPECIFICATIONS
When having multi-emails input in an email field, 'email_normalized' field is
currently 'False', as they expect the field to contain a single email. This
has several drawbacks
* searching partners or fetching information based on emails does not work as
most tool methods use 'email_normalized' which is False (see e.g.
'_message_partner_info_from_emails', '_mail_find_partner_from_emails'
or 'find_or_create');
* blacklist is not available as it is based on 'email_normalized';
* mass_mailing wrongly considers those emails as invalid and cancel their
mail and related trace, as it tries to skip sending emails to invalid
emails;
Be more defensive and use first found email in case of multi-emails field.
Other emails are ignored. It is already an improvement that does not break
flows in stable and allow more emails to be sent.
before
-> email: '"Raoul" <raoul1@raoul.fr>, raoul2@raoul.fr'
-> email_normalized: False
after
-> email: '"Raoul" <raoul1@raoul.fr>, raoul2@raoul.fr'
-> email_normalized: raoul1@raoul.fr
A side effect is that it helps finding back some partners, as indicated in
tests where less phantom partners are created.
Task-2612945 (Mail: Defensive email formatting)
X-original-commit: odoo/odoo@bafc060f639539475abf06221ab590637e6fb8dc
Part-of: odoo/odoo#134188
---
addons/mail/models/mail_thread_blacklist.py | 2 +-
addons/mail/tests/test_mail_tools.py | 2 +-
addons/mail/tests/test_res_partner.py | 21 +++++++---
addons/test_mail/tests/test_mail_composer.py | 42 +++++++------------
addons/test_mail/tests/test_mail_gateway.py | 10 ++---
.../tests/test_mail_thread_mixins.py | 4 +-
.../test_mass_mailing/tests/test_mailing.py | 8 +++-
odoo/addons/base/tests/test_mail.py | 2 +-
odoo/tools/mail.py | 7 +++-
9 files changed, 52 insertions(+), 46 deletions(-)
diff --git a/addons/mail/models/mail_thread_blacklist.py b/addons/mail/models/mail_thread_blacklist.py
index c7c32fc1b4f4..a1eed2714bf9 100644
--- a/addons/mail/models/mail_thread_blacklist.py
+++ b/addons/mail/models/mail_thread_blacklist.py
@@ -47,7 +47,7 @@ class MailBlackListMixin(models.AbstractModel):
def _compute_email_normalized(self):
self._assert_primary_email()
for record in self:
- record.email_normalized = tools.email_normalize(record[self._primary_email])
+ record.email_normalized = tools.email_normalize(record[self._primary_email], strict=False)
@api.model
def _search_is_blacklisted(self, operator, value):
diff --git a/addons/mail/tests/test_mail_tools.py b/addons/mail/tests/test_mail_tools.py
index 7682331e70d6..cc0313f9a9f9 100644
--- a/addons/mail/tests/test_mail_tools.py
+++ b/addons/mail/tests/test_mail_tools.py
@@ -158,7 +158,7 @@ class TestMailTools(MailCommon):
(f'{self._test_email}, {_test_email_2}', True), # multi-email, both matching, depends on comparison
(f'{self._test_email}, {_test_email_2}', False) # multi-email, both matching, depends on comparison
]
- expected = [self.env['res.partner'], self.env['res.partner'],
+ expected = [follower_partner, test_partner,
self.env['res.partner'], self.env['res.partner'],
self.env['res.partner'], self.env['res.partner'],
self.env['res.partner'], self.env['res.partner']]
diff --git a/addons/mail/tests/test_res_partner.py b/addons/mail/tests/test_res_partner.py
index 13fbb8313745..9cb035c47b64 100644
--- a/addons/mail/tests/test_res_partner.py
+++ b/addons/mail/tests/test_res_partner.py
@@ -126,9 +126,11 @@ class TestPartner(MailCommon):
'"FindMe Format" <find.me.format@test.example.com>',
'"FindMe Multi" <find.me.multi.1@test.example.com,find.me.multi.2@test.example.com>']
)
+ # when having multi emails, first found one is taken as normalized email
self.assertEqual(
partners.mapped('email_normalized'),
- ['classic.format@test.example.com', 'find.me.format@test.example.com', False]
+ ['classic.format@test.example.com', 'find.me.format@test.example.com',
+ 'find.me.multi.1@test.example.com']
)
# classic find or create: use normalized email to compare records
@@ -140,11 +142,18 @@ class TestPartner(MailCommon):
with self.subTest(email=email):
self.assertEqual(self.env['res.partner'].find_or_create(email), partners[1])
# multi-emails -> no normalized email -> fails each time, create new partner (FIXME)
- for email in ('find.me.multi.1@test.example.com', 'find.me.multi.2@test.example.com'):
- with self.subTest(email=email):
- partner = self.env['res.partner'].find_or_create(email)
- self.assertNotIn(partner, partners)
- self.assertEqual(partner.email, email)
+ for email_input, match_partner in [
+ ('find.me.multi.1@test.example.com', partners[2]),
+ ('find.me.multi.2@test.example.com', self.env['res.partner']),
+ ]:
+ with self.subTest(email_input=email_input):
+ partner = self.env['res.partner'].find_or_create(email_input)
+ # either matching existing, either new partner
+ if match_partner:
+ self.assertEqual(partner, match_partner)
+ else:
+ self.assertNotIn(partner, partners)
+ self.assertEqual(partner.email, email_input)
partner.unlink() # do not mess with subsequent tests
# now input is multi email -> '_parse_partner_name' used in 'find_or_create'
diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py
index 7c7e14f3a546..c346726d0980 100644
--- a/addons/test_mail/tests/test_mail_composer.py
+++ b/addons/test_mail/tests/test_mail_composer.py
@@ -1385,15 +1385,14 @@ class TestComposerResultsComment(TestMailComposer):
# FIXME: currently email finding based on formatted / multi emails does
# not work
new_partners = self.env['res.partner'].search([]).search([('id', 'not in', existing_partners.ids)])
- self.assertEqual(len(new_partners), 9,
- 'Mail (FIXME): multiple partner creation due to formatted / multi emails: 2 extra partners')
+ self.assertEqual(len(new_partners), 8,
+ 'Mail (FIXME): multiple partner creation due to formatted / multi emails: 1 extra partners')
self.assertIn(partner_format_tofind, new_partners)
self.assertIn(partner_multi_tofind, new_partners)
self.assertEqual(
sorted(new_partners.mapped('email')),
sorted(['"FindMe Format" <find.me.format@test.example.com>',
'find.me.multi.1@test.example.com, "FindMe Multi" <find.me.multi.2@test.example.com>',
- 'find.me.multi.1@test.example.com',
'find.me.multi.2@test.example.com',
'test.cc.1@example.com', 'test.cc.2@example.com', 'test.cc.2.2@example.com',
'test.to.1@example.com', 'test.to.2@example.com']),
@@ -1403,7 +1402,6 @@ class TestComposerResultsComment(TestMailComposer):
sorted(new_partners.mapped('email_formatted')),
sorted(['"FindMe Format" <find.me.format@test.example.com>',
'"FindMe Multi" <find.me.multi.1@test.example.com,find.me.multi.2@test.example.com>',
- '"find.me.multi.1@test.example.com" <find.me.multi.1@test.example.com>',
'"find.me.multi.2@test.example.com" <find.me.multi.2@test.example.com>',
'"test.cc.1@example.com" <test.cc.1@example.com>',
'"test.cc.2@example.com" <test.cc.2@example.com>',
@@ -1415,7 +1413,6 @@ class TestComposerResultsComment(TestMailComposer):
sorted(new_partners.mapped('name')),
sorted(['FindMe Format',
'FindMe Multi',
- 'find.me.multi.1@test.example.com',
'find.me.multi.2@test.example.com',
'test.cc.1@example.com', 'test.to.1@example.com', 'test.to.2@example.com',
'test.cc.2@example.com', 'test.cc.2.2@example.com']),
@@ -1429,14 +1426,10 @@ class TestComposerResultsComment(TestMailComposer):
# FIXME: more partners created than real emails (see above) -> due to
# transformation from email -> partner in template 'generate_recipients'
# there are more partners than email to notify;
- # NOTE: 'Findme Multi' is excluded as it has the same email as 'find.me.multi.1@test.example.com'
- # (created by template) and comes second in a search based on email
- mailed_new_partners = new_partners.filtered(lambda p: p.name != 'FindMe Multi')
- self.assertEqual(len(mailed_new_partners), 8)
self.assertEqual(len(self._new_mails), 2, 'Should have created 2 mail.mail')
self.assertEqual(
- len(self._mails), len(mailed_new_partners) + 3,
- f'Should have sent {len(mailed_new_partners) + 3} emails, one / recipient ({len(mailed_new_partners)} mailed partners + partner_1 + partner_2 + partner_employee)')
+ len(self._mails), len(new_partners) + 3,
+ f'Should have sent {len(new_partners) + 3} emails, one / recipient ({len(new_partners)} mailed partners + partner_1 + partner_2 + partner_employee)')
self.assertMailMail(
self.partner_employee_2, 'sent',
author=self.partner_employee,
@@ -1453,12 +1446,14 @@ class TestComposerResultsComment(TestMailComposer):
mail_message=self.test_record.message_ids[0],
)
self.assertMailMail(
- self.partner_1 + self.partner_2 + mailed_new_partners, 'sent',
+ self.partner_1 + self.partner_2 + new_partners, 'sent',
author=self.partner_employee,
email_to_recipients=[
[self.partner_1.email_formatted],
[f'"{self.partner_2.name}" <valid.other.1@agrolait.com>', f'"{self.partner_2.name}" <valid.other.cc@agrolait.com>'],
- ] + [[email] for email in mailed_new_partners.mapped('email_formatted')],
+ ] + [[new_partners[0]['email_formatted']],
+ ['"FindMe Multi" <find.me.multi.1@test.example.com>', '"FindMe Multi" <find.me.multi.2@test.example.com>']
+ ] + [[email] for email in new_partners[2:].mapped('email_formatted')],
email_values={
'body_content': f'TemplateBody {self.test_record.name}',
# single email event if email field is multi-email
@@ -1958,15 +1953,14 @@ class TestComposerResultsMass(TestMailComposer):
# FIXME: currently email finding based on formatted / multi emails does
# not work
new_partners = self.env['res.partner'].search([]).search([('id', 'not in', existing_partners.ids)])
- self.assertEqual(len(new_partners), 9,
- 'Mail (FIXME): did not find existing partners for formatted / multi emails: 2 extra partners')
+ self.assertEqual(len(new_partners), 8,
+ 'Mail (FIXME): did not find existing partners for formatted / multi emails: 1 extra partners')
self.assertIn(partner_format_tofind, new_partners)
self.assertIn(partner_multi_tofind, new_partners)
self.assertEqual(
sorted(new_partners.mapped('email')),
sorted(['"FindMe Format" <find.me.format@test.example.com>',
'find.me.multi.1@test.example.com, "FindMe Multi" <find.me.multi.2@test.example.com>',
- 'find.me.multi.1@test.example.com',
'find.me.multi.2@test.example.com',
'test.cc.1@example.com', 'test.cc.2@example.com', 'test.cc.2.2@example.com',
'test.to.1@example.com', 'test.to.2@example.com']),
@@ -1976,7 +1970,6 @@ class TestComposerResultsMass(TestMailComposer):
sorted(new_partners.mapped('email_formatted')),
sorted(['"FindMe Format" <find.me.format@test.example.com>',
'"FindMe Multi" <find.me.multi.1@test.example.com,find.me.multi.2@test.example.com>',
- '"find.me.multi.1@test.example.com" <find.me.multi.1@test.example.com>',
'"find.me.multi.2@test.example.com" <find.me.multi.2@test.example.com>',
'"test.cc.1@example.com" <test.cc.1@example.com>',
'"test.cc.2@example.com" <test.cc.2@example.com>',
@@ -1988,7 +1981,6 @@ class TestComposerResultsMass(TestMailComposer):
sorted(new_partners.mapped('name')),
sorted(['FindMe Format',
'FindMe Multi',
- 'find.me.multi.1@test.example.com',
'find.me.multi.2@test.example.com',
'test.cc.1@example.com', 'test.to.1@example.com', 'test.to.2@example.com',
'test.cc.2@example.com', 'test.cc.2.2@example.com']),
@@ -2003,23 +1995,21 @@ class TestComposerResultsMass(TestMailComposer):
# FIXME: more partners created than real emails (see above) -> due to
# transformation from email -> partner in template 'generate_recipients'
# there are more partners than email to notify;
- # NOTE: 'Findme Multi' is excluded as it has the same email as 'find.me.multi.1@test.example.com'
- # (created by template) and comes second in a search based on email
- mailed_new_partners = new_partners.filtered(lambda p: p.name != 'FindMe Multi')
- self.assertEqual(len(mailed_new_partners), 8)
self.assertEqual(len(self._new_mails), 2, 'Should have created 2 mail.mail')
self.assertEqual(
- len(self._mails), (len(mailed_new_partners) + 2) * 2,
- f'Should have sent {(len(mailed_new_partners) + 2) * 2} emails, one / recipient ({len(mailed_new_partners)} mailed partners + partner_1 + partner_2) * 2 records')
+ len(self._mails), (len(new_partners) + 2) * 2,
+ f'Should have sent {(len(new_partners) + 2) * 2} emails, one / recipient ({len(new_partners)} mailed partners + partner_1 + partner_2) * 2 records')
for record in self.test_records:
self.assertMailMail(
- self.partner_1 + self.partner_2 + mailed_new_partners,
+ self.partner_1 + self.partner_2 + new_partners,
'sent',
author=self.partner_employee,
email_to_recipients=[
[self.partner_1.email_formatted],
[f'"{self.partner_2.name}" <valid.other.1@agrolait.com>', f'"{self.partner_2.name}" <valid.other.cc@agrolait.com>'],
- ] + [[email] for email in mailed_new_partners.mapped('email_formatted')],
+ ] + [[new_partners[0]['email_formatted']],
+ ['"FindMe Multi" <find.me.multi.1@test.example.com>', '"FindMe Multi" <find.me.multi.2@test.example.com>']
+ ] + [[email] for email in new_partners[2:].mapped('email_formatted')],
email_values={
'body_content': f'TemplateBody {record.name}',
# single email event if email field is multi-email
diff --git a/addons/test_mail/tests/test_mail_gateway.py b/addons/test_mail/tests/test_mail_gateway.py
index a587059d675e..74ca6b91179a 100644
--- a/addons/test_mail/tests/test_mail_gateway.py
+++ b/addons/test_mail/tests/test_mail_gateway.py
@@ -486,8 +486,8 @@ class TestMailgateway(TestMailCommon):
record = self.format_and_process(
MAIL_TEMPLATE, f'"Valid Lelitre" <{test_email}>', 'groups@test.com', subject='Test3')
- self.assertFalse(record.message_ids[0].author_id,
- 'message_process (FIXME): unrecognized email -> author_id due to multi email')
+ self.assertEqual(record.message_ids[0].author_id, self.partner_1,
+ 'message_process: found author based on first found email normalized, even with multi emails')
self.assertEqual(record.message_ids[0].email_from, f'"Valid Lelitre" <{test_email}>')
self.assertNotSentEmail() # No notification / bounce should be sent
@@ -496,8 +496,8 @@ class TestMailgateway(TestMailCommon):
record = self.format_and_process(
MAIL_TEMPLATE, test_email, 'groups@test.com', subject='Test4')
- self.assertFalse(record.message_ids[0].author_id,
- 'message_process (FIXME): unrecognized email -> author_id due to multi email')
+ self.assertEqual(record.message_ids[0].author_id, self.partner_1,
+ 'message_process: found author based on first found email normalized, even with multi emails')
self.assertEqual(record.message_ids[0].email_from, test_email)
self.assertNotSentEmail() # No notification / bounce should be sent
@@ -717,7 +717,7 @@ class TestMailgateway(TestMailCommon):
for partner_email, passed in [
(formataddr((self.partner_1.name, self.partner_1.email_normalized)), True),
- (f'{self.partner_1.email_normalized}, "Multi Email" <multi.email@test.example.com>', False),
+ (f'{self.partner_1.email_normalized}, "Multi Email" <multi.email@test.example.com>', True),
(f'"Multi Email" <multi.email@test.example.com>, {self.partner_1.email_normalized}', False),
]:
with self.subTest(partner_email=partner_email):
diff --git a/addons/test_mail/tests/test_mail_thread_mixins.py b/addons/test_mail/tests/test_mail_thread_mixins.py
index 5588a6ffb79f..fb2c2e203ced 100644
--- a/addons/test_mail/tests/test_mail_thread_mixins.py
+++ b/addons/test_mail/tests/test_mail_thread_mixins.py
@@ -29,9 +29,9 @@ class TestMailThread(TestMailCommon, TestRecipients):
(' ', False)]
multi_pairs = [
(f'{base_email}, other.email@test.example.com',
- False), # multi not supported currently
+ base_email), # multi supports first found
(f'{tools.formataddr(("Another Name", base_email))}, other.email@test.example.com',
- False), # multi not supported currently
+ base_email), # multi supports first found
]
for email_from, exp_email_normalized in valid_pairs + void_pairs + multi_pairs:
with self.subTest(email_from=email_from, exp_email_normalized=exp_email_normalized):
diff --git a/addons/test_mass_mailing/tests/test_mailing.py b/addons/test_mass_mailing/tests/test_mailing.py
index 91d6f71495b4..899b4eb502da 100644
--- a/addons/test_mass_mailing/tests/test_mailing.py
+++ b/addons/test_mass_mailing/tests/test_mailing.py
@@ -207,6 +207,8 @@ class TestMassMailing(TestMassMailCommon):
# as it uses email_normalized if possible
if dst_model == 'mailing.test.customer':
formatted_mailmail_email = '"Formatted Record" <record.format@example.com>'
+ multi_mail_mail_email = 'record.multi.1@example.com, "Record Multi 2" <record.multi.2@example.com>'
+ multi_outgoing_emails = ['record.multi.1@example.com', '"Record Multi 2" <record.multi.2@example.com>']
unicode_email = '"Unicode Record" <record.😊@example.com>'
unicode_mailmail_email = '"Unicode Record" <record.😊@example.com>'
record_case_email = 'TEST.RECORD.CASE@EXAMPLE.COM'
@@ -217,6 +219,8 @@ class TestMassMailing(TestMassMailCommon):
else:
formatted_mailmail_email = 'record.format@example.com'
unicode_email = 'record.😊@example.com'
+ multi_mail_mail_email = 'record.multi.1@example.com'
+ multi_outgoing_emails = ['record.multi.1@example.com']
unicode_mailmail_email = 'record.😊@example.com'
record_case_email = 'test.record.case@example.com'
record_weird_email = 'test.record.weird@example.comweirdformat'
@@ -258,8 +262,8 @@ class TestMassMailing(TestMassMailCommon):
'partner': customer_weird_2,
'trace_status': 'sent'},
{'email': 'record.multi.1@example.com',
- 'email_to_mail': 'record.multi.1@example.com, "Record Multi 2" <record.multi.2@example.com>',
- 'email_to_recipients': [['record.multi.1@example.com', '"Record Multi 2" <record.multi.2@example.com>']],
+ 'email_to_mail': multi_mail_mail_email,
+ 'email_to_recipients': [multi_outgoing_emails],
'failure_type': False,
'trace_status': 'sent'},
{'email': 'record.format@example.com',
diff --git a/odoo/addons/base/tests/test_mail.py b/odoo/addons/base/tests/test_mail.py
index e4f1902765b1..a2bd023d7e6a 100644
--- a/odoo/addons/base/tests/test_mail.py
+++ b/odoo/addons/base/tests/test_mail.py
@@ -440,7 +440,7 @@ class TestEmailTools(BaseCase):
]
for source, expected in zip(sources, expected_list):
with self.subTest(source=source):
- self.assertEqual(email_normalize(source), expected)
+ self.assertEqual(email_normalize(source, strict=True), expected)
def test_email_split(self):
""" Test 'email_split' """
diff --git a/odoo/tools/mail.py b/odoo/tools/mail.py
index ce5544ca10fe..720344aecf6f 100644
--- a/odoo/tools/mail.py
+++ b/odoo/tools/mail.py
@@ -560,8 +560,11 @@ def email_normalize(text, strict=True):
- Possible Input Email : 'Name <NaMe@DoMaIn.CoM>'
- Normalized Output Email : 'name@domain.com'
- :param bool strict: text should contain exactly one email (default behavior
- and unique behavior before Odoo16);
+ :param boolean strict: if True, text should contain a single email
+ (default behavior in stable 14+). If more than one email is found no
+ normalized email is returned. If False the first found candidate is used
+ e.g. if email is 'tony@e.com, "Tony2" <tony2@e.com>', result is either
+ False (strict=True), either 'tony@e.com' (strict=False).
:return: False if no email found (or if more than 1 email found when being
in strict mode); normalized email otherwise;
--
GitLab