-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
O3-2635: prevent duplicate queue entries #48
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,8 +34,10 @@ | |
import org.openmrs.module.queue.api.QueueEntryService; | ||
import org.openmrs.module.queue.api.dao.QueueEntryDao; | ||
import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; | ||
import org.openmrs.module.queue.exception.DuplicateQueueEntryException; | ||
import org.openmrs.module.queue.model.Queue; | ||
import org.openmrs.module.queue.model.QueueEntry; | ||
import org.openmrs.module.queue.utils.QueueUtils; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Slf4j | ||
|
@@ -83,6 +85,17 @@ public QueueEntry saveQueueEntry(QueueEntry queueEntry) { | |
throw new IllegalArgumentException("Patient mismatch - visit.patient does not match patient"); | ||
} | ||
} | ||
QueueEntrySearchCriteria searchCriteria = new QueueEntrySearchCriteria(); | ||
searchCriteria.setPatient(queueEntry.getPatient()); | ||
searchCriteria.setQueues(Collections.singletonList(queueEntry.getQueue())); | ||
List<QueueEntry> queueEntries = getQueueEntries(searchCriteria); | ||
for (QueueEntry qe : queueEntries) { | ||
if (!qe.equals(queueEntry)) { | ||
if (QueueUtils.datesOverlap(qe.getStartedAt(), qe.getEndedAt(), qe.getStartedAt(), qe.getEndedAt())) { | ||
throw new DuplicateQueueEntryException("queue.entry.duplicate.patient"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cioan - note, I removed the translation of the exception altogether. This can be handled downstream in the view layer. |
||
} | ||
} | ||
} | ||
mseaton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return dao.createOrUpdate(queueEntry); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* This Source Code Form is subject to the terms of the Mozilla Public License, | ||
* v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
* | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cioan - note, I moved this to a new package |
||
|
||
import org.openmrs.api.APIException; | ||
|
||
public class DuplicateQueueEntryException extends APIException { | ||
|
||
private static final long serialVersionUID = 1L; | ||
|
||
public DuplicateQueueEntryException() { | ||
} | ||
|
||
/** | ||
* @param message | ||
*/ | ||
public DuplicateQueueEntryException(String message) { | ||
super(message); | ||
} | ||
|
||
/** | ||
* @param message | ||
* @param cause | ||
*/ | ||
public DuplicateQueueEntryException(String message, Throwable cause) { | ||
super(message, cause); | ||
} | ||
|
||
/** | ||
* @param cause | ||
*/ | ||
public DuplicateQueueEntryException(Throwable cause) { | ||
super(cause); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,4 +74,28 @@ public static double computeAverageWaitTimeInMinutes(List<QueueEntry> queueEntri | |
} | ||
return averageWaitTime; | ||
} | ||
|
||
/** | ||
* @param startDate1, endDate1 - the start and end date of one timeframe | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
long startTime1 = (startDate1 == null ? Long.MIN_VALUE : startDate1.getTime()); | ||
long endTime1 = (endDate1 == null ? Long.MAX_VALUE : endDate1.getTime()); | ||
long startTime2 = (startDate2 == null ? Long.MIN_VALUE : startDate2.getTime()); | ||
long endTime2 = (endDate2 == null ? Long.MAX_VALUE : endDate2.getTime()); | ||
// If time1 starts earlier, then it overlaps time2 if it ends after time2 starts | ||
if (startTime1 < startTime2) { | ||
return endTime1 > startTime2; | ||
} | ||
// Otherwise, if time2 starts earlier, then it overlaps time1 if it ends after time1 starts | ||
else if (startTime2 < startTime1) { | ||
return endTime2 > startTime1; | ||
} | ||
// Otherwise, if both start at the same time, they overlap | ||
else { | ||
return true; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
# This Source Code Form is subject to the terms of the Mozilla Public License, | ||
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
# the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
# | ||
# Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
# graphic logo is a trademark of OpenMRS Inc. | ||
# | ||
|
||
queue.entry.duplicate.patient=Patient already in the queue |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
# This Source Code Form is subject to the terms of the Mozilla Public License, | ||
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
# the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
# | ||
# Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
# graphic logo is a trademark of OpenMRS Inc. | ||
# | ||
|
||
queue.entry.duplicate.patient=Paciente ya en la cola |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
# This Source Code Form is subject to the terms of the Mozilla Public License, | ||
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
# the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
# | ||
# Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
# graphic logo is a trademark of OpenMRS Inc. | ||
# | ||
|
||
queue.entry.duplicate.patient=Patient déjà dans la queue |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
# This Source Code Form is subject to the terms of the Mozilla Public License, | ||
# v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
# obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
# the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
# | ||
# Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
# graphic logo is a trademark of OpenMRS Inc. | ||
# | ||
|
||
queue.entry.duplicate.patient=Pasyan deja nan keu a |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,15 +10,13 @@ | |
package org.openmrs.module.queue.api; | ||
|
||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.notNullValue; | ||
import static org.hamcrest.Matchers.nullValue; | ||
import static org.hamcrest.Matchers.*; | ||
import static org.junit.Assert.fail; | ||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.when; | ||
import static org.mockito.Mockito.*; | ||
|
||
import java.util.Collections; | ||
import java.util.Date; | ||
import java.util.Optional; | ||
|
||
import org.junit.Before; | ||
|
@@ -31,6 +29,7 @@ | |
import org.mockito.junit.MockitoJUnitRunner; | ||
import org.openmrs.Concept; | ||
import org.openmrs.Location; | ||
import org.openmrs.Patient; | ||
import org.openmrs.User; | ||
import org.openmrs.Visit; | ||
import org.openmrs.VisitAttributeType; | ||
|
@@ -40,6 +39,7 @@ | |
import org.openmrs.module.queue.api.dao.QueueEntryDao; | ||
import org.openmrs.module.queue.api.impl.QueueEntryServiceImpl; | ||
import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; | ||
import org.openmrs.module.queue.exception.DuplicateQueueEntryException; | ||
import org.openmrs.module.queue.model.Queue; | ||
import org.openmrs.module.queue.model.QueueEntry; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cioan - note, I removed all of the mocking in favor of just creating and populating the actual objects, they are just beans. |
||
Patient patient = new Patient(); | ||
Concept conceptStatus = new Concept(); | ||
Concept conceptPriority = new Concept(); | ||
Date queueStartDate = new Date(); | ||
|
||
QueueEntry savedQueueEntry = new QueueEntry(); | ||
savedQueueEntry.setQueueEntryId(QUEUE_ENTRY_ID); | ||
savedQueueEntry.setQueue(queue); | ||
savedQueueEntry.setPatient(patient); | ||
savedQueueEntry.setStatus(conceptStatus); | ||
savedQueueEntry.setPriority(conceptPriority); | ||
savedQueueEntry.setStartedAt(queueStartDate); | ||
|
||
QueueEntry duplicateQueueEntry = new QueueEntry(); | ||
duplicateQueueEntry.setQueue(queue); | ||
duplicateQueueEntry.setPatient(patient); | ||
duplicateQueueEntry.setStartedAt(queueStartDate); | ||
|
||
QueueEntrySearchCriteria searchCriteria = new QueueEntrySearchCriteria(); | ||
searchCriteria.setPatient(patient); | ||
searchCriteria.setQueues(Collections.singletonList(queue)); | ||
|
||
when(dao.createOrUpdate(savedQueueEntry)).thenReturn(savedQueueEntry); | ||
when(dao.getQueueEntries(searchCriteria)).thenReturn(Collections.singletonList(savedQueueEntry)); | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cioan - note, I added this test to ensure that saving the same queue entry multiple times doesn't fail validation |
||
|
||
// Should hit a validation error if a new queue entry is saved with overlapping start date | ||
try { | ||
queueEntryService.saveQueueEntry(duplicateQueueEntry); | ||
fail("Expected DuplicateQueueEntryException"); | ||
} | ||
catch (DuplicateQueueEntryException e) { | ||
assertThat(e.getMessage(), is("queue.entry.duplicate.patient")); | ||
} | ||
} | ||
|
||
@Test | ||
public void shouldVoidQueueEntry() { | ||
User user = new User(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/* | ||
* This Source Code Form is subject to the terms of the Mozilla Public License, | ||
* v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
* | ||
* 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.utils; | ||
|
||
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.is; | ||
|
||
import java.util.Date; | ||
|
||
import org.junit.Test; | ||
|
||
public class QueueUtilsTest { | ||
|
||
private static final Date NULL = null; | ||
|
||
private static final Date AUG_1 = QueueUtils.parseDate("2023-08-01 10:00:00"); | ||
|
||
private static final Date AUG_2 = QueueUtils.parseDate("2023-08-02 10:00:00"); | ||
|
||
private static final Date AUG_3 = QueueUtils.parseDate("2023-08-03 10:00:00"); | ||
|
||
private static final Date AUG_4 = QueueUtils.parseDate("2023-08-04 10:00:00"); | ||
|
||
@Test | ||
public void shouldReturnTrueIfDatesOverlap() { | ||
// Test that nulls are handled as open-ended dates | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
assertThat(QueueUtils.datesOverlap(NULL, NULL, NULL, NULL), is(true)); | ||
assertThat(QueueUtils.datesOverlap(NULL, AUG_2, AUG_3, AUG_4), is(false)); | ||
assertThat(QueueUtils.datesOverlap(AUG_1, NULL, AUG_3, AUG_4), is(true)); | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, NULL, AUG_4), is(true)); | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, AUG_3, NULL), is(false)); | ||
|
||
// Test that order of date periods does not matter | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, AUG_3, AUG_4), is(false)); | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_3, AUG_2, AUG_4), is(true)); | ||
assertThat(QueueUtils.datesOverlap(AUG_3, AUG_4, AUG_1, AUG_2), is(false)); | ||
assertThat(QueueUtils.datesOverlap(AUG_2, AUG_4, AUG_1, AUG_3), is(true)); | ||
|
||
// Test date overlaps without nulls | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, AUG_3, AUG_4), is(false)); // one before two | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, AUG_2, AUG_3), is(false)); // one ends when two begins | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_3, AUG_2, AUG_4), is(true)); // one ends within two | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_4, AUG_2, AUG_3), is(true)); // one ends after two ends | ||
assertThat(QueueUtils.datesOverlap(AUG_2, AUG_4, AUG_1, AUG_3), is(true)); // one starts within two | ||
assertThat(QueueUtils.datesOverlap(AUG_3, AUG_4, AUG_1, AUG_2), is(false)); // one after two | ||
assertThat(QueueUtils.datesOverlap(AUG_1, AUG_2, AUG_1, AUG_3), is(true)); // one starts when two starts | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cioan - note that this was missing and I've added it in - that we need to allow re-saving a saved queue entry. Without this you'd get a validation error trying to save / update the same queue entry.