EA-188 - Improve Java and Rest API for retrieving inpatient admission requests #233

mseaton · 2024-07-09T20:28:37Z

See: https://openmrs.atlassian.net/browse/EA-188

Note, I have tested this fairly extensively against a production database backup that has several hundred active admission requests and related disposition data from the past 10 years, and many millions of observations overall to test performance.

mogoodrich

Generally looks really good, @mseaton , thanks!

My main way of reviewing was to to make sure that all the tests cases were there... the admit test cases look pretty robust (though per my comment on the code I think we should add some comment to clarify that this is only fetching "active" requests, though maybe that implicit in the name "request")?

Are you planning on adding tests for transfer and discharge, and particularly to confirm that "most recent disposition" always wins (I'm assuming that is what we want, at least, or we can discuss?)... ie if a patient as a disposition of "Admit" during a course of a visit, but a subsequent disposition of "Discharge" (but with no admission encounter nor admission deny obs) the "Discharge" request should take precedent and the admission request should not be returned when requesting active admission requests?

api/src/main/java/org/openmrs/module/emrapi/adt/InpatientRequestSearchCriteria.java

api/src/main/resources/hql/inpatient_request_dispositions.hql

… mitigate errors related to infinite recursion when serializing rest data

…t in the visit.

mseaton · 2024-07-11T19:36:48Z

Are you planning on adding tests for transfer and discharge, and particularly to confirm that "most recent disposition" always wins (I'm assuming that is what we want, at least, or we can discuss?)... ie if a patient as a disposition of "Admit" during a course of a visit, but a subsequent disposition of "Discharge" (but with no admission encounter nor admission deny obs) the "Discharge" request should take precedent and the admission request should not be returned when requesting active admission requests?

This is a really good point, and I have added a couple of test cases for this and it indeed shows that this was not originally factored in. However, since this was adapted from the pre-existing code, this also highlights that this has also not been factored into the existing admission request functionality that has been in place for years. So the new functionality will return fewer and more correct results than the current functionality.

As a followup to this @mogoodrich . I have some new commits that also modify the pre-existing functionality, which now returns consistent results for both the old and new methods.

…ime, favoring the one with the later obsId. Add same constraint to existing awaiting admission query.

…within the visit.

mseaton · 2024-07-11T21:32:45Z

omod/src/main/java/org/openmrs/module/emrapi/web/controller/InpatientRequestController.java

+            @RequestParam(required = false, value = "dispositionLocation") List<Location> dispositionLocations,
+            @RequestParam(required = false, value = "dispositionType") List<DispositionType> dispositionTypes
+    ) {
+        RequestContext context = RestUtil.getRequestContext(request, response, Representation.REF);


@mogoodrich here is another example of someplace I am just defaulting to REF. How do we feel about this?

I guess I still think this is really the "default", not the "ref", but I don't feel super strongly.... you can ask Burke if you want? :)

That's fine. I've changed it to DEFAULT. In the DB I'm testing on, on a request that returns 302 results, the ref representation is 458 KB uncompressed, and the default representation is 8.8 MB uncompressed, which is pretty significant. Hopefully compression helps with that significantly, but generally using custom representations is likely what is called for here, especially as there is some duplication of properties (eg. patient + visit.patient)

Not to re-open this, but to be clear, I think we could decide that the "default" rep of this endpoint only includes, for example, the "ref" reference to the underlying visit. @mseaton

I'm basically basing this on this pattern here: https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/omod-1.8/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/PatientResource1_8.java#L121 ... ie just because you are asking for the default, doesn't mean it needs to give back the default of all the nested resources.

Fair. So what is your opinion on what to return for the default and/or full representations?

Full should return close to the full representation, within reason. Default would return whatever we think is a "reasonable" default ref of this resource, balancing size vs useful information. I know that's a vague answer but you are the one looking closest at what it actually returns now.

For instance, taking a look at visit:

https://github.com/openmrs/openmrs-module-webservices.rest/blob/master/omod-1.9/src/main/java/org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_9/VisitResource1_9.java

it would seem reasonable to return the DEFAULT visit rep by default (which looks like it generally only returns refs of nested resources) and the FULL visit rep if we ask for the FULL.

I haven't looked at the other properties... we might want just a REF version of the dispositionConcept since with all the names, etc, a default concept resource is likely overly verbose? Do we anticipate needing anything other that the uuid of the disposition concept?

Anyway, to you point, we probably will need a custom rep at the end of the day, so I'd just pick something that is arguably "reasonable". Hope that helps?

OK, so I changed things so that when FULL is requested, you get a FULL visit, and a DEFAULT everything else, and when DEFAULT is requested you get a DEFAULT visit and a REF everything else. Sound OK?

That seems to make sense to me.

mseaton · 2024-07-12T10:54:59Z

api/src/main/java/org/openmrs/module/emrapi/adt/AdtServiceImpl.java

+        }
+        if (dispositionTypes.contains(DispositionType.DISCHARGE)) {
+            adtEncounterTypes.add(emrApiProperties.getExitFromInpatientEncounterType());
+        }


This is kind of a question mark for me @mogoodrich . The way I coded this up, if you ask for only ADMIT requests, then only Admission Encounters will be checked as those that might render them fulfilled. Same with TRANSFER and DISCHARGE. If we think that an admit disposition followed by a transfer or discharge encounter should lead to that admit request being fulfilled, then we may want to rethink this logic here, and just add all 3 encounter types in all cases. Thoughts?

More of a business logic thing but doubtful will get a answer, but my general thought is a request is "active" if 1) it is the most recent disposition in the visit, and 2) there hasn't been any successive "ADT encounter type" (so to your direct question, I would add all three encounter types) and 3) there is no successive "deny admission" obs (if the disposition is admit)

