The last commit in the above sequence probably was addressing an issue that arose due to the Person/Patient domain objects being annotated. @bistenes, @mogoodrich , would reverting this, affect progress of developments that led to this? Thank you.
@ruhanga I don’t think TRUNK-5675 is related to the Person/Patient domain object change and we wouldn’t want to revert it… if I’m missing something and reverting the Person/Patient changes without reverting TRUNK-5675 causes issues, lmk…
Also, I don’t know if there’s any harm in keeping TRUNK-5685? Seems nice to keep that protected so subclasses can access it?
Thanks @mogoodrich. Yes, reverting the annotation changes on Patient/Person affects the commits on accents (TRUNK-5675), I get failing tests related to the commit on this. Also the reverts on Patient/Person annotations causes failing tests related to TRUNK-5685 for the protected method.
A safe revert from annotations on Patient/Person domain object currently requires the above sequence of commits get reverted where all tests pass. Perhaps with a few tweaks we could keep the commit on TRUNK-5675.
@ruhanga I was doing some testing with the 2.3.x and couldn’t reproduce the issues you were seeing, but then I just tested with master and was getting some errors but ran out of time tonight, but will need to look more into it…
Reverting TRUNK-5685 isn’t a problem, but we will to keep TRUNK-5675…
That being said, if you want to go ahead and revert it for now, feel free, just please reopen the ticket and flag me here so I can look into it further.
@mogoodrich, after building the branch 2.3.x with the reverted changes except for TRUNK-5675 in a windows and macOS environments, some tests related to TRUNK-5675 failed on windows. There is a fix that was not backported to the 2.3.x branch. I hope I can release the platform as soon as it is backported. Otherwise there are no other blockers and we can keep TRUNK-5675. Thank you.
I was going crazy about what annotations have to do with accents in strings.
I personally strongly believe that we should use annotations and start to move away from xml because xml masks bugs that would otherwise be exposed by annotations since they are part of the java language, also a lot of advancements and libraries of the language are implemented with annotations.
I had thought that a switch to annotations would be obvious. But it turned out not to be so, basing on the side effects that we have experienced, and probably more would show up. Therefore, instead of annotations delaying the 2.3.0 release, i would bump them to 2.4.0 or even 3.0.0
Thank you @mogoodrich, @dkayiwa, @wyclif, @mozzy and @all for your thoughts and support. For now I’m going ahead to release the platform without the annotations for the purpose of consistency and not having to postponed this release.
However annotations on domain objects are more contemporary and convenient for use. This could be part of our GSoC projects and future releases.
I agree with this… annotations are certainly the more “modern” way for doing things so we should seriously consider upgrading, but we should do it at a point when we are confident to have the resources for a details regression test, and a strategy for avoiding things like the “unused_tables”.
I’ve gone through the thread you shared but unfortunately I’m running out of bandwidth. I hope to get all this tested after reverting locally before merging this though.
@wyclif, the above revert issue is annotations unrelated.
@mozzy, I’ve been doing a couple of tests, and reverting this is effective when using the SDK. By this I mean, installing a fresh instance of the platform with the current version of the SDK adds the admin user name to the admin systemID as was fixed. However this is not the case when running on an enterprise tomcat server. I would think the later is not a big issue.
thats only possible when running with the latest SDK , Docker or the standalone , if am not mistaken.
So i think ideally thats the expected behaviour
cc @dkayiwa