feat: add serverName variable on alert template

[kryskool]

Ref #258

[github-actions]

Test Results

  105 files  ±0    202 suites  ±0   6m 58s ⏱️ + 1m 11s
  633 tests ±0    633 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 266 runs  ±0  1 266 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 8d42d1d. ± Comparison against base commit 7723b34.

[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.