Skip to content

Commit

Permalink
Updates following code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@mseaton
mseaton committed Dec 6, 2023
1 parent 7454f58 commit 863d46f
Show file tree
Hide file tree
Showing 7 changed files with 89 additions and 171 deletions.
18 changes: 6 additions & 12 deletions api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,10 @@
import org.openmrs.api.VisitService;
import org.openmrs.api.context.Context;
import org.openmrs.api.impl.BaseOpenmrsService;
import org.openmrs.messagesource.MessageSourceService;
import org.openmrs.module.queue.api.DuplicateQueueEntryException;
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;
Expand All @@ -50,8 +49,6 @@ public class QueueEntryServiceImpl extends BaseOpenmrsService implements QueueEn

private VisitService visitService;

private MessageSourceService messageSourceService;

public void setDao(QueueEntryDao<QueueEntry> dao) {
this.dao = dao;
}
Expand All @@ -60,10 +57,6 @@ public void setVisitService(VisitService visitService) {
this.visitService = visitService;
}

public void setMessageSourceService(MessageSourceService messageSourceService) {
this.messageSourceService = messageSourceService;
}

/**
* @see QueueEntryService#getQueueEntryByUuid(String)
*/
Expand Down Expand Up @@ -96,10 +89,11 @@ public QueueEntry saveQueueEntry(QueueEntry queueEntry) {
searchCriteria.setPatient(queueEntry.getPatient());
searchCriteria.setQueues(Collections.singletonList(queueEntry.getQueue()));
List<QueueEntry> queueEntries = getQueueEntries(searchCriteria);
for (QueueEntry entry : queueEntries) {
if (QueueUtils.datesOverlap(entry.getStartedAt(), entry.getEndedAt(), queueEntry.getStartedAt(),
queueEntry.getEndedAt())) {
throw new DuplicateQueueEntryException(messageSourceService.getMessage("queue.entry.duplicate.patient"));
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");
}
}
}
return dao.createOrUpdate(queueEntry);
Expand Down
2 changes: 1 addition & 1 deletion ...eue/api/DuplicateQueueEntryException.java → ...ception/DuplicateQueueEntryException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* 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;
package org.openmrs.module.queue.exception;

import org.openmrs.api.APIException;

Expand Down
29 changes: 18 additions & 11 deletions api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,19 +76,26 @@ public static double computeAverageWaitTimeInMinutes(List<QueueEntry> queueEntri
}

