From 3191aa820757c9765ec22c6f3d76858a2c7896df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Delavall=C3=A9e?= 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 . 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' -> before '"Name" <"Name" >" -> after '"Name" '' * 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" -> after: "Name" * 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) X-original-commit: odoo/odoo@9175bbd8e277a334651e6065e4bc5395884d811f Part-of: odoo/odoo#134934 --- .../crm/tests/test_crm_lead_notification.py | 2 +- addons/mail/tests/test_res_partner.py | 4 +- addons/test_mail/tests/test_mail_composer.py | 63 ++++++++++++------- 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, 92 insertions(+), 38 deletions(-) diff --git a/addons/crm/tests/test_crm_lead_notification.py b/addons/crm/tests/test_crm_lead_notification.py index f55074add07..420c0c391bf 100644 --- a/addons/crm/tests/test_crm_lead_notification.py +++ b/addons/crm/tests/test_crm_lead_notification.py @@ -72,7 +72,7 @@ class NewLeadNotification(TestCrmCommon): 'name': 'Std Name', 'user_id': self.user_sales_leads.id, 'team_id': self.sales_team_1.id, }), - (self.contact_1.id, '"Philip J Fry" <"Philip, J. Fry" >', self.contact_1.lang, 'Customer', + (self.contact_1.id, '"Philip J Fry" ', self.contact_1.lang, 'Customer', {}), # creation values only if not partner ] ): diff --git a/addons/mail/tests/test_res_partner.py b/addons/mail/tests/test_res_partner.py index f5a1f2a4084..6b4f2cb0138 100644 --- a/addons/mail/tests/test_res_partner.py +++ b/addons/mail/tests/test_res_partner.py @@ -179,8 +179,8 @@ class TestPartner(MailCommon): self.assertEqual( partners.mapped('email_formatted'), ['"Classic Format" ', - '"FindMe Format" <"FindMe Format" >', - '"FindMe Multi" >'] + '"FindMe Format" ', + '"FindMe Multi" '] ) 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 c173b8a610c..99b88dfb49f 100644 --- a/addons/test_mail/tests/test_mail_composer.py +++ b/addons/test_mail/tests/test_mail_composer.py @@ -1717,15 +1717,15 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): # ensure values used afterwards for testing self.assertEqual( self.partner_employee.email_formatted, - '"Ernest Employee" ', + '"Ernest Employee" ', 'Formatting: wrong formatting due to multi-email') self.assertEqual( self.partner_1.email_formatted, - '"Valid Lelitre" <"Valid Formatted" >', - 'Formatting: wrong double encapsulation') + '"Valid Lelitre" ', + 'Formatting: avoid wrong double encapsulation') self.assertEqual( self.partner_2.email_formatted, - '"Valid Poilvache" ', + '"Valid Poilvache" ', 'Formatting: wrong formatting due to multi-email') # instantiate composer, post message @@ -1760,8 +1760,8 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): ) self.assertEqual( sorted(new_partners.mapped('email_formatted')), - sorted(['"FindMe Format" <"FindMe Format" >', - '"FindMe Multi" >', + sorted(['"FindMe Format" ', + '"FindMe Multi" ', '"find.me.multi.1@test.example.com" ', '"find.me.multi.2@test.example.com" ', '"test.cc.1@example.com" ', @@ -1802,12 +1802,12 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): 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], ) @@ -1816,12 +1816,12 @@ class TestComposerResultsComment(TestMailComposer, CronMixinCase): 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], ) @@ -2183,12 +2183,27 @@ class TestComposerResultsMass(TestMailComposer): # check layouting and language. Note that standard layout # is not tested against translations, only the custom one # to ease translations checks. - email = self._find_sent_email(self.partner_employee_2.email_formatted, [record.customer_id.email_formatted]) - self.assertTrue(bool(email), 'Email not found, check recipients') + sent_mail = self._find_sent_email( + self.partner_employee_2.email_formatted, + [formataddr((record.customer_id.name, record.customer_id.email))] + ) + debug_info = '' + if not sent_mail: + debug_info = '-'.join('From: %s-To: %s' % (mail['email_from'], mail['email_to']) for mail in self._mails) + self.assertTrue( + bool(sent_mail), + f'Expected mail from {self.partner_employee_2.email_formatted} to {formataddr((record.customer_id.name, record.customer_id.email))} not found in {debug_info}' + ) + if record == self.test_records[0]: + self.assertEqual(sent_mail['email_to'], ['"Partner_0" '], + 'Should take email normalized in to') + else: + self.assertEqual(sent_mail['email_to'], ['"Partner_1" '], + 'Should take email normalized in to') if not email_layout_xmlid: self.assertEqual( - email['body'], + sent_mail['body'], f'

