Skip to content
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

Merged
merged 4 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Copy link
Member

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.

if (!qe.equals(queueEntry)) {
if (QueueUtils.datesOverlap(qe.getStartedAt(), qe.getEndedAt(), qe.getStartedAt(), qe.getEndedAt())) {
throw new DuplicateQueueEntryException("queue.entry.duplicate.patient");
Copy link
Member

Choose a reason for hiding this comment

The 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);
}

Expand Down
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;
Copy link
Member

Choose a reason for hiding this comment

The 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);
}
}
24 changes: 24 additions & 0 deletions api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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;
}
}
}
11 changes: 11 additions & 0 deletions api/src/main/resources/messages.properties
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
11 changes: 11 additions & 0 deletions api/src/main/resources/messages_es.properties
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
11 changes: 11 additions & 0 deletions api/src/main/resources/messages_fr.properties
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
11 changes: 11 additions & 0 deletions api/src/main/resources/messages_ht.properties
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
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -109,6 +109,48 @@ public void shouldCreateNewQueueEntryRecord() {
assertThat(result.getPriority(), is(conceptPriority));
}

@Test
public void shouldNotCreateDuplicateOverlappingQueueEntryRecords() {
Queue queue = new Queue();
Copy link
Member

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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();
Expand Down
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
Copy link
Member

Choose a reason for hiding this comment

The 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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -179,11 +179,8 @@ public Object assignTicketToServicePoint(HttpServletRequest request) throws Exce
JsonNode actualObj = mapper.readTree(requestBody);

if (!actualObj.has("ticketNumber")) {
return new ResponseEntity<Object>(
"No ticketNumber passed, skipping ticket assignment",
new HttpHeaders(),
HttpStatus.OK
);
String msg = "No ticketNumber passed, skipping ticket assignment";
return new ResponseEntity<Object>(msg, new HttpHeaders(), HttpStatus.OK);
}

String servicePointName = actualObj.get("servicePointName").textValue();
Expand Down
Loading