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.

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

Disallow overriding email template replacement parameters #2447

Merged
merged 2 commits into from Jul 12, 2021

Conversation

labkey-adam
Copy link
Contributor

Rationale

EmailTemplate seems to let subclasses override replacement parameters, but the behavior is inconsistent (e.g., duplicate parameters appear on the customize page). This just seems like a bad idea, so remove existing overrides and add a check to prevent others in the future. Should fix MessagesLongTest, which flagged a related change in behavior.

Related Pull Requests

Changes

  • AnnouncementDigestProvider: remove redundant override of folderName; just use folderPath
  • ReportAndDatasetChangeDigestEmailTemplate: remove redundant overrides of folderUrl and folderPath
  • Add assert that disallows adding a parameter that matches an existing parameter name for that template

Copy link
Contributor

@labkey-jeckels labkey-jeckels left a comment

Choose a reason for hiding this comment

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

Are we case-insensitive when doing the substitutions into the template? If not, we probably should make the change.

@labkey-adam
Copy link
Contributor Author

The two places that enumerate getAllReplacements() resolve names via equalsIgnoreCase(). Would be nice if these were held in a (case-insensitive) map instead of constantly enumerating, but I'm done making changes for now.

@labkey-adam
Copy link
Contributor Author

@labkey-adam labkey-adam merged commit 0dabe80 into develop Jul 12, 2021
@labkey-adam labkey-adam deleted the fb_foldName_vs_folderPath branch July 12, 2021 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants