fix ehr tests that are enabling notifications

[ankurjuneja]

Rationale

See related

Related Pull Requests

• fix ehr tests that are enabling notifications #185

[ankurjuneja]

tests are passing now https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_EhrSqlserver?branch=fixEHRNotificationsTest&buildTypeTab=overview&mode=builds

[bbimber]

@ankurjuneja: do you have any more information on the upstream changes that cause this to be needed? if there is some fundamental change in how JSON is sent or parsed on the client related to the tomcat 10 migrations, like implied in #185, that could cause lots of unanticipated issues that might not always be completed covered on tests.

[labkey-jeckels]

Digging into this, the serialization changed when we flipped over to Tomcat 10 and Jakarta Mail in develop on February 1. That switched the package name for javax.servlet to jakarta.servlet and javax.mail to jakarta.mail.

LDKController has been putting a javax.mail.internet.InternetAddress into the API response as the replyEmail property. See line 129. JSONObject does an automatic toString() on java and javax classes.

https://github.com/stleary/JSON-java/blob/master/src/main/java/org/json/JSONObject.java#L2667

When it switched to the jakarta namespace, it started serializing it as a full JSON object, which didn't line up with what the JS code is expecting.

I think the right thing here is to put a String in the API response. I pushed a commit to do this and get tests passing again.

We should separately consider if this will impact other serialization. I hope we're not sending back any Servlet objects. Address, as used here, seems like the most likely thing to show up in other API responses. @labkey-adam @labkey-matthewb

[bbimber]

OK, thanks for the investigation @labkey-jeckels. That does sound this like this is probably scoped to a more narrow case, which is good.

I think the server-side change it appears you checked in earlier today is a better solution than updating the client-side. I doubt anything is using GetNotificationsAction besides this UI, but I think it's better to keep the JSON consistent.