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

Bounce address #25

Merged
merged 2 commits into from Jan 19, 2015
Merged

Bounce address #25

merged 2 commits into from Jan 19, 2015

Conversation

kpmeen
Copy link

@kpmeen kpmeen commented Jan 18, 2015

Added support for setting a custom bounce address as specified in commons mail (here: http://commons.apache.org/proper/commons-email/userguide.html#Handling_Bounced_Messages).

@ggrossetie
Copy link
Member

Hi @kpmeen,

Thank you for your contribution. Would you mind signing our CLA?

http://www.typesafe.com/contribute/cla

Thanks!

@@ -182,6 +185,7 @@ abstract class CommonsMailer(smtpHost: String, smtpPort: Int,
email.setSubject(data.subject)
email.setFrom(data.from)
data.replyTo.foreach(setAddress(_) { (address, name) => email.addReplyTo(address, name)})
data.bounceAddress.map(email.setBounceAddress)
Copy link
Member

Choose a reason for hiding this comment

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

Can you use data.bounceAddress.foreach(email.setBounceAddress) to stay consistent with the rest of the code ?

@ggrossetie
Copy link
Member

Could you do a rebase to avoid the merge commit 426a734 ?

@kpmeen
Copy link
Author

kpmeen commented Jan 18, 2015

Thanks for swift feedback! Updated and pushed :-)

@jroper
Copy link
Member

jroper commented Jan 19, 2015

lgtm

@ggrossetie
Copy link
Member

Yep 👍

ggrossetie added a commit that referenced this pull request Jan 19, 2015
@ggrossetie ggrossetie merged commit ca946dc into playframework:master Jan 19, 2015
@ggrossetie
Copy link
Member

Great work, thanks again!

@mkurz
Copy link
Member

mkurz commented May 19, 2015

@Mogztter Could it be that for adding the bounce address we also could run into encoding problems like #28? Shouldn't setAddress also get called here? I am not sure, I was just thinking about it during reviewing the code...

@ggrossetie
Copy link
Member

AFAIK there's only one method to set the bounce address:
https://commons.apache.org/proper/commons-email/apidocs/org/apache/commons/mail/Email.html#setBounceAddress(java.lang.String)

This means that you are not supposed to define a name (just an email
address).
If we define a name and an email I think we will have encoding problems...
We need to add a test to verify that assertion and open an issue in commons
mailer ;)
Le 19 mai 2015 9:30 AM, "Matthias Kurz" notifications@github.com a écrit :

@Mogztter https://github.com/Mogztter Could it be that for adding the
bounce address
https://github.com/playframework/play-mailer/blob/master/src/main/scala/play/api/libs/mailer/MailerPlugin.scala#L134
we also could run into encoding problems like #28
#28? Shouldn't
setAddress also get called here? I am not sure, I was just thinking about
it during reviewing the code...


Reply to this email directly or view it on GitHub
#25 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants