From 963e04779fbf227092cd88761f0d776d8a4aaf7f Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Sun, 30 Aug 2026 21:41:23 +0800 Subject: [PATCH 1/5] O3-5770: Report queue entry metrics per queue, and for open waits --- .../module/queue/utils/QueueUtils.java | 61 +++++++ .../module/queue/utils/QueueUtilsTest.java | 68 ++++++++ .../web/QueueEntryMetricRestController.java | 118 ++++++++++++- .../QueueEntryMetricRestControllerTest.java | 161 +++++++++++++++++- 4 files changed, 402 insertions(+), 6 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java index d8ba3d44..43ec4feb 100644 --- a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java +++ b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java @@ -79,6 +79,67 @@ public static Double computeAverageWaitTimeInMinutes(List queueEntri return null; } + /** + * Measures those still waiting, unlike {@link #computeAverageWaitTimeInMinutes(List)}, which + * averages waits that have already finished + * + * @param queueEntries the QueueEntries to check + * @param asOf the point in time to measure the open durations against + * @return the average duration, in minutes, between startedAt and asOf for entries that have a + * startedAt and no endedAt, or null if there are no such entries + */ + public static Double computeAverageOpenWaitTimeInMinutes(List queueEntries, Date asOf) { + if (queueEntries != null) { + double totalWaitTime = 0.0; + int numEntries = 0; + for (QueueEntry e : queueEntries) { + Long waitTime = computeOpenWaitTimeInMinutes(e, asOf); + if (waitTime != null) { + totalWaitTime += waitTime; + numEntries++; + } + } + // Returning 0.0 here would be indistinguishable from a genuine zero-minute wait + if (numEntries > 0) { + return totalWaitTime / numEntries; + } + } + return null; + } + + /** + * @param queueEntry the QueueEntry to measure + * @param asOf the point in time to measure the open duration against + * @return how long the entry has been waiting, in minutes, or null if it has no startedAt or has + * already ended + */ + public static Long computeOpenWaitTimeInMinutes(QueueEntry queueEntry, Date asOf) { + if (queueEntry == null || asOf == null || queueEntry.getStartedAt() == null || queueEntry.getEndedAt() != null) { + return null; + } + // Measured between instants rather than between local date times, so that a wait spanning a + // daylight saving change reports the time that actually elapsed + return Duration.between(queueEntry.getStartedAt().toInstant(), asOf.toInstant()).toMinutes(); + } + + /** + * @param queueEntries the QueueEntries to check + * @return the entry that has a startedAt, has no endedAt, and started earliest, or null if there is + * no such entry + */ + public static QueueEntry findLongestOpenWait(List queueEntries) { + QueueEntry longestWaiting = null; + if (queueEntries != null) { + for (QueueEntry e : queueEntries) { + if (e.getStartedAt() != null && e.getEndedAt() == null + && (longestWaiting == null || e.getStartedAt().before(longestWaiting.getStartedAt()))) { + longestWaiting = e; + } + } + } + return longestWaiting; + } + /** * @param startDate1, endDate1 - the start and end date of one timeframe * @param startDate2, endDate2 - the start and end date of second timeframe diff --git a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java index dc06d9fd..a37debba 100644 --- a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java +++ b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java @@ -13,9 +13,11 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; +import java.time.Instant; import java.util.Arrays; import java.util.Date; import java.util.List; +import java.util.TimeZone; import org.junit.Test; import org.openmrs.module.queue.model.QueueEntry; @@ -77,6 +79,72 @@ public void shouldComputeAverageWaitTimeInMinutes() { assertThat(QueueUtils.computeAverageWaitTimeInMinutes(null), is(nullValue())); } + @Test + public void shouldComputeAverageOpenWaitTimeInMinutes() { + // Measured against the given instant rather than the end of the entry, so entries that have + // not ended are exactly the ones that count. Waits of 48 and 24 hours average out to 36. + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(entry(AUG_1, NULL), entry(AUG_2, NULL)), AUG_3), + is(2160.0)); + + // An entry that has already ended is not still waiting, whatever its duration was + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(entry(AUG_1, NULL), entry(AUG_1, AUG_2)), AUG_2), + is(1440.0)); + + // Entries without a start cannot be measured + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(entry(NULL, NULL), entry(AUG_1, NULL)), AUG_2), + is(1440.0)); + + // Test that there is no average to report rather than dividing by zero and returning NaN + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(entry(AUG_1, AUG_2)), AUG_3), is(nullValue())); + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(), AUG_3), is(nullValue())); + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(null, AUG_3), is(nullValue())); + assertThat(QueueUtils.computeAverageOpenWaitTimeInMinutes(entries(entry(AUG_1, NULL)), NULL), is(nullValue())); + } + + @Test + public void shouldFindLongestOpenWait() { + QueueEntry waitingSinceAug1 = entry(AUG_1, NULL); + QueueEntry waitingSinceAug2 = entry(AUG_2, NULL); + QueueEntry endedAfterTwoDays = entry(AUG_1, AUG_3); + + // The entry that has been waiting longest is the one that started earliest + assertThat(QueueUtils.findLongestOpenWait(entries(waitingSinceAug2, waitingSinceAug1)), is(waitingSinceAug1)); + + // An entry that has ended is not waiting at all, however long it ran for + assertThat(QueueUtils.findLongestOpenWait(entries(endedAfterTwoDays, waitingSinceAug2)), is(waitingSinceAug2)); + + assertThat(QueueUtils.findLongestOpenWait(entries(endedAfterTwoDays)), is(nullValue())); + assertThat(QueueUtils.findLongestOpenWait(entries(entry(NULL, NULL))), is(nullValue())); + assertThat(QueueUtils.findLongestOpenWait(entries()), is(nullValue())); + assertThat(QueueUtils.findLongestOpenWait(null), is(nullValue())); + } + + @Test + public void shouldComputeOpenWaitTimeInMinutes() { + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(AUG_1, NULL), AUG_2), is(1440L)); + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(AUG_1, AUG_2), AUG_3), is(nullValue())); + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(NULL, NULL), AUG_2), is(nullValue())); + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(null, AUG_2), is(nullValue())); + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(AUG_1, NULL), NULL), is(nullValue())); + } + + @Test + public void shouldMeasureAnOpenWaitAcrossADaylightSavingChange() { + TimeZone originalTimeZone = TimeZone.getDefault(); + try { + TimeZone.setDefault(TimeZone.getTimeZone("Europe/London")); + // British clocks go forward an hour at 01:00 UTC on this date, so an hour of waiting looks + // like two if it is measured between local date times rather than between instants + Date startedAt = Date.from(Instant.parse("2023-03-26T00:30:00Z")); + Date asOf = Date.from(Instant.parse("2023-03-26T01:30:00Z")); + + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(startedAt, NULL), asOf), is(60L)); + } + finally { + TimeZone.setDefault(originalTimeZone); + } + } + private List entries(QueueEntry... queueEntries) { return Arrays.asList(queueEntries); } diff --git a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java index 7267b2db..9e138509 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java @@ -13,16 +13,24 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Date; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import org.openmrs.Concept; import org.openmrs.module.queue.api.QueueServicesWrapper; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.api.search.QueueSearchCriteria; +import org.openmrs.module.queue.model.Queue; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.module.queue.utils.QueueUtils; import org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser; import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.ConversionUtil; import org.openmrs.module.webservices.rest.web.RestConstants; +import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation; +import org.openmrs.module.webservices.rest.web.representation.Representation; import org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; @@ -43,6 +51,22 @@ public class QueueEntryMetricRestController extends BaseRestController { public static final String AVERAGE_WAIT_TIME = "averageWaitTime"; + public static final String AVERAGE_OPEN_WAIT_TIME = "averageOpenWaitTime"; + + public static final String LONGEST_OPEN_WAIT = "longestOpenWait"; + + public static final String COUNTS_BY_STATUS = "countsByStatus"; + + public static final String GROUP_BY = "groupBy"; + + public static final String QUEUE = "queue"; + + public static final String QUEUES = "queues"; + + // Narrower than REF, which for a queue entry also carries the queue, status, visit and priority, + // each a lazy load, for every queue reported on + private static final String LONGEST_OPEN_WAIT_REP = "uuid,display,startedAt,patient:(uuid,display)"; + private final QueueEntrySearchCriteriaParser searchCriteriaParser; private final QueueServicesWrapper services; @@ -66,22 +90,106 @@ public Object handleRequest(HttpServletRequest request) { QueueEntrySearchCriteria criteria = searchCriteriaParser.constructFromRequest(parameters); + String[] groupByArray = parameters.get(GROUP_BY); + boolean groupByQueue = groupByArray != null && Arrays.asList(groupByArray).contains(QUEUE); + // If we only want count, then use the ore efficient service to get counts - if (metrics.size() == 1 && metrics.get(0).equals(COUNT)) { + if (!groupByQueue && metrics.size() == 1 && metrics.get(0).equals(COUNT)) { ret.add(COUNT, services.getQueueEntryService().getCountOfQueueEntries(criteria).intValue()); } else { List queueEntries = services.getQueueEntryService().getQueueEntries(criteria); - if (metrics.isEmpty() || metrics.contains(COUNT)) { - ret.add(COUNT, queueEntries.size()); + // One instant for every duration, so the per-queue figures and the totals cannot disagree + Date asOf = new Date(); + addMetrics(ret, queueEntries, metrics, asOf); + if (groupByQueue) { + ret.add(QUEUES, getMetricsPerQueue(queueEntries, criteria, metrics, asOf)); } - if (metrics.isEmpty() || metrics.contains(AVERAGE_WAIT_TIME)) { - ret.add(AVERAGE_WAIT_TIME, QueueUtils.computeAverageWaitTimeInMinutes(queueEntries)); + } + + return ret; + } + + // Adds the requested metrics, or all of them if none were requested + private void addMetrics(SimpleObject target, List queueEntries, List metrics, Date asOf) { + if (metrics.isEmpty() || metrics.contains(COUNT)) { + target.add(COUNT, queueEntries.size()); + } + if (metrics.isEmpty() || metrics.contains(AVERAGE_WAIT_TIME)) { + target.add(AVERAGE_WAIT_TIME, QueueUtils.computeAverageWaitTimeInMinutes(queueEntries)); + } + // Unlike the two above, these are only reported when asked for, so that a caller that names no + // metric keeps receiving exactly what it received before they existed + if (metrics.contains(AVERAGE_OPEN_WAIT_TIME)) { + target.add(AVERAGE_OPEN_WAIT_TIME, QueueUtils.computeAverageOpenWaitTimeInMinutes(queueEntries, asOf)); + } + if (metrics.contains(LONGEST_OPEN_WAIT)) { + target.add(LONGEST_OPEN_WAIT, getLongestOpenWait(queueEntries, asOf)); + } + if (metrics.contains(COUNTS_BY_STATUS)) { + target.add(COUNTS_BY_STATUS, getCountsByStatus(queueEntries)); + } + } + + private SimpleObject getLongestOpenWait(List queueEntries, Date asOf) { + QueueEntry longestWaiting = QueueUtils.findLongestOpenWait(queueEntries); + if (longestWaiting == null) { + return null; + } + SimpleObject ret = new SimpleObject(); + ret.add("minutes", QueueUtils.computeOpenWaitTimeInMinutes(longestWaiting, asOf)); + ret.add("queueEntry", + ConversionUtil.convertToRepresentation(longestWaiting, new CustomRepresentation(LONGEST_OPEN_WAIT_REP))); + return ret; + } + + // Keyed by status concept rather than by particular named statuses, as which statuses matter is a + // matter of configuration in the calling application rather than something this module fixes + private Map getCountsByStatus(List queueEntries) { + Map ret = new LinkedHashMap<>(); + for (QueueEntry queueEntry : queueEntries) { + Concept status = queueEntry.getStatus(); + if (status != null) { + ret.merge(status.getUuid(), 1, Integer::sum); } } + return ret; + } + + // Seeded from the queues so that a queue nobody is in still reports a row of zeroes, then extended + // by the entries so that everything counted in the totals is also counted in a row: the queue + // search excludes retired queues, while the queue entry search has no such filter. + private List getMetricsPerQueue(List queueEntries, QueueEntrySearchCriteria criteria, + List metrics, Date asOf) { + Map> entriesByQueue = new LinkedHashMap<>(); + for (Queue queue : getQueuesToReport(criteria)) { + entriesByQueue.put(queue, new ArrayList<>()); + } + for (QueueEntry queueEntry : queueEntries) { + entriesByQueue.computeIfAbsent(queueEntry.getQueue(), q -> new ArrayList<>()).add(queueEntry); + } + List ret = new ArrayList<>(); + for (Map.Entry> e : entriesByQueue.entrySet()) { + SimpleObject queueMetrics = new SimpleObject(); + // Default rather than ref, as it carries the queue's location and service + queueMetrics.add(QUEUE, ConversionUtil.convertToRepresentation(e.getKey(), Representation.DEFAULT)); + addMetrics(queueMetrics, e.getValue(), metrics, asOf); + ret.add(queueMetrics); + } return ret; } + // Queues are limited by those criteria they share with the queue entry search + private List getQueuesToReport(QueueEntrySearchCriteria criteria) { + if (criteria.getQueues() != null) { + return new ArrayList<>(criteria.getQueues()); + } + QueueSearchCriteria queueSearchCriteria = new QueueSearchCriteria(); + queueSearchCriteria.setLocations(criteria.getLocations()); + queueSearchCriteria.setServices(criteria.getServices()); + return services.getQueueService().getQueues(queueSearchCriteria); + } + @Override public String getNamespace() { return "v1/queue-entry-metric"; diff --git a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java index 71a176e5..22223df0 100644 --- a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java @@ -13,19 +13,31 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.AVERAGE_OPEN_WAIT_TIME; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.AVERAGE_WAIT_TIME; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.COUNT; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.COUNTS_BY_STATUS; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.GROUP_BY; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.LONGEST_OPEN_WAIT; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUE; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUES; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_STATUS; import javax.servlet.http.HttpServletRequest; import java.util.Arrays; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -49,8 +61,11 @@ import org.openmrs.module.queue.api.QueueServicesWrapper; import org.openmrs.module.queue.api.RoomProviderMapService; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.Queue; +import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser; import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.ConversionUtil; import org.openmrs.module.webservices.rest.web.RestUtil; @ExtendWith(MockitoExtension.class) @@ -86,6 +101,8 @@ public class QueueEntryMetricRestControllerTest { private MockedStatic context; + private MockedStatic conversionUtil; + HttpServletRequest request; Map parameterMap; @@ -96,6 +113,10 @@ public class QueueEntryMetricRestControllerTest { public void prepareMocks() { restUtil = mockStatic(RestUtil.class); context = mockStatic(Context.class); + // The controller embeds refs to the queue and to the longest-waiting entry; converting those + // needs the REST framework, so stand in a marker and assert on the metrics themselves + conversionUtil = mockStatic(ConversionUtil.class, withSettings().lenient()); + conversionUtil.when(() -> ConversionUtil.convertToRepresentation(any(), any())).thenAnswer(i -> i.getArgument(0)); lenient().when(queueServicesWrapper.getQueueService()).thenReturn(queueService); lenient().when(queueServicesWrapper.getQueueEntryService()).thenReturn(queueEntryService); lenient().when(queueServicesWrapper.getQueueRoomService()).thenReturn(queueRoomService); @@ -120,13 +141,14 @@ public void prepareMocks() { parameterMap = new HashMap<>(); when(request.getParameterMap()).thenReturn(parameterMap); queueEntryArgumentCaptor = ArgumentCaptor.forClass(QueueEntrySearchCriteria.class); - when(queueEntryService.getCountOfQueueEntries(any())).thenReturn(50L); + lenient().when(queueEntryService.getCountOfQueueEntries(any())).thenReturn(50L); } @AfterEach public void cleanup() { restUtil.close(); context.close(); + conversionUtil.close(); } @Test @@ -143,4 +165,141 @@ public void shouldRetrieveCountOfQueueEntriesByStatus() { assertThat(criteria.getStatuses(), hasSize(2)); assertThat(criteria.getStatuses(), containsInAnyOrder(vals.get(0), vals.get(1))); } + + @Test + public void shouldReportOnlyTheOriginalMetricsWhenNoneAreNamed() { + when(queueEntryService.getQueueEntries(any())).thenReturn(Collections.emptyList()); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + // The metrics added since are reported only on request, so that a caller naming none keeps + // receiving exactly what it received before they existed + assertThat(result.keySet(), containsInAnyOrder(COUNT, AVERAGE_WAIT_TIME)); + } + + @Test + public void shouldStillGroupByQueueWhenOnlyTheCountIsAskedFor() { + Queue triage = queue("Triage"); + when(queueService.getQueues(any())).thenReturn(Collections.singletonList(triage)); + when(queueEntryService.getQueueEntries(any())) + .thenReturn(Collections.singletonList(entry(triage, minutesAgo(10), null))); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + // Counting alone is otherwise served by a cheaper query that cannot break the total down + List queues = (List) result.get(QUEUES); + assertThat(queues, hasSize(1)); + assertThat(queues.get(0).get(COUNT), equalTo(1)); + } + + @Test + public void shouldReturnMetricsForEachQueueWhenGroupingByQueue() { + Queue triage = queue("Triage"); + Queue pharmacy = queue("Pharmacy"); + when(queueService.getQueues(any())).thenReturn(Arrays.asList(triage, pharmacy)); + when(queueEntryService.getQueueEntries(any())) + .thenReturn(Arrays.asList(entry(triage, minutesAgo(30), null), entry(triage, minutesAgo(10), null))); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, + new String[] { COUNT, AVERAGE_OPEN_WAIT_TIME, LONGEST_OPEN_WAIT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + assertThat(result.get(COUNT), equalTo(2)); + List queues = (List) result.get(QUEUES); + assertThat(queues, hasSize(2)); + assertThat(queues.get(0).get(COUNT), equalTo(2)); + // A queue that nobody is in still gets a row, rather than dropping out of the list entirely + assertThat(queues.get(1).get(COUNT), equalTo(0)); + assertThat(queues.get(1).get(AVERAGE_OPEN_WAIT_TIME), is(nullValue())); + assertThat(queues.get(1).get(LONGEST_OPEN_WAIT), is(nullValue())); + } + + @Test + public void shouldGiveARowToAQueueTheQueueSearchDoesNotReturn() { + Queue triage = queue("Triage"); + // A retired queue is the real case: the queue search excludes those, the queue entry search does + // not, so its entries would count towards the total while belonging to no row + Queue missingFromQueueSearch = queue("Retired triage"); + when(queueService.getQueues(any())).thenReturn(Collections.singletonList(triage)); + when(queueEntryService.getQueueEntries(any())).thenReturn( + Arrays.asList(entry(triage, minutesAgo(30), null), entry(missingFromQueueSearch, minutesAgo(10), null))); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + List queues = (List) result.get(QUEUES); + assertThat(queues, hasSize(2)); + assertThat(result.get(COUNT), equalTo(2)); + assertThat(queues.get(0).get(COUNT), equalTo(1)); + assertThat(queues.get(1).get(COUNT), equalTo(1)); + } + + @Test + public void shouldMeasureOpenWaitsSeparatelyFromCompletedOnes() { + Queue triage = queue("Triage"); + when(queueEntryService.getQueueEntries(any())).thenReturn(Arrays.asList( + // Still waiting, for 40 and 20 minutes so far, averaging 30 + entry(triage, minutesAgo(40), null), entry(triage, minutesAgo(20), null), + // Already seen after a 10 minute wait, so it counts towards the completed average only + entry(triage, minutesAgo(70), minutesAgo(60)))); + parameterMap.put(QueueEntryMetricRestController.METRIC, + new String[] { AVERAGE_WAIT_TIME, AVERAGE_OPEN_WAIT_TIME, LONGEST_OPEN_WAIT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + assertThat((Double) result.get(AVERAGE_WAIT_TIME), equalTo(10.0)); + assertThat((Double) result.get(AVERAGE_OPEN_WAIT_TIME), equalTo(30.0)); + SimpleObject longestOpenWait = (SimpleObject) result.get(LONGEST_OPEN_WAIT); + assertThat(longestOpenWait, is(notNullValue())); + assertThat((Long) longestOpenWait.get("minutes"), equalTo(40L)); + } + + @Test + public void shouldCountEntriesByStatusConcept() { + Queue triage = queue("Triage"); + Concept waiting = concept("waiting-uuid"); + Concept inService = concept("in-service-uuid"); + QueueEntry first = entry(triage, minutesAgo(10), null); + first.setStatus(waiting); + QueueEntry second = entry(triage, minutesAgo(20), null); + second.setStatus(waiting); + QueueEntry third = entry(triage, minutesAgo(5), null); + third.setStatus(inService); + when(queueEntryService.getQueueEntries(any())).thenReturn(Arrays.asList(first, second, third)); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNTS_BY_STATUS }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + Map countsByStatus = (Map) result.get(COUNTS_BY_STATUS); + assertThat(countsByStatus.get("waiting-uuid"), equalTo(2)); + assertThat(countsByStatus.get("in-service-uuid"), equalTo(1)); + } + + private Queue queue(String name) { + Queue queue = new Queue(); + queue.setName(name); + return queue; + } + + private Concept concept(String uuid) { + Concept concept = new Concept(); + concept.setUuid(uuid); + return concept; + } + + private QueueEntry entry(Queue queue, Date startedAt, Date endedAt) { + QueueEntry queueEntry = new QueueEntry(); + queueEntry.setQueue(queue); + queueEntry.setStartedAt(startedAt); + queueEntry.setEndedAt(endedAt); + return queueEntry; + } + + private Date minutesAgo(int minutes) { + return new Date(System.currentTimeMillis() - (minutes * 60L * 1000L)); + } } From c3a8acddc76d2a8c3f69a3a8164a9dac3b223c0c Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Tue, 1 Sep 2026 01:59:02 +0800 Subject: [PATCH 2/5] O3-5770: Address review feedback on per-queue metrics - Floor an open wait at zero, so a startedAt in the future reports as not yet waiting rather than as a negative duration - Report the queue under a custom representation: REF carries neither the location and service that label a row nor the retired flag, and DEFAULT also carries the allowed priorities and statuses - Sort the queue rows by name, so repeating a request returns them in the same order - Cover the branch that reports only the queues the request named, and the one where an unrecognised groupBy value keeps the flat response --- .../module/queue/utils/QueueUtils.java | 9 ++-- .../module/queue/utils/QueueUtilsTest.java | 2 + .../web/QueueEntryMetricRestController.java | 27 ++++++---- .../QueueEntryMetricRestControllerTest.java | 53 ++++++++++++++++--- 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java index 43ec4feb..967df79e 100644 --- a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java +++ b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java @@ -110,16 +110,17 @@ public static Double computeAverageOpenWaitTimeInMinutes(List queueE /** * @param queueEntry the QueueEntry to measure * @param asOf the point in time to measure the open duration against - * @return how long the entry has been waiting, in minutes, or null if it has no startedAt or has - * already ended + * @return how long the entry has been waiting, in minutes, never negative, or null if it has no + * startedAt or has already ended */ public static Long computeOpenWaitTimeInMinutes(QueueEntry queueEntry, Date asOf) { if (queueEntry == null || asOf == null || queueEntry.getStartedAt() == null || queueEntry.getEndedAt() != null) { return null; } // Measured between instants rather than between local date times, so that a wait spanning a - // daylight saving change reports the time that actually elapsed - return Duration.between(queueEntry.getStartedAt().toInstant(), asOf.toInstant()).toMinutes(); + // daylight saving change reports the time that actually elapsed. Floored at zero, so that an + // entry with a startedAt in the future reports as not yet waiting rather than as negative + return Math.max(0, Duration.between(queueEntry.getStartedAt().toInstant(), asOf.toInstant()).toMinutes()); } /** diff --git a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java index a37debba..35b329c9 100644 --- a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java +++ b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java @@ -126,6 +126,8 @@ public void shouldComputeOpenWaitTimeInMinutes() { assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(NULL, NULL), AUG_2), is(nullValue())); assertThat(QueueUtils.computeOpenWaitTimeInMinutes(null, AUG_2), is(nullValue())); assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(AUG_1, NULL), NULL), is(nullValue())); + // An entry whose startedAt is in the future has not started waiting yet + assertThat(QueueUtils.computeOpenWaitTimeInMinutes(entry(AUG_2, NULL), AUG_1), is(0L)); } @Test diff --git a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java index 9e138509..9163bf58 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java @@ -13,6 +13,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.Date; import java.util.LinkedHashMap; import java.util.List; @@ -30,7 +31,6 @@ import org.openmrs.module.webservices.rest.web.ConversionUtil; import org.openmrs.module.webservices.rest.web.RestConstants; import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation; -import org.openmrs.module.webservices.rest.web.representation.Representation; import org.openmrs.module.webservices.rest.web.v1_0.controller.BaseRestController; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Controller; @@ -67,6 +67,11 @@ public class QueueEntryMetricRestController extends BaseRestController { // each a lazy load, for every queue reported on private static final String LONGEST_OPEN_WAIT_REP = "uuid,display,startedAt,patient:(uuid,display)"; + // Wider than REF, which carries neither the location and service that label a row nor the retired + // flag, but narrower than DEFAULT, which also carries the allowed priorities and statuses + private static final String QUEUE_REP = "uuid,display,name,description,retired," + + "location:(uuid,display),service:(uuid,display)"; + private final QueueEntrySearchCriteriaParser searchCriteriaParser; private final QueueServicesWrapper services; @@ -171,23 +176,27 @@ private List getMetricsPerQueue(List queueEntries, Que List ret = new ArrayList<>(); for (Map.Entry> e : entriesByQueue.entrySet()) { SimpleObject queueMetrics = new SimpleObject(); - // Default rather than ref, as it carries the queue's location and service - queueMetrics.add(QUEUE, ConversionUtil.convertToRepresentation(e.getKey(), Representation.DEFAULT)); + queueMetrics.add(QUEUE, ConversionUtil.convertToRepresentation(e.getKey(), new CustomRepresentation(QUEUE_REP))); addMetrics(queueMetrics, e.getValue(), metrics, asOf); ret.add(queueMetrics); } return ret; } - // Queues are limited by those criteria they share with the queue entry search + // Queues are limited by those criteria they share with the queue entry search, and are sorted by + // name so that repeating the same request returns the rows in the same order private List getQueuesToReport(QueueEntrySearchCriteria criteria) { + List queues; if (criteria.getQueues() != null) { - return new ArrayList<>(criteria.getQueues()); + queues = new ArrayList<>(criteria.getQueues()); + } else { + QueueSearchCriteria queueSearchCriteria = new QueueSearchCriteria(); + queueSearchCriteria.setLocations(criteria.getLocations()); + queueSearchCriteria.setServices(criteria.getServices()); + queues = new ArrayList<>(services.getQueueService().getQueues(queueSearchCriteria)); } - QueueSearchCriteria queueSearchCriteria = new QueueSearchCriteria(); - queueSearchCriteria.setLocations(criteria.getLocations()); - queueSearchCriteria.setServices(criteria.getServices()); - return services.getQueueService().getQueues(queueSearchCriteria); + queues.sort(Comparator.comparing(Queue::getName)); + return queues; } @Override diff --git a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java index 22223df0..2472e81b 100644 --- a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java @@ -20,6 +20,7 @@ import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.Mockito.withSettings; @@ -31,6 +32,7 @@ import static org.openmrs.module.queue.web.QueueEntryMetricRestController.LONGEST_OPEN_WAIT; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUE; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUES; +import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_QUEUE; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_STATUS; import javax.servlet.http.HttpServletRequest; @@ -61,6 +63,7 @@ import org.openmrs.module.queue.api.QueueServicesWrapper; import org.openmrs.module.queue.api.RoomProviderMapService; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.api.search.QueueSearchCriteria; import org.openmrs.module.queue.model.Queue; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser; @@ -113,8 +116,9 @@ public class QueueEntryMetricRestControllerTest { public void prepareMocks() { restUtil = mockStatic(RestUtil.class); context = mockStatic(Context.class); - // The controller embeds refs to the queue and to the longest-waiting entry; converting those - // needs the REST framework, so stand in a marker and assert on the metrics themselves + // The controller adds representations of the queue and of the longest-waiting entry to the + // response. The conversion needs the REST framework, and this test does not have it. + // The mock returns its input unchanged. The assertions examine only the metric values. conversionUtil = mockStatic(ConversionUtil.class, withSettings().lenient()); conversionUtil.when(() -> ConversionUtil.convertToRepresentation(any(), any())).thenAnswer(i -> i.getArgument(0)); lenient().when(queueServicesWrapper.getQueueService()).thenReturn(queueService); @@ -210,11 +214,48 @@ public void shouldReturnMetricsForEachQueueWhenGroupingByQueue() { assertThat(result.get(COUNT), equalTo(2)); List queues = (List) result.get(QUEUES); assertThat(queues, hasSize(2)); - assertThat(queues.get(0).get(COUNT), equalTo(2)); + // Rows come back sorted by queue name, whatever order the queue search returned them in + assertThat(((Queue) queues.get(0).get(QUEUE)).getName(), equalTo("Pharmacy")); + assertThat(((Queue) queues.get(1).get(QUEUE)).getName(), equalTo("Triage")); + assertThat(queues.get(1).get(COUNT), equalTo(2)); // A queue that nobody is in still gets a row, rather than dropping out of the list entirely - assertThat(queues.get(1).get(COUNT), equalTo(0)); - assertThat(queues.get(1).get(AVERAGE_OPEN_WAIT_TIME), is(nullValue())); - assertThat(queues.get(1).get(LONGEST_OPEN_WAIT), is(nullValue())); + assertThat(queues.get(0).get(COUNT), equalTo(0)); + assertThat(queues.get(0).get(AVERAGE_OPEN_WAIT_TIME), is(nullValue())); + assertThat(queues.get(0).get(LONGEST_OPEN_WAIT), is(nullValue())); + } + + @Test + public void shouldReportOnlyTheQueuesAskedForWhenTheRequestNamesThem() { + Queue triage = queue("Triage"); + String[] refs = new String[] { "triage-uuid" }; + when(queueServicesWrapper.getQueues(refs)).thenReturn(Collections.singletonList(triage)); + when(queueEntryService.getQueueEntries(any())) + .thenReturn(Collections.singletonList(entry(triage, minutesAgo(10), null))); + parameterMap.put(SEARCH_PARAM_QUEUE, refs); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + List queues = (List) result.get(QUEUES); + assertThat(queues, hasSize(1)); + assertThat(((Queue) queues.get(0).get(QUEUE)).getName(), equalTo("Triage")); + assertThat(queues.get(0).get(COUNT), equalTo(1)); + // The request has already named the queues, so there is nothing for the queue search to add + verify(queueService, never()).getQueues(any(QueueSearchCriteria.class)); + } + + @Test + public void shouldIgnoreAnUnknownGroupByValue() { + parameterMap.put(GROUP_BY, new String[] { "Queue" }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + // An unrecognized groupBy value keeps the flat response and the count fast path + assertThat(result.keySet(), containsInAnyOrder(COUNT)); + assertThat(result.get(COUNT), equalTo(50)); + verify(queueEntryService).getCountOfQueueEntries(any()); } @Test From 6368e350cae41fd259d323a4103415aecf5500cc Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Tue, 1 Sep 2026 21:58:24 +0800 Subject: [PATCH 3/5] O3-5770: Measure open waits over the statuses named as waiting An entry's startedAt is reset when it is called in to be seen, so a client asking for the open wait metrics with only isEnded=false has In Service entries measured on their time in service rather than on how long they waited. Adds a waitStatus parameter naming the statuses that count as waiting; averageOpenWaitTime and longestOpenWait are then computed over only those entries. count, averageWaitTime and countsByStatus keep the full list, and a request without the parameter behaves as before. --- .../web/QueueEntryMetricRestController.java | 39 ++++++++++++++----- .../QueueEntryMetricRestControllerTest.java | 26 +++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java index 9163bf58..e907464d 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java @@ -59,6 +59,8 @@ public class QueueEntryMetricRestController extends BaseRestController { public static final String GROUP_BY = "groupBy"; + public static final String WAIT_STATUS = "waitStatus"; + public static final String QUEUE = "queue"; public static final String QUEUES = "queues"; @@ -105,9 +107,11 @@ public Object handleRequest(HttpServletRequest request) { List queueEntries = services.getQueueEntryService().getQueueEntries(criteria); // One instant for every duration, so the per-queue figures and the totals cannot disagree Date asOf = new Date(); - addMetrics(ret, queueEntries, metrics, asOf); + String[] waitStatusArray = parameters.get(WAIT_STATUS); + List waitStatuses = (waitStatusArray == null ? null : services.getConcepts(waitStatusArray)); + addMetrics(ret, queueEntries, metrics, asOf, waitStatuses); if (groupByQueue) { - ret.add(QUEUES, getMetricsPerQueue(queueEntries, criteria, metrics, asOf)); + ret.add(QUEUES, getMetricsPerQueue(queueEntries, criteria, metrics, asOf, waitStatuses)); } } @@ -115,26 +119,43 @@ public Object handleRequest(HttpServletRequest request) { } // Adds the requested metrics, or all of them if none were requested - private void addMetrics(SimpleObject target, List queueEntries, List metrics, Date asOf) { + private void addMetrics(SimpleObject target, List queueEntries, List metrics, Date asOf, + List waitStatuses) { if (metrics.isEmpty() || metrics.contains(COUNT)) { target.add(COUNT, queueEntries.size()); } if (metrics.isEmpty() || metrics.contains(AVERAGE_WAIT_TIME)) { target.add(AVERAGE_WAIT_TIME, QueueUtils.computeAverageWaitTimeInMinutes(queueEntries)); } - // Unlike the two above, these are only reported when asked for, so that a caller that names no - // metric keeps receiving exactly what it received before they existed + // Unlike the two above, the remaining metrics are only reported when asked for, so that a caller + // that names no metric keeps receiving exactly what it received before they existed. + // An entry's startedAt is reset when it is called in to be seen, so the two open wait metrics are + // measured over only the statuses the caller counts as waiting, where it names any. + List waitingEntries = filterByStatus(queueEntries, waitStatuses); if (metrics.contains(AVERAGE_OPEN_WAIT_TIME)) { - target.add(AVERAGE_OPEN_WAIT_TIME, QueueUtils.computeAverageOpenWaitTimeInMinutes(queueEntries, asOf)); + target.add(AVERAGE_OPEN_WAIT_TIME, QueueUtils.computeAverageOpenWaitTimeInMinutes(waitingEntries, asOf)); } if (metrics.contains(LONGEST_OPEN_WAIT)) { - target.add(LONGEST_OPEN_WAIT, getLongestOpenWait(queueEntries, asOf)); + target.add(LONGEST_OPEN_WAIT, getLongestOpenWait(waitingEntries, asOf)); } if (metrics.contains(COUNTS_BY_STATUS)) { target.add(COUNTS_BY_STATUS, getCountsByStatus(queueEntries)); } } + private List filterByStatus(List queueEntries, List statuses) { + if (statuses == null || statuses.isEmpty()) { + return queueEntries; + } + List ret = new ArrayList<>(); + for (QueueEntry queueEntry : queueEntries) { + if (statuses.contains(queueEntry.getStatus())) { + ret.add(queueEntry); + } + } + return ret; + } + private SimpleObject getLongestOpenWait(List queueEntries, Date asOf) { QueueEntry longestWaiting = QueueUtils.findLongestOpenWait(queueEntries); if (longestWaiting == null) { @@ -164,7 +185,7 @@ private Map getCountsByStatus(List queueEntries) { // by the entries so that everything counted in the totals is also counted in a row: the queue // search excludes retired queues, while the queue entry search has no such filter. private List getMetricsPerQueue(List queueEntries, QueueEntrySearchCriteria criteria, - List metrics, Date asOf) { + List metrics, Date asOf, List waitStatuses) { Map> entriesByQueue = new LinkedHashMap<>(); for (Queue queue : getQueuesToReport(criteria)) { entriesByQueue.put(queue, new ArrayList<>()); @@ -177,7 +198,7 @@ private List getMetricsPerQueue(List queueEntries, Que for (Map.Entry> e : entriesByQueue.entrySet()) { SimpleObject queueMetrics = new SimpleObject(); queueMetrics.add(QUEUE, ConversionUtil.convertToRepresentation(e.getKey(), new CustomRepresentation(QUEUE_REP))); - addMetrics(queueMetrics, e.getValue(), metrics, asOf); + addMetrics(queueMetrics, e.getValue(), metrics, asOf, waitStatuses); ret.add(queueMetrics); } return ret; diff --git a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java index 2472e81b..3540e43f 100644 --- a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java @@ -32,6 +32,7 @@ import static org.openmrs.module.queue.web.QueueEntryMetricRestController.LONGEST_OPEN_WAIT; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUE; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUES; +import static org.openmrs.module.queue.web.QueueEntryMetricRestController.WAIT_STATUS; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_QUEUE; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_STATUS; @@ -320,6 +321,31 @@ public void shouldCountEntriesByStatusConcept() { assertThat(countsByStatus.get("in-service-uuid"), equalTo(1)); } + @Test + public void shouldMeasureOpenWaitsOverOnlyTheStatusesNamedAsWaiting() { + Queue triage = queue("Triage"); + Concept waiting = concept("waiting-uuid"); + Concept inService = concept("in-service-uuid"); + QueueEntry stillWaiting = entry(triage, minutesAgo(40), null); + stillWaiting.setStatus(waiting); + // startedAt is reset when an entry is called in, so this one's 5 minutes are time in service + QueueEntry beingSeen = entry(triage, minutesAgo(5), null); + beingSeen.setStatus(inService); + String[] refs = new String[] { "waiting-uuid" }; + when(queueServicesWrapper.getConcepts(refs)).thenReturn(Collections.singletonList(waiting)); + when(queueEntryService.getQueueEntries(any())).thenReturn(Arrays.asList(stillWaiting, beingSeen)); + parameterMap.put(WAIT_STATUS, refs); + parameterMap.put(QueueEntryMetricRestController.METRIC, + new String[] { COUNT, AVERAGE_OPEN_WAIT_TIME, LONGEST_OPEN_WAIT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + // The entry being seen is still counted, it is only left out of the two wait measurements + assertThat(result.get(COUNT), equalTo(2)); + assertThat((Double) result.get(AVERAGE_OPEN_WAIT_TIME), equalTo(40.0)); + assertThat((Long) ((SimpleObject) result.get(LONGEST_OPEN_WAIT)).get("minutes"), equalTo(40L)); + } + private Queue queue(String name) { Queue queue = new Queue(); queue.setName(name); From 456a50e7e4bc9af8127ba453a89fbd1d3fbaa00b Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Thu, 3 Sep 2026 16:07:27 +0800 Subject: [PATCH 4/5] O3-5770: Skip a blank queue ref when grouping by queue The wrapper resolves a blank queue ref to a null element, which would then reach the sort by name and fail there. Also covers the per-queue rows in the waitStatus test, and that the location and service filters reach the queue search. --- .../web/QueueEntryMetricRestController.java | 2 + .../QueueEntryMetricRestControllerTest.java | 53 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java index e907464d..a445d1d8 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java @@ -210,6 +210,8 @@ private List getQueuesToReport(QueueEntrySearchCriteria criteria) { List queues; if (criteria.getQueues() != null) { queues = new ArrayList<>(criteria.getQueues()); + // A blank queue ref resolves to a null element + queues.removeIf(q -> q == null); } else { QueueSearchCriteria queueSearchCriteria = new QueueSearchCriteria(); queueSearchCriteria.setLocations(criteria.getLocations()); diff --git a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java index 3540e43f..fb1b0cc9 100644 --- a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java @@ -33,7 +33,9 @@ import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUE; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.QUEUES; import static org.openmrs.module.queue.web.QueueEntryMetricRestController.WAIT_STATUS; +import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_LOCATION; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_QUEUE; +import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_SERVICE; import static org.openmrs.module.queue.web.resources.parser.QueueEntrySearchCriteriaParser.SEARCH_PARAM_STATUS; import javax.servlet.http.HttpServletRequest; @@ -54,6 +56,7 @@ import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension; import org.openmrs.Concept; +import org.openmrs.Location; import org.openmrs.api.ConceptService; import org.openmrs.api.LocationService; import org.openmrs.api.PatientService; @@ -334,7 +337,9 @@ public void shouldMeasureOpenWaitsOverOnlyTheStatusesNamedAsWaiting() { String[] refs = new String[] { "waiting-uuid" }; when(queueServicesWrapper.getConcepts(refs)).thenReturn(Collections.singletonList(waiting)); when(queueEntryService.getQueueEntries(any())).thenReturn(Arrays.asList(stillWaiting, beingSeen)); + when(queueService.getQueues(any())).thenReturn(Collections.singletonList(triage)); parameterMap.put(WAIT_STATUS, refs); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT, AVERAGE_OPEN_WAIT_TIME, LONGEST_OPEN_WAIT }); @@ -344,6 +349,54 @@ public void shouldMeasureOpenWaitsOverOnlyTheStatusesNamedAsWaiting() { assertThat(result.get(COUNT), equalTo(2)); assertThat((Double) result.get(AVERAGE_OPEN_WAIT_TIME), equalTo(40.0)); assertThat((Long) ((SimpleObject) result.get(LONGEST_OPEN_WAIT)).get("minutes"), equalTo(40L)); + // The per-queue rows are measured the same way as the totals + SimpleObject triageRow = ((List) result.get(QUEUES)).get(0); + assertThat(triageRow.get(COUNT), equalTo(2)); + assertThat((Double) triageRow.get(AVERAGE_OPEN_WAIT_TIME), equalTo(40.0)); + assertThat((Long) ((SimpleObject) triageRow.get(LONGEST_OPEN_WAIT)).get("minutes"), equalTo(40L)); + } + + @Test + public void shouldLimitTheQueueSearchToTheRequestedLocationAndService() { + Location clinic = new Location(); + Concept service = concept("service-uuid"); + String[] locationRefs = new String[] { "clinic-uuid" }; + String[] serviceRefs = new String[] { "service-uuid" }; + when(queueServicesWrapper.getLocations(locationRefs)).thenReturn(Collections.singletonList(clinic)); + when(queueServicesWrapper.getConcepts(serviceRefs)).thenReturn(Collections.singletonList(service)); + when(queueService.getQueues(any())).thenReturn(Collections.emptyList()); + when(queueEntryService.getQueueEntries(any())).thenReturn(Collections.emptyList()); + parameterMap.put(SEARCH_PARAM_LOCATION, locationRefs); + parameterMap.put(SEARCH_PARAM_SERVICE, serviceRefs); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + controller.handleRequest(request); + + // The rows have to be drawn from the same queues the entries were counted over + ArgumentCaptor captor = ArgumentCaptor.forClass(QueueSearchCriteria.class); + verify(queueService).getQueues(captor.capture()); + assertThat(captor.getValue().getLocations(), containsInAnyOrder(clinic)); + assertThat(captor.getValue().getServices(), containsInAnyOrder(service)); + } + + @Test + public void shouldSkipABlankQueueRef() { + Queue triage = queue("Triage"); + String[] refs = new String[] { "triage-uuid", "" }; + // The wrapper resolves a blank ref to a null element + when(queueServicesWrapper.getQueues(refs)).thenReturn(Arrays.asList(triage, null)); + when(queueEntryService.getQueueEntries(any())) + .thenReturn(Collections.singletonList(entry(triage, minutesAgo(10), null))); + parameterMap.put(SEARCH_PARAM_QUEUE, refs); + parameterMap.put(GROUP_BY, new String[] { QUEUE }); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { COUNT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + List queues = (List) result.get(QUEUES); + assertThat(queues, hasSize(1)); + assertThat(((Queue) queues.get(0).get(QUEUE)).getName(), equalTo("Triage")); } private Queue queue(String name) { From 493b716ab88e319aa89000f909b07743637d55d3 Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Sun, 6 Sep 2026 17:59:26 +0800 Subject: [PATCH 5/5] O3-5770: Skip a blank waitStatus ref A blank ref resolves to a null concept, which matched no entry, so both open wait metrics came back null. Also assert the entry lands on the row that survives the blank queue ref filter. --- .../web/QueueEntryMetricRestController.java | 7 ++++++- .../QueueEntryMetricRestControllerTest.java | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java index a445d1d8..9ae0ce5c 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/QueueEntryMetricRestController.java @@ -108,7 +108,12 @@ public Object handleRequest(HttpServletRequest request) { // One instant for every duration, so the per-queue figures and the totals cannot disagree Date asOf = new Date(); String[] waitStatusArray = parameters.get(WAIT_STATUS); - List waitStatuses = (waitStatusArray == null ? null : services.getConcepts(waitStatusArray)); + List waitStatuses = null; + if (waitStatusArray != null) { + waitStatuses = new ArrayList<>(services.getConcepts(waitStatusArray)); + // A blank status ref resolves to a null element + waitStatuses.removeIf(c -> c == null); + } addMetrics(ret, queueEntries, metrics, asOf, waitStatuses); if (groupByQueue) { ret.add(QUEUES, getMetricsPerQueue(queueEntries, criteria, metrics, asOf, waitStatuses)); diff --git a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java index fb1b0cc9..8da78638 100644 --- a/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/web/QueueEntryMetricRestControllerTest.java @@ -397,6 +397,27 @@ public void shouldSkipABlankQueueRef() { List queues = (List) result.get(QUEUES); assertThat(queues, hasSize(1)); assertThat(((Queue) queues.get(0).get(QUEUE)).getName(), equalTo("Triage")); + assertThat(queues.get(0).get(COUNT), equalTo(1)); + } + + @Test + public void shouldSkipABlankWaitStatusRef() { + Queue triage = queue("Triage"); + Concept waiting = concept("waiting-uuid"); + QueueEntry stillWaiting = entry(triage, minutesAgo(40), null); + stillWaiting.setStatus(waiting); + String[] refs = new String[] { "" }; + // The wrapper resolves a blank ref to a null element + when(queueServicesWrapper.getConcepts(refs)).thenReturn(Collections.singletonList((Concept) null)); + when(queueEntryService.getQueueEntries(any())).thenReturn(Collections.singletonList(stillWaiting)); + parameterMap.put(WAIT_STATUS, refs); + parameterMap.put(QueueEntryMetricRestController.METRIC, new String[] { AVERAGE_OPEN_WAIT_TIME, LONGEST_OPEN_WAIT }); + + SimpleObject result = (SimpleObject) controller.handleRequest(request); + + // Naming no usable status leaves the wait metrics measured over every entry + assertThat((Double) result.get(AVERAGE_OPEN_WAIT_TIME), equalTo(40.0)); + assertThat((Long) ((SimpleObject) result.get(LONGEST_OPEN_WAIT)).get("minutes"), equalTo(40L)); } private Queue queue(String name) {