O3-2635: prevent duplicate queue entries #48
Conversation
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
| * @param message | ||
| */ | ||
| public DuplicateQueueEntryException(String message) { | ||
| super(Context.getMessageSourceService().getMessage(message)); |
There was a problem hiding this comment.
Unfortunately, we should not put calls to the Context like this within this constructor. We need to pass in a pre-translated message, not translate within here.
| # graphic logo is a trademark of OpenMRS Inc. | ||
| # | ||
|
|
||
| queue.entry.duplicate.patient=Patient already in the queue |
There was a problem hiding this comment.
Either provide reasonable default translations in the appropriate language or remove the non-english translation files from the module. They can always be translated downstream via Initializer.
| searchCriteria.setPatient(secondQueueEntry.getPatient()); | ||
| searchCriteria.setQueues(Collections.singletonList(queue)); | ||
| //tell the dao to return the first queueEntry that was successfully saved above | ||
| when(dao.getQueueEntries(searchCriteria)).thenReturn(Collections.singletonList(result)); |
There was a problem hiding this comment.
This doesn't actually test the most meaningful part of what needs testing - namely that the date overlapping logic works as intended. Hopefully moving that out to a separate method will facilitate that.
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Show resolved
Hide resolved
| || ((entry.getEndedAt() != null) && (queueEntry.getStartedAt().after(entry.getEndedAt()))))) { | ||
| //if the new queueEntry endDate is NOT before the entry startDate OR the new queueEntry startDate is NOT after the entry endDate, | ||
| //then the new queueEntry overlaps with the existing entry | ||
| throw new DuplicateQueueEntryException("queue.entry.duplicate.patient"); |
There was a problem hiding this comment.
Will need to add the messageSourceService to this, wire it up via the application context xml, and then use it here to do the necessary translation.
api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
Outdated
Show resolved
Hide resolved
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.module.queue.api; |
There was a problem hiding this comment.
Let's move this into a different package "org.openmrs.module.queue.exception".
| for (QueueEntry entry : queueEntries) { | ||
| if (QueueUtils.datesOverlap(entry.getStartedAt(), entry.getEndedAt(), queueEntry.getStartedAt(), | ||
| queueEntry.getEndedAt())) { | ||
| throw new DuplicateQueueEntryException(messageSourceService.getMessage("queue.entry.duplicate.patient")); |
There was a problem hiding this comment.
Now that I look at this, I'm curious about your thoughts on what would be better @cioan - to translate here and put the translation in the exception, or to just add the message code to the exception, and let this be translated downstream as appropriate.
| * @return boolean - indicating whether or not the timeframes overlap | ||
| */ | ||
| public static boolean datesOverlap(Date startDate1, Date endDate1, Date startDate2, Date endDate2) { | ||
| if (startDate1 != null && startDate2 != null) { |
There was a problem hiding this comment.
If a startDate == null, it should assumed to be all times, not no times, up to endDate
| */ | ||
| public static boolean datesOverlap(Date startDate1, Date endDate1, Date startDate2, Date endDate2) { | ||
| if (startDate1 != null && startDate2 != null) { | ||
| if (!((startDate2.before(startDate1) && (endDate2 != null && endDate2.compareTo(startDate1) <= 0)) |
There was a problem hiding this comment.
What if any of the endDates are null?
|
|
||
| @Test | ||
| public void shouldReturnFalseWhenDatesAreNull() { | ||
| assertThat(QueueUtils.datesOverlap(null, null, null, null), is(false)); |
There was a problem hiding this comment.
This test case is incorrect. Should return true when dates are null. Infinite dates overlap with other infinite dates.
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.module.queue.exception; |
There was a problem hiding this comment.
@cioan - note, I moved this to a new package
| * @param startDate2, endDate2 - the start and end date of second timeframe | ||
| * @return boolean - indicating whether the timeframes overlap | ||
| */ | ||
| public static boolean datesOverlap(Date startDate1, Date endDate1, Date startDate2, Date endDate2) { |
There was a problem hiding this comment.
@cioan - I rewrote this implementation, as there were several use cases that it didn't seem to cover. Also, I updated the handling of nulls so that they indicate an unbounded condition (eg. overlapping nulls should be overlapping, not non-overlapping). Have a look at this new implementation and let me know if you see any issues.
| @@ -109,6 +109,48 @@ public void shouldCreateNewQueueEntryRecord() { | |||
| assertThat(result.getPriority(), is(conceptPriority)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void shouldNotCreateDuplicateOverlappingQueueEntryRecords() { | |||
| Queue queue = new Queue(); | |||
There was a problem hiding this comment.
@cioan - note, I removed all of the mocking in favor of just creating and populating the actual objects, they are just beans.
|
|
||
| // Should be able to save and re-save a queue entry without causing validation failure | ||
| savedQueueEntry = queueEntryService.saveQueueEntry(savedQueueEntry); | ||
| queueEntryService.saveQueueEntry(savedQueueEntry); |
There was a problem hiding this comment.
@cioan - note, I added this test to ensure that saving the same queue entry multiple times doesn't fail validation
|
|
||
| @Test | ||
| public void shouldReturnTrueIfDatesOverlap() { | ||
| // Test that nulls are handled as open-ended dates |
There was a problem hiding this comment.
@cioan - see how I rewrote this test. I found this style a lot easier for me to follow, and more conducive to adding lots of test criteria. Have a look and let me know if you think any test cases are missing, and if you have questions about whether certain conditions are handled correctly.
mseaton
changed the title
|
Thanks @mseaton ! LGTM. Can I merge in this PR? Thanks! |
@mseaton , just wanted to see if this is the right direction to fix the problem with the duplicate queue entries? I could add unit tests too, but first wanted to make sure we want to fix this at the service layer or we could also add some checks in the rest controller. Thanks!