-
Notifications
You must be signed in to change notification settings - Fork 85
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
[ i18n sprint ] Decouple email templates and translations #334
[ i18n sprint ] Decouple email templates and translations #334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me overall. I don't have an easy way of testing email transmission with real SMTP at the moment, but am inclined to approve the PR. One question inline regarding the asynchronous transmission.
return true; | ||
} catch (MessagingException e) { | ||
log.error("Failed to send message.", e); | ||
return false; | ||
} | ||
} | ||
|
||
private void sendMessage(MimeMessage msg) { | ||
Thread thread = new Thread() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the asynchronous send? I think we already have a problem where the GUI claims that emails were sent successfully even if errors occurred. It seems that this might make that harder to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before that if smtp server have some delays 20-120 sec (it is one of greylisting approaches) then web interface will freeze for that period.
I agree, current notification about successfully sent email is misleading. I guess It should be replaced with something like "The message is going to be sent shortly" and an option to look up the results in notifications journal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable. I guess I had never encountered delays that long, but that would certainly be something to avoid. We should have a better way of verifying that emails were sent, but that seems out of scope for this PR.
Not merging yet pending approval of companion PR. |
* Decouple email templates and translations * provide siteName variable to error templates
Companion Vitro-languages PR
Modified FreemarkerEmailMessage and classes that used it:
Removed EmailDirective and error-email.ftl
Send email in separate thread to prevent web interface delays.
With this PR translations, extracted from email templates into property files could be used.
#How to test#
For each of installed languages execute following actions:
Interested parties
@VIVO-project/vivo-committers