/**
* @param date startDate1, endDate1 - the start and end date of one timeframe
* @param date startDate2, endDate2 - the start and end date of second timeframe
* @return boolean - indicating whether or not the timeframes overlap
* @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) {
if (startDate1 != null && startDate2 != null) {
if (!((startDate2.before(startDate1) && (endDate2 != null && endDate2.compareTo(startDate1) <= 0))
|| ((endDate1 != null) && (startDate2.compareTo(endDate1) >= 0)))) {
//if the endDate2 is NOT before the startDate1 OR the startDate2 is NOT after the entry endDate1,
//then the time intervals overlap
return true;
}
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;
}
return false;
}
}
1 change: 0 additions & 1 deletion api/src/main/resources/moduleApplicationContext.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
<bean class="org.openmrs.module.queue.api.impl.QueueEntryServiceImpl">
<property name="dao" ref="queueEntryDao"/>
<property name="visitService" ref="visitService"/>
<property name="messageSourceService" ref="messageSourceService"/>
</bean>
</property>
<property name="preInterceptors" ref="serviceInterceptors"/>
Expand Down
79 changes: 29 additions & 50 deletions api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,10 @@
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;
Expand All @@ -41,10 +36,10 @@
import org.openmrs.api.VisitService;
import org.openmrs.api.context.Context;
import org.openmrs.api.context.UserContext;
import org.openmrs.messagesource.MessageSourceService;
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 All @@ -63,9 +58,6 @@ public class QueueEntryServiceTest {
@Mock
private VisitService visitService;

@Mock
private MessageSourceService messageSourceService;

@Captor
ArgumentCaptor<QueueEntrySearchCriteria> queueEntrySearchCriteriaArgumentCaptor;

Expand All @@ -75,7 +67,6 @@ public void setupMocks() {
queueEntryService = new QueueEntryServiceImpl();
queueEntryService.setDao(dao);
queueEntryService.setVisitService(visitService);
queueEntryService.setMessageSourceService(messageSourceService);
}

@Test
Expand Down Expand Up @@ -120,56 +111,44 @@ public void shouldCreateNewQueueEntryRecord() {

@Test
public void shouldNotCreateDuplicateOverlappingQueueEntryRecords() {
Queue queue = mock(Queue.class);
QueueEntry queueEntry = mock(QueueEntry.class);

Patient patient = mock(Patient.class);
Concept conceptStatus = mock(Concept.class);
Concept conceptPriority = mock(Concept.class);
Queue queue = new Queue();
Patient patient = new Patient();
Concept conceptStatus = new Concept();
Concept conceptPriority = new Concept();
Date queueStartDate = new Date();

when(queueEntry.getQueueEntryId()).thenReturn(QUEUE_ENTRY_ID);
when(queueEntry.getQueue()).thenReturn(queue);
when(queueEntry.getStatus()).thenReturn(conceptStatus);
when(queueEntry.getPriority()).thenReturn(conceptPriority);
when(queueEntry.getPatient()).thenReturn(patient);
when(queueEntry.getStartedAt()).thenReturn(queueStartDate);
when(queueEntry.getEndedAt()).thenReturn(null);
when(dao.createOrUpdate(queueEntry)).thenReturn(queueEntry);

QueueEntry result = queueEntryService.saveQueueEntry(queueEntry);
assertThat(result, notNullValue());
assertThat(result.getQueueEntryId(), is(QUEUE_ENTRY_ID));
assertThat(result.getQueue(), is(queue));
assertThat(result.getStatus(), is(conceptStatus));
assertThat(result.getPriority(), is(conceptPriority));
assertThat(result.getPatient(), is(patient));
assertThat(result.getStartedAt(), is(queueStartDate));
QueueEntry savedQueueEntry = new QueueEntry();
savedQueueEntry.setQueueEntryId(QUEUE_ENTRY_ID);
savedQueueEntry.setQueue(queue);
savedQueueEntry.setPatient(patient);
savedQueueEntry.setStatus(conceptStatus);
savedQueueEntry.setPriority(conceptPriority);
savedQueueEntry.setStartedAt(queueStartDate);

//attempt to add a second queue entry for the same patient to the same queue
Date secondQueueStartDate = new Date();
QueueEntry secondQueueEntry = mock(QueueEntry.class);
when(secondQueueEntry.getQueue()).thenReturn(queue);
when(secondQueueEntry.getPatient()).thenReturn(patient);
when(secondQueueEntry.getStartedAt()).thenReturn(secondQueueStartDate);
when(messageSourceService.getMessage("queue.entry.duplicate.patient")).thenReturn("Patient already in the queue");
QueueEntry duplicateQueueEntry = new QueueEntry();
duplicateQueueEntry.setQueue(queue);
duplicateQueueEntry.setPatient(patient);
duplicateQueueEntry.setStartedAt(queueStartDate);

QueueEntrySearchCriteria searchCriteria = new QueueEntrySearchCriteria();
searchCriteria.setPatient(secondQueueEntry.getPatient());
searchCriteria.setPatient(patient);
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));

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

// Should hit a validation error if a new queue entry is saved with overlapping start date
try {
//attempt to add the same queue entry with the same patient to the same queue
queueEntryService.saveQueueEntry(secondQueueEntry);
//if the exception is not thrown, the test will fail
queueEntryService.saveQueueEntry(duplicateQueueEntry);
fail("Expected DuplicateQueueEntryException");
}
catch (DuplicateQueueEntryException e) {
assertThat(e.getMessage(), is(messageSourceService.getMessage("queue.entry.duplicate.patient")));
assertThat(e.getMessage(), is("queue.entry.duplicate.patient"));
}

}

@Test
Expand Down
124 changes: 33 additions & 91 deletions api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,102 +12,44 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

import java.util.Calendar;
import java.util.Date;

import org.junit.Test;

public class QueueUtilsTest {

@Test
public void shouldReturnFalseWhenDatesAreNull() {
assertThat(QueueUtils.datesOverlap(null, null, null, null), is(false));
}

@Test
public void shouldReturnFalseWhenSecondTimeIntervalIsBeforeFirstTimeInterval() {
// time interval startDate2-endDate2(t-12hr, t-10hr) is to the left of time interval startDate1-endDate1
Calendar calendar = Calendar.getInstance();
Date startDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, -12);
Date startDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, 2);
Date endDate2 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, null, startDate2, endDate2), is(false));
}

@Test
public void shouldReturnFalseWhenSecondTimeIntervalEndsWhenFirstTimeIntervalStarts() {
// time interval startDate2-endDate2(t-12hr, t) ends when time interval startDate1-endDate1(t) starts
Calendar calendar = Calendar.getInstance();
Date startDate1 = calendar.getTime();
Date endDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, -12);
Date startDate2 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, null, startDate2, endDate2), is(false));
}

@Test
public void shouldReturnTrueWhenDatesOverlap() {
Calendar calendar = Calendar.getInstance();
Date startDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, 1);
Date startDate2 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, null, startDate2, null), is(true));
}

@Test
public void shouldReturnTrueWhenDateIntervalsAreBoundedAndInclusive() {
// Dt1 = t, t+12
// Dt2 = t+1,t+8
Calendar calendar = Calendar.getInstance();
Date startDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, 1);
Date startDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, 7);
Date endDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, 5);
Date endDate1 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, endDate1, startDate2, endDate2), is(true));
}

@Test
public void shouldReturnTrueWhenDateIntervalsAreBoundedAndOverlap() {
// DT1 = t, t+4
// DT2 = t+1, t+8
Calendar calendar = Calendar.getInstance();
Date startDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, 1);
Date startDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, 3);
Date endDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, 4);
Date endDate2 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, endDate1, startDate2, endDate2), is(true));
}

@Test
public void shouldReturnFalseWhenSecondTimeIntervalStartsAfterFirstTimeIntervalEndsl() {
// time interval startDate2-endDate2(t) is to the right of time interval startDate1-endDate1(t-12hr, t-10hr)
Calendar calendar = Calendar.getInstance();
Date startDate2 = calendar.getTime();
calendar.add(Calendar.HOUR, -10);
Date endDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, -2);
Date startDate1 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, endDate1, startDate2, null), is(false));
}

@Test
public void shouldReturnFalseWhenSecondTimeIntervalStartsWhenFirstTimeIntervalEndsl() {
// time interval startDate2(t) starts when first time interval startDate1-endDate1(t-12hr, t) ends
// DT1 = t-12, t
// DT2 = t
Calendar calendar = Calendar.getInstance();
Date startDate2 = calendar.getTime();
Date endDate1 = calendar.getTime();
calendar.add(Calendar.HOUR, -12);
Date startDate1 = calendar.getTime();
assertThat(QueueUtils.datesOverlap(startDate1, endDate1, startDate2, null), is(false));
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
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
}
}
7 changes: 2 additions & 5 deletions omod/src/main/java/org/openmrs/module/queue/web/Legacy1xRestController.java
View file
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

0 comments on commit 863d46f

Please sign in to comment.