Skip to content

Fix sending mail with a non-latin display name. #2102 #2559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Sep 21, 2017
Merged

Fix sending mail with a non-latin display name. #2102 #2559

merged 4 commits into from
Sep 21, 2017

Conversation

rems4e
Copy link
Contributor

@rems4e rems4e commented Sep 20, 2017

Signed-off-by: Rémi Saurel contact@remi-saurel.com

This pull request fixes #2102, where display name of email addresses containing non-latin characters cannot be used in the From: field when creating/updating issues (resulting in no email sent at all).

The fix is simple, it uses Message.SetAddressHeader, instead of Message.SetHeader with a custom-formatted DisplayName <email@address> from address.
@rems4e
Fix sending mail with a non-latin display name. #2102
56f9555 
Signed-off-by: Rémi Saurel <contact@remi-saurel.com>
@codecov-io
Copy link

codecov-io commented Sep 20, 2017 •
edited
Loading

Codecov Report

Merging #2559 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #2559 +/- ## ======================================= Coverage 26.99% 26.99% ======================================= Files 85 85 Lines 17097 17097 ======================================= Hits 4615 4615 Misses 11807 11807 Partials 675 675
Impacted Files Coverage Δ
models/mail.go 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fbfccb...4774207. Read the comment docs.
@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 20, 2017
lafriks
lafriks requested changes Sep 20, 2017
View reviewed changes
modules/mailer/mailer.go Outdated
// NewMessage creates new mail message object with default From header.
func NewMessage(to []string, subject, body string) *Message {
return NewMessageFrom(to, setting.MailService.From, subject, body)
return NewMessageFrom(to, setting.MailService.From, "", subject, body)
Copy link
Member

Choose a reason for hiding this comment

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

But as stated in https://github.com/go-gitea/gitea/blob/master/conf/app.ini#L312 From can be in format "Sender" <sender@domain.ext> so it should be parsed somehow and passed in both arguments
@lunny lunny added this to the 1.x.x milestone Sep 20, 2017
@lunny lunny added the type/bug label Sep 20, 2017
@rems4e
Copy link
Contributor Author

rems4e commented Sep 20, 2017 •
edited
Loading

@lafriks OK, I overlooked this part. But parsing must also be done here too:

https://github.com/go-gitea/gitea/pull/2559/files#diff-91afb18937651f768645eff6cf5779a8R169

and keeping only the address part.

I will look into this.

EDIT Nevermind, the place I pointed uses setting.MailService.FromEmail.
@lafriks
Copy link
Member

lafriks commented Sep 20, 2017

@rems4e it actually already parses it here:
https://github.com/go-gitea/gitea/blob/master/modules/setting/setting.go#L1344

so probably new type field needs to be added FromDisplayName and assigned in same place where FromEmail is set and than it can be reused in your code
@rems4e
Copy link
Contributor Author

rems4e commented Sep 20, 2017

@lafriks Yes, see my edit above, it was my mistake and I managed to follow the track too ^^
rems4e and others added 2 commits September 20, 2017 13:44
@rems4e
Take into account the possibility that setting.MailService.From is in…
d35beee 
… `name <email@address>` format. #2102 Signed-off-by: Rémi Saurel <contact@remi-saurel.com>
@rems4e
Merge branch 'master' into master
e5a8745
@rems4e
Copy link
Contributor Author

rems4e commented Sep 20, 2017

This time it should be cleaner.
@lafriks
Copy link
Member

lafriks commented Sep 20, 2017

LGTM
@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 20, 2017
@ethantkoenig
Copy link
Member

LGTM
@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 21, 2017
@lunny lunny modified the milestones: 1.x.x, 1.3.0 Sep 21, 2017
@lunny lunny merged commit 66bc0ac into go-gitea:master Sep 21, 2017
@ptman
Copy link
Contributor

ptman commented Oct 25, 2017

Any chance of getting this backported to 1.2.x?
@lunny
Copy link
Member

lunny commented Oct 25, 2017

seems it's not difficult.
@ptman ptman mentioned this pull request Oct 26, 2017
Merged
@lafriks lafriks added backport/done All backports for this PR have been created backport/v1.2 labels Oct 26, 2017
lafriks pushed a commit that referenced this pull request Oct 26, 2017
@ptman @lafriks
Fix sending mail with a non-latin display name. #2102 (#2559) (#2783)
33e1641 
* Fix sending mail with a non-latin display name. #2102 Signed-off-by: Rémi Saurel <contact@remi-saurel.com> * Take into account the possibility that setting.MailService.From is in `name <email@address>` format. #2102 Signed-off-by: Rémi Saurel <contact@remi-saurel.com>
@zeripath zeripath mentioned this pull request May 25, 2019
Closed
7 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

7 participants

@rems4e @codecov-io @lafriks @ethantkoenig @ptman @lunny @tboerger