diff --git a/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java b/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java index c450a74c..0c541187 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java +++ b/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java @@ -85,7 +85,7 @@ public void handle(Patient patient, User originalVoidingUser, Date originalVoide /** * @return true if the entry was voided together with the patient and can be restored with it. An - * entry on a voided visit was taken down by VisitWithQueueEntriesSaveHandler, not by this + * entry on a voided visit was taken down by VisitWithQueueEntriesVoidHandler, not by this * cascade; restoring it would put an active entry back on a visit that is still voided. */ private static boolean shouldRestore(QueueEntry qe, User originalVoidingUser, Date originalVoidedDate) { diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java new file mode 100644 index 00000000..04eefe03 --- /dev/null +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -0,0 +1,117 @@ +/* + * 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.api; + +import java.util.List; + +import org.aopalliance.intercept.MethodInterceptor; +import org.aopalliance.intercept.MethodInvocation; +import org.openmrs.Visit; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.module.queue.utils.PrivilegeConstants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.TransactionStatus; + +/** + * Purges the queue entries of a visit before {@link org.openmrs.api.VisitService#purgeVisit} + * deletes it. Core's purge cascade knows nothing about queue entries, so without this the delete + * fails on the queue_entry foreign key to visit. + *

+ * Purging cannot be done from a handler. RequiredDataAdvice dispatches handlers only for method + * names beginning save, create, void, unvoid, retire or unretire, so a purge reaches none of them, + * and the module hooks the service instead. Voiding is owned by + * {@link VisitWithQueueEntriesVoidHandler}, which core does reach on the {@code voidVisit} path. + *

+ * The cascade and core's own delete run in one transaction, taken out here, because the entries + * must not go without the visit. {@code purgeVisit} refuses a visit that still has encounters, and + * it does so after this advice has run, so the two have to stand or fall together. + *

+ * The transaction is taken out here rather than relied upon, because {@code Context.addAdvice} + * gives no guarantee that one is open by the time the advice runs. Whether one is depends on how + * the platform wraps {@code visitService}, which is not this module's to depend on and has already + * changed between supported versions. Propagation is the default, so this joins a transaction where + * the platform provides one and opens its own where it does not. Do not remove it on the strength + * of a platform that provides one: {@code VisitWithQueueEntriesDeleteAdviceTransactionTest} is what + * says whether the entries survive a refused delete, and it is green either way on such a platform. + */ +public class VisitWithQueueEntriesDeleteAdvice implements MethodInterceptor { + + private static final Logger log = LoggerFactory.getLogger(VisitWithQueueEntriesDeleteAdvice.class); + + @Override + public Object invoke(MethodInvocation invocation) throws Throwable { + Object[] args = invocation.getArguments(); + if (!"purgeVisit".equals(invocation.getMethod().getName()) || args.length == 0 || !(args[0] instanceof Visit)) { + return invocation.proceed(); + } + Visit visit = (Visit) args[0]; + if (visit.getVisitId() == null) { + return invocation.proceed(); + } + + // Looked up per call, not held in a field: ModuleUtil and DispatcherServlet re-run + // ModuleFactory.loadAdvice after a Spring refresh, and AdvicePoint hands them this same cached + // instance, so a field would outlive the context the bean came from + PlatformTransactionManager transactionManager = Context.getRegisteredComponent("transactionManager", + PlatformTransactionManager.class); + TransactionStatus transaction = transactionManager.getTransaction(TransactionDefinition.withDefaults()); + Object result; + try { + purgeQueueEntries(visit); + result = invocation.proceed(); + } + catch (Throwable t) { + try { + transactionManager.rollback(transaction); + } + catch (RuntimeException | Error rollbackFailure) { + // t is the only thing that says why the purge was refused, and the rollback failure is + // what propagates in its place. Suppressing it keeps it in the printed stack trace, so + // it reaches the server log; error responses built from getCause() will not show it. + rollbackFailure.addSuppressed(t); + throw rollbackFailure; + } + throw t; + } + transactionManager.commit(transaction); + return result; + } + + private void purgeQueueEntries(Visit visit) { + // Purging a visit is driven by a core service whose callers need not hold queue privileges, + // so grant them for the duration of this cascade, as the queue handlers do + Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.addProxyPrivilege(PrivilegeConstants.PURGE_QUEUE_ENTRIES); + try { + QueueEntryService queueEntryService = Context.getService(QueueEntryService.class); + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + // voided entries hold the same foreign key, and a voided patient hides them from the default search + criteria.setIncludedVoided(true); + List queueEntries = queueEntryService.getQueueEntries(criteria); + if (!queueEntries.isEmpty()) { + log.debug("Purging {} queue entries of visit {} being purged", queueEntries.size(), visit.getVisitId()); + } + for (QueueEntry qe : queueEntries) { + queueEntryService.purgeQueueEntry(qe); + log.trace("Purged queue entry {}", qe); + } + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.removeProxyPrivilege(PrivilegeConstants.PURGE_QUEUE_ENTRIES); + } + } +} diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java index 4a11ac37..feeacee9 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java @@ -19,15 +19,19 @@ import org.openmrs.annotation.Handler; import org.openmrs.api.context.Context; import org.openmrs.api.handler.SaveHandler; -import org.openmrs.api.handler.VoidHandler; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.module.queue.utils.PrivilegeConstants; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; +/** + * Ends a visit's open queue entries when the visit is stopped. Voiding is owned by + * {@link VisitWithQueueEntriesVoidHandler}, which core reaches on the {@code voidVisit} path, and + * purging by {@link VisitWithQueueEntriesDeleteAdvice}. + */ @Handler(supports = Visit.class) -public class VisitWithQueueEntriesSaveHandler implements SaveHandler, VoidHandler { +public class VisitWithQueueEntriesSaveHandler implements SaveHandler { private final Log log = LogFactory.getLog(getClass()); @@ -40,7 +44,7 @@ public VisitWithQueueEntriesSaveHandler(@Qualifier("queue.QueueEntryService") Qu @Override public void handle(Visit visit, User user, Date date, String s) { - // Voiding or unvoiding is driven by core services whose callers need not hold queue privileges, + // Stopping a visit is driven by core services whose callers need not hold queue privileges, // so grant them for the duration of this cascade, as core's PatientDataVoidHandler does Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); @@ -59,23 +63,6 @@ public void handle(Visit visit, User user, Date date, String s) { log.trace("Closed queue entry " + qe + " on " + visit.getStopDatetime()); } } - if (visit.getVisitId() != null && visit.getVoided()) { - QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); - criteria.setVisit(visit); - // The visit's patient may itself be voided, which would hide its entries from the default search - criteria.setIncludedVoided(true); - List queueEntries = queueEntryService.getQueueEntries(criteria); - for (QueueEntry qe : queueEntries) { - if (!qe.getVoided()) { - qe.setVoided(true); - qe.setVoidReason(visit.getVoidReason()); - qe.setVoidedBy(visit.getVoidedBy()); - qe.setDateVoided(visit.getDateVoided()); - queueEntryService.saveQueueEntry(qe); - } - log.trace("Voided queue entry " + qe + " on " + date); - } - } } finally { Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java new file mode 100644 index 00000000..27e067df --- /dev/null +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java @@ -0,0 +1,94 @@ +/* + * 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.api; + +import java.util.Date; +import java.util.List; + +import org.openmrs.User; +import org.openmrs.Visit; +import org.openmrs.annotation.Handler; +import org.openmrs.api.context.Context; +import org.openmrs.api.handler.VoidHandler; +import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.module.queue.utils.PrivilegeConstants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Voids all queue entries of a visit when that visit is voided. Core knows nothing about queue + * entries, so nothing in its void cascade touches them; without this handler a deleted visit would + * leave its entries active and the patient would stay in the service queue. + *

+ * The void state is read from the handler arguments rather than from {@code visit.getVoided()}, so + * this does not depend on running after core's {@code BaseVoidHandler}. That matters: both handlers + * carry the default {@code @Handler} order, {@code HandlerUtil} sorts stably, and the tie is broken + * by the iteration order of the map {@code ServiceContext.getRegisteredComponents} builds, which + * moves with the number of {@code VoidHandler} beans a deployment happens to register. Core's own + * {@code VisitVoidHandler} takes the same approach for a visit's encounters. + *

+ * {@code RequiredDataAdvice} passes every handler the same void date and reason it hands to + * {@code BaseVoidHandler}, so the entries carry the visit's own void stamp whichever runs first. + * Entries that were already voided keep the stamp they had, so an unvoid can tell them apart from + * entries taken down with the visit. + */ +@Handler(supports = Visit.class) +public class VisitWithQueueEntriesVoidHandler implements VoidHandler { + + private static final Logger log = LoggerFactory.getLogger(VisitWithQueueEntriesVoidHandler.class); + + private final QueueEntryService queueEntryService; + + @Autowired + public VisitWithQueueEntriesVoidHandler(@Qualifier("queue.QueueEntryService") QueueEntryService queueEntryService) { + this.queueEntryService = queueEntryService; + } + + @Override + public void handle(Visit visit, User voidingUser, Date voidedDate, String voidReason) { + if (visit.getVisitId() == null) { + return; + } + // Voiding is driven by core services whose callers need not hold queue privileges, so grant + // them for the duration of this cascade, as core's PatientDataVoidHandler does + Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + try { + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + // The visit's patient may itself be voided, which would hide its entries from the default search + criteria.setIncludedVoided(true); + List queueEntries = queueEntryService.getQueueEntries(criteria); + int voidedCount = 0; + for (QueueEntry qe : queueEntries) { + if (qe.getVoided()) { + continue; + } + qe.setVoided(true); + qe.setVoidReason(voidReason); + qe.setVoidedBy(voidingUser); + qe.setDateVoided(voidedDate); + queueEntryService.saveQueueEntry(qe); + voidedCount++; + log.trace("Voided queue entry {} on {}", qe, voidedDate); + } + if (voidedCount > 0) { + log.info("Voided {} queue entries of visit {} with reason: {}", voidedCount, visit.getVisitId(), voidReason); + } + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.removeProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + } + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java new file mode 100644 index 00000000..3ae3e3e7 --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -0,0 +1,127 @@ +/* + * 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.api; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Visit; +import org.openmrs.api.VisitService; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; + +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesDeleteAdviceTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + private final VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); + + private Visit visit; + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitService visitService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + visit = queueEntryService.getQueueEntryById(3).get().getVisit(); + // the module test harness does not register advice from config.xml, so register it here to + // exercise the same interceptor chain that production purges go through + Context.addAdvice(VisitService.class, advice); + } + + @After + public void tearDown() { + // the Spring context is shared across test classes, so this must not outlive the test + Context.removeAdvice(VisitService.class, advice); + } + + @Test + public void shouldPurgeQueueEntriesWhenVisitIsPurged() { + Integer visitId = visit.getVisitId(); + assertTrue(queueEntryService.getQueueEntryById(3).isPresent()); + + visitService.purgeVisit(visit); + Context.flushSession(); + + assertNull(visitService.getVisit(visitId)); + assertFalse(queueEntryService.getQueueEntryById(3).isPresent()); + } + + @Test + public void shouldPurgeQueueEntriesForUserWithoutQueuePrivileges() { + // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the + // core visit privileges so the advice has to obtain the queue privileges itself + Integer visitId = visit.getVisitId(); + Context.becomeUser("3-4"); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.PURGE_VISITS); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + try { + assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.PURGE_QUEUE_ENTRIES)); + visitService.purgeVisit(visit); + Context.flushSession(); + } + finally { + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.PURGE_VISITS); + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + authenticate(); + } + assertNull(visitService.getVisit(visitId)); + assertFalse(queueEntryService.getQueueEntryById(3).isPresent()); + } + + @Test + public void shouldPurgeVoidedQueueEntriesOfAPurgedVisit() { + // entry 10 on visit 101 is voided, and a voided entry is just as much of a foreign key to the visit + Visit visitWithVoidedEntry = visitService.getVisit(101); + assertTrue(queueEntryService.getQueueEntryById(10).isPresent()); + + visitService.purgeVisit(visitWithVoidedEntry); + Context.flushSession(); + + assertNull(visitService.getVisit(101)); + assertFalse(queueEntryService.getQueueEntryById(10).isPresent()); + } + + @Test + public void shouldLeaveOtherVisitServiceCallsAlone() { + // Context.addAdvice registers against the whole point, so every VisitService call in a server + // with the module installed reaches this advice and not just the purges. What is new is that an + // around interceptor has to proceed and hand back the result itself, where a before-advice did + // not. A no-arg method also covers the empty argument array it tolerates on the way past. + assertFalse(visitService.getAllVisitTypes().isEmpty()); + assertNotNull(visitService.getVisit(visit.getVisitId())); + assertTrue(queueEntryService.getQueueEntryById(3).isPresent()); + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java new file mode 100644 index 00000000..a658fd20 --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java @@ -0,0 +1,123 @@ +/* + * 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.api; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Encounter; +import org.openmrs.Visit; +import org.openmrs.api.APIException; +import org.openmrs.api.EncounterService; +import org.openmrs.api.LocationService; +import org.openmrs.api.VisitService; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +/** + * Covers what {@link VisitWithQueueEntriesDeleteAdviceTest} structurally cannot: that the queue + * entries the advice purges go only if the visit goes too. This needs its own class because the + * test has to run outside the transaction the other tests rely on. Inside one the cascade's deletes + * are still unflushed when the delete is refused and {@code Context.clearSession()} discards them, + * so the entry reads as present again whether or not anything was rolled back, and the assertion + * holds either way. + */ +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + private static final int QUEUE_ENTRY_ID = 3; + + private final VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitService visitService; + + @Autowired + private EncounterService encounterService; + + @Autowired + private LocationService locationService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + // the module test harness does not register advice from config.xml, so register it here to + // exercise the same interceptor chain that production purges go through + Context.addAdvice(VisitService.class, advice); + } + + @After + public void tearDown() { + Context.removeAdvice(VisitService.class, advice); + // This test runs outside a transaction, so everything it wrote is committed, including an + // encounter that no dataset would replace. Wipe it rather than leave it to whatever runs next. + deleteAllData(); + } + + @Test + @Transactional(propagation = Propagation.NOT_SUPPORTED) + public void shouldKeepQueueEntriesWhenCoreRefusesToPurgeTheVisit() { + Visit visit = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get().getVisit(); + Integer visitId = visit.getVisitId(); + giveTheVisitAnEncounter(visit); + assertFalse("the visit needs an encounter for core to refuse the purge", + encounterService.getEncountersByVisit(visit, true).isEmpty()); + + // core checks for encounters only once this advice has already run + APIException refused = assertThrows(APIException.class, () -> visitService.purgeVisit(visit)); + // pin what refused it, so this cannot pass on an exception raised before the cascade ran at all + // (the wording is core's Visit.purge.inUse) + assertThat(refused.getMessage(), containsString("encounters")); + Context.clearSession(); + + assertNotNull("the visit survived, so its queue entries must too", visitService.getVisit(visitId)); + assertTrue("the refused purge must not have taken the queue entry down with it", + queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).isPresent()); + } + + private void giveTheVisitAnEncounter(Visit visit) { + Encounter encounter = new Encounter(); + encounter.setPatient(visit.getPatient()); + encounter.setEncounterType(encounterService.getEncounterType(1)); + encounter.setEncounterDatetime(visit.getStartDatetime()); + encounter.setLocation(locationService.getLocation(1)); + encounter.setVisit(visit); + encounterService.saveEncounter(encounter); + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java index 820f57e5..830e430c 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java @@ -10,11 +10,8 @@ package org.openmrs.module.queue.api; import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; import java.util.Arrays; import java.util.Date; @@ -25,7 +22,6 @@ import org.junit.Test; import org.openmrs.Visit; import org.openmrs.api.VisitService; -import org.openmrs.api.context.Context; import org.openmrs.module.queue.SpringTestConfiguration; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.test.BaseModuleContextSensitiveTest; @@ -86,78 +82,4 @@ public void shouldEndQueueEntriesIfVisitIsStopped() { assertThat(visit.getStopDatetime(), equalTo(stopDate)); assertThat(queueEntry.getEndedAt(), equalTo(stopDate)); } - - @Test - public void shouldNotVoidQueueEntriesIfVisitIsNotVoided() { - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - visit = visitService.saveVisit(visit); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - } - - @Test - public void shouldVoidQueueEntriesIfVisitIsVoided() { - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - visit.setVoided(true); - String voidReason = "for testing"; - visit.setVoidReason(voidReason); - visit = visitService.saveVisit(visit); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertTrue(visit.getVoided()); - assertThat(visit.getVoidReason(), equalTo(voidReason)); - assertNotNull(visit.getDateVoided()); - assertNotNull(visit.getVoidedBy()); - assertTrue(queueEntry.getVoided()); - assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); - assertThat(queueEntry.getDateVoided(), equalTo(visit.getDateVoided())); - assertThat(queueEntry.getVoidedBy(), equalTo(visit.getVoidedBy())); - } - - @Test - public void shouldVoidQueueEntriesWhenVisitIsVoidedThroughVoidVisit() { - // voidVisit is the path REST and patient deletion use. Whether this handler sees the visit as - // voided there depends on it running after core's BaseVoidHandler, so pin the outcome. - assertFalse(queueEntry.getVoided()); - String voidReason = "for testing"; - visit = visitService.voidVisit(visit, voidReason); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertTrue(visit.getVoided()); - assertTrue(queueEntry.getVoided()); - assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); - assertThat(queueEntry.getDateVoided(), equalTo(visit.getDateVoided())); - assertThat(queueEntry.getVoidedBy(), equalTo(visit.getVoidedBy())); - } - - @Test - public void shouldVoidQueueEntriesForUserWithoutQueuePrivileges() { - // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the - // core visit privileges so the handler has to obtain the queue privileges itself - Context.becomeUser("3-4"); - Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); - Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); - try { - assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.GET_QUEUE_ENTRIES)); - visit = visitService.voidVisit(visit, "for testing"); - } - finally { - Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); - Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); - authenticate(); - } - assertTrue(visit.getVoided()); - assertTrue(queueEntryService.getQueueEntryById(queueEntry.getId()).get().getVoided()); - } - - @Test - public void shouldVoidOverlappingQueueEntriesInTheSameQueueIfVisitIsVoided() { - executeDataSet("org/openmrs/module/queue/api/dao/QueueEntryDaoTest_overlappingEntriesInitialDataset.xml"); - visit.setVoided(true); - visit.setVoidReason("for testing"); - visitService.saveVisit(visit); - assertTrue(queueEntryService.getQueueEntryById(3).get().getVoided()); - assertTrue(queueEntryService.getQueueEntryById(12).get().getVoided()); - } } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java new file mode 100644 index 00000000..5f9c07fc --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java @@ -0,0 +1,196 @@ +/* + * 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.api; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.Date; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Visit; +import org.openmrs.api.VisitService; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; + +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesVoidHandlerTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + // Visit 102 owns the single active queue entry 3 + private static final int VISIT_ID = 102; + + private static final int QUEUE_ENTRY_ID = 3; + + // Visit 101 owns queue entries 1 (ended), 2 (active) and 10 (voided on its own) + private static final int VISIT_WITH_VOIDED_ENTRY_ID = 101; + + private Visit visit; + + private QueueEntry queueEntry; + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitWithQueueEntriesVoidHandler handler; + + @Autowired + private VisitService visitService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + queueEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + visit = queueEntry.getVisit(); + } + + @Test + public void shouldVoidQueueEntriesWhenVisitIsVoided() { + assertFalse(queueEntry.getVoided()); + String voidReason = "for testing"; + + visitService.voidVisit(visit, voidReason); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_ID); + QueueEntry voidedEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + assertTrue(voidedVisit.getVoided()); + assertTrue(voidedEntry.getVoided()); + assertThat(voidedEntry.getVoidReason(), equalTo(voidReason)); + assertThat(voidedEntry.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + assertThat(voidedEntry.getVoidedBy(), equalTo(voidedVisit.getVoidedBy())); + } + + @Test + public void shouldVoidQueueEntriesBeforeTheVisitItselfIsFlaggedVoided() { + // This is the state the handler sees whenever it is ordered ahead of core's BaseVoidHandler, and + // nothing pins that order: the two carry the same @Handler order, HandlerUtil sorts stably, and + // the tie falls to the iteration order of the map ServiceContext.getRegisteredComponents builds, + // which moves with the number of VoidHandler beans a deployment registers. Taking the void state + // from the arguments rather than from the visit is what makes the cascade independent of that. + assertFalse(visit.getVoided()); + assertFalse(queueEntry.getVoided()); + // whole seconds: date_voided does not round-trip milliseconds + Date voidedDate = new Date(System.currentTimeMillis() / 1000 * 1000); + + handler.handle(visit, Context.getAuthenticatedUser(), voidedDate, "for testing"); + Context.flushSession(); + Context.clearSession(); + + QueueEntry voidedEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + assertFalse(visitService.getVisit(VISIT_ID).getVoided()); + assertTrue(voidedEntry.getVoided()); + assertThat(voidedEntry.getVoidReason(), equalTo("for testing")); + assertThat(voidedEntry.getDateVoided().getTime(), equalTo(voidedDate.getTime())); + assertThat(voidedEntry.getVoidedBy(), equalTo(Context.getAuthenticatedUser())); + } + + @Test + public void shouldVoidQueueEntriesForUserWithoutQueuePrivileges() { + // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the + // core visit privileges so the handler has to obtain the queue privileges itself + Context.becomeUser("3-4"); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + try { + assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.GET_QUEUE_ENTRIES)); + visitService.voidVisit(visit, "for testing"); + Context.flushSession(); + } + finally { + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + authenticate(); + } + Context.clearSession(); + assertTrue(visitService.getVisit(VISIT_ID).getVoided()); + assertTrue(queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get().getVoided()); + } + + @Test + public void shouldVoidOverlappingQueueEntriesInTheSameQueueIfVisitIsVoided() { + executeDataSet("org/openmrs/module/queue/api/dao/QueueEntryDaoTest_overlappingEntriesInitialDataset.xml"); + + visitService.voidVisit(visit, "for testing"); + Context.flushSession(); + Context.clearSession(); + + assertTrue(queueEntryService.getQueueEntryById(3).get().getVoided()); + assertTrue(queueEntryService.getQueueEntryById(12).get().getVoided()); + } + + @Test + public void shouldLeaveTheStampOfAnAlreadyVoidedQueueEntryAlone() { + // entry 10 was voided on its own, so an unvoid must be able to tell it apart from the entries + // that went down with the visit + QueueEntry alreadyVoided = queueEntryService.getQueueEntryById(10).get(); + assertTrue(alreadyVoided.getVoided()); + Date originalDateVoided = alreadyVoided.getDateVoided(); + String originalVoidReason = alreadyVoided.getVoidReason(); + assertNotNull(originalDateVoided); + + visitService.voidVisit(visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID), "for testing"); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID); + alreadyVoided = queueEntryService.getQueueEntryById(10).get(); + assertThat(alreadyVoided.getDateVoided().getTime(), equalTo(originalDateVoided.getTime())); + assertThat(alreadyVoided.getVoidReason(), equalTo(originalVoidReason)); + assertThat(alreadyVoided.getDateVoided().getTime(), not(equalTo(voidedVisit.getDateVoided().getTime()))); + + QueueEntry cascaded = queueEntryService.getQueueEntryById(2).get(); + assertTrue(cascaded.getVoided()); + assertThat(cascaded.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + } + + @Test + public void shouldVoidQueueEntriesThatHadAlreadyEnded() { + // An entry is ended rather than removed when the patient is served or moved on to another queue, + // so a visit deleted after that has ended entries on it and they belong to a visit that now never + // happened. Nothing else pins this: the cascade covers them only because + // QueueEntrySearchCriteria.isEnded defaults to null, and the sibling save handler sets it to false. + QueueEntry ended = queueEntryService.getQueueEntryById(1).get(); + assertNotNull(ended.getEndedAt()); + assertFalse(ended.getVoided()); + + visitService.voidVisit(visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID), "for testing"); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID); + ended = queueEntryService.getQueueEntryById(1).get(); + assertTrue(ended.getVoided()); + assertThat(ended.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + } +} diff --git a/omod/src/main/resources/config.xml b/omod/src/main/resources/config.xml index db9cd7e6..9a343b8c 100644 --- a/omod/src/main/resources/config.xml +++ b/omod/src/main/resources/config.xml @@ -35,6 +35,11 @@ org.openmrs.module.legacyui + + org.openmrs.api.VisitService + org.openmrs.module.queue.api.VisitWithQueueEntriesDeleteAdvice + + ${project.parent.artifactId}.statusConceptSetName Status diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java new file mode 100644 index 00000000..ab37cb71 --- /dev/null +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -0,0 +1,110 @@ +/* + * 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; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; + +import java.io.File; +import java.lang.reflect.Modifier; + +import org.aopalliance.aop.Advice; +import org.junit.Test; +import org.springframework.aop.Advisor; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; + +/** + * The {@code } elements in config.xml are what registers this module's AOP advice in a + * deployed server, and this test is the build's only check on them: module tests register advice + * themselves rather than going through config.xml. + *

+ * The checks below mirror what {@code ModuleFactory.loadAdvice} and + * {@code AdvicePoint.getClassInstance} do with the two values, so that a config.xml this build + * accepts is one a server can load. The type check earns its place twice over: {@code loadAdvice} + * casts to {@code Advice} without testing, catching only {@code ClassNotFoundException} and + * {@code NoClassDefFoundError}, and its caller in {@code ModuleUtil.refreshApplicationContext} has + * no catch at all, so a wrong type propagates out of the post-refresh loop and the started modules + * behind this one never get their advice loaded. + */ +public class ModuleAdviceConfigTest { + + @Test + public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception { + NodeList adviceElements = queueConfigXml().getElementsByTagName("advice"); + assertTrue("config.xml should declare at least one advice", adviceElements.getLength() > 0); + + for (int i = 0; i < adviceElements.getLength(); i++) { + Element advice = (Element) adviceElements.item(i); + String point = childText(advice, "point"); + String adviceClassName = childText(advice, "class"); + assertNotNull("advice needs a point", point); + assertNotNull("advice needs a class", adviceClassName); + + // The point is the service interface ServiceContext keys its map by. Context.addAdvice does + // services.get(cls) with no null check, so naming an implementation here throws out of the + // same post-refresh loop a wrong advice type does. Being an interface is necessary and not + // sufficient; only a running Context could say whether it is a registered service. + Class pointClass = Class.forName(point); + assertTrue(point + " must be the service interface, not an implementation class", pointClass.isInterface()); + + // AdvicePoint.getClassInstance loads the class and calls its public no-arg constructor, and + // ModuleFactory.loadAdvice then registers the instance as an Advisor if it is one and as an + // Advice otherwise, so either type is legal here. The constructor is looked up rather than + // called, so an advice that legitimately touches Context on construction is not failed for it, + // which is why abstractness is checked separately instead of by trying to instantiate. + Class adviceClass = Class.forName(adviceClassName); + assertFalse(adviceClassName + " must not be abstract for AdvicePoint to instantiate it", + Modifier.isAbstract(adviceClass.getModifiers())); + try { + adviceClass.getConstructor(); + } + catch (NoSuchMethodException e) { + throw new AssertionError( + adviceClassName + " needs a public no-arg constructor for AdvicePoint to instantiate it", e); + } + assertTrue( + adviceClassName + " must implement " + Advice.class.getName() + " or " + Advisor.class.getName() + + " to be registered on " + point, + Advice.class.isAssignableFrom(adviceClass) || Advisor.class.isAssignableFrom(adviceClass)); + } + } + + /** + * Reads this module's own config.xml from the source tree. Reading it off the classpath instead + * would mean parsing every {@code config.xml} there to find this one, and a required module ships + * one: a malformed or entity-bearing document of somebody else's then decides whether this test + * passes. The source copy is the one a typo lands in, and reading it requires the {@code } + * elements to stay free of the {@code ${...}} tokens the rest of the file uses, since Maven would + * resolve those in the packaged copy and this test would not. + */ + private Document queueConfigXml() throws Exception { + // surefire runs with the module directory as its working directory + File configXml = new File("src/main/resources/config.xml"); + assertTrue("expected to find " + configXml.getAbsolutePath(), configXml.isFile()); + + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setExpandEntityReferences(false); + DocumentBuilder builder = factory.newDocumentBuilder(); + return builder.parse(configXml); + } + + private static String childText(Element parent, String tagName) { + NodeList children = parent.getElementsByTagName(tagName); + return children.getLength() == 0 ? null : children.item(0).getTextContent().trim(); + } +}