O3-2453 - remove where conditions from property JPA annotations #42

mseaton · 2023-09-27T12:45:35Z

No description provided.

mseaton · 2023-09-27T12:48:03Z

api/src/main/java/org/openmrs/module/queue/model/Queue.java

+		if (queueEntries != null) {
+			for (QueueEntry e : queueEntries) {
+				if (BooleanUtils.isNotTrue(e.getVoided())) {
+					if (!e.getStartedAt().after(now) && e.getEndedAt() == null) {


Note: the intention here was to exactly replicate the pre-existing logic. I don't know if this is what we want or not (eg. we consider the possibility of "future" queue entries, and don't want entries that start in the future to be considered active, but we don't consider the possibility of queue entries that have already started but whose endDate is in the future, but rather anything with an end date is inactive. We can discuss the correctness of this logic, and happy to change this to whatever we come up with, but for the initial PR, just replicating existing behavior.

Yeah, there's maybe some discussion to be had. I'm not exactly sure what the concrete use-case for future-dated entries is, but it seems like it's getting out of the realm of a "list of patients waiting for a resource".

mseaton · 2023-09-27T12:49:07Z

api/src/test/java/org/openmrs/module/queue/tasks/AutoCloseVisitQueueEntryTaskTest.java

@@ -7,11 +7,12 @@
 * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS


Note: All the changes in this file are just due to the fact that I had put this in the wrong package, and I noticed this while creating a unit test for the Queue class, so this just moves the task unit test from the "model" to the "tasks" package.

mseaton · 2023-09-27T12:50:29Z

omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryCountSubResource.java

@@ -52,7 +52,7 @@ public void setParent(QueueEntryCount queueEntryCount, Queue queue) {
 	@Override
 	public PageableResult doGetAll(Queue queue, RequestContext requestContext) throws ResponseException {
 		return new GenericSingleObjectResult(Arrays.asList(new PropValue("queueName", queue.getName()),
-		    new PropValue("queueEntriesCount", queue.getQueueEntries().size())));
+		    new PropValue("queueEntriesCount", queue.getActiveQueueEntries().size())));


This change is needed for this to replicate the existing functionality, but is this what we want? Should the "getAll" resource limit itself to only active queue entries? Or should getAll really ...get all. And then we can limit this to only active entries by passing in a filter or search parameter.

This is a weird resource. Just returns the counts. A response like:

{ "QueueName": "Triage Queue", "count": "36" }

The idea is to get the number of patients in a queue. Mainly used in the UI to display totals for queues. We don't about active or voided queueEntries.

Hmmm... It doesn't seem like this resource is going to perform in an ideal way. I'd prefer it if we had one endpoint that returned "the current queue", i.e., queue name, all active entries, and counts. Then we could use the custom view code if someone really want to limit things down to just requesting, e.g., the name and count. This just encourages a pattern of access where we're making a lot of REST requests, which is bad for performance.

@mseaton In the context of a queue, I think it's fine if the default is to return active entries only. It seems reasonable to have a query property that be used to also request the queue with inactive entries, but I'm not clear on the use-case for that. This may be a little different from other domains, but I tend to think of inactive queue entries as similar to retired metadata.

I understand the primary use case. A potentially more comprehensive endpoint might return an object that contains data that is a bit more complete and includes counts as the service/api deals with them, so it's clear. For example, if our "getActiveQueueEntries" filters out voided, started-in-future, and ended entries (which it does), then we could ensure that all of these are represented in this endpoint comprehensively like:

{ "QueueName": "Triage Queue", "totalCount": "63", "activeCount": "36", "futureCount": "5", "voidedCount": "2", "endedCount": "20" }

Sorry, I made this comment before I saw @ibacher response. Must have been typing at the same time. You can disregard my comment, especially if we get rid of this endpoint as Ian seems to be suggesting :)

This may be a little different from other domains, but I tend to think of inactive queue entries as similar to retired metadata

I understand your point and I'm fine if the endpoint returns active entries by default. I just wanted to bring it up to make sure. As you indicate, we can always add support for query parameters like "includeEnded=true", "includeVoided=true", and "includeFuture=true" for those use cases if/when they arise.

Yeah, haven't dug into it in much detail but the use cases for get all "ended" queue members or a count of all of them seems relatively small. If I'm understand it correctly the "ended" queue entries will just continue to grow and grow indefinitely and the general need when asking for "all" would be "get me all currently in the queue". There might be a case to say, "get me all cases when patient X was in this queue in the past month" but "getAllEntires" seems about as useful as "getAllObs". (Again, If I'm understanding this all correctly).

mseaton · 2023-09-27T12:50:42Z

omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntrySubResource.java

@@ -53,7 +53,7 @@ public void setParent(QueueEntry queueEntry, Queue queue) {

 	@Override
 	public PageableResult doGetAll(Queue queue, RequestContext requestContext) throws ResponseException {
-		Collection<QueueEntry> queueEntries = queue.getQueueEntries();
+		Collection<QueueEntry> queueEntries = queue.getActiveQueueEntries();


Same comment here as above. This change is needed for this to replicate the existing functionality, but is this what we want? Should the "getAll" resource limit itself to only active queue entries? Or should getAll really ...get all. And then we can limit this to only active entries by passing in a filter or search parameter.

Do we have a use case for that already?
Yeah getAll should really get all queue entries and probably add more search parameters through search handlers.

I don't have an explicit use case yet. It was more just a matter of wanting to ensure our API was flexible enough to handle these use cases if/when they come up and also that it makes intuitive sense and matches what users would expect. Once we establish that this endpoint only returns active queue entries, if a use case does come up where we want all queue entries regardless of active or not, how would we handle that? Would we have to pass in a parameter saying "includeInactive=true" for example? Not the end of the world, just putting it out there for consideration as to how we'd want/expect our REST apis to work, and if there are existing conventions in OpenMRS that we honor those.

corneliouzbett

Some suggestions. Otherwise all good.

api/src/main/java/org/openmrs/module/queue/model/Queue.java

Co-authored-by: Kipchumba BETT <[email protected]>

O3-2453 - remove where conditions from property JPA annotations

8cc7776

mseaton requested review from corneliouzbett, CynthiaKamau and ibacher September 27, 2023 12:45

mseaton commented Sep 27, 2023

View reviewed changes

corneliouzbett reviewed Sep 27, 2023

View reviewed changes

api/src/main/java/org/openmrs/module/queue/model/Queue.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/openmrs/module/queue/model/Queue.java

91405f6

Co-authored-by: Kipchumba BETT <[email protected]>

ibacher approved these changes Sep 27, 2023

View reviewed changes

corneliouzbett approved these changes Sep 27, 2023

View reviewed changes

corneliouzbett merged commit c8896cc into main Sep 27, 2023
1 check passed

corneliouzbett deleted the O3-2453 branch September 27, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-2453 - remove where conditions from property JPA annotations #42

O3-2453 - remove where conditions from property JPA annotations #42

mseaton commented Sep 27, 2023

mseaton Sep 27, 2023

ibacher Sep 27, 2023

mseaton Sep 27, 2023

mseaton Sep 27, 2023

corneliouzbett Sep 27, 2023 •

edited

Loading

ibacher Sep 27, 2023

mseaton Sep 27, 2023

mseaton Sep 27, 2023

mseaton Sep 27, 2023

mogoodrich Sep 28, 2023

mseaton Sep 27, 2023

corneliouzbett Sep 27, 2023

mseaton Sep 27, 2023

corneliouzbett left a comment

		@@ -7,11 +7,12 @@
		* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS

O3-2453 - remove where conditions from property JPA annotations #42

O3-2453 - remove where conditions from property JPA annotations #42

Conversation

mseaton commented Sep 27, 2023

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

corneliouzbett Sep 27, 2023 • 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

corneliouzbett left a comment

Choose a reason for hiding this comment

corneliouzbett Sep 27, 2023 •

edited

Loading