Skip to content

(fix) O3-4985: End current queue entry when transitioning before checking for possible duplicates #85

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
dkayiwa merged 2 commits into from
Sep 8, 2025
Merged

(fix) O3-4985: End current queue entry when transitioning before checking for possible duplicates #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
314d8c5
(fix) O3-4985 end current queue entry before checking whether it conf…
chibongho Sep 5, 2025
aca81a4
fix test
chibongho Sep 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,16 @@ private boolean isDuplicate(QueueEntry queueEntry) {
public QueueEntry transitionQueueEntry(QueueEntryTransition queueEntryTransition) {
// Create a new queue entry
QueueEntry queueEntryToStart = queueEntryTransition.constructNewQueueEntry();
if (isDuplicate(queueEntryToStart)) {
throw new DuplicateQueueEntryException("queue.entry.duplicate.patient");
}

// End the initial queue entry
QueueEntry queueEntryToStop = queueEntryTransition.getQueueEntryToTransition();
queueEntryToStop.setEndedAt(queueEntryTransition.getTransitionDate());
getProxiedQueueEntryService().saveQueueEntry(queueEntryToStop);

if (isDuplicate(queueEntryToStart)) {
throw new DuplicateQueueEntryException("queue.entry.duplicate.patient");
}

Copy link
Member

@mogoodrich mogoodrich Sep 5, 2025

Choose a reason for hiding this comment

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

Do we have tests/can we confirm that throwing this exception causes the "saveQueueEntry" listed above to be rolled back (which I think was what this fix was trying to achieve).

I would assumed it would be if the method is transactionally, but would be good to verify (which there is likely already a test for, just wanted to confirm).

Alternatively, could the isDuplicate check be changed to be smart enough to ignore the queueEntryToStop when checking for duplicates?

Copy link
Member

@mseaton mseaton Sep 5, 2025

Choose a reason for hiding this comment

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

It is pretty difficult to test any real transactional behavior in our context-sensitive unit testing framework @mogoodrich . I don't think we really do so anywhere, across all of OpenMRS, to be honest, except in very limited cases

Copy link
Member

@mogoodrich mogoodrich Sep 5, 2025

Choose a reason for hiding this comment

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

Fair... I guess, can we confirm manually at least that this works? Or just sanity check for me that's what is supposed to happen, correct? The PR wasn't showing enough to see whether there was a transactional annotation on the parent method (though I assume there was). It's just that if it doesn't work it defeats the whole purpose of the initial commit in the first place, if I'm understanding correctly.

Copy link
Member

@dkayiwa dkayiwa Sep 7, 2025

Choose a reason for hiding this comment

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

@chibongho if you can manually test the webap and confirm that the original problem which was reported by @denniskigen is still fixed with these changes, i will approve this pull request. 😊

Copy link
Contributor Author

@chibongho chibongho Sep 8, 2025

Choose a reason for hiding this comment

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

Manually tested in RefApp version 3.6.0-SNAPSHOT with changes in PR:

  • Have a patient be put into 2 different queues ("Outpatient Consultation" and "Outpatient Triage")
  • In the O3 queues app, move entry of patient's "Outpatient Consultation" to "Outpatient Triage"

Verified that:

  • "Patient already in queue" shows in UI
  • Patient's existing queue entry (in "Outpatient Consultation") is NOT ended

// Save the new queue entry
return getProxiedQueueEntryService().saveQueueEntry(queueEntryToStart);
}
Expand Down
2 changes: 1 addition & 1 deletion api/src/main/java/org/openmrs/module/queue/tasks/AutoCloseVisitQueueEntryTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ protected List<QueueEntry> getActiveVisitQueueEntries() {

/**
* @param queueEntry the QueueEntry to save
*/
*/
protected void saveQueueEntry(QueueEntry queueEntry) {
Context.getService(QueueEntryService.class).saveQueueEntry(queueEntry);
}
Expand Down
18 changes: 8 additions & 10 deletions integration-tests/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import static org.junit.Assert.assertNull;

import java.util.Arrays;
import java.util.Date;
import java.util.List;

import org.junit.Before;
Expand Down Expand Up @@ -47,26 +48,23 @@ public void setup() {
INITIAL_DATASET_XML.forEach(this::executeDataSet);
}

@Test
@Test(expected = DuplicateQueueEntryException.class)
public void transitionQueueEntryShouldNotEndInitialIfNewIsDuplicate() {
QueueEntry queueEntry = queueEntryService.getQueueEntryById(3).get();
QueueEntryTransition transition = new QueueEntryTransition();
transition.setQueueEntryToTransition(queueEntry);
transition.setTransitionDate(queueEntry.getStartedAt());
try {
queueEntryService.transitionQueueEntry(transition);
}
catch (DuplicateQueueEntryException ex) {
assertNull(queueEntryService.getQueueEntryById(3).get().getEndedAt());
}
queueEntryService.transitionQueueEntry(transition);
Copy link
Member

@mseaton mseaton Sep 5, 2025

Choose a reason for hiding this comment

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

This looks right to me (the pre-existing assertNull is not actually testing any aspect of transactionality, it is just testing whether that property was set before the exception occurred, so fine to remove that.

}

@Test
public void transitionQueueEntryShouldEndInitialIfNewIsNotDuplicate() {
QueueEntry queueEntry = queueEntryService.getQueueEntryById(1).get();
QueueEntry queueEntry = queueEntryService.getQueueEntryById(2).get();
assertNull(queueEntry.getEndedAt());
Copy link
Member

@mseaton mseaton Sep 5, 2025

Choose a reason for hiding this comment

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

Why is this change needed? How is this related to the PR?

Copy link
Contributor Author

@chibongho chibongho Sep 5, 2025

Choose a reason for hiding this comment

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

The test was not valid. Queue entry 1 is initialized to have a non-null endedAt datetime, so I changed it to another queue entry with no endedAt time to test that it gets ended properly after the transition.

QueueEntryTransition transition = new QueueEntryTransition();
transition.setQueueEntryToTransition(queueEntry);
transition.setTransitionDate(queueEntry.getStartedAt());
assertNotNull(queueEntryService.getQueueEntryById(1).get().getEndedAt());
transition.setTransitionDate(new Date());
queueEntryService.transitionQueueEntry(transition);
assertNotNull(queueEntryService.getQueueEntryById(2).get().getEndedAt());
}
}