OpenMRS Platform 2.3.0-beta released for testing

Hi all, I’ll be reverting the following commits as a follow up on the changes introduced due to Patient/Person domain objects being annotated.

TRUNK-5492

TRUNK-5685

TRUNK-5685

TRUNK-5492

TRUNK-5675

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.

Kind regards.

Nathan

1 Like

@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! Mark

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.

Thanks @ruhanga! Let me take a quick look…

1 Like

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

Thanks! Mark

1 Like

@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. :slight_smile:

Thanks @ruhanga, good catch… I will backport that now…

@ruhanga backport has been applied, it’s going through the build now, let me know if you notice any issues!

1 Like

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.

2 Likes

@wyclif , i personally agree with you.
My opinion was not to do reverts but rather to put in more effort to switch completely to 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

3 Likes

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.

@ruhanga , a momment before that , from this talk thread here

we are coming to a resolution of reverting this commit here

see talk discussion that led to that commit here

, So it may need reverting and also some testing

but please first follow the discusion above for more insight

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

Take care, Mark

1 Like

Thanks @mozzy for timely pointing this out,

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.

1 Like

What were the other issues? I thought we addressed them

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

1 Like

Have you done the tests, after reverting the commit ?

Yes @mozzy. :slight_smile:

1 Like

sure thats the expected behaviour.

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

1 Like