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.

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

Email template params interpolation #1345

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Email template params interpolation #1345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd93d48
Email template will now allow params to be interpolated in body strin…
avifatal Oct 6, 2018
ffa6149
typo
avifatal Oct 6, 2018
742539a
safer execution, if there are no params, just return the value. + rel…
avifatal Oct 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions modules/core/src/com/haulmont/cuba/core/app/Emailer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ public List<SendingMessage> sendEmailAsync(EmailInfo info, Integer attemptsCount

protected void prepareEmailInfo(EmailInfo emailInfo) {
processBodyTemplate(emailInfo);
processCaptionTemplate(emailInfo);

if (emailInfo.getFrom() == null) {
String defaultFromAddress = config.getFromAddress();
Expand All @@ -138,23 +139,36 @@ protected void prepareEmailInfo(EmailInfo emailInfo) {
}
}

protected void processCaptionTemplate(EmailInfo emailInfo) {
String caption = buildStringWithTemplateParams(emailInfo, emailInfo.getCaption());
Copy link
Contributor

Choose a reason for hiding this comment

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

This call can break valid caption if template params are empty in case of caption contains FreeMarker reserved symbols.
Probably we should add test for this case

emailInfo.setCaption(caption);
}

protected void processBodyTemplate(EmailInfo info) {
String templatePath = info.getTemplatePath();
if (templatePath == null) {
return;
String templateContents;
if (templatePath != null) {
templateContents = resources.getResourceAsString(templatePath);
if (templateContents == null) {
throw new IllegalArgumentException("Could not find template by path: " + templatePath);
}
}else{
templateContents = info.getBody();
}

Map<String, Serializable> params = info.getTemplateParameters() == null
? Collections.<String, Serializable>emptyMap()
: info.getTemplateParameters();
String templateContents = resources.getResourceAsString(templatePath);
if (templateContents == null) {
throw new IllegalArgumentException("Could not find template by path: " + templatePath);
templateContents = buildStringWithTemplateParams(info, templateContents);
Copy link
Contributor

Choose a reason for hiding this comment

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

And here too. Do not try to call template processor if params are empty, this will break existing code

Copy link
Author

@avifatal avifatal Oct 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

buildStringWithTemplateParams initialize the map if its null. isn't that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

You still call > TemplateHelper.processTemplate(templateContents, params);
even for empty maps.

FreeMarker will process it even if Map is empty. It is not OK. Body/subject can contain symbols that will be interpreted as FreeMarker commands.

Copy link
Author

@avifatal avifatal Oct 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

so, if not null instead?
Just to make sure I'm not missing something here. The original code does also that:

https://github.com/cuba-https://github.com/cuba-platform/cuba/blob/e7098974c9eca1c3e3d61e00cf205794f284586b/modules/core/src/com/haulmont/cuba/core/app/Emailer.java#L147-L154

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, processBodyTemplate is not performed for body and subject if templatePath is null.

Please pass <#assign link = "x"> as subject to your code without template params (empty template params). It should be inserted to email subject as is, but your code performs processing in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks compatibility with existing code where developers do not pass parameters

info.setBody(templateContents);
}

private String buildStringWithTemplateParams(EmailInfo info, String templateContents) {
String content = templateContents;
if(info.getTemplateParameters() != null && info.getTemplateParameters().size() > 0) {
content = TemplateHelper.processTemplate(templateContents, info.getTemplateParameters());
}
String body = TemplateHelper.processTemplate(templateContents, params);
info.setBody(body);
return content;
}


protected List<SendingMessage> splitEmail(EmailInfo info, @Nullable Integer attemptsCount, @Nullable Date deadline) {
List<SendingMessage> sendingMessageList = new ArrayList<>();
String[] splitAddresses = info.getAddresses().split("[,;]");
Expand Down
30 changes: 28 additions & 2 deletions modules/core/test/com/haulmont/cuba/core/app/EmailerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,16 +407,38 @@ public void testEmailTemplate() throws Exception {

Map<String, Serializable> params = new HashMap<>();
params.put("userName", "Bob");
params.put("greeting", "Hello");
params.put("dateParam", new SimpleDateFormat("dd/MM/yyyy").parse("01/05/2013"));

EmailInfo info = new EmailInfo("bob@example.com", "Test", null, templateFileName, params);
EmailInfo info = new EmailInfo("bob@example.com", "${greeting} Test", null, templateFileName, params);
emailer.sendEmailAsync(info);
emailer.processQueuedEmails();

String body = getBody(testMailSender.fetchSentEmail());
MimeMessage sent = testMailSender.fetchSentEmail();
String body = getBody(sent);
assertEquals("Greetings, Bob! 01-05-2013", body.trim());

String caption = getCaption(sent);
assertEquals("Hello Test", caption.trim());

info = new EmailInfo("bob@example.com", "${greeting} Test", null, null, params);
info.setBody("Greetings, ${userName}!");
emailer.sendEmailAsync(info);
emailer.processQueuedEmails();
sent = testMailSender.fetchSentEmail();
assertEquals("Greetings, Bob!", getBody(sent).trim());

info = new EmailInfo("bob@example.com", "${greeting} Test", null, null);
info.setBody("Greetings, ${userName}!");
emailer.sendEmailAsync(info);
emailer.processQueuedEmails();
sent = testMailSender.fetchSentEmail();

assertEquals("Greetings, ${userName}!", getBody(sent).trim());
assertEquals("${greeting} Test", getCaption(sent).trim());
}


@Test
public void testTextAttachment() throws Exception {
doTestTextAttachment(false);
Expand Down Expand Up @@ -609,6 +631,10 @@ private String getBody(MimeMessage msg) throws Exception {
return (String) textPart.getContent();
}

private String getCaption(MimeMessage msg) throws Exception {
return msg.getSubject();
}

private String getBodyContentType(MimeMessage msg) throws Exception {
MimeBodyPart textPart = getTextPart(msg);
return textPart.getContentType();
Expand Down