Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

cioan
Copy link
Contributor

@cioan cioan commented Jan 16, 2024

@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
Copy link
Collaborator

mseaton commented Jan 16, 2024

@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
Copy link
Contributor Author

cioan commented Jan 17, 2024

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

@ibacher
Copy link
Member

ibacher commented Jan 17, 2024

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.*" })

Copy link
Collaborator

@mseaton mseaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments in the PR @cioan . A few other things:

  • You will need to update the README as part of this PR to reflect these additions. Make sure you indicate that these are supported for Queue version 2.1.0 and higher.
  • Will will need to update the release notes as a part of this PR.

@cioan
Copy link
Contributor Author

cioan commented Jan 18, 2024

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

@mseaton mseaton requested a review from ibacher January 18, 2024 15:12
@mseaton
Copy link
Collaborator

mseaton commented Jan 18, 2024

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
Copy link
Collaborator

mseaton commented Jan 18, 2024

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
Copy link
Member

ibacher commented Jan 18, 2024

@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
Copy link
Contributor Author

cioan commented Jan 18, 2024

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

@ibacher
Copy link
Member

ibacher commented Jan 18, 2024

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
Copy link
Collaborator

mseaton commented Jan 18, 2024

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.

@mks-d mks-d changed the title O3-2446: load queue.status_concept_set field O3-2446: 'queues' domain to support loading Status and Priority entries. Jan 18, 2024
@ibacher
Copy link
Member

ibacher commented Jan 18, 2024

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

@mseaton mseaton merged commit 935aaa2 into mekomsolutions:master Jan 18, 2024
2 checks passed
@cioan
Copy link
Contributor Author

cioan commented Jan 18, 2024

Thanks @mseaton and @ibacher !

@cioan cioan deleted the O3-2446 branch January 18, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants