Skip to content
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.

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

Use FullName in Emails to address the recipient if possible #31527

Merged
merged 14 commits into from
Jul 8, 2024
30 changes: 30 additions & 0 deletions models/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"context"
"encoding/hex"
"fmt"
"mime"
"net/mail"
"net/url"
"path/filepath"
"regexp"
Expand Down Expand Up @@ -413,6 +415,34 @@ func (u *User) DisplayName() string {
return u.Name
}

var emailToReplacer = strings.NewReplacer(
Copy link
Member

Choose a reason for hiding this comment

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

emailNameReplacer?
I don't see any reason why we should only use it for To.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now we do, if it gets reused some can just rename it

"\n", "",
"\r", "",
"<", "",
">", "",
",", "",
":", "",
";", "",
)

// EmailTo returns a string suitable to be put into a e-mail `To:` header.
func (u *User) EmailTo() string {
sanitizedDisplayName := emailToReplacer.Replace(u.DisplayName())

// should be an edge case but nice to have
if sanitizedDisplayName == u.Email {
return u.Email
}

to := fmt.Sprintf("%s <%s>", sanitizedDisplayName, u.Email)
add, err := mail.ParseAddress(to)
if err != nil {
return u.Email
}

return fmt.Sprintf("%s <%s>", mime.QEncoding.Encode("utf-8", add.Name), add.Address)
Copy link
Member

Choose a reason for hiding this comment

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

Why not encode the sanitizedDisplayName beforehand?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because then the mime encoding is undone

}

// GetDisplayName returns full name if it's not empty and DEFAULT_SHOW_FULL_NAME is set,
// returns username otherwise.
func (u *User) GetDisplayName() string {
Expand Down
23 changes: 23 additions & 0 deletions models/user/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,29 @@ func Test_NormalizeUserFromEmail(t *testing.T) {
}
}

func TestEmailTo(t *testing.T) {
testCases := []struct {
fullName string
mail string
result string
}{
{"Awareness Hub", "awareness@hub.net", "Awareness Hub <awareness@hub.net>"},
{"name@example.com", "name@example.com", "name@example.com"},
{"Hi Its <Mee>", "ee@mail.box", "Hi Its Mee <ee@mail.box>"},
{"Sinéad.O'Connor", "sinead.oconnor@gmail.com", "=?utf-8?q?Sin=C3=A9ad.O'Connor?= <sinead.oconnor@gmail.com>"},
{"Æsir", "aesir@gmx.de", "=?utf-8?q?=C3=86sir?= <aesir@gmx.de>"},
{"new😀user", "new.user@alo.com", "=?utf-8?q?new=F0=9F=98=80user?= <new.user@alo.com>"},
{`"quoted"`, "quoted@test.com", "quoted <quoted@test.com>"},
}

for _, testCase := range testCases {
t.Run(testCase.result, func(t *testing.T) {
testUser := &user_model.User{FullName: testCase.fullName, Email: testCase.mail}
assert.EqualValues(t, testCase.result, testUser.EmailTo())
})
}
}

func TestDisabledUserFeatures(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())

Expand Down
6 changes: 3 additions & 3 deletions services/mailer/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func sendUserMail(language string, u *user_model.User, tpl base.TplName, code, s
return
}

msg := NewMessage(u.Email, subject, content.String())
msg := NewMessage(u.EmailTo(), subject, content.String())
msg.Info = fmt.Sprintf("UID: %d, %s", u.ID, info)

SendAsync(msg)
Expand Down Expand Up @@ -158,7 +158,7 @@ func SendRegisterNotifyMail(u *user_model.User) {
return
}

msg := NewMessage(u.Email, locale.TrString("mail.register_notify"), content.String())
msg := NewMessage(u.EmailTo(), locale.TrString("mail.register_notify"), content.String())
msg.Info = fmt.Sprintf("UID: %d, registration notify", u.ID)

SendAsync(msg)
Expand Down Expand Up @@ -189,7 +189,7 @@ func SendCollaboratorMail(u, doer *user_model.User, repo *repo_model.Repository)
return
}

msg := NewMessage(u.Email, subject, content.String())
msg := NewMessage(u.EmailTo(), subject, content.String())
msg.Info = fmt.Sprintf("UID: %d, add collaborator", u.ID)

SendAsync(msg)
Expand Down
8 changes: 4 additions & 4 deletions services/mailer/mail_release.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) {
return
}

langMap := make(map[string][]string)
langMap := make(map[string][]*user_model.User)
for _, user := range recipients {
if user.ID != rel.PublisherID {
langMap[user.Language] = append(langMap[user.Language], user.Email)
langMap[user.Language] = append(langMap[user.Language], user)
}
}

Expand All @@ -52,7 +52,7 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) {
}
}

func mailNewRelease(ctx context.Context, lang string, tos []string, rel *repo_model.Release) {
func mailNewRelease(ctx context.Context, lang string, tos []*user_model.User, rel *repo_model.Release) {
locale := translation.NewLocale(lang)

var err error
Expand Down Expand Up @@ -89,7 +89,7 @@ func mailNewRelease(ctx context.Context, lang string, tos []string, rel *repo_mo
publisherName := rel.Publisher.DisplayName()
msgID := generateMessageIDForRelease(rel)
for _, to := range tos {
msg := NewMessageFrom(to, publisherName, setting.MailService.FromEmail, subject, mailBody.String())
msg := NewMessageFrom(to.EmailTo(), publisherName, setting.MailService.FromEmail, subject, mailBody.String())
msg.Info = subject
msg.SetHeader("Message-ID", msgID)
msgs = append(msgs, msg)
Expand Down
12 changes: 6 additions & 6 deletions services/mailer/mail_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model.
return err
}

langMap := make(map[string][]string)
langMap := make(map[string][]*user_model.User)
for _, user := range users {
if !user.IsActive {
// don't send emails to inactive users
continue
}
langMap[user.Language] = append(langMap[user.Language], user.Email)
langMap[user.Language] = append(langMap[user.Language], user)
}

for lang, tos := range langMap {
Expand All @@ -46,11 +46,11 @@ func SendRepoTransferNotifyMail(ctx context.Context, doer, newOwner *user_model.
return nil
}

return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []string{newOwner.Email}, repo)
return sendRepoTransferNotifyMailPerLang(newOwner.Language, newOwner, doer, []*user_model.User{newOwner}, repo)
}

// sendRepoTransferNotifyMail triggers a notification e-mail when a pending repository transfer was created for each language
func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.User, emails []string, repo *repo_model.Repository) error {
func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.User, emailTos []*user_model.User, repo *repo_model.Repository) error {
var (
locale = translation.NewLocale(lang)
content bytes.Buffer
Expand Down Expand Up @@ -78,8 +78,8 @@ func sendRepoTransferNotifyMailPerLang(lang string, newOwner, doer *user_model.U
return err
}

for _, to := range emails {
msg := NewMessage(to, subject, content.String())
for _, to := range emailTos {
msg := NewMessage(to.EmailTo(), subject, content.String())
msg.Info = fmt.Sprintf("UID: %d, repository pending transfer notification", newOwner.ID)

SendAsync(msg)
Expand Down