Thoughts? Am I missing anything? At the end of the day, since a lot of the stuff is edge cases, probably best to just come up with the most succinct description/logic?

FWIW we could come up with a better name than "active", but I think is better than "fulfilled"... ie in your above example (admit disposition followed by transfer or discharge), I don't think the request has been been "fulfilled", but it is no longer active, which is what we care about there?

I would tend to agree, hence why I asked the question, but this is not what has been in place for all of these years. The existing functionality only looks at Admission Encounters. It does not concern itself with transfer or discharge encounters at all.

I suspect this isn't getting a fair amount/any use outside of PIH, so I think it's fine to change if we think it's more correct...

In the interest in getting this committed, let's ticket this separately and decide what to do, since it affects both existing and new code.

Ticketed separately here @mogoodrich : https://openmrs.atlassian.net/browse/EA-195

…requests based on patient(s) and visit(s) for compatibility with previous deprecated method.

…oller in favor of inpatient requests controller.

…perties except visit, which is default

mseaton added 4 commits June 28, 2024 16:50

In progress

eb5a4dd

Merge branch 'master' into EA-188

9ec13f1

Add initial set of unit tests

b2606aa

Additional unit tests

7adca3b

mseaton requested a review from mogoodrich July 9, 2024 20:28

Javadoc

aae9f3e

mogoodrich reviewed Jul 10, 2024

View reviewed changes

api/src/main/java/org/openmrs/module/emrapi/adt/InpatientRequestSearchCriteria.java Show resolved Hide resolved

mogoodrich reviewed Jul 10, 2024

View reviewed changes

api/src/main/resources/hql/inpatient_request_dispositions.hql Show resolved Hide resolved

mseaton added 2 commits July 11, 2024 12:42

Add initial REST controller and add some fixes for performance and to…

5727a3d

… mitigate errors related to infinite recursion when serializing rest data

Add additional constraint to ensure the disposition used is the lates…

a059657

…t in the visit.

mseaton added 4 commits July 11, 2024 16:33

Cover edge case where multiple dispositions have the exact same datet…

1e153da

…ime, favoring the one with the later obsId. Add same constraint to existing awaiting admission query.

For consistency with other controllers, don't limit to v1

066a948

Only consider requests fullfilled if adt encounter occurs after them …

69d555b

…within the visit.

Formatting

2165c36

mseaton commented Jul 11, 2024

View reviewed changes

mseaton marked this pull request as ready for review July 11, 2024 21:44

mseaton requested review from dkayiwa, ibacher and mogoodrich July 11, 2024 21:45

Set the default representation to DEFAULT

bea208d

mseaton commented Jul 12, 2024

View reviewed changes

mseaton added 3 commits July 12, 2024 06:57

Merge branch 'master' into EA-188

a6623a2

Re-organize and add additional unit tests, add ability to search for …

d58090b

…requests based on patient(s) and visit(s) for compatibility with previous deprecated method.

Remove specific inpatient request methods from inpatient visits contr…

fd9edf8

…oller in favor of inpatient requests controller.

mogoodrich approved these changes Jul 12, 2024

View reviewed changes

mseaton added 2 commits July 12, 2024 10:20

Change default representation of inpatient request to ref for all pro…

6dae3e2

…perties except visit, which is default

Change default representation of inpatient request to ref for all pro…

e2d0123

…perties except visit, which is default

mseaton merged commit 49513d5 into master Jul 12, 2024
1 check passed

mseaton deleted the EA-188 branch July 12, 2024 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EA-188 - Improve Java and Rest API for retrieving inpatient admission requests #233

EA-188 - Improve Java and Rest API for retrieving inpatient admission requests #233

mseaton commented Jul 9, 2024 •

edited

Loading

mogoodrich left a comment

mseaton commented Jul 11, 2024 •

edited

Loading

mseaton Jul 11, 2024

mogoodrich Jul 11, 2024

mseaton Jul 12, 2024

mogoodrich Jul 12, 2024

mogoodrich Jul 12, 2024 •

edited

Loading

mseaton Jul 12, 2024

mogoodrich Jul 12, 2024 •

edited

Loading

mseaton Jul 12, 2024

ibacher Jul 12, 2024

mseaton Jul 12, 2024

mogoodrich Jul 12, 2024

mseaton Jul 12, 2024

mogoodrich Jul 12, 2024

mseaton Jul 12, 2024

mseaton Jul 12, 2024

EA-188 - Improve Java and Rest API for retrieving inpatient admission requests #233

EA-188 - Improve Java and Rest API for retrieving inpatient admission requests #233

Conversation

mseaton commented Jul 9, 2024 • edited Loading

mogoodrich left a comment

Choose a reason for hiding this comment

mseaton commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseaton commented Jul 9, 2024 •

edited

Loading

mseaton commented Jul 11, 2024 •

edited

Loading

mogoodrich Jul 12, 2024 •

edited

Loading

mogoodrich Jul 12, 2024 •

edited

Loading