From 863d46f24407e9682cba9fb6c4ad50450ecd5221 Mon Sep 17 00:00:00 2001 From: Michael Seaton Date: Wed, 6 Dec 2023 13:49:43 -0500 Subject: [PATCH] Updates following code review --- .../queue/api/impl/QueueEntryServiceImpl.java | 18 +-- .../DuplicateQueueEntryException.java | 2 +- .../module/queue/utils/QueueUtils.java | 29 ++-- .../resources/moduleApplicationContext.xml | 1 - .../queue/api/QueueEntryServiceTest.java | 79 ++++------- .../module/queue/utils/QueueUtilsTest.java | 124 +++++------------- .../queue/web/Legacy1xRestController.java | 7 +- 7 files changed, 89 insertions(+), 171 deletions(-) rename api/src/main/java/org/openmrs/module/queue/{api => exception}/DuplicateQueueEntryException.java (95%) diff --git a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java index b5e15dc..a92a4aa 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/queue/api/impl/QueueEntryServiceImpl.java @@ -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; @@ -50,8 +49,6 @@ public class QueueEntryServiceImpl extends BaseOpenmrsService implements QueueEn private VisitService visitService; - private MessageSourceService messageSourceService; - public void setDao(QueueEntryDao dao) { this.dao = dao; } @@ -60,10 +57,6 @@ public void setVisitService(VisitService visitService) { this.visitService = visitService; } - public void setMessageSourceService(MessageSourceService messageSourceService) { - this.messageSourceService = messageSourceService; - } - /** * @see QueueEntryService#getQueueEntryByUuid(String) */ @@ -96,10 +89,11 @@ public QueueEntry saveQueueEntry(QueueEntry queueEntry) { searchCriteria.setPatient(queueEntry.getPatient()); searchCriteria.setQueues(Collections.singletonList(queueEntry.getQueue())); List 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); diff --git a/api/src/main/java/org/openmrs/module/queue/api/DuplicateQueueEntryException.java b/api/src/main/java/org/openmrs/module/queue/exception/DuplicateQueueEntryException.java similarity index 95% rename from api/src/main/java/org/openmrs/module/queue/api/DuplicateQueueEntryException.java rename to api/src/main/java/org/openmrs/module/queue/exception/DuplicateQueueEntryException.java index e614d77..9019ebe 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/DuplicateQueueEntryException.java +++ b/api/src/main/java/org/openmrs/module/queue/exception/DuplicateQueueEntryException.java @@ -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; diff --git a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java index 5142b14..20a7e44 100644 --- a/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java +++ b/api/src/main/java/org/openmrs/module/queue/utils/QueueUtils.java @@ -76,19 +76,26 @@ public static double computeAverageWaitTimeInMinutes(List 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; } } diff --git a/api/src/main/resources/moduleApplicationContext.xml b/api/src/main/resources/moduleApplicationContext.xml index 32d1236..441f902 100644 --- a/api/src/main/resources/moduleApplicationContext.xml +++ b/api/src/main/resources/moduleApplicationContext.xml @@ -51,7 +51,6 @@ - diff --git a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java index c9ade39..351d6f1 100644 --- a/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java +++ b/api/src/test/java/org/openmrs/module/queue/api/QueueEntryServiceTest.java @@ -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; @@ -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; @@ -63,9 +58,6 @@ public class QueueEntryServiceTest { @Mock private VisitService visitService; - @Mock - private MessageSourceService messageSourceService; - @Captor ArgumentCaptor queueEntrySearchCriteriaArgumentCaptor; @@ -75,7 +67,6 @@ public void setupMocks() { queueEntryService = new QueueEntryServiceImpl(); queueEntryService.setDao(dao); queueEntryService.setVisitService(visitService); - queueEntryService.setMessageSourceService(messageSourceService); } @Test @@ -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 diff --git a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java index ec298d8..8c927e8 100644 --- a/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java +++ b/api/src/test/java/org/openmrs/module/queue/utils/QueueUtilsTest.java @@ -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 } } diff --git a/omod/src/main/java/org/openmrs/module/queue/web/Legacy1xRestController.java b/omod/src/main/java/org/openmrs/module/queue/web/Legacy1xRestController.java index 7ede308..2248d97 100644 --- a/omod/src/main/java/org/openmrs/module/queue/web/Legacy1xRestController.java +++ b/omod/src/main/java/org/openmrs/module/queue/web/Legacy1xRestController.java @@ -179,11 +179,8 @@ public Object assignTicketToServicePoint(HttpServletRequest request) throws Exce JsonNode actualObj = mapper.readTree(requestBody); if (!actualObj.has("ticketNumber")) { - return new ResponseEntity( - "No ticketNumber passed, skipping ticket assignment", - new HttpHeaders(), - HttpStatus.OK - ); + String msg = "No ticketNumber passed, skipping ticket assignment"; + return new ResponseEntity(msg, new HttpHeaders(), HttpStatus.OK); } String servicePointName = actualObj.get("servicePointName").textValue();