Repository navigation
Conversation
0f91658 to
fd5d84f
Compare
|
You can tell how often I use git for these situations. Need help getting just my commit signed and pushed. |
fd5d84f to
b0684db
Compare
7ebe149 to
4302644
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds two new Velocity template variables (serverName, environmentName) for alert templates, exposing values from server settings and listing them in the client alert editor UI.
Changes:
- Server: Populate alert template context with
serverNameandenvironmentNamefromServerSettings. - Client: Add
serverNameandenvironmentNameto the alert template variable list in the alert editor UI.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/src/com/mirth/connect/server/alert/Alert.java |
Adds server settings values into the Velocity context for alert template rendering. |
client/src/com/mirth/connect/client/ui/alert/DefaultAlertEditPanel.java |
Adds the new variables to the UI’s template variable list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4302644 to
4e038df
Compare
tonygermano
left a comment
There was a problem hiding this comment.
I've rebased this and resolved the merge conflicts since #258 was merged. It works, but I'm reluctant to approve at this time.
The currently used ConfigurationController.getInstance().getServerName() executes quickly because the server name is cached by the DefaultConfigurationController.
ConfigurationController.getInstance().getServerSettings(), however, is a much more expensive call, as it makes multiple calls to the database each time.
The expensive call has the benefit of returning up-to-date information, even if the settings were changed by a different server instance connected to the same database. I wonder if we should add a separate getCachedServerSettings method on ConfigurationController that this would call instead?
I have not done any benchmarking. I'll defer to others if consensus is that these extra db calls should not be considered a problem.
Issue: nextgenhealthcare/connect#6261 Original-pr: nextgenhealthcare/connect#6262 Signed-off-by: Richard Ogin <rogin@users.noreply.github.com>
4e038df to
6f0dcf7
Compare
pacmano1
left a comment
There was a problem hiding this comment.
On the question from March of whether the extra DB calls matter, I measured on engines built from this head. On PostgreSQL each alert now runs three configuration queries, each in its own read-only transaction. getServerSettings() averaged about 3.7 ms per call there (PostgreSQL in a container on the same host, so no real network in that number) and 1 to 1.5 ms on Derby, against microseconds for the cached getServerName() that main uses.
I'd rather not pay that on every alert. Can the environment name be cached the way the server name is, with createContext() reading both from the cache?
The description still says Java 9+ and mentions deleting KeyEncryptorTest, and serverName came in with #259, so it could use an update.
Co-authored with Claude Code.
Adds "serverName" and "environmentName" to the alert template variable list.
Requires Java 9+.
Fixes NextGen request #6261.
Previous PR is #6262.
Slight tangent: I deleted
KeyEncryptorTestto get over / ignore its java 9+ error that I don't care to investigate as I believe it's unrelated to this code change:error: package com.sun.crypto.provider is not visible.Assuming we have documentation, it should be updated as two additional variables will display in the Alert Template variable list.
Test procedure:
throws.error.xml.txt
Receiver.xml.txt
Foo.alert.xml.txt