{exp_body}

' ) else: @@ -2197,12 +2212,12 @@ class TestComposerResultsMass(TestMailComposer): exp_button_en = 'View Ticket-like model' exp_button_es = 'Spanish Layout para Spanish Model Description' if exp_lang == 'es_ES': - self.assertIn(exp_layout_content_es, email['body']) - self.assertIn(exp_button_es, email['body']) + self.assertIn(exp_layout_content_es, sent_mail['body']) + self.assertIn(exp_button_es, sent_mail['body']) else: - self.assertIn(exp_layout_content_en, email['body']) - # self.assertIn(exp_button_es, email['body']) - self.assertIn(exp_button_en, email['body']) + self.assertIn(exp_layout_content_en, sent_mail['body']) + # self.assertIn(exp_button_es, sent_mail['body']) + self.assertIn(exp_button_en, sent_mail['body']) @users('employee') @mute_logger('odoo.models.unlink', 'odoo.addons.mail.models.mail_mail') @@ -2373,15 +2388,15 @@ class TestComposerResultsMass(TestMailComposer): # ensure values used afterwards for testing self.assertEqual( self.partner_employee.email_formatted, - '"Ernest Employee" ', + '"Ernest Employee" ', 'Formatting: wrong formatting due to multi-email') self.assertEqual( self.partner_1.email_formatted, - '"Valid Lelitre" <"Valid Formatted" >', - 'Formatting: wrong double encapsulation') + '"Valid Lelitre" ', + 'Formatting: avoid wrong double encapsulation') self.assertEqual( self.partner_2.email_formatted, - '"Valid Poilvache" ', + '"Valid Poilvache" ', 'Formatting: wrong formatting due to multi-email') # instantiate composer, send mailing @@ -2416,8 +2431,8 @@ class TestComposerResultsMass(TestMailComposer): ) self.assertEqual( sorted(new_partners.mapped('email_formatted')), - sorted(['"FindMe Format" <"FindMe Format" >', - '"FindMe Multi" >', + sorted(['"FindMe Format" ', + '"FindMe Multi" ', '"find.me.multi.1@test.example.com" ', '"find.me.multi.2@test.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 756ee6123e0..ddb960dc4c1 100644 --- a/odoo/addons/base/models/res_partner.py +++ b/odoo/addons/base/models/res_partner.py @@ -507,11 +507,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' we should not use it as it to compute + email formatted like "Name <'Name' >"; + * 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" + # 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 2947c959b32..1b3d47dac79 100644 --- a/odoo/addons/base/tests/test_res_partner.py +++ b/odoo/addons/base/tests/test_res_partner.py @@ -130,25 +130,25 @@ class TestPartner(TransactionCase): # encapsulated email ( "Vlad the Impaler ", - '"Balázs" >' + '"Balázs" ' ), ( '"Balázs" ', - '"Balázs" <"Balázs" >' + '"Balázs" ' ), # multi email ( "vlad.the.impaler@example.com, vlad.the.dragon@example.com", - '"Balázs" ' + '"Balázs" ' ), ( "vlad.the.impaler.com, vlad.the.dragon@example.com", - '"Balázs" ' + '"Balázs" ' ), ( 'vlad.the.impaler.com, "Vlad the Dragon" ', - '"Balázs" >' + '"Balázs" ' ), # 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 8391b44f01f..d40fcaff12a 100644 --- a/odoo/tools/mail.py +++ b/odoo/tools/mail.py @@ -570,6 +570,20 @@ def email_normalize(text, strict=True): 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"