Refactor email template handling, #43424

[labkey-adam]

Rationale

Issue 43424: Refactor EmailTemplate, etc. describes improvements we should make to EmailTemplate and related classes.

Changes

• Reduce to a single EmailTemplate constructor that takes the critical parameters: name, description, subject, body, content type, and scope. Every email template implementation must make explicit choices for these and none will be forgotten.
• Remove setters for immutable parameters: name, description, content type, and scope.
• Keep setters for subject, body, sender name, and reply-to email since these can be overwritten by customizing the template.
• Simplify replacement parameter handling:
  • Split "standard" (those present for every email template) vs. "custom" parameters and display them in separate groupings
  • Gather custom parameters via abstract addCustomReplacements() method, simplifying constructors and eliminating bookkeeping in all the subclasses
  • Introduce simplified add() method that takes a lambda to eliminate replacement parameter definition boilerplate code
• Finish removing priority

[labkey-jeckels]

An optional idea - make a concrete subclass that takes a Function<Container, Type> as a constructor argument, which would let you pass in a lamba for a slightly leaner syntax than having to do the anonymous inner classes

[labkey-adam]

Good idea... I like it. But taking it one step further: implementing a new add() method in Replacements that takes the constructor arguments plus lambda is simpler and reduces boilerplate further.

[labkey-adam]

@labkey-jeckels you can check out these changes applied to platform and let me know if you have feedback. Other repos coming shortly, but I don't think I need re-review on those.

[labkey-adam]

One thing I'm still debating: new add() method requires ContentType while ReplacementParam has a constructor that defaults to ContentType.Plain. I'm in a "must specify all properties explicitly" mood right now, but maybe it's pedantic in this case.

[labkey-jeckels]

This looks good to me.

In the future, we might think about having addPlain() and addHtml(), perhaps with the latter somehow generating HTMLString instead of maybe-but-not-always HTML encoded Strings, but this seems like a good stopping point for this round.

[labkey-jeckels]

This looks good to me.

In the future, we might think about having addPlain() and addHtml(), perhaps with the latter somehow generating HTMLString instead of maybe-but-not-always HTML encoded Strings, but this seems like a good stopping point for this round.

[labkey-adam]

Thanks. I've spent several hours staring at all the EmailTemplate code and still don't understand if/how the HTML encoding works. Switching to HtmlString would certainly help. But I also agree that now's not the time.