O3-5459: Void and purge a visit's queue entries when the visit is deleted

[dkayiwa]

Summary

Deleting a visit leaves its queue entries behind, so the patient stays in the service queue. Both halves of "delete" are affected, and both are fixed here:

• Void (DELETE /visit/{uuid}, which is what the O3 chart's "Delete visit" calls) left the entries active. This is the symptom O3-5459 reports.
• Purge (DELETE /visit/{uuid}?purge=true) failed outright on the queue_entry foreign key to visit, so the visit could not be deleted at all.

JIRA: O3-5459

Root cause

Void. The module already had a void cascade, in the VoidHandler half of VisitWithQueueEntriesSaveHandler, but that branch is guarded by visit.getVoided(). On the voidVisit path the flag is only set once core's BaseVoidHandler has run, and nothing pins the two in that order: both carry the default @Handler order, HandlerUtil sorts stably, and the tie falls to the iteration order of the HashMap that ServiceContext.getRegisteredComponents builds over bean names. That order moves with the number of VoidHandler beans a deployment registers, and @Handler(order = ...) cannot pin it either, since lower wins and the default is already Integer.MAX_VALUE.

Driving the real visitService.voidVisit, the outcome flips with the bean count. At six beans, which is core's five plus the single one the queue-3.0.0 tag ships, the queue handler goes first and the entry is left active with dateVoided null. At seven, core goes first and the entry is voided. The module's own test context has seven, which is why the existing tests never saw the failure.

Purge. Core's purge cascade knows nothing about queue entries, and no handler can cover it: RequiredDataAdvice dispatches only on method names beginning save, create, void, unvoid, retire or unretire, so a purge reaches none of them. That leaves advice on the service.

Purge, second half. Clearing the foreign key from a MethodBeforeAdvice commits that half before core has decided whether it will delete the visit at all, and VisitServiceImpl.purgeVisit refuses a visit that still has encounters, after the advice has run. Advice registered through Context.addAdvice gives no guarantee that a transaction is open when it runs, and on 2.7.x none is: the entry deletes committed, the refusal could not roll them back, so the queue rows went, the visit stayed, and the caller got an error saying nothing had been deleted. On 2.8.x the platform does provide a transaction, which is why this is version-dependent and why a test inside the usual test transaction cannot see it either way.

Changes

• Added VisitWithQueueEntriesVoidHandler, a VoidHandler<Visit> that voids a visit's queue entries. It reads the void user, date and reason from the handler arguments rather than off the visit, so handler order stops mattering. RequiredDataAdvice hands every handler the same values it gives BaseVoidHandler, so the entries still carry the visit's own void stamp. This is the shape PatientWithQueueEntriesVoidHandler already uses, and core's own VisitVoidHandler with it. Entries that were already voided keep the stamp they had.
• Retired the void branch of VisitWithQueueEntriesSaveHandler so the cascade has one owner. It now only ends open entries when a visit is stopped, and no longer implements VoidHandler<Visit>.
• Added VisitWithQueueEntriesDeleteAdvice, a MethodInterceptor on VisitService that purges a visit's queue entries and then lets core delete the visit, both inside one transaction it takes out itself, so the entries go only if the visit goes. Propagation is the default, so it joins a transaction where the platform provides one and opens its own where it does not. Voided entries are included, since they hold the same foreign key. The advice proxies Get Queue Entries and Purge Queue Entries for the duration of the cascade, so a caller holding Purge Visits but no queue privileges can still delete the visit. If the rollback itself fails, the exception saying why the purge was refused is attached to it with addSuppressed so that it still reaches the log.
• Registered the advice in config.xml via the <advice> element.

Testing

mvn install is green: 59 api, 74 omod, 123 integration tests, both against the 2.7.4 the pom defaults to and against 2.8.7.

VisitWithQueueEntriesVoidHandlerTest covers the void half. The one that pins the fix is shouldVoidQueueEntriesBeforeTheVisitItselfIsFlaggedVoided, which invokes the handler on a visit whose voided flag is still false, exactly what it sees when ordered ahead of BaseVoidHandler. It fails if the getVoided() guard comes back, while the tests that go through voidVisit stay green either way, since the test context lands on the favourable order. Only the direct-handler test can see that failure mode.

shouldVoidQueueEntriesThatHadAlreadyEnded covers an entry already ended by a queue transition, which is the shape a visit deleted after "serve patient" actually has. Adding an isEnded(false) filter to the cascade fails that test and only it.

VisitWithQueueEntriesDeleteAdviceTest covers the purge half, for an active entry, a voided entry, a caller without queue privileges, and the pass-through every other VisitService method now takes through the interceptor. The three purge tests fail on the foreign key without the advice. The module test harness does not register advice from config.xml, so the test registers it through Context.addAdvice, the same call ModuleFactory makes for a deployed module.

VisitWithQueueEntriesDeleteAdviceTransactionTest covers the atomicity, in its own class because it has to run outside that transaction. Inside one, the cascade's deletes are still unflushed when the delete is refused and Context.clearSession() discards them, so the entry reads as present whether or not anything rolled back. It gives a visit an encounter, expects core's refusal, and asserts the visit and the entry both survive. Stripping the transaction fails it on 2.7.4; on 2.8.x it is green either way, which is worth knowing before anyone simplifies the transaction away.

ModuleAdviceConfigTest walks the config.xml <advice> elements and does what ModuleFactory.loadAdvice and AdvicePoint.getClassInstance do with them: the point loads and is a service interface, and the class loads, is not abstract, has a public no-arg constructor, and is an Advice or an Advisor. Nothing else in the build reads those elements.

🤖 Generated with Claude Code

[RoshanMadival08]

great!!

One question: is there a risk of partial failure if purging one queue entry fails midway? A transactional guarantee would be worth confirming.

[denniskigen]

Reviewed and verified this end to end, including a live deployment. Verdict: works as described, and the advice approach is the right mechanism.

Live verification (RefApp stack, core 2.8.7, module built from this branch): DELETE /visit/{uuid}?reason=... voided the associated queue entry with the reason propagated (DB-verified), where queue 3.0.0 leaves it active, which is the O3-5459 bug. DELETE /visit/{uuid}?purge=true purged the queue entry and visit cleanly, where 3.0.0 fails on the queue_entry_visit_id_fk constraint. This also confirms the config.xml advice registration fires in a deployed module, which the integration tests can't exercise (as your test comments note).

On the root cause, to sharpen "does not reliably fire": the O3-4416 void branch in VisitWithQueueEntriesSaveHandler is guarded by visit.getVoided(), which is only true if core's BaseVoidHandler has already run. Both handlers carry the default @Handler order, and core returns tied handlers in HashMap iteration order via ServiceContext.getRegisteredComponents, so on the current RefApp the queue handler runs first and silently no-ops. Its test passes because it calls saveVisit() on a pre-voided visit, a path the real delete flow never takes. Your advice reads the method arguments directly, so it has no ordering dependency, and it is the only mechanism that can cover purgeVisit at all.

On @RoshanMadival08's question: it's better than transactional. ServiceContext.addAdvice appends this advice to the tail of the service proxy chain, inside the TransactionInterceptor, so the queue-entry writes and the visit void commit or roll back together. Notably, the mechanism this replaces had the weaker guarantee: RequiredDataAdvice is registered as a pre-interceptor (outside the transaction interceptor), so the old handler's saveQueueEntry calls committed independently of the visit operation.

Three non-blocking suggestions:

1. Consider retiring the void branch in VisitWithQueueEntriesSaveHandler once this merges. It isn't dead code, which is the problem: whether it fires depends on @Handler order ties resolved by HashMap iteration order, so it runs in some deployments (O3-5594 arose through its saveQueueEntry path) and silently no-ops in others (verified on the RefApp). With this advice in place it's redundant on every voidVisit path, and I could find no production caller that saves a pre-voided visit, so removing it would leave the behavior with a single deterministic owner. Its shouldVoidQueueEntriesIfVisitIsVoided test would go with it.
2. unvoidVisit is still unhandled, so restore-after-delete leaves entries voided. Seems fine as a follow-up ticket; the natural semantics would unvoid only entries whose void metadata matches the visit's.
3. The branch merges clean onto current main (checked, zero conflicts); worth a rebase before merge, and a multi-entry-per-visit test would round out coverage.

Small observation in favor of the implementation: using voidQueueEntry rather than saveQueueEntry keeps QueueEntryValidator out of the cascade path entirely, which sidesteps the class of multi-entry issues O3-5594 addressed.

For cross-reference: openmrs/openmrs-esm-patient-chart#3427 is a frontend workaround for the same ticket; once this merges and ships in a release, that PR can be closed.

[dkayiwa]

This grants the wrong privilege, and it needs fixing before merge. purgeQueueEntry is annotated @Authorized(PrivilegeConstants.PURGE_QUEUE_ENTRIES) (QueueEntryService.java:132), so a caller holding Purge Visits but not Purge Queue Entries still can't get through the cascade, which is exactly the case the comment above says this block is here to cover.

I ran it against this branch. With Context.becomeUser("3-4") and only Purge Visits and Get Encounters proxied, visitService.purgeVisit(visit) throws APIAuthenticationException: Privileges required: Purge Queue Entries out of the before-advice, so the visit doesn't get purged either. Swapping the constant here and in the finally on line 70 makes that same call succeed, which also shows MANAGE_QUEUE_ENTRIES is doing nothing on this path; the advice only calls getQueueEntries and purgeQueueEntry.

Suggested change

	Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES);
	Context.addProxyPrivilege(PrivilegeConstants.PURGE_QUEUE_ENTRIES);

Worth a test with it. VisitWithQueueEntriesSaveHandlerTest.shouldVoidQueueEntriesForUserWithoutQueuePrivileges is the pattern to copy. Both tests in the new class run as the superuser, which is why this got past them.

[dkayiwa]

Not a blocker, but the PR description hasn't caught up with this class. It still says the advice intercepts voidVisit() and "voids all associated queue entries with the same void reason", and its Root Cause section says the save handler "does not reliably fire when visitService.voidVisit() is called. The handler only works through the saveVisit() path", which is the opposite of what this paragraph says. That text is what lands in the merge commit, so could you bring it in line with what the code now does: purge only, with the void path owned by VisitWithQueueEntriesSaveHandler on main?

One qualification on the paragraph itself while you're here. "Already voids the entries on both the saveVisit and voidVisit paths" holds only for a particular handler order. BaseVoidHandler and VisitWithQueueEntriesSaveHandler both carry the default @Handler order of Integer.MAX_VALUE, and HandlerUtil.getHandlersForType breaks that tie with the iteration order of the new HashMap<>(...) in ServiceContext.getRegisteredComponents, so it moves with the set of registered VoidHandler beans. Printing the order in the module tests gives BaseVoidHandler first, which is why it holds there; calling the save handler directly on a visit that isn't voided yet leaves its entries untouched. "Voids them when it runs after core's BaseVoidHandler" would say it without promising more than the code guarantees.

[dkayiwa]

Left over from the merge, QueueEntry isn't referenced anywhere in the class any more, so this import can go.

[dkayiwa]

A Context.flushSession() after each purge would make these cover the foreign key failure the PR is fixing. As they stand, all three assert through getQueueEntryById and getVisit, both of which read the Hibernate session, so nothing forces a flush and the DELETE FROM visit that queue_entry_visit_id_fk rejects never reaches H2.

I checked what that leaves out by removing the Context.addAdvice call and running the same purge on this branch. visitService.purgeVisit(visit) returns without complaint, and the constraint only speaks up on an explicit flush: Referential integrity constraint violation: "PUBLIC.QUEUE_ENTRY FOREIGN KEY(VISIT_ID) REFERENCES PUBLIC.VISIT(VISIT_ID) (102)". So the tests currently pin "the advice removed the entries" but not "the visit row can now be deleted", which is the failure described at the top of the PR.

I added the flush to all three tests and they stay green.

Suggested change

	visitService.purgeVisit(visit);
	visitService.purgeVisit(visit);
	Context.flushSession();

[dkayiwa]

A voided visit doesn't hide its entries, so the first half of this is off. QueueEntryDaoImpl.createCriteriaFromSearchCriteria filters on qe.voided and, through the patient alias, on p.voided; the visit shows up only as Restrictions.eq("qe.visit", visit), with no join to the visit table, so its own voided flag plays no part. I confirmed it: voiding visit 102, then unvoiding its queue entries again, still leaves the entry in the default search, whereas voiding the patient drops it.

The reason that does apply here is the one in the PR description, that voided entries hold the same foreign key, which is what shouldPurgeVoidedQueueEntriesOfAPurgedVisit covers. PatientWithQueueEntriesVoidHandler words the patient half accurately if you want a model.

Suggested change

	// the visit or its patient may already be voided, which would hide the entries from the default search
	// voided entries hold the same foreign key, and a voided patient hides them from the default search

[dkayiwa]

Narrowing the advice to purgeVisit leaves the half of O3-5459 that the ticket actually reports resting on handler order, and I think that has to be settled before this merges.

The chart's "delete visit" is a void, not a purge: VisitResource1_9.delete calls voidVisit(visit, reason) and only ?purge=true reaches purgeVisit. So the reported symptom sits entirely on the void path, which this PR now hands to VisitWithQueueEntriesSaveHandler.

That handler does work today, but only by luck. Its void branch is guarded by visit.getVoided(), and VisitServiceImpl.voidVisit is just dao.saveVisit(visit), so the only thing that runs is its VoidHandler side. Whether the guard is satisfied depends on BaseVoidHandler having gone first, the two are tied at the default @Handler order, and HandlerUtil sorts stably, so the tie falls to the iteration order of the HashMap that ServiceContext.getRegisteredComponents builds over bean names. baseVoidHandler and visitWithQueueEntriesSaveHandler land in buckets 4 and 1 when that table is 8 wide, 4 and 9 at 16, 20 and 25 at 32, then 52 and 25 at 64. Core wins only while the deployment sits between 7 and 24 VoidHandler beans.

I drove both ends of that through the real visitService.voidVisit in the integration-test context:

• Six beans, which is core's five plus the module's one, exactly what queue 3.0.0 ships. Order comes out [VisitWithQueueEntriesSaveHandler, VisitVoidHandler, BaseVoidHandler] and the entry is left voided=false, dateVoided=null. That is O3-5459 reproducing.
• Seven beans, which is main today. Order is [BaseVoidHandler, VisitWithQueueEntriesSaveHandler, VisitVoidHandler] and the entry is voided.

The only thing that moved us from the first to the second is PatientWithQueueEntriesVoidHandler, added last week for O3-5955. Nothing in the module pins it, and @Handler(order=...) can't either, since lower wins and the default is already Integer.MAX_VALUE, so an explicit order only moves us earlier. No test covers the losing side: shouldVoidQueueEntriesWhenVisitIsVoidedThroughVoidVisit runs at seven beans and structurally cannot see it.

If merged as-is, O3-5459 is closed on behaviour that reverts the moment a distro drops that unrelated handler or grows past 24 VoidHandler beans, and nothing fails to tell us. It is also the void+purge version that was verified live on a RefApp in the approving review above (against 36b2040, which still had the branch), and on the strength of which the contributor closed esm-patient-chart#3427 "in favor of that backend fix".

I'd put the voidVisit branch back as it was at 36b2040: it takes the reason from args[1] rather than off the entity, so handler order stops mattering. If you'd rather the void half stay a handler, give it its own VoidHandler<Visit> shaped like PatientWithQueueEntriesVoidHandler, or like core's VisitVoidHandler sitting next to it in the same list; both take the user, date and reason from the handler arguments and carry no getVoided() guard. Either way the handler's current void branch should then go, so the cascade has one owner and the void stamp is deterministic.

[dkayiwa]

This needs fixing before merge. Purging the entries from a before-advice commits that half before core has decided whether it will delete the visit at all, and VisitServiceImpl.purgeVisit has a refusal path that is easy to hit: if the visit still has encounters (getEncountersByVisit(visit, true), so voided ones count) it throws APIException("Visit.purge.inUse") at VisitServiceImpl.java:221-222, before dao.deleteVisit is ever reached.

If merged as-is, DELETE /visit/{uuid}?purge=true on a visit that has any encounter deletes that visit's queue entries and then fails with "Cannot permanently delete a visit that has encounters associated to it". The visit survives, the queue rows are gone, and the caller is handed an error that says nothing was deleted. On main nothing is lost in that case, because core throws before the foreign key is reached and nothing has touched the entries, so this is new.

The transaction does not save it, which is the part I did not expect. Context.addAdvice appends to the outer visitService proxy, but that proxy's target is already a transaction proxy: applicationContext-service.xml registers a DefaultAdvisorAutoProxyCreator (line 46) and a TransactionAttributeSourceAdvisor (line 52), so visitServiceTarget gets wrapped, and the outer TransactionProxyFactoryBean (line 515) resolves no transaction attribute for purgeVisit because its target class is a JDK interface proxy. In the module's integration-test context, which loads that same file, printing the chain behind Context.getVisitService() gives [AuthorizationAdvice, RequiredDataAdvice, LoggingAdvice, CacheInterceptor, TransactionInterceptor, VisitWithQueueEntriesDeleteAdvice] sitting over a second proxy whose only advisor is the TransactionInterceptor, and inside before() during a real purgeVisit, TransactionSynchronizationManager.isActualTransactionActive() is false.

I reproduced the loss with a control alongside it. In a test method marked @Transactional(propagation = NOT_SUPPORTED), so there is no ambient test transaction, purging a visit that has one encounter throws that APIException, the queue entry is gone from the database, and the visit row is still there. In the same test a purgeQueueEntry inside a TransactionTemplate that then throws leaves its entry intact, so rollback does work in that setup; these deletes are simply not in the transaction being rolled back.

Making it an around interceptor fixes it. I changed the class to implement MethodInterceptor and ran the entry purge plus invocation.proceed() inside a TransactionTemplate built from Context.getRegisteredComponent("transactionManager", PlatformTransactionManager.class). After the same refusal the entry is still present, and the three tests in VisitWithQueueEntriesDeleteAdviceTest stay green. MethodInterceptor is an Advice, so config.xml and ModuleFactory.loadAdvice need no change. Worth a test with it: a visit with an encounter, the purge refused, the entries still there.

This is also the answer to the partial-failure question in the conversation above, which I think was pointing at exactly this.

[dkayiwa]

Nothing in the build would notice if either of these two strings were wrong. I deleted this whole <advice> block and ran mvn install: green, with 59 api, 73 omod and 120 integration tests. VisitWithQueueEntriesDeleteAdviceTest registers the advice itself through Context.addAdvice, which is the right way to exercise the interceptor chain, but it does leave config.xml as the one piece of this fix that no test reads.

The failure would be quiet too. With a wrong class name, AdvicePoint.getClassInstance catches Exception | LinkageError and logs a warning, ModuleFactory.loadAdvice then logs at debug and carries on, and the module starts normally with the purge cascade simply absent. A small test in omod that parses the packaged config.xml and asserts that each <advice>'s point and class both load, and that the class is an Advice, would close that. The filtered file lands in omod/target/classes and queue-api is already an omod dependency, so it can be a plain unit test. Both values are right today, so this is only about keeping it that way.

[dkayiwa]

Entry 1 on this visit is the case that isn't asserted, and it's the one the reported workflow produces. It's already in the dataset, ended at 2022-02-02 18:40:56 and not voided, and the cascade does void it, because QueueEntrySearchCriteria.isEnded defaults to null so the search doesn't filter on it.

Nothing pins that, though. I added criteria.setIsEnded(false) to the handler, which is exactly what VisitWithQueueEntriesSaveHandler does two files over, and all 120 integration tests still passed while entry 1 was left active on a voided visit. O3-5461's repro is add to queue, call, serve, then delete the visit, and serving transitions the entry, so a deleted visit in that flow has one ended entry and one open one. VisitWithQueueEntriesValidator writes criteria.setIsEnded(null) explicitly for the same reason. The cascade is right today, this is just to hold it there.

Suggested change

	QueueEntry cascaded = queueEntryService.getQueueEntryById(2).get();
	assertTrue(cascaded.getVoided());
	assertThat(cascaded.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime()));
	QueueEntry cascaded = queueEntryService.getQueueEntryById(2).get();
	assertTrue(cascaded.getVoided());
	assertThat(cascaded.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime()));
	// entry 1 was already ended before the void, which must not keep it off the cascade
	QueueEntry endedBeforeTheVoid = queueEntryService.getQueueEntryById(1).get();
	assertNotNull(endedBeforeTheVoid.getEndedAt());
	assertTrue(endedBeforeTheVoid.getVoided());

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.7% Duplication on New Code

See analysis details on SonarQube Cloud

[dkayiwa]

Nothing performs that discrimination today, so this claims more than the module does. PatientWithQueueEntriesUnvoidHandler.shouldRestore is 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 pairs VisitVoidHandler with VisitUnvoidHandler, and the module pairs PatientWithQueueEntriesVoidHandler with 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 unvoidVisit so restoring a deleted visit also restores its queue entries"). It is reachable, for what it's worth: VisitResource1_9.undelete calls unvoidVisit, and POST /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 reading shouldRestore's own comment (which now points at this class) isn't left to work the gap out for themselves.

Suggested change

	* Entries that were already voided keep the stamp they had, so an unvoid can tell them apart from
	* entries taken down with the visit.
	* Entries that were already voided keep the stamp they had, so their own void is not overwritten.
	* Nothing restores any of them: the module has no {@code UnvoidHandler<Visit>}, so unvoiding a
	* visit leaves its queue entries voided.

[dkayiwa]

{"voided": false} doesn't reach unvoidVisit, so the aside above names the wrong request. The Javadoc suggestion doesn't depend on it, so nothing changes in the PR.

MainResourceController.update only routes a POST to undelete when the body is exactly {"deleted": "false"}, and every other body goes through DelegatingCrudResource.update to saveVisit. {"voided": false} is the body the chart's "Restore visit" sends (restoreVisit in visit.resource.tsx). I ran both through webservices.rest 3.0.0's own controller test harness with an advice on VisitService recording each call: {"voided": false} reached saveVisit and never unvoidVisit, and {"deleted": "false"} reached unvoidVisit.

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?

[dkayiwa]

The last sentence doesn't hold. getAllVisitTypes() never reaches the args.length == 0 clause, because || short-circuits on the method-name check first, and nothing else on VisitService reaches it either, since purgeVisit(Visit) is the only purgeVisit there. That clause and the instanceof Visit one beside it are unreachable belt and braces, worth keeping but not worth claiming coverage of.

The pass-through half of the comment is real: I changed the non-purge branch to invocation.proceed(); return null; and this test failed on getAllVisitTypes() coming back null.

Suggested change

	// not. A no-arg method also covers the empty argument array it tolerates on the way past.
	// not. A no-arg method is enough to prove that much.

[jayasanka-sack]

This is marked as a nice to have fix for the RefApp 3.8.0.