Skip to content

[MERGE] base/mail: fix multiple issues in email address handling after #35929#39516

Closed
Julien00859 wants to merge 3 commits intoodoo:masterfrom
odoo-dev:master-email_trailing_carrial-juc
Closed

[MERGE] base/mail: fix multiple issues in email address handling after #35929#39516
Julien00859 wants to merge 3 commits intoodoo:masterfrom
odoo-dev:master-email_trailing_carrial-juc

Conversation

@Julien00859
Copy link
Copy Markdown
Member

@Julien00859 Julien00859 commented Oct 29, 2019

[FIX] tools: correctly format emails when having an encoding issue
[IMP] tools: restore P2 formataddr default behavior
[FIX] ir.mail.server: squash redundant CRs (bpo-34424)

Task: 2003936

@Julien00859
Copy link
Copy Markdown
Member Author

@tde-banana-odoo @odony

@C3POdoo C3POdoo added the RD research & development, internal work label Oct 29, 2019
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Oct 29, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from 648f4c7 to d83fe11 Compare October 30, 2019 13:24
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Oct 30, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from d83fe11 to fef1eaf Compare October 30, 2019 14:33
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Oct 30, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from fef1eaf to 58e0b72 Compare October 30, 2019 14:39
Copy link
Copy Markdown
Contributor

@odony odony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spell-checking, couldn't resist ;)

@odony odony changed the title Trailing carrial and P2 formataddr Remove extra CRs (caused by bpo-34424) and revert to readable formataddr() behavior (more P2-like) Oct 30, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from 58e0b72 to 402cb25 Compare October 30, 2019 15:28
@Julien00859
Copy link
Copy Markdown
Member Author

All done, thank you for the extensive review 😃

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Oct 30, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from 402cb25 to 000540f Compare October 31, 2019 12:16
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Oct 31, 2019
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from 000540f to ed12b33 Compare October 31, 2019 13:01
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Oct 31, 2019
@Julien00859
Copy link
Copy Markdown
Member Author

Hello @odony can we merge this one ? 🙂

Copy link
Copy Markdown
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the followup, seems like a good work :)

tde-banana-odoo and others added 2 commits November 12, 2019 14:48
It seems the emails > < are missing.

By the way one manual formataddr is replaced by an email_formatted field.
Result is the same but let us use fields doing it for us.
Python 2 `email.tools.formataddr` doesn't encode not RFC-2822 compliant
realname to base64 or quoted-printable. This is the wanted behavior to
output formatted email addresses on screen.

Python 3 `email.tools.formataddr` does encode the realname to be
RFC-2822 compliant. This is the wanted behavior when connecting to a
smtp server to send emails.

That method has been deprecated by pep-594 as the new python email API
is capable of automatically formatting email address in a RFC compliant
way. As the method was still in use in a lot of modules as the
preferred way to format email addresses, a refined P3-like
implementation as been included in the tool suite.

This commit changes the default behavior so it mimics P2 implementation
with an easy way to use the P3 behavior.

Task: 2003936
@Julien00859 Julien00859 force-pushed the master-email_trailing_carrial-juc branch from ed12b33 to bbd9243 Compare November 12, 2019 13:48
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Nov 12, 2019
@Julien00859
Copy link
Copy Markdown
Member Author

There is a customer on v13 that is affected by the py3 behavior, if it is possible to review this one so I can backport it there

@Julien00859 Julien00859 requested a review from odony November 14, 2019 10:06
Problem description:
Using the chatter, send an email to someone having non-ascii characters
in their name: the body of the received email looks like it contains
a mix of the original body and headers.

Python encodes the name into either base64 or quoted-printable but
leaves some redundant carriage returns at the end of the header. Those
redundant carriage returns are not RFC conformant but are interpreted
as newlines by some email clients, thus are read like a headers/body
separator and the next headers are considered part of the body,
corrupting the email structure.

This problem is internal to Python and has been fixed in 3.8, cfr
bpo-34424 [1], and later backported to 3.7.4. As we also support 3.6
and earlier 3.7 and it is not possible to easily monkey-patch the
function, we fixed it by squashing duplicate carriage returns.
(There is no case where a series of bare CRs can occur in a normal
RFC5322 email message)

Task: 2003936

[1]: https://bugs.python.org/issue34424
@odony odony force-pushed the master-email_trailing_carrial-juc branch from bbd9243 to 3e39a9b Compare November 14, 2019 13:09
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Nov 14, 2019
@odony odony changed the title Remove extra CRs (caused by bpo-34424) and revert to readable formataddr() behavior (more P2-like) [MERGE] mail: fix multiple issues in email address handling after #35929 Nov 14, 2019
@odony odony changed the title [MERGE] mail: fix multiple issues in email address handling after #35929 [MERGE] base/mail: fix multiple issues in email address handling after #35929 Nov 14, 2019
Copy link
Copy Markdown
Contributor

@odony odony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, seems everything is green for merging now 👍 I've pushed some minor edits to the commit messages (typos + some extra info).

@robodoo r+ rebase-merge

@robodoo
Copy link
Copy Markdown
Contributor

robodoo commented Nov 14, 2019

Merge method set to rebase and merge, using the PR as merge commit message

@odony
Copy link
Copy Markdown
Contributor

odony commented Nov 14, 2019

There is a customer on v13 that is affected by the py3 behavior, if it is possible to review this one so I can backport it there

Could you elaborate on what you intend to do? This PR is an afterthought on top of #35929, and it cannot be cherry-picked in 13.0 at all as far as I can see. For instance the old email API did not perform the appropriate header encoding transparently, did it?

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Nov 14, 2019
robodoo pushed a commit that referenced this pull request Nov 14, 2019
…#35929

[FIX] tools: correctly format emails when having an encoding issue
[IMP] tools: restore P2 formataddr default behavior
[FIX] ir.mail.server: squash redundant CRs (bpo-34424)

closes #39516

Task: 2003936
Signed-off-by: Olivier Dony (odo) <odo@openerp.com>
@Julien00859
Copy link
Copy Markdown
Member Author

Julien00859 commented Nov 14, 2019

@odony opw-2115767, a customer is having problem in v13 because the python formataddr show the name as quoted printable instead of utf-8 on various form views, this prevent him from migrating from v12 (where the names are showed in utf-8). My plan is to backport c024d89 (our own formataddr), a66e16e (tde's fix) and 81003c1 (p2 behavior)

@robodoo
Copy link
Copy Markdown
Contributor

robodoo commented Nov 14, 2019

Merged at 7352f0d, thanks!

@robodoo robodoo closed this Nov 14, 2019
@odony odony deleted the master-email_trailing_carrial-juc branch November 15, 2019 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI 🤖 Robodoo has seen passing statuses RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants