From 6d1537a13e97b95f4b1015bcf522604ba9289c6a Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Sat, 13 Jun 2026 21:15:27 +0800 Subject: [PATCH 1/4] O3-5693: Resolve previousQueueEntry via a column to avoid N+1 --- .../queue/api/impl/QueueEntryServiceImpl.java | 24 +------ .../module/queue/model/QueueEntry.java | 10 +++ api/src/main/resources/liquibase.xml | 67 +++++++++++++++++++ .../queue/api/QueueEntryServiceTest.java | 4 +- .../queue/api/QueueEntryServiceTest.java | 34 ++++++++++ 5 files changed, 114 insertions(+), 25 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java index 15c1c479..e857f80a 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java @@ -134,6 +134,7 @@ public QueueEntry transitionQueueEntry(QueueEntryTransition queueEntryTransition queueEntryTransition.setTransitionDate(transitionDate); QueueEntry queueEntryToStart = queueEntryTransition.constructNewQueueEntry(); + queueEntryToStart.setPreviousQueueEntry(queueEntryToStop); // Use optimistic locking to end the current entry queueEntryToStop.setEndedAt(transitionDate); @@ -295,27 +296,6 @@ private static Date roundToSecond(Date date) { @Override @Transactional(readOnly = true) public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) { - Queue queueComingFrom = queueEntry.getQueueComingFrom(); - if (queueComingFrom == null) { - return null; - } - - QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); - criteria.setPatient(queueEntry.getPatient()); - criteria.setVisit(queueEntry.getVisit()); - criteria.setEndedOn(queueEntry.getStartedAt()); - criteria.setQueues(Collections.singletonList(queueComingFrom)); - - List prevQueueEntries = dao.getQueueEntries(criteria); - - if (prevQueueEntries.size() == 1) { - return prevQueueEntries.get(0); - } else if (prevQueueEntries.size() > 1) { - // TODO: Exceptions should be translatable and human readable on the frontend. - // See: https://openmrs.atlassian.net/browse/O3-2988 - throw new IllegalStateException("Multiple previous queue entries found"); - } else { - return null; - } + return queueEntry.getPreviousQueueEntry(); } } diff --git a/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java b/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java index a2ab33be..9f1aa61a 100644 --- a/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java +++ b/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java @@ -11,6 +11,7 @@ import javax.persistence.Column; import javax.persistence.Entity; +import javax.persistence.FetchType; import javax.persistence.GeneratedValue; import javax.persistence.GenerationType; import javax.persistence.Id; @@ -26,6 +27,7 @@ import lombok.NoArgsConstructor; import lombok.Setter; import lombok.ToString; +import org.hibernate.annotations.BatchSize; import org.openmrs.BaseChangeableOpenmrsData; import org.openmrs.Concept; import org.openmrs.Location; @@ -93,6 +95,14 @@ public class QueueEntry extends BaseChangeableOpenmrsData { @JoinColumn(name = "queue_coming_from", referencedColumnName = "queue_id") private Queue queueComingFrom; + //The queue entry the patient was transitioned from, if any. + @ToString.Exclude + @EqualsAndHashCode.Exclude + @ManyToOne(fetch = FetchType.LAZY) + @BatchSize(size = 100) + @JoinColumn(name = "previous_queue_entry", referencedColumnName = "queue_entry_id") + private QueueEntry previousQueueEntry; + @Column(name = "started_at", nullable = false) private Date startedAt; diff --git a/api/src/main/resources/liquibase.xml b/api/src/main/resources/liquibase.xml index 5e4c38b0..cf466c70 100644 --- a/api/src/main/resources/liquibase.xml +++ b/api/src/main/resources/liquibase.xml @@ -749,4 +749,71 @@ + + + + + + + Add column previous_queue_entry to queue entry table + + + + + + + + + + + + + Backfill previous_queue_entry for existing transitioned entries, only where the predecessor + (same patient, same visit, queue = queue_coming_from, ended_at = started_at, not voided) is unambiguous. + + + update queue_entry curr + inner join ( + select curr2.queue_entry_id as curr_id, min(prev.queue_entry_id) as prev_id, count(*) as n + from queue_entry curr2 + inner join queue_entry prev + on prev.patient_id = curr2.patient_id + and prev.queue_id = curr2.queue_coming_from + and prev.ended_at = curr2.started_at + and prev.voided = 0 + and (prev.visit_id = curr2.visit_id or (prev.visit_id is null and curr2.visit_id is null)) + where curr2.queue_coming_from is not null and curr2.voided = 0 + group by curr2.queue_entry_id + ) m on m.curr_id = curr.queue_entry_id and m.n = 1 + set curr.previous_queue_entry = m.prev_id; + + + + + + + + + Backfill previous_queue_entry for existing transitioned entries, only where the predecessor + (same patient, same visit, queue = queue_coming_from, ended_at = started_at, not voided) is unambiguous. + + + UPDATE queue_entry curr + SET previous_queue_entry = m.prev_id + FROM ( + SELECT curr2.queue_entry_id AS curr_id, MIN(prev.queue_entry_id) AS prev_id, COUNT(*) AS n + FROM queue_entry curr2 + JOIN queue_entry prev + ON prev.patient_id = curr2.patient_id + AND prev.queue_id = curr2.queue_coming_from + AND prev.ended_at = curr2.started_at + AND prev.voided = false + AND (prev.visit_id = curr2.visit_id OR (prev.visit_id IS NULL AND curr2.visit_id IS NULL)) + WHERE curr2.queue_coming_from IS NOT NULL AND curr2.voided = false + GROUP BY curr2.queue_entry_id + ) m + WHERE m.curr_id = curr.queue_entry_id AND m.n = 1; + + + diff --git a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index ac356c2b..d5f57226 100644 --- a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -16,7 +16,6 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.*; -import java.util.Arrays; import java.util.Calendar; import java.util.Date; import java.util.HashMap; @@ -327,7 +326,6 @@ public void shouldUndoTransitionQueueEntry() { transition1.setTransitionDate(date2); QueueEntry queueEntry2 = queueEntryService.transitionQueueEntry(transition1); - when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(queueEntry1)); User user = new User(1); UserContext userContext = mock(UserContext.class); when(userContext.getAuthenticatedUser()).thenReturn(user); @@ -453,9 +451,9 @@ public void shouldThrowWhenUndoingTransitionOnConcurrentlyModifiedPreviousEntry( currentEntry.setPriority(concept1); currentEntry.setStartedAt(date2); currentEntry.setQueueComingFrom(queue1); + currentEntry.setPreviousQueueEntry(prevEntry); when(dao.get(2)).thenReturn(Optional.of(currentEntry)); - when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(prevEntry)); when(dao.updateIfUnmodified(any(), any())).thenReturn(false); queueEntryService.undoTransition(currentEntry); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index b1f3e3b1..c4f2702f 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -117,4 +117,38 @@ public void transitionQueueEntryShouldEndInitialIfNewIsNotDuplicate() { queueEntryService.transitionQueueEntry(transition); assertThat(queueEntryService.getQueueEntryById(2).get().getEndedAt(), is(notNullValue())); } + + @Test + public void getPreviousQueueEntryShouldReturnNullWhenNoPredecessor() { + // entry 3 has no previous_queue_entry set + QueueEntry entry3 = queueEntryService.getQueueEntryById(3).get(); + assertThat(queueEntryService.getPreviousQueueEntry(entry3), is(nullValue())); + } + + @Test + public void transitionQueueEntryShouldSetPreviousQueueEntryOnTheNewEntry() { + QueueEntry queueEntry = queueEntryService.getQueueEntryById(3).get(); + QueueEntryTransition transition = new QueueEntryTransition(); + transition.setQueueEntryToTransition(queueEntry); + transition.setTransitionDate(new Date()); + QueueEntry newEntry = queueEntryService.transitionQueueEntry(transition); + assertThat(newEntry.getPreviousQueueEntry(), is(notNullValue())); + assertThat(newEntry.getPreviousQueueEntry().getQueueEntryId(), is(queueEntry.getQueueEntryId())); + assertThat(queueEntryService.getPreviousQueueEntry(newEntry).getQueueEntryId(), is(queueEntry.getQueueEntryId())); + } + + @Test + public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { + // Transition entry 3, producing a new entry whose previous (entry 3) is linked via the column. + // Undo must resolve that predecessor from the column rather than throwing "no previous queue entry". + QueueEntry entry3 = queueEntryService.getQueueEntryById(3).get(); + QueueEntryTransition transition = new QueueEntryTransition(); + transition.setQueueEntryToTransition(entry3); + transition.setTransitionDate(new Date()); + QueueEntry newEntry = queueEntryService.transitionQueueEntry(transition); + assertThat(queueEntryService.getQueueEntryById(entry3.getQueueEntryId()).get().getEndedAt(), is(notNullValue())); + + QueueEntry reopened = queueEntryService.undoTransition(newEntry); + assertThat(reopened.getQueueEntryId(), is(entry3.getQueueEntryId())); + } } From 524d556837d175a13de4bebcaad612a1c1cff466 Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Fri, 24 Jul 2026 21:54:54 +0800 Subject: [PATCH 2/4] O3-5693: Populate previousQueueEntry on all writes; fix backfill self-join --- .../module/queue/api/QueueEntryService.java | 12 ++-- .../queue/api/impl/QueueEntryServiceImpl.java | 23 +++++++- .../module/queue/model/QueueEntry.java | 2 +- .../queue/model/QueueEntryTransition.java | 1 + api/src/main/resources/liquibase.xml | 16 ++---- .../queue/api/QueueEntryServiceTest.java | 56 +++++++++++++++++++ .../queue/api/QueueEntryServiceTest.java | 21 ++++++- .../web/resources/QueueEntryResource.java | 5 -- 8 files changed, 111 insertions(+), 25 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java index b28a818e..518e964a 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java +++ b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java @@ -62,13 +62,14 @@ public interface QueueEntryService { List getOverlappingQueueEntries(Patient patient, Queue queue, Date startedAt, Date endedAt); /** - * Given a specified queue entry Q, return its previous queue entry P, where P has same patient and - * visit as Q, and P.endedAt time is same as Q.startedAt time, and P.queue is same as - * Q.queueComingFrom + * Given a specified queue entry Q, return the previous queue entry recorded against it. This is set + * when Q is created, either directly by a transition or, for a queue entry saved with a + * queueComingFrom, by looking up the entry P with the same patient and visit as Q, whose endedAt + * time is the same as Q.startedAt time, and whose queue is Q.queueComingFrom * * @param queueEntry - * @return the previous queue entry, null otherwise. - * @throws IllegalStateException if multiple previous queue entries are identified + * @return the previous queue entry, null if there is none or if it could not be identified + * unambiguously when the queue entry was created. */ @Authorized(PrivilegeConstants.GET_QUEUE_ENTRIES) QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry); @@ -100,7 +101,6 @@ public interface QueueEntryService { * @param queueEntry the queue entry to undo transition to. Must be active * @return the previous queue entry, re-activated * @throws IllegalArgumentException if the previous queue entry does not exist - * @throws IllegalStateException if multiple previous entries are identified */ @Authorized({ PrivilegeConstants.MANAGE_QUEUE_ENTRIES }) QueueEntry undoTransition(@NotNull QueueEntry queueEntry); diff --git a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java index e857f80a..552cb0cd 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java @@ -93,6 +93,9 @@ public List getOverlappingQueueEntries(Patient patient, Queue queue, public QueueEntry saveQueueEntry(QueueEntry queueEntry) { Double sortWeight = getSortWeightGenerator().generateSortWeight(queueEntry); queueEntry.setSortWeight(sortWeight); + if (queueEntry.getId() == null && queueEntry.getPreviousQueueEntry() == null) { + queueEntry.setPreviousQueueEntry(resolvePreviousQueueEntry(queueEntry)); + } return dao.createOrUpdate(queueEntry); } @@ -134,7 +137,6 @@ public QueueEntry transitionQueueEntry(QueueEntryTransition queueEntryTransition queueEntryTransition.setTransitionDate(transitionDate); QueueEntry queueEntryToStart = queueEntryTransition.constructNewQueueEntry(); - queueEntryToStart.setPreviousQueueEntry(queueEntryToStop); // Use optimistic locking to end the current entry queueEntryToStop.setEndedAt(transitionDate); @@ -184,6 +186,8 @@ public QueueEntry undoTransition(@NotNull QueueEntry queueEntry) { throw new IllegalStateException("Previous queue entry was modified by another transaction"); } + // Cleared before voiding so that voidQueueEntry's own save persists it, rather than a second write + queueEntry.setPreviousQueueEntry(null); getProxiedQueueEntryService().voidQueueEntry(queueEntry, "Transition undone"); // Reload the previous entry to return the updated state @@ -298,4 +302,21 @@ private static Date roundToSecond(Date date) { public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) { return queueEntry.getPreviousQueueEntry(); } + + // Resolves the predecessor from queueComingFrom, returning null if there is no unambiguous match. + private QueueEntry resolvePreviousQueueEntry(QueueEntry queueEntry) { + Queue queueComingFrom = queueEntry.getQueueComingFrom(); + if (queueComingFrom == null) { + return null; + } + + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setPatient(queueEntry.getPatient()); + criteria.setVisit(queueEntry.getVisit()); + criteria.setEndedOn(queueEntry.getStartedAt()); + criteria.setQueues(Collections.singletonList(queueComingFrom)); + + List prevQueueEntries = dao.getQueueEntries(criteria); + return prevQueueEntries.size() == 1 ? prevQueueEntries.get(0) : null; + } } diff --git a/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java b/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java index 9f1aa61a..258cf4a4 100644 --- a/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java +++ b/api/src/main/java/org/openmrs/module/queue/model/QueueEntry.java @@ -41,6 +41,7 @@ @Getter @ToString @Entity +@BatchSize(size = 100) @Table(name = "queue_entry") public class QueueEntry extends BaseChangeableOpenmrsData { @@ -99,7 +100,6 @@ public class QueueEntry extends BaseChangeableOpenmrsData { @ToString.Exclude @EqualsAndHashCode.Exclude @ManyToOne(fetch = FetchType.LAZY) - @BatchSize(size = 100) @JoinColumn(name = "previous_queue_entry", referencedColumnName = "queue_entry_id") private QueueEntry previousQueueEntry; diff --git a/api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java b/api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java index 367db1d5..5047c628 100644 --- a/api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java +++ b/api/src/main/java/org/openmrs/module/queue/model/QueueEntryTransition.java @@ -52,6 +52,7 @@ public QueueEntry constructNewQueueEntry() { queueEntry.setLocationWaitingFor(queueEntryToTransition.getLocationWaitingFor()); queueEntry.setProviderWaitingFor(queueEntryToTransition.getProviderWaitingFor()); queueEntry.setQueueComingFrom(queueEntryToTransition.getQueue()); + queueEntry.setPreviousQueueEntry(queueEntryToTransition); queueEntry.setStartedAt(transitionDate); return queueEntry; } diff --git a/api/src/main/resources/liquibase.xml b/api/src/main/resources/liquibase.xml index cf466c70..679e6e78 100644 --- a/api/src/main/resources/liquibase.xml +++ b/api/src/main/resources/liquibase.xml @@ -767,17 +767,15 @@ - - Backfill previous_queue_entry for existing transitioned entries, only where the predecessor - (same patient, same visit, queue = queue_coming_from, ended_at = started_at, not voided) is unambiguous. - + Backfill previous_queue_entry for existing entries whose predecessor is unambiguous update queue_entry curr inner join ( select curr2.queue_entry_id as curr_id, min(prev.queue_entry_id) as prev_id, count(*) as n from queue_entry curr2 inner join queue_entry prev - on prev.patient_id = curr2.patient_id + on prev.queue_entry_id <> curr2.queue_entry_id + and prev.patient_id = curr2.patient_id and prev.queue_id = curr2.queue_coming_from and prev.ended_at = curr2.started_at and prev.voided = 0 @@ -793,10 +791,7 @@ - - Backfill previous_queue_entry for existing transitioned entries, only where the predecessor - (same patient, same visit, queue = queue_coming_from, ended_at = started_at, not voided) is unambiguous. - + Backfill previous_queue_entry for existing entries whose predecessor is unambiguous UPDATE queue_entry curr SET previous_queue_entry = m.prev_id @@ -804,7 +799,8 @@ SELECT curr2.queue_entry_id AS curr_id, MIN(prev.queue_entry_id) AS prev_id, COUNT(*) AS n FROM queue_entry curr2 JOIN queue_entry prev - ON prev.patient_id = curr2.patient_id + ON prev.queue_entry_id <> curr2.queue_entry_id + AND prev.patient_id = curr2.patient_id AND prev.queue_id = curr2.queue_coming_from AND prev.ended_at = curr2.started_at AND prev.voided = false diff --git a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index d5f57226..560a1463 100644 --- a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -16,7 +16,9 @@ import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.*; +import java.util.Arrays; import java.util.Calendar; +import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.Map; @@ -131,6 +133,56 @@ public void shouldCreateNewQueueEntryRecord() { assertThat(result.getPriority(), is(conceptPriority)); } + @Test + public void saveQueueEntryShouldResolvePreviousEntryFromQueueComingFrom() { + Queue queueComingFrom = new Queue(); + Patient patient = new Patient(); + Visit visit = new Visit(); + Date startedAt = DateUtils.truncate(new Date(), Calendar.SECOND); + + QueueEntry prevEntry = new QueueEntry(); + prevEntry.setQueueEntryId(1); + + QueueEntry newEntry = new QueueEntry(); + newEntry.setQueue(new Queue()); + newEntry.setPatient(patient); + newEntry.setVisit(visit); + newEntry.setStatus(new Concept()); + newEntry.setPriority(new Concept()); + newEntry.setStartedAt(startedAt); + newEntry.setQueueComingFrom(queueComingFrom); + + when(dao.getQueueEntries(any())).thenReturn(Collections.singletonList(prevEntry)); + when(dao.createOrUpdate(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + QueueEntry result = queueEntryService.saveQueueEntry(newEntry); + + assertThat(result.getPreviousQueueEntry(), equalTo(prevEntry)); + } + + @Test + public void saveQueueEntryShouldNotSetPreviousEntryWhenMatchIsAmbiguous() { + QueueEntry candidate1 = new QueueEntry(); + candidate1.setQueueEntryId(1); + QueueEntry candidate2 = new QueueEntry(); + candidate2.setQueueEntryId(2); + + QueueEntry newEntry = new QueueEntry(); + newEntry.setQueue(new Queue()); + newEntry.setPatient(new Patient()); + newEntry.setStatus(new Concept()); + newEntry.setPriority(new Concept()); + newEntry.setStartedAt(DateUtils.truncate(new Date(), Calendar.SECOND)); + newEntry.setQueueComingFrom(new Queue()); + + when(dao.getQueueEntries(any())).thenReturn(Arrays.asList(candidate1, candidate2)); + when(dao.createOrUpdate(any())).thenAnswer(invocation -> invocation.getArgument(0)); + + QueueEntry result = queueEntryService.saveQueueEntry(newEntry); + + assertNull(result.getPreviousQueueEntry()); + } + @Test public void shouldVoidQueueEntry() { User user = new User(1); @@ -246,6 +298,7 @@ public void shouldTransitionQueueEntry() { assertThat(queueEntry2.getQueueComingFrom(), equalTo(queue1)); assertThat(queueEntry2.getStartedAt(), equalTo(date2)); assertNull(queueEntry2.getEndedAt()); + assertThat(queueEntry2.getPreviousQueueEntry(), equalTo(queueEntry1)); // Next transition test that appropriate fields can be changed QueueEntryTransition transition2 = new QueueEntryTransition(); @@ -269,6 +322,7 @@ public void shouldTransitionQueueEntry() { assertThat(queueEntry3.getQueueComingFrom(), equalTo(queue1)); assertThat(queueEntry3.getStartedAt(), equalTo(date3)); assertNull(queueEntry3.getEndedAt()); + assertThat(queueEntry3.getPreviousQueueEntry(), equalTo(queueEntry2)); } @Test @@ -325,6 +379,7 @@ public void shouldUndoTransitionQueueEntry() { transition1.setQueueEntryToTransition(queueEntry1); transition1.setTransitionDate(date2); QueueEntry queueEntry2 = queueEntryService.transitionQueueEntry(transition1); + assertThat(queueEntry2.getPreviousQueueEntry(), equalTo(queueEntry1)); User user = new User(1); UserContext userContext = mock(UserContext.class); @@ -335,6 +390,7 @@ public void shouldUndoTransitionQueueEntry() { queueEntryService.undoTransition(queueEntry2); assertThat(queueEntry2.getVoided(), equalTo(true)); + assertNull(queueEntry2.getPreviousQueueEntry()); assertNull(queueEntry1.getEndedAt()); } finally { diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index c4f2702f..c896fd68 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -28,6 +28,7 @@ import org.openmrs.api.ConceptService; import org.openmrs.api.PatientService; import org.openmrs.api.ValidationException; +import org.openmrs.api.context.Context; import org.openmrs.module.queue.SpringTestConfiguration; import org.openmrs.module.queue.model.Queue; import org.openmrs.module.queue.model.QueueEntry; @@ -118,6 +119,22 @@ public void transitionQueueEntryShouldEndInitialIfNewIsNotDuplicate() { assertThat(queueEntryService.getQueueEntryById(2).get().getEndedAt(), is(notNullValue())); } + @Test + public void getPreviousQueueEntryShouldReadTheLinkBackFromTheDatabase() { + QueueEntry entry3 = queueEntryService.getQueueEntryById(3).get(); + QueueEntryTransition transition = new QueueEntryTransition(); + transition.setQueueEntryToTransition(entry3); + transition.setTransitionDate(new Date()); + Integer newEntryId = queueEntryService.transitionQueueEntry(transition).getQueueEntryId(); + Context.flushSession(); + Context.clearSession(); + + QueueEntry reloaded = queueEntryService.getQueueEntryById(newEntryId).get(); + QueueEntry previous = queueEntryService.getPreviousQueueEntry(reloaded); + assertThat(previous.getQueueEntryId(), is(entry3.getQueueEntryId())); + assertThat(previous.getStartedAt(), is(notNullValue())); + } + @Test public void getPreviousQueueEntryShouldReturnNullWhenNoPredecessor() { // entry 3 has no previous_queue_entry set @@ -139,8 +156,7 @@ public void transitionQueueEntryShouldSetPreviousQueueEntryOnTheNewEntry() { @Test public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { - // Transition entry 3, producing a new entry whose previous (entry 3) is linked via the column. - // Undo must resolve that predecessor from the column rather than throwing "no previous queue entry". + // Undo must resolve the predecessor from the column, and must not leave the voided entry claiming one. QueueEntry entry3 = queueEntryService.getQueueEntryById(3).get(); QueueEntryTransition transition = new QueueEntryTransition(); transition.setQueueEntryToTransition(entry3); @@ -150,5 +166,6 @@ public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { QueueEntry reopened = queueEntryService.undoTransition(newEntry); assertThat(reopened.getQueueEntryId(), is(entry3.getQueueEntryId())); + assertThat(newEntry.getPreviousQueueEntry(), is(nullValue())); } } diff --git a/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java b/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java index 8f63592b..9cad7bcd 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java @@ -285,11 +285,6 @@ public String getDisplay(QueueEntry queueEntry) { return (personName == null ? queueEntry.getPatient().toString() : personName.getFullName()); } - @PropertyGetter("previousQueueEntry") - public QueueEntry getPreviousQueueEntry(QueueEntry queueEntry) { - return getServices().getQueueEntryService().getPreviousQueueEntry(queueEntry); - } - @Override public String getResourceVersion() { return "2.3"; From 527fc8fcd816c35cd2065df32fd03e53262a514b Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Thu, 30 Jul 2026 01:16:39 +0800 Subject: [PATCH 3/4] O3-5693: Fix changelog XML parsing; ignore voided previous entries --- .../module/queue/api/QueueEntryService.java | 4 +- .../queue/api/impl/QueueEntryServiceImpl.java | 7 +++- api/src/main/resources/liquibase.xml | 4 +- .../queue/api/QueueEntryServiceTest.java | 26 +++++++----- .../queue/api/QueueEntryServiceTest.java | 40 +++++++++++++------ .../web/resources/QueueEntryResource.java | 5 +++ 6 files changed, 59 insertions(+), 27 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java index 518e964a..5ae6548d 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java +++ b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java @@ -68,8 +68,8 @@ public interface QueueEntryService { * time is the same as Q.startedAt time, and whose queue is Q.queueComingFrom * * @param queueEntry - * @return the previous queue entry, null if there is none or if it could not be identified - * unambiguously when the queue entry was created. + * @return the previous queue entry, null if there is none, if it could not be identified + * unambiguously when the queue entry was created, or if it has since been voided. */ @Authorized(PrivilegeConstants.GET_QUEUE_ENTRIES) QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry); diff --git a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java index 552cb0cd..28c95369 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java @@ -300,10 +300,13 @@ private static Date roundToSecond(Date date) { @Override @Transactional(readOnly = true) public QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry) { - return queueEntry.getPreviousQueueEntry(); + QueueEntry previousQueueEntry = queueEntry.getPreviousQueueEntry(); + if (previousQueueEntry == null || previousQueueEntry.getVoided()) { + return null; + } + return previousQueueEntry; } - // Resolves the predecessor from queueComingFrom, returning null if there is no unambiguous match. private QueueEntry resolvePreviousQueueEntry(QueueEntry queueEntry) { Queue queueComingFrom = queueEntry.getQueueComingFrom(); if (queueComingFrom == null) { diff --git a/api/src/main/resources/liquibase.xml b/api/src/main/resources/liquibase.xml index 679e6e78..0ae8a024 100644 --- a/api/src/main/resources/liquibase.xml +++ b/api/src/main/resources/liquibase.xml @@ -774,7 +774,7 @@ select curr2.queue_entry_id as curr_id, min(prev.queue_entry_id) as prev_id, count(*) as n from queue_entry curr2 inner join queue_entry prev - on prev.queue_entry_id <> curr2.queue_entry_id + on prev.queue_entry_id != curr2.queue_entry_id and prev.patient_id = curr2.patient_id and prev.queue_id = curr2.queue_coming_from and prev.ended_at = curr2.started_at @@ -799,7 +799,7 @@ SELECT curr2.queue_entry_id AS curr_id, MIN(prev.queue_entry_id) AS prev_id, COUNT(*) AS n FROM queue_entry curr2 JOIN queue_entry prev - ON prev.queue_entry_id <> curr2.queue_entry_id + ON prev.queue_entry_id != curr2.queue_entry_id AND prev.patient_id = curr2.patient_id AND prev.queue_id = curr2.queue_coming_from AND prev.ended_at = curr2.started_at diff --git a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index 560a1463..089093ec 100644 --- a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -135,22 +135,17 @@ public void shouldCreateNewQueueEntryRecord() { @Test public void saveQueueEntryShouldResolvePreviousEntryFromQueueComingFrom() { - Queue queueComingFrom = new Queue(); - Patient patient = new Patient(); - Visit visit = new Visit(); - Date startedAt = DateUtils.truncate(new Date(), Calendar.SECOND); - QueueEntry prevEntry = new QueueEntry(); prevEntry.setQueueEntryId(1); QueueEntry newEntry = new QueueEntry(); newEntry.setQueue(new Queue()); - newEntry.setPatient(patient); - newEntry.setVisit(visit); + newEntry.setPatient(new Patient()); + newEntry.setVisit(new Visit()); newEntry.setStatus(new Concept()); newEntry.setPriority(new Concept()); - newEntry.setStartedAt(startedAt); - newEntry.setQueueComingFrom(queueComingFrom); + newEntry.setStartedAt(DateUtils.truncate(new Date(), Calendar.SECOND)); + newEntry.setQueueComingFrom(new Queue()); when(dao.getQueueEntries(any())).thenReturn(Collections.singletonList(prevEntry)); when(dao.createOrUpdate(any())).thenAnswer(invocation -> invocation.getArgument(0)); @@ -183,6 +178,19 @@ public void saveQueueEntryShouldNotSetPreviousEntryWhenMatchIsAmbiguous() { assertNull(result.getPreviousQueueEntry()); } + @Test + public void getPreviousQueueEntryShouldTreatAVoidedPredecessorAsAbsent() { + QueueEntry prevEntry = new QueueEntry(); + prevEntry.setQueueEntryId(1); + prevEntry.setVoided(true); + + QueueEntry queueEntry = new QueueEntry(); + queueEntry.setQueueEntryId(2); + queueEntry.setPreviousQueueEntry(prevEntry); + + assertNull(queueEntryService.getPreviousQueueEntry(queueEntry)); + } + @Test public void shouldVoidQueueEntry() { User user = new User(1); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index c896fd68..4b80d42f 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -142,18 +142,6 @@ public void getPreviousQueueEntryShouldReturnNullWhenNoPredecessor() { assertThat(queueEntryService.getPreviousQueueEntry(entry3), is(nullValue())); } - @Test - public void transitionQueueEntryShouldSetPreviousQueueEntryOnTheNewEntry() { - QueueEntry queueEntry = queueEntryService.getQueueEntryById(3).get(); - QueueEntryTransition transition = new QueueEntryTransition(); - transition.setQueueEntryToTransition(queueEntry); - transition.setTransitionDate(new Date()); - QueueEntry newEntry = queueEntryService.transitionQueueEntry(transition); - assertThat(newEntry.getPreviousQueueEntry(), is(notNullValue())); - assertThat(newEntry.getPreviousQueueEntry().getQueueEntryId(), is(queueEntry.getQueueEntryId())); - assertThat(queueEntryService.getPreviousQueueEntry(newEntry).getQueueEntryId(), is(queueEntry.getQueueEntryId())); - } - @Test public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { // Undo must resolve the predecessor from the column, and must not leave the voided entry claiming one. @@ -168,4 +156,32 @@ public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { assertThat(reopened.getQueueEntryId(), is(entry3.getQueueEntryId())); assertThat(newEntry.getPreviousQueueEntry(), is(nullValue())); } + + @Test + public void undoTransitionShouldNotActOnAVoidedPreviousQueueEntry() { + QueueEntry entry3 = queueEntryService.getQueueEntryById(3).get(); + QueueEntryTransition transition = new QueueEntryTransition(); + transition.setQueueEntryToTransition(entry3); + transition.setTransitionDate(new Date()); + Integer newEntryId = queueEntryService.transitionQueueEntry(transition).getQueueEntryId(); + queueEntryService.voidQueueEntry(entry3, "voided after the transition"); + Context.flushSession(); + Context.clearSession(); + + // The column is still set; the voided predecessor is filtered out on read rather than cleared + QueueEntry reloaded = queueEntryService.getQueueEntryById(newEntryId).get(); + assertThat(reloaded.getPreviousQueueEntry(), is(notNullValue())); + try { + queueEntryService.undoTransition(reloaded); + fail("Expected IllegalArgumentException to be thrown"); + } + catch (IllegalArgumentException e) { + assertThat(e.getMessage(), containsString("does not have a previous queue entry")); + } + Context.clearSession(); + + // The voided predecessor must stay ended, and the successor must stay active + assertThat(queueEntryService.getQueueEntryById(3).get().getEndedAt(), is(notNullValue())); + assertThat(queueEntryService.getQueueEntryById(newEntryId).get().getVoided(), is(false)); + } } diff --git a/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java b/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java index 9cad7bcd..8f63592b 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/resources/QueueEntryResource.java @@ -285,6 +285,11 @@ public String getDisplay(QueueEntry queueEntry) { return (personName == null ? queueEntry.getPatient().toString() : personName.getFullName()); } + @PropertyGetter("previousQueueEntry") + public QueueEntry getPreviousQueueEntry(QueueEntry queueEntry) { + return getServices().getQueueEntryService().getPreviousQueueEntry(queueEntry); + } + @Override public String getResourceVersion() { return "2.3"; From 817a603306d8b409cb8e453baf3bbb2719fb3a04 Mon Sep 17 00:00:00 2001 From: Ujjawal Prabhat Date: Mon, 31 Aug 2026 22:36:36 +0800 Subject: [PATCH 4/4] O3-5693: Address review feedback on previousQueueEntry - getPreviousQueueEntry javadoc: note that the visit is only matched when Q has one, and simplify the @return wording. - Unit test: assert the criteria the write-time lookup builds (patient, visit, endedOn, queues), not just the resolved result. - Integration test: flush and clear before asserting undoTransition cleared the link, so the assertion reads the database rather than the instance it just mutated. --- .../openmrs/module/queue/api/QueueEntryService.java | 10 ++++++---- .../module/queue/api/QueueEntryServiceTest.java | 6 ++++++ .../module/queue/api/QueueEntryServiceTest.java | 5 ++++- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java index 5ae6548d..91d1ad16 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java +++ b/api/src/main/java/org/openmrs/module/queue/api/QueueEntryService.java @@ -64,12 +64,14 @@ public interface QueueEntryService { /** * Given a specified queue entry Q, return the previous queue entry recorded against it. This is set * when Q is created, either directly by a transition or, for a queue entry saved with a - * queueComingFrom, by looking up the entry P with the same patient and visit as Q, whose endedAt - * time is the same as Q.startedAt time, and whose queue is Q.queueComingFrom + * queueComingFrom, by looking up the entry P with the same patient and, when Q has a visit, the + * same visit as Q, whose endedAt time is the same as Q.startedAt time, and whose queue is + * Q.queueComingFrom * * @param queueEntry - * @return the previous queue entry, null if there is none, if it could not be identified - * unambiguously when the queue entry was created, or if it has since been voided. + * @return the previous queue entry recorded against this entry. Returns null when no predecessor + * was recorded at creation (no match, or more than one match), or when the recorded + * predecessor is voided. */ @Authorized(PrivilegeConstants.GET_QUEUE_ENTRIES) QueueEntry getPreviousQueueEntry(@NotNull QueueEntry queueEntry); diff --git a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index 089093ec..6de650ef 100644 --- a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -153,6 +153,12 @@ public void saveQueueEntryShouldResolvePreviousEntryFromQueueComingFrom() { QueueEntry result = queueEntryService.saveQueueEntry(newEntry); assertThat(result.getPreviousQueueEntry(), equalTo(prevEntry)); + verify(dao).getQueueEntries(queueEntrySearchCriteriaArgumentCaptor.capture()); + QueueEntrySearchCriteria criteria = queueEntrySearchCriteriaArgumentCaptor.getValue(); + assertThat(criteria.getPatient(), equalTo(newEntry.getPatient())); + assertThat(criteria.getVisit(), equalTo(newEntry.getVisit())); + assertThat(criteria.getEndedOn(), equalTo(newEntry.getStartedAt())); + assertThat(criteria.getQueues(), equalTo(Collections.singletonList(newEntry.getQueueComingFrom()))); } @Test diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index 4b80d42f..1cd0cf0c 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -154,7 +154,10 @@ public void undoTransitionShouldResolveThePreviousEntryViaTheColumn() { QueueEntry reopened = queueEntryService.undoTransition(newEntry); assertThat(reopened.getQueueEntryId(), is(entry3.getQueueEntryId())); - assertThat(newEntry.getPreviousQueueEntry(), is(nullValue())); + Context.flushSession(); + Context.clearSession(); + assertThat(queueEntryService.getQueueEntryById(newEntry.getQueueEntryId()).get().getPreviousQueueEntry(), + is(nullValue())); } @Test