From 853bb98b81bcc6376c7f468391643306aea25d99 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= <tde@odoo.com>
Date: Fri, 30 Jul 2021 09:42:42 +0200
Subject: [PATCH] [IMP] base: avoid double formatting in partner
'email_formatted' field
PURPOSE
Be defensive when dealing with email fields, notably when having multi-emails
or email field containing an already-formatted email.
SPECIFICATIONS
Main fix in this commit: fix multiple nested formatting in 'email_formatted'
computation for <res.partner>. Other use cases are mainly left untouched as
we let users deal with their input. In summary :
* double format: if email already holds a formatted email, we should not use
it to compute email_formatted, like
name: Name / email: 'Format' <email@domain.com>
-> before '"Name" <"Name" <email@domain.com>>"
-> after '"Name" <email@domain.com>''
* multi emails: sometimes this field is used to hold several addresses
like email1@domain.com, email2@domain.com. We currently let this value
globally untouched by extracting emails and joining them, as we do not
expect email_formatted to be a list of emails. Extractin emails allows
to filter out extra text stored in email field, like
name: Name / email: text, email1@domain.com, email2@domain.com
-> before: "Name" <text, email1@domain.com, email2@domain.com>
-> after: "Name" <email1@domain.com,email2@domain.com>
* invalid email: if something is wrong, better keep it in email_formatted
than harcoding "False". Indeed this eases management and understanding
of failures at mail.mail, mail.notification and mailing.trace level. This
behavior does not change as it was already implemented like that even if
not sure it was intended;
Task-2612945 (Mail: Defensive email formatting)
Part-of: odoo/odoo#74474
---
.../crm/tests/test_crm_lead_notification.py | 2 +-
addons/mail/tests/test_res_partner.py | 4 +--
addons/test_mail/tests/test_mail_composer.py | 32 +++++++++---------
odoo/addons/base/models/res_partner.py | 33 ++++++++++++++++---
odoo/addons/base/tests/test_res_partner.py | 14 ++++----
odoo/tools/mail.py | 14 ++++++++
6 files changed, 69 insertions(+), 30 deletions(-)
diff --git a/addons/crm/tests/test_crm_lead_notification.py b/addons/crm/tests/test_crm_lead_notification.py
index ca7df34cc4ff..ec6c4e6039cc 100644
--- a/addons/crm/tests/test_crm_lead_notification.py
+++ b/addons/crm/tests/test_crm_lead_notification.py
@@ -39,7 +39,7 @@ class NewLeadNotification(TestCrmCommon):
[(False, '"New Customer" <new.customer.format@test.example.com>', 'Customer Email'),
(False, 'new.customer.multi.1@test.example.com, new.customer.2@test.example.com', 'Customer Email'),
(False, 'new.customer.simple@test.example.com', 'Customer Email'),
- (self.contact_1.id, '"Philip J Fry" <"Philip, J. Fry" <philip.j.fry@test.example.com>>', 'Customer'),
+ (self.contact_1.id, '"Philip J Fry" <philip.j.fry@test.example.com>', 'Customer'),
]
):
with self.subTest(lead=lead, email_from=lead.email_from):
diff --git a/addons/mail/tests/test_res_partner.py b/addons/mail/tests/test_res_partner.py
index 56fb86f4db59..588a969ca2b9 100644
--- a/addons/mail/tests/test_res_partner.py
+++ b/addons/mail/tests/test_res_partner.py
@@ -51,8 +51,8 @@ class TestPartner(MailCommon):
self.assertEqual(
partners.mapped('email_formatted'),
['"Classic Format" <classic.format@test.example.com>',
- '"FindMe Format" <"FindMe Format" <find.me.format@test.example.com>>',
- '"FindMe Multi" <find.me.multi.1@test.example.com, "FindMe Multi" <find.me.multi.2@test.example.com>>']
+ '"FindMe Format" <find.me.format@test.example.com>',
+ '"FindMe Multi" <find.me.multi.1@test.example.com,find.me.multi.2@test.example.com>']
)
self.assertEqual(
partners.mapped('email_normalized'),
diff --git a/addons/test_mail/tests/test_mail_composer.py b/addons/test_mail/tests/test_mail_composer.py
index 107f6abad4bf..99b6664fa6b7 100644
--- a/addons/test_mail/tests/test_mail_composer.py
+++ b/addons/test_mail/tests/test_mail_composer.py
@@ -637,15 +637,15 @@ class TestComposerResultsComment(TestMailComposer):
# ensure values used afterwards for testing
self.assertEqual(
self.partner_employee.email_formatted,
- '"Ernest Employee" <email.from.1@test.example.com, email.from.2@test.example.com>',
+ '"Ernest Employee" <email.from.1@test.example.com,email.from.2@test.example.com>',
'Formatting: wrong formatting due to multi-email')
self.assertEqual(
self.partner_1.email_formatted,
- '"Valid Lelitre" <"Valid Formatted" <valid.lelitre@agrolait.com>>',
- 'Formatting: wrong double encapsulation')
+ '"Valid Lelitre" <valid.lelitre@agrolait.com>',
+ 'Formatting: avoid wrong double encapsulation')
self.assertEqual(
self.partner_2.email_formatted,
- '"Valid Poilvache" <valid.other.1@agrolait.com, valid.other.cc@agrolait.com>',
+ '"Valid Poilvache" <valid.other.1@agrolait.com,valid.other.cc@agrolait.com>',
'Formatting: wrong formatting due to multi-email')
# instantiate composer, post message
@@ -680,8 +680,8 @@ class TestComposerResultsComment(TestMailComposer):
)
self.assertEqual(
sorted(new_partners.mapped('email_formatted')),
- sorted(['"FindMe Format" <"FindMe Format" <find.me.format@test.example.com>>',
- '"FindMe Multi" <find.me.multi.1@test.example.com, "FindMe Multi" <find.me.multi.2@test.example.com>>',
+ 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>',
@@ -722,12 +722,12 @@ class TestComposerResultsComment(TestMailComposer):
email_values={
'body_content': f'TemplateBody {self.test_record.name}',
# currently holding multi-email 'from'
- 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com, email.from.2@test.example.com')),
+ 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com,email.from.2@test.example.com')),
'subject': f'TemplateSubject {self.test_record.name}',
},
fields_values={
# currently holding multi-email 'email_from'
- 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com, email.from.2@test.example.com')),
+ 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com,email.from.2@test.example.com')),
},
mail_message=self.test_record.message_ids[0],
)
@@ -736,12 +736,12 @@ class TestComposerResultsComment(TestMailComposer):
author=self.partner_employee,
email_values={
'body_content': f'TemplateBody {self.test_record.name}',
- 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com, email.from.2@test.example.com')),
+ 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com,email.from.2@test.example.com')),
'subject': f'TemplateSubject {self.test_record.name}',
},
fields_values={
# currently holding multi-email 'email_from'
- 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com, email.from.2@test.example.com')),
+ 'email_from': formataddr((self.user_employee.name, 'email.from.1@test.example.com,email.from.2@test.example.com')),
},
mail_message=self.test_record.message_ids[0],
)
@@ -1080,15 +1080,15 @@ class TestComposerResultsMass(TestMailComposer):
# ensure values used afterwards for testing
self.assertEqual(
self.partner_employee.email_formatted,
- '"Ernest Employee" <email.from.1@test.example.com, email.from.2@test.example.com>',
+ '"Ernest Employee" <email.from.1@test.example.com,email.from.2@test.example.com>',
'Formatting: wrong formatting due to multi-email')
self.assertEqual(
self.partner_1.email_formatted,
- '"Valid Lelitre" <"Valid Formatted" <valid.lelitre@agrolait.com>>',
- 'Formatting: wrong double encapsulation')
+ '"Valid Lelitre" <valid.lelitre@agrolait.com>',
+ 'Formatting: avoid wrong double encapsulation')
self.assertEqual(
self.partner_2.email_formatted,
- '"Valid Poilvache" <valid.other.1@agrolait.com, valid.other.cc@agrolait.com>',
+ '"Valid Poilvache" <valid.other.1@agrolait.com,valid.other.cc@agrolait.com>',
'Formatting: wrong formatting due to multi-email')
# instantiate composer, send mailing
@@ -1123,8 +1123,8 @@ class TestComposerResultsMass(TestMailComposer):
)
self.assertEqual(
sorted(new_partners.mapped('email_formatted')),
- sorted(['"FindMe Format" <"FindMe Format" <find.me.format@test.example.com>>',
- '"FindMe Multi" <find.me.multi.1@test.example.com, "FindMe Multi" <find.me.multi.2@test.example.com>>',
+ 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>',
diff --git a/odoo/addons/base/models/res_partner.py b/odoo/addons/base/models/res_partner.py
index f468951a0a39..2d26e11df9af 100644
--- a/odoo/addons/base/models/res_partner.py
+++ b/odoo/addons/base/models/res_partner.py
@@ -379,11 +379,36 @@ class Partner(models.Model):
@api.depends('name', 'email')
def _compute_email_formatted(self):
+ """ Compute formatted email for partner, using formataddr. Be defensive
+ in computation, notably
+
+ * double format: if email already holds a formatted email like
+ 'Name' <email@domain.com> we should not use it as it to compute
+ email formatted like "Name <'Name' <email@domain.com>>";
+ * multi emails: sometimes this field is used to hold several addresses
+ like email1@domain.com, email2@domain.com. We currently let this value
+ untouched, but remove any formatting from multi emails;
+ * invalid email: if something is wrong, keep it in email_formatted as
+ this eases management and understanding of failures at mail.mail,
+ mail.notification and mailing.trace level;
+ * void email: email_formatted is False, as we cannot do anything with
+ it;
+ """
+ self.email_formatted = False
for partner in self:
- if partner.email:
- partner.email_formatted = tools.formataddr((partner.name or u"False", partner.email or u"False"))
- else:
- partner.email_formatted = ''
+ emails_normalized = tools.email_normalize_all(partner.email)
+ if emails_normalized:
+ # note: multi-email input leads to invalid email like "Name" <email1, email2>
+ # but this is current behavior in Odoo 14+ and some servers allow it
+ partner.email_formatted = tools.formataddr((
+ partner.name or u"False",
+ ','.join(emails_normalized)
+ ))
+ elif partner.email:
+ partner.email_formatted = tools.formataddr((
+ partner.name or u"False",
+ partner.email
+ ))
@api.depends('is_company')
def _compute_company_type(self):
diff --git a/odoo/addons/base/tests/test_res_partner.py b/odoo/addons/base/tests/test_res_partner.py
index 2ec3458de803..3160db404fae 100644
--- a/odoo/addons/base/tests/test_res_partner.py
+++ b/odoo/addons/base/tests/test_res_partner.py
@@ -61,25 +61,25 @@ class TestPartner(TransactionCase):
# encapsulated email
(
"Vlad the Impaler <vlad.the.impaler@example.com>",
- '"Balázs" <Vlad the Impaler <vlad.the.impaler@example.com>>'
+ '"Balázs" <vlad.the.impaler@example.com>'
), (
'"Balázs" <balazs@adam.hu>',
- '"Balázs" <"Balázs" <balazs@adam.hu>>'
+ '"Balázs" <balazs@adam.hu>'
),
# multi email
(
"vlad.the.impaler@example.com, vlad.the.dragon@example.com",
- '"Balázs" <vlad.the.impaler@example.com, vlad.the.dragon@example.com>'
+ '"Balázs" <vlad.the.impaler@example.com,vlad.the.dragon@example.com>'
), (
"vlad.the.impaler.com, vlad.the.dragon@example.com",
- '"Balázs" <vlad.the.impaler.com, vlad.the.dragon@example.com>'
+ '"Balázs" <vlad.the.dragon@example.com>'
), (
'vlad.the.impaler.com, "Vlad the Dragon" <vlad.the.dragon@example.com>',
- '"Balázs" <vlad.the.impaler.com, "Vlad the Dragon" <vlad.the.dragon@example.com>>'
+ '"Balázs" <vlad.the.dragon@example.com>'
),
# falsy emails
- (False, ''),
- ('', ''),
+ (False, False),
+ ('', False),
(' ', '"Balázs" <@ >'),
('notanemail', '"Balázs" <@notanemail>'),
]:
diff --git a/odoo/tools/mail.py b/odoo/tools/mail.py
index 3562b1b726c3..9e8468110781 100644
--- a/odoo/tools/mail.py
+++ b/odoo/tools/mail.py
@@ -552,6 +552,20 @@ def email_normalize(text):
return False
return emails[0].lower()
+def email_normalize_all(text):
+ """ Tool method allowing to extract email addresses from a text input and returning
+ normalized version of all found emails. If no email is found, a void list
+ is returned.
+
+ e.g. if email is 'tony@e.com, "Tony2" <tony2@e.com' returned result is ['tony@e.com, tony2@e.com']
+
+ :return list: list of normalized emails found in text
+ """
+ if not text:
+ return []
+ emails = email_split(text)
+ return list(filter(None, [email_normalize(email) for email in emails]))
+
def email_escape_char(email_address):
""" Escape problematic characters in the given email address string"""
return email_address.replace('\\', '\\\\').replace('%', '\\%').replace('_', '\\_')
--
GitLab