Email template params interpolation

[avifatal]

will now allow params to be interpolated in body strig (not from file) and subject.

[CLAassistant]

All committers have signed the CLA.

[jreznot]

We should not try to process something as template if template params is empty.
There could be special symbols in caption / description that are used in Freemarker as commands.

[jreznot]

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

[jreznot]

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

[avifatal]

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

[jreznot]

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.

[avifatal]

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

[jreznot]

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.

[jreznot]

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

[jreznot]

Before running tests, create and start DB:

gradlew startDb createTestDb

[jreznot]

Now code still is not correct for the situation: templatePath + empty parameters. This is a valid case in the current implementation. String loaded from templatePath must always be processed with FreeMarker.

[jreznot]

Moreover, it seems that we cannot and should not decide whether body should be processed automatically. It should be a parameter of EmailInfo object.

[avifatal]

Now code still is not correct for the situation: templatePath + empty parameters. This is a valid case in the current implementation. String loaded from templatePath must always be processed with FreeMarker.

Isn't that the opposite of what you told me? see here: #1345 (comment)

[avifatal]

Moreover, it seems that we cannot and should not decide whether body should be processed automatically. It should be a parameter of EmailInfo object.

I disagree, the template is dynamic, sometimes you have params and sometimes not. you dont have this knowledge at compile time in most cases. WDYT?

[jreznot]

I disagree, the template is dynamic, sometimes you have params and sometimes not.

I mean that only developer who creates EmailInfo should specify whether or not body must be processed. It cannot be derived from empty / non-empty parameters. Now, I see clearly that there is a lack of an additional parameter for that.

And also, body/subject must be plain (non processed as template) by default in order to not break compatibility. Template path, as its name states, must be used always as template, even if params are empty.

[avifatal]

I disagree, the template is dynamic, sometimes you have params and sometimes not.

I mean, that only developer who creates EmailInfo should specify whether or not body must be processed. It cannot be derived from empty/non-empty parameters. Now, I see clearly that there is a lack of additional parameter for that.

And also, body/subject must be plain (non processed as template) by default in order to not break compatibility. Template path, as its name states, must be used always as template, even if params are empty.

In my opinion, this is pitfally API, let's say that I have a UI to manage the templates (email text) and the params.
The content manager / employee, at runtime, sometimes decides to have params and sometimes not. Sometimes the template contains ${param} and sometimes not. it supposed to be implicitly decided by the API, if you have Map of params, it means it should be processed and if not it is not. Unless Im missing something about freemarker (this is new to me.)
Sorry for taking to much of your time.

[jreznot]

By design, FreeMaker template can be used even without parameters. It supports loops, variables, comments, etc. There is no guarantee that existing systems do not use this behaviour. Moreover, existing static templates use symbol escaping for FreeMarker control symbols in templates. That's why files from templatePath should always be processed as templates.

The content manager / employee, at runtime, sometimes decides to have params and sometimes not.

If you are planning to use it in your high level feature you can simply preprocess message body yourself. In EmailInfo we have only API, and developers should decide if they want to use template processing with body or not. If we are talking about templatePath - it is obvious from the property name, in case of body and subject it is not obvious at all.

[MeyersJeff]

I do not know the reason for this change, but would like to add a comment from a user's perspective. My application makes heavy use of emails and FreeMarker templates.

I agree with @jreznot, the developer can pre-process the template and build the email body themselves. I had to do this because my templates are stored in DB with a CUBA screen for easy editing. I submitted a request for a feature like this last year and was told it was not necessary as I could build the email body myself.

It would be nice, however, to have a templateString in addition to templatePath. templateString would hold the string contents of the FreeMarker template and would be used to build the email body. That would have saved me from writing my own custom template loader methods for FreeMarker. Someone would need to decide how to handle the case when both are populated.

The ability to process the email caption as a FreeMarker template would be nice, but for me is not necessary. Like the email body, the developer can choose to pre-process before assigning the caption to emailInfo.