feat: add serverName variable on alert template

[kryskool]

Ref #258

[github-actions]

Test Results

  105 files  ±0    202 suites  ±0   7m 0s ⏱️ + 1m 9s
  633 tests ±0    633 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 266 runs  ±0  1 266 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 9a36030. ± Comparison against base commit 0f0d8a4.

♻️ This comment has been updated with latest results.

[tonygermano]

This seems like a reasonable change.

For other reviewers, there are also variables added here

engine/client/src/com/mirth/connect/client/ui/alert/DefaultAlertTriggerPane.java

Lines 156 to 170 in 7723b34

	public List<String> getVariables() {
	List<String> variables = new ArrayList<String>();
	variables.add("systemTime");
	variables.add("error");
	variables.add("errorMessage");
	variables.add("errorType");
	variables.add("channelId");
	variables.add("channelName");
	variables.add("connectorName");
	variables.add("connectorType");
	variables.add("messageId");
	return variables;
	}

and context here

engine/server/src/com/mirth/connect/server/alert/DefaultAlertWorker.java

Lines 139 to 149 in 7723b34

	context.put("systemTime", String.valueOf(errorEvent.getDateTime()));
	context.put("channelId", channelId);
	context.put("channelName", channelName);
	context.put("connectorName", errorEvent.getConnectorName());
	context.put("connectorType", errorEvent.getConnectorType());
	context.put("error", fullErrorMessage);
	context.put("errorMessage", (errorEvent.getThrowable() == null) ? "No exception message." : errorEvent.getThrowable().getMessage());
	context.put("errorType", errorEvent.getType());
	if (errorEvent.getMessageId() != null) {
	context.put("messageId", errorEvent.getMessageId());
	}

The changes in the PR appear to be in the correct place since they are added to the location for the "global" values, whereas the sections above look to be for trigger specific values.

However, care seems to be taken to not add null values to the context for variables which can possibly be null. The first time I tested the changes in this PR it did not replace the velocity token because I did not have a server name set, but it was not immediately clear that was the reason. I could see people who are unaware that the setting exists thinking that this should report the server's hostname. I wonder what other's thoughts are on taking the approach of the errorMessage variable above, and using a "Server Name not configured" value when the server name is null or empty.

[gibson9583]

My preference would be to leave the token behind when server isn't configure, but we'd be deviating from the existing pattern only found in Alerts. I'm ok with either path with the rationale being to keep consistency in alerts. I think there is justification for either, but then it'd be weird errorMessage behaves one way.

[kryskool]

Hi @tonygermano

Thanks for the review, i can check if value is null or empty and replace with text "ServerName not defined" if it's a reasonable fix ?

[tonygermano]

Hi @tonygermano

Thanks for the review, i can check if value is null or empty and replace with text "ServerName not defined" if it's a reasonable fix ?

I'm agreeable to that, but I was trying to get consensus from the other maintainers if that's the direction we want to go or if it should stay as you originally submitted it. What is your opinion?

[kpalang]

Hi @tonygermano
Thanks for the review, i can check if value is null or empty and replace with text "ServerName not defined" if it's a reasonable fix ?

I'm agreeable to that, but I was trying to get consensus from the other maintainers if that's the direction we want to go or if it should stay as you originally submitted it. What is your opinion?

I'm leaning on keeping the variable empty if no server name is set. "Empty" is already a good default, and it gives a good indicator to set one if none are set.

I could see people who are unaware that the setting exists thinking that this should report the server's hostname.

I'd like to think hostname is an industry specific term vs a "server name", but I can also see how it could be mixed up. I would leave it up to the user to know the difference.

[tonygermano]

So, I just realized when seeing the results of the co-pilot review Sean kicked off two days ago that this is actually a partial duplicate of #47 , which adds serverName and environmentName. It couldn't be merged at the time that it was opened because it required at least java 9, and we had not upgraded from java 8 to 17 yet.

In regard to the question I had asked above, I verified that the internal value can't actually be an empty string, only null. I tested this by

• starting a new instance
• setting values for both fields in the ui and saving
• checking the new values were being used
• clearing the values in the ui and saving
• verifying the internal values were set back to null rather than an empty string

Attempting to use null values in the template results in the token remaining in the output and not being replaced rather than showing an empty value.

PR #47 already addresses this by inserting an empty string into the alert velocity context for serverName or environmentName if either of them are null. As far as I can tell, that seems to be an acceptable solution for everyone who has already commented on this PR.

I'm going to merge this one as it is since most of the discussion has happened here, and then we'll get the other one applied on top of it.