4580 change from and reply to addresses in contact email #4600
Conversation
and set reply-to to the requested 'from' address
Changing from address personal name to reflect reply address
|
Added issue title and number to comment from @qqmyers above in order to link the PR and issue. |
There was a problem hiding this comment.
I might need to think about this pull request some more before explicitly approving it, but I'm at least leaving some comments. I'm glad to read in #4580 that this is a good fix for preventing emails from being blocked on Amazon SES. In that issue @qqmyers observed that there are many "sendMail" methods and we might want to reduce this number if we can.
| @@ -89,7 +90,10 @@ public void sendMail(String host, String from, String to, String subject, String | |||
| String[] recipientStrings = to.split(","); | |||
| InternetAddress[] recipients = new InternetAddress[recipientStrings.length]; | |||
| try { | |||
| msg.setFrom(new InternetAddress(from, charset)); | |||
| InternetAddress fromAddress=getSystemAddress(); | |||
| fromAddress.setPersonal(fromAddress.getPersonal() + "on behalf of " + reply, charset); | |||
There was a problem hiding this comment.
Should "on behalf of" be put into Bundle.properties so it can be translated into various languages?
There was a problem hiding this comment.
See BundleUtil.getStringFromBundle examples in this file (lower down).
| //Always send from system address to avoid email being blocked | ||
| InternetAddress fromAddress=getSystemAddress(); | ||
| try { | ||
| fromAddress.setPersonal(fromAddress.getPersonal() + "on behalf of " + reply, charset); |
There was a problem hiding this comment.
There should be a space before the "on" here and see above about internationalization.
| fromAddress.setPersonal(fromAddress.getPersonal() + "on behalf of " + reply, charset); | ||
| } catch (UnsupportedEncodingException ex) { | ||
| logger.severe(ex.getMessage()); | ||
| } |
There was a problem hiding this comment.
We're catching UnsupportedEncodingException here and logging the exception, which is good, but should we return in this catch rather than marching forward?
There was a problem hiding this comment.
I was mostly following the pattern in the other methods, but this exception would be non-fatal (and presumably not likely since charset is set to 'UTF-8' on line 73). The message could go out without the 'on behalf of ' addition, which would probably be better (?) than failing to send anything.
| msg.setFrom(getSystemAddress()); | ||
| messageText = "From: " + from + "\n\n" + messageText; | ||
| //Otherwise include the invalid 'from' address in the message | ||
| messageText = "From: " + reply + "\n\n" + messageText; |
| @@ -79,7 +80,7 @@ | |||
| public MailServiceBean() { | |||
| } | |||
|
|
|||
| public void sendMail(String host, String from, String to, String subject, String messageText) { | |||
| public void sendMail(String host, String reply, String to, String subject, String messageText) { | |||
There was a problem hiding this comment.
I didn't see any use of this method in the current source...
|
Changed to use a message from Bundle.properties and merged with the develop branch... |
qqmyers
deleted the
4580-change-from-and-reply-to-addresses-in-contact-email
branch
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist