mekomsolutions / openmrs-module-initializer

The OpenMRS Initializer module is an API-only module that processes the content of the configuration folder when it is found inside OpenMRS' application data directory.
MIT License
23 stars 79 forks source link

O3-2446: 'queues' domain to support loading Status and Priority entries. #260

Closed cioan closed 9 months ago

cioan commented 9 months ago

@mseaton , I just added the ability to load the new queue.status_concept_set field that you added as part of the O3-2446 ticket. Thanks!

mseaton commented 9 months ago

@cioan - the issue is that this will likely fail for anyone not running the latest snapshot of queue. It assumes that anyone loading in from the queue domain is running the latest version of the module, which is likely not true.

The most straightforward thing to do would be to use reflection to set properties if they are found in the header (the implication being that if they are being set in the header, then the implementation knows they are running a version that supports them), and so you'd set those properties via reflection if found.

However, it may be that there are alternative solutions that leverage OpenmrsProfile and LineLoaders that that would add conditional line processing based on module version, but I don't see any clear existing examples of this at first glance. @mks-d or @ibacher may be able to advise.

It's possible also that you can just bump up the version of queue in the dependencies (once we release it), and all will be fine as long those lines are only hit if those headers are encountered.

cioan commented 9 months ago

Thanks @mseaton ! I just updated the dependencies to depend on the released version of the queue module.

ibacher commented 9 months ago

I don't see any clear existing examples of this at first glance.

I think we've been trying not to have the endless series of configurations for various versions of things. If we wanted that though, it would be possible to do two LineProcessors like this:

@OpenmrsProfile(modules = { "queue:* - 1.*" })
@OpenmrsProfile(modules = { "queue:2.*" })
cioan commented 9 months ago

Thanks @mseaton ! I have addressed all your comments above. I also add @ibacher and @mks-d to the reviewers list. Thanks!

mseaton commented 9 months ago

I have addressed all your comments above

@cioan - please see comment above about adding to release notes and readme. Also, I didn't see @ibacher added as a reviewer, so I added him.

mseaton commented 9 months ago

I think we've been trying not to have the endless series of configurations for various versions of things

In this case, I don't think we need to do anything, as the lines that hit these methods are only executed if the headers are configured for them. Would that match your understanding @cioan and @ibacher ?

ibacher commented 9 months ago

@mseaton Yes, I think this should work as long as the documentation is clear. @cioan We need to update this README file with the new options.

Looking at things, it looks like the queue support is only part of the 2.7.0-SNAPSHOT? In that case, it's much easier to just say that the queues domain requires version 2.1.0. This should have a very minimal effect on anyone using the existing domain, as that's likely no one currently.

cioan commented 9 months ago

Thanks @mseaton and @ibacher ! I have updated the queues readme file. @mseaton , where is the release notes file that needs to be updated?

ibacher commented 9 months ago

Usually, it's in the main README. In this case, since we haven't done a release since we've added the queue module, I think it's already covered.

mseaton commented 9 months ago

Usually, it's in the main README. In this case, since we haven't done a release since we've added the queue module, I think it's already covered

Agreed. @cioan no need to update the release notes, since support for queues has actually not been in a release yet, and what we put in for 2.7.0 covers queues generally.

ibacher commented 9 months ago

@mseaton If you're happy with this, do you mind approving? I think this is ready to merge.

cioan commented 9 months ago

Thanks @mseaton and @ibacher !