From bafc060f639539475abf06221ab590637e6fb8dc 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)
Part-of: odoo/odoo#74474
---
addons/mail/models/mail_thread_blacklist.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 +-
addons/test_mail/tests/test_mail_tools.py | 2 +-
.../test_mass_mailing/tests/test_mailing.py | 11 ++++-
odoo/addons/base/tests/test_mail.py | 2 +-
odoo/tools/mail.py | 10 ++++-
9 files changed, 58 insertions(+), 46 deletions(-)
diff --git a/addons/mail/models/mail_thread_blacklist.py b/addons/mail/models/mail_thread_blacklist.py
index 48b9e734ab80..171a4b1562a4 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], force_single=False)
@api.model
def _search_is_blacklisted(self, operator, value):
diff --git a/addons/mail/tests/test_res_partner.py b/addons/mail/tests/test_res_partner.py
index 588a969ca2b9..18ec6eea7353 100644
--- a/addons/mail/tests/test_res_partner.py
+++ b/addons/mail/tests/test_res_partner.py
@@ -54,9 +54,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
@@ -68,11 +70,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 2a7cf12f3201..b4af1a5b201a 100644
--- a/addons/test_mail/tests/test_mail_composer.py
+++ b/addons/test_mail/tests/test_mail_composer.py
@@ -664,15 +664,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']),
@@ -682,7 +681,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>',
@@ -694,7 +692,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']),
@@ -708,14 +705,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,
@@ -732,12 +725,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
@@ -1112,15 +1107,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']),
@@ -1130,7 +1124,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>',
@@ -1142,7 +1135,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']),
@@ -1157,23 +1149,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 103877f6b24d..6ed94dce423a 100644
--- a/addons/test_mail/tests/test_mail_gateway.py
+++ b/addons/test_mail/tests/test_mail_gateway.py
@@ -440,8 +440,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
@@ -450,8 +450,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
@@ -661,7 +661,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 875a0e8aa0b8..a1a7ad2a04bb 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_mail/tests/test_mail_tools.py b/addons/test_mail/tests/test_mail_tools.py
index 284b3623a83d..e30cdf05e9af 100644
--- a/addons/test_mail/tests/test_mail_tools.py
+++ b/addons/test_mail/tests/test_mail_tools.py
@@ -132,7 +132,7 @@ class TestMailTools(TestMailCommon, TestRecipients):
(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/test_mass_mailing/tests/test_mailing.py b/addons/test_mass_mailing/tests/test_mailing.py
index ae4de93985c5..7f17d6b52b7b 100644
--- a/addons/test_mass_mailing/tests/test_mailing.py
+++ b/addons/test_mass_mailing/tests/test_mailing.py
@@ -200,9 +200,13 @@ 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_trace_email = 'record.multi.1@example.com, "Record Multi 2" <record.multi.2@example.com>'
+ multi_state = 'ignored'
unicode_mailmail_email = '"Unicode Record" <record.😊@example.com>'
else:
formatted_mailmail_email = 'record.format@example.com'
+ multi_trace_email = 'record.multi.1@example.com'
+ multi_state = 'sent'
unicode_mailmail_email = 'record.😊@example.com'
self.assertMailTraces(
[
@@ -221,20 +225,23 @@ class TestMassMailing(TestMassMailCommon):
'partner': customer_unic,
'state': 'ignored'}, # email_re usage forbids mailing to unicode
{'email': 'test.customer.case@example.com',
+ 'email_to_recipients': [[f'"{customer_case.name}" <test.customer.case@example.com>']],
'failure_type': False,
'partner': customer_case,
'state': 'sent'}, # lower cased
{'email': 'test.customer.weird@example.comweirdformat',
+ 'email_to_recipients': [[f'"{customer_weird.name}" <test.customer.weird@example.comweirdformat>']],
'failure_type': False,
'partner': customer_weird,
'state': 'sent'}, # concatenates everything after domain
{'email': 'weird format2 test.customer.weird.2@example.com',
+ 'email_to_recipients': [[f'"{customer_weird_2.name}" <weird format2 test.customer.weird.2@example.com>']],
'failure_type': False,
'partner': customer_weird_2,
'state': 'sent'},
- {'email': 'record.multi.1@example.com, "Record Multi 2" <record.multi.2@example.com>',
+ {'email': multi_trace_email,
'failure_type': False,
- 'state': 'ignored'},
+ 'state': multi_state},
{'email': 'record.format@example.com',
'email_to_mail': formatted_mailmail_email,
'failure_type': False,
diff --git a/odoo/addons/base/tests/test_mail.py b/odoo/addons/base/tests/test_mail.py
index 7ed4164e36e8..aa161764872b 100644
--- a/odoo/addons/base/tests/test_mail.py
+++ b/odoo/addons/base/tests/test_mail.py
@@ -419,7 +419,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, force_single=True), expected)
def test_email_split(self):
""" Test 'email_split' """
diff --git a/odoo/tools/mail.py b/odoo/tools/mail.py
index 9e8468110781..ee3a3e49b3bc 100644
--- a/odoo/tools/mail.py
+++ b/odoo/tools/mail.py
@@ -537,7 +537,7 @@ def email_split_and_format(text):
return []
return [formataddr((name, email)) for (name, email) in email_split_tuples(text)]
-def email_normalize(text):
+def email_normalize(text, force_single=True):
""" Sanitize and standardize email address entries.
A normalized email is considered as :
- having a left part + @ + a right part (the domain can be without '.something')
@@ -546,9 +546,15 @@ def email_normalize(text):
Ex:
- Possible Input Email : 'Name <NaMe@DoMaIn.CoM>'
- Normalized Output Email : 'name@domain.com'
+
+ :param boolean force_single: 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 (force_single=True), either 'tony@e.com' (force_single=False).
"""
emails = email_split(text)
- if not emails or len(emails) != 1:
+ if not emails or (len(emails) != 1 and force_single):
return False
return emails[0].lower()
--
GitLab