Repository navigation
O3-5459: Void and purge a visit's queue entries when the visit is deleted #96
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
Open
dkayiwa
wants to merge
13
commits into
main
Choose a base branch
from
O3-5459/void-purge-queue-entries-on-visit-delete
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
39195a0
O3-5459: Void/purge queue entries when associated visit is voided or …
dkayiwa 3fc1731
Use SLF4J logging instead of Commons Logging
dkayiwa 36b2040
Improve test coverage for void and purge advice
dkayiwa ae7f8fa
Merge branch 'main' into O3-5459/void-purge-queue-entries-on-visit-de…
dkayiwa 1c3a5e4
O3-5459: Proxy the privilege purgeQueueEntry actually requires
dkayiwa 95ed745
O3-5459: Flush in the purge tests so they reach the foreign key
dkayiwa 77ff35b
O3-5459: Void queue entries from a handler that ignores handler order
dkayiwa 3c43c4f
O3-5459: Purge queue entries in the same transaction as the visit
dkayiwa 9328cd6
O3-5459: Tighten the purge cascade's tests and what its comments claim
dkayiwa 9411e0a
O3-5459: Accept an Advisor, keep the refusal reason, fix two comments
dkayiwa d51817d
O3-5459: Stop explaining the proxy stack, and read our own config.xml
dkayiwa 2546daf
O3-5459: Stop attributing core's log lines, and state the filtering c…
dkayiwa 57e8eec
O3-5459: Guard the advice point, not just that it loads
dkayiwa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
117 changes: 117 additions & 0 deletions
117
api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
| * <p> | ||
| * 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. | ||
| * <p> | ||
| * 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. | ||
| * <p> | ||
| * 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<QueueEntry> 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); | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
| * <p> | ||
| * 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. | ||
| * <p> | ||
| * {@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<Visit> { | ||
|
|
||
| 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<QueueEntry> 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); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing performs that discrimination today, so this claims more than the module does.
PatientWithQueueEntriesUnvoidHandler.shouldRestoreis the only unvoid that looks at queue entries, and it rejects any entry whose visit is voided (PatientWithQueueEntriesUnvoidHandler.java:95) before it gets as far as comparing stamps, so for entries taken down with the visit the stamp plays no part. The stamp comparison below that check is answering a different question, telling the patient cascade's own entries apart from independently voided ones.The reason is that there is no
UnvoidHandler<Visit>anywhere in the module, which makes this cascade the odd one out among its siblings: core pairsVisitVoidHandlerwithVisitUnvoidHandler, and the module pairsPatientWithQueueEntriesVoidHandlerwith an unvoid handler of its own.I'm not asking for that handler here, since O3-5459 already scopes it as a follow-up to be filed separately ("handle
unvoidVisitso restoring a deleted visit also restores its queue entries"). It is reachable, for what it's worth:VisitResource1_9.undeletecallsunvoidVisit, andPOST /visit/{uuid}with{"voided": false}gets there. I searched O3 and couldn't find a ticket for it yet. What I would change is just the sentence, so that the next person readingshouldRestore's own comment (which now points at this class) isn't left to work the gap out for themselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{"voided": false}doesn't reachunvoidVisit, so the aside above names the wrong request. The Javadoc suggestion doesn't depend on it, so nothing changes in the PR.MainResourceController.updateonly routes a POST toundeletewhen the body is exactly{"deleted": "false"}, and every other body goes throughDelegatingCrudResource.updatetosaveVisit.{"voided": false}is the body the chart's "Restore visit" sends (restoreVisitinvisit.resource.tsx). I ran both through webservices.rest 3.0.0's own controller test harness with an advice onVisitServicerecording each call:{"voided": false}reachedsaveVisitand neverunvoidVisit, and{"deleted": "false"}reachedunvoidVisit.Where it does matter is the unvoid follow-up. When that gets filed, could it note that an
UnvoidHandler<Visit>on its own won't fire when the chart restores a visit?