(fix) O3-4985: End current queue entry when transitioning before checking for possible duplicates #85
Conversation
…licts with new entry when transitioning
| catch (DuplicateQueueEntryException ex) { | ||
| assertNull(queueEntryService.getQueueEntryById(3).get().getEndedAt()); | ||
| } | ||
| queueEntryService.transitionQueueEntry(transition); |
There was a problem hiding this comment.
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.
| public void transitionQueueEntryShouldEndInitialIfNewIsNotDuplicate() { | ||
| QueueEntry queueEntry = queueEntryService.getQueueEntryById(1).get(); | ||
| QueueEntry queueEntry = queueEntryService.getQueueEntryById(2).get(); | ||
| assertNull(queueEntry.getEndedAt()); |
There was a problem hiding this comment.
Why is this change needed? How is this related to the PR?
There was a problem hiding this comment.
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.
mogoodrich
left a comment
mogoodrich
left a comment
There was a problem hiding this comment.
Had a couple questions, but if it's working properly, feel free to merge.
| if (isDuplicate(queueEntryToStart)) { | ||
| throw new DuplicateQueueEntryException("queue.entry.duplicate.patient"); | ||
| } | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. 😊
There was a problem hiding this comment.
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
|
@chibongho is it possible to also include a test reproducing the regression that was introduced? |
@dkayiwa I just fixed that test. Turned out it was still wrong. I have verified that it passes with this fix and fails without. |
|
Thank you so much @chibongho 👍 |
|
Thanks, all! @dkayiwa can we proceed to release a patch version of the queue module? |
|
@denniskigen i already kick started the process but still flexing with the usual bamboo dances: https://ci.openmrs.org/browse/QM-QUEUE-47 |
denniskigen
changed the title
|
@denniskigen i have done the needful. |
The
isDuplicatecheck added to thetransitionQueueEntryfunction was failing whenever we transition to the same queue, because the current active (un-ended) queue entry always overlaps the the new queue entry we attempt to create.This PR fixes it by moving the
isDuplicatecheck to after ending the current active queue entry. As the class is annotated as@Transitional, the current active queue entry should not have the end datetime set when theDuplicateQueueEntryExceptionis thrown.Testing done: