DeathGun44 commented on code in PR #5719:
URL: https://github.com/apache/fineract/pull/5719#discussion_r3015770985
##########
fineract-provider/src/test/java/org/apache/fineract/commands/service/SynchronousCommandProcessingServiceTest.java:
##########
@@ -598,4 +601,103 @@ public void testExecuteCommandWithMaxRetryFailure() {
underTest.executeCommand(commandWrapper, jsonCommand, false);
verify(commandSourceService, times(4)).getCommandSource(commandId);
}
+
+ /**
+ * Test that when running inside an enclosing batch transaction, hook
events are NOT published immediately but
+ * deferred to afterCommit. This prevents webhooks (e.g. SMS
notifications) from firing for commands that are
+ * subsequently rolled back when a later command in the batch fails.
+ */
+ @Test
+ public void testHookEventDeferredInEnclosingTransaction() {
+ CommandWrapper commandWrapper = getCommandWrapper();
+
+ long commandId = 1L;
+ JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
+ when(jsonCommand.commandId()).thenReturn(commandId);
+
+ NewCommandSourceHandler commandHandler =
Mockito.mock(NewCommandSourceHandler.class);
+ CommandProcessingResult commandProcessingResult =
Mockito.mock(CommandProcessingResult.class);
+
when(commandProcessingResult.isRollbackTransaction()).thenReturn(false);
+
when(commandHandler.processCommand(jsonCommand)).thenReturn(commandProcessingResult);
+ when(commandHandlerProvider.getHandler(Mockito.any(),
Mockito.any())).thenReturn(commandHandler);
+
+
when(configurationDomainService.isMakerCheckerEnabledForTask(Mockito.any())).thenReturn(false);
+ String idk = "idk";
+ when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idk);
+ CommandSource commandSource = Mockito.mock(CommandSource.class);
+ when(commandSource.getId()).thenReturn(commandId);
+ when(commandSourceService.findCommandSource(commandWrapper,
idk)).thenReturn(null);
+
when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource);
+
+ AppUser appUser = Mockito.mock(AppUser.class);
+
when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser);
+ when(commandSourceService.saveInitialNewTransaction(commandWrapper,
jsonCommand, appUser, idk)).thenReturn(commandSource);
+
when(commandSourceService.saveResultSameTransaction(commandSource)).thenReturn(commandSource);
+
when(commandSource.getStatus()).thenReturn(CommandProcessingResultType.PROCESSED.getValue());
+
+ when(commandSourceService.processCommand(commandHandler, jsonCommand,
commandSource, appUser, false))
+ .thenReturn(commandProcessingResult);
+
+ // Simulate enclosing batch transaction with active synchronization
+ BatchRequestContextHolder.setIsEnclosingTransaction(true);
+ TransactionSynchronizationManager.initSynchronization();
+ try {
+ underTest.executeCommand(commandWrapper, jsonCommand, false);
+
+ // Hook event should NOT have been published immediately
+ verify(applicationContext, never()).publishEvent(any());
+
+ // It should be registered as an afterCommit synchronization
+ assertEquals(1,
TransactionSynchronizationManager.getSynchronizations().size());
+ } finally {
+ TransactionSynchronizationManager.clearSynchronization();
+ BatchRequestContextHolder.resetIsEnclosingTransaction();
+ }
+ }
+
+ /**
+ * Test that when NOT in an enclosing transaction, hook events are
published immediately (existing behaviour).
+ */
+ @Test
+ public void testHookEventPublishedImmediatelyOutsideEnclosingTransaction()
{
+ CommandWrapper commandWrapper = getCommandWrapper();
+
+ long commandId = 1L;
+ JsonCommand jsonCommand = Mockito.mock(JsonCommand.class);
+ when(jsonCommand.commandId()).thenReturn(commandId);
+ when(jsonCommand.json()).thenReturn(null); // publishHookEvent exits
early when json is null
Review Comment:
I get the intent here returning null saves us from having to mock a bunch of
stuff for publishHookEvent.
But right now, the test doesn't have any assertions; it just passes because
nothing breaks. At the very least, we can drop in an
assertFalse(TransactionSynchronizationManager.isSynchronizationActive()) to
prove no sync was registered.
Also, the test name mentions "PublishedImmediately", but returning null
skips publication altogether. Might be worth either renaming the test to fit
what's actually happening or adding the mocks to verify the immediate
publication.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
