[stable35] fix(rejection): notify the next signing order only after the commit

lib/Service/SequentialSigningService.php

@@ -9,9 +9,11 @@
 namespace OCA\Libresign\Service;
 
 use OCA\Libresign\Db\File as FileEntity;
+use OCA\Libresign\Db\SignRequest as SignRequestEntity;
 use OCA\Libresign\Db\SignRequestMapper;
 use OCA\Libresign\Enum\SignatureFlow;
 use OCA\Libresign\Enum\SignRequestStatus;
+use Psr\Log\LoggerInterface;
 
 class SequentialSigningService {
 	private int $currentOrder = 1;
@@ -20,6 +22,7 @@ class SequentialSigningService {
 	public function __construct(
 		private SignRequestMapper $signRequestMapper,
 		private IdentifyMethodService $identifyMethodService,
+		private LoggerInterface $logger,
 	) {
 	}
 
@@ -69,22 +72,77 @@ public function determineSigningOrder(?int $userProvidedOrder): int {
 	 * @param int $completedOrder The order that was just completed
 	 */
 	public function releaseNextOrder(int $fileId, int $completedOrder): void {
+		$this->notifyActivatedSigners($this->activateNextOrder($fileId, $completedOrder));
+	}
+
+	/**
+	 * Same as releaseNextOrder(), but it only persists the activation and returns
+	 * the signers it activated instead of notifying them.
+	 *
+	 * A caller running inside a transaction must collect the returned signers and
+	 * call notifyActivatedSigners() only after the commit: a notification sent
+	 * before the commit is already delivered when a rollback reverts the
+	 * activation it announced.
+	 *
+	 * @return list<SignRequestEntity> The signers moved to ABLE_TO_SIGN.
+	 */
+	public function activateNextOrder(int $fileId, int $completedOrder): array {
 		if (!$this->isOrderedNumericFlow()) {
-			return;
+			return [];
 		}
 
 		$allSignRequests = $this->signRequestMapper->getByFileId($fileId);
 
 		if (!$this->isOrderFullyCompleted($allSignRequests, $completedOrder)) {
-			return;
+			return [];
 		}
 
 		$nextOrder = $this->findNextOrder($allSignRequests, $completedOrder);
 		if ($nextOrder === null) {
+			return [];
+		}
+
+		return $this->activateSignersForOrder($allSignRequests, $nextOrder);
+	}
+
+	/**
+	 * @param list<SignRequestEntity> $signers
+	 */
+	public function notifyActivatedSigners(array $signers): void {
+		foreach ($signers as $signer) {
+			$this->notifySigner($signer);
+		}
+	}
+
+	/**
+	 * Isolate recoverable lookup and delivery failures so other signers can proceed.
+	 * Channel exceptions vary; programming errors must still propagate.
+	 */
+	private function notifySigner(SignRequestEntity $signer): void {
+		try {
+			$identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId());
+		} catch (\Exception $e) {
+			$this->logger->error('Error loading identification methods for an activated signer: ' . $e->getMessage(), [
+				'exception' => $e,
+				'signRequestId' => $signer->getId(),
+			]);
 			return;
 		}
 
-		$this->activateSignersForOrder($allSignRequests, $nextOrder);
+		foreach ($identifyMethods as $methodGroup) {
+			foreach ($methodGroup as $identifyMethod) {
+				try {
+					$identifyMethod->willNotifyUser(true);
+					$identifyMethod->notify();
+				} catch (\Exception $e) {
+					$this->logger->error('Error notifying an activated signer: ' . $e->getMessage(), [
+						'exception' => $e,
+						'signRequestId' => $signer->getId(),
+						'identifyMethod' => $identifyMethod->getName(),
+					]);
+				}
+			}
+		}
 	}
 
 	/**
@@ -111,7 +169,7 @@ public function reorderAfterDeletion(int $fileId, int $deletedOrder): void {
 			if ($previousOrder > 0 && $this->isOrderFullyCompleted($allSignRequests, $previousOrder)) {
 				$nextOrder = $this->findNextOrder($allSignRequests, $deletedOrder);
 				if ($nextOrder !== null) {
-					$this->activateSignersForOrder($allSignRequests, $nextOrder);
+					$this->notifyActivatedSigners($this->activateSignersForOrder($allSignRequests, $nextOrder));
 				}
 			}
 		}
@@ -140,7 +198,12 @@ private function findNextOrder(array $signRequests, int $completedOrder): ?int {
 		return null;
 	}
 
-	private function activateSignersForOrder(array $signRequests, int $order): void {
+	/**
+	 * @param list<SignRequestEntity> $signRequests
+	 * @return list<SignRequestEntity> The signers moved to ABLE_TO_SIGN.
+	 */
+	private function activateSignersForOrder(array $signRequests, int $order): array {
+		$activated = [];
 		$signersToActivate = array_filter(
 			$signRequests,
 			fn ($sr) => $sr->getSigningOrder() === $order
@@ -150,16 +213,11 @@ private function activateSignersForOrder(array $signRequests, int $order): void
 			if ($signer->getStatusEnum() === SignRequestStatus::DRAFT) {
 				$signer->setStatusEnum(SignRequestStatus::ABLE_TO_SIGN);
 				$this->signRequestMapper->update($signer);
-
-				$identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId());
-				foreach ($identifyMethods as $methodGroup) {
-					foreach ($methodGroup as $identifyMethod) {
-						$identifyMethod->willNotifyUser(true);
-						$identifyMethod->notify();
-					}
-				}
+				$activated[] = $signer;
 			}
 		}
+
+		return $activated;
 	}
 
 	private function getSignatureFlow(): SignatureFlow {

lib/Service/SignatureRejection/SignatureRejectionService.php

@@ -76,6 +76,7 @@ public function reject(
 		$workflowCanceled = $policy['cancel_workflow'];
 		$this->persistRejection($libreSignFile, $signRequest, $normalizedComment, $commentIsPrivate, $workflowCanceled);
 		$this->dispatchRejectedEvent($signRequest, $libreSignFile, $workflowCanceled);
+		$this->sequentialSigningService->notifyActivatedSigners($activatedSigners);
 
 		return $signRequest;
 	}
@@ -92,14 +93,16 @@ public function isWorkflowCanceled(FileEntity $libreSignFile): bool {
 	 * Marking the signer as rejected and closing the workflow describe a single
 	 * decision, so they are written together: a failure must never leave a signer
 	 * who already rejected on a document that stays open for everybody else.
+	 *
+	 * @return list<SignRequestEntity> Signers activated by the committed rejection.
 	 */
 	private function persistRejection(
 		FileEntity $libreSignFile,
 		SignRequestEntity $signRequest,
 		?string $comment,
 		bool $commentIsPrivate,
 		bool $workflowCanceled,
-	): void {
+	): array {
 		$signRequests = $this->collectSignRequestsToReject($libreSignFile, $signRequest);
 		$previousSignerStatuses = array_map(
 			static fn (SignRequestEntity $each): int => $each->getStatus(),
@@ -134,6 +137,8 @@ private function persistRejection(
 			// TRANSLATORS Error shown when the rejection could not be saved and nothing was changed.
 			throw new LibresignException($this->l10n->t('It was not possible to register the rejection. Nothing was changed.'));
 		}
+
+		return $activatedSigners;
 	}
 
 	/**

tests/php/Unit/Service/SequentialSigningServiceTest.php

@@ -18,20 +18,24 @@
 use OCA\Libresign\Tests\Unit\TestCase;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 final class SequentialSigningServiceTest extends TestCase {
 	private SignRequestMapper&MockObject $signRequestMapper;
 	private IdentifyMethodService&MockObject $identifyMethodService;
+	private LoggerInterface&MockObject $logger;
 	private SequentialSigningService $service;
 
 	public function setUp(): void {
 		parent::setUp();
 		$this->signRequestMapper = $this->createMock(SignRequestMapper::class);
 		$this->identifyMethodService = $this->createMock(IdentifyMethodService::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->service = new SequentialSigningService(
 			$this->signRequestMapper,
-			$this->identifyMethodService
+			$this->identifyMethodService,
+			$this->logger
 		);
 	}
 
@@ -202,6 +206,150 @@ public function testReleaseNextOrderOrderedFlow(array $requests, int $completedO
 		$this->service->releaseNextOrder(99, $completedOrder);
 	}
 
+	public function testActivateNextOrderReturnsTheActivatedSignersWithoutNotifyingThem(): void {
+		$file = $this->createMock(FileEntity::class);
+		$file->method('getSignatureFlowEnum')
+			->willReturn(SignatureFlow::ORDERED_NUMERIC);
+		$this->service->setFile($file);
+
+		$signRequests = $this->buildSignRequests([
+			[1, SignRequestStatus::SIGNED, 1],
+			[2, SignRequestStatus::DRAFT, 2],
+			[3, SignRequestStatus::DRAFT, 2],
+		]);
+		$this->signRequestMapper->expects($this->once())
+			->method('getByFileId')
+			->with(99)
+			->willReturn($signRequests);
+
+		$this->signRequestMapper->expects($this->exactly(2))
+			->method('update')
+			->with($this->callback(fn (SignRequest $request): bool => $request->getStatusEnum() === SignRequestStatus::ABLE_TO_SIGN));
+
+		$this->identifyMethodService->expects($this->never())->method('getIdentifyMethodsFromSignRequestId');
+
+		$activated = $this->service->activateNextOrder(99, 1);
+
+		$this->assertCount(2, $activated);
+		$this->assertSame([2, 3], array_map(fn (SignRequest $request): int => $request->getId(), $activated));
+	}
+
+	public function testNotifyActivatedSignersNotifiesEverySignerItReceives(): void {
+		$this->service->setFile($this->createMock(FileEntity::class));
+
+		$identifyMethod = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class);
+		$identifyMethod->expects($this->exactly(2))
+			->method('willNotifyUser')
+			->with(true);
+		$identifyMethod->expects($this->exactly(2))
+			->method('notify');
+
+		$this->identifyMethodService->expects($this->exactly(2))
+			->method('getIdentifyMethodsFromSignRequestId')
+			->willReturn([[$identifyMethod]]);
+
+		$this->service->notifyActivatedSigners($this->buildSignRequests([
+			[2, SignRequestStatus::ABLE_TO_SIGN, 2],
+			[3, SignRequestStatus::ABLE_TO_SIGN, 2],
+		]));
+	}
+
+	public function testNotifyActivatedSignersIsANoOpWithoutActivatedSigners(): void {
+		$this->service->setFile($this->createMock(FileEntity::class));
+
+		$this->identifyMethodService->expects($this->never())->method('getIdentifyMethodsFromSignRequestId');
+
+		$this->service->notifyActivatedSigners([]);
+	}
+
+	public function testNotifyActivatedSignersKeepsNotifyingTheRemainingSignersWhenADeliveryFails(): void {
+		$this->service->setFile($this->createMock(FileEntity::class));
+
+		$failing = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class);
+		$failing->method('getName')->willReturn('email');
+		$failing->expects($this->once())->method('willNotifyUser')->with(true);
+		$failing->expects($this->once())->method('notify')
+			->willThrowException(new \RuntimeException('the mail server is unreachable'));
+
+		$delivered = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class);
+		$delivered->expects($this->exactly(2))->method('willNotifyUser')->with(true);
+		$delivered->expects($this->exactly(2))->method('notify');
+
+		$this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId')
+			->willReturnCallback(fn (int $signRequestId): array => $signRequestId === 2
+				? [[$failing, $delivered]]
+				: [[$delivered]]);
+
+		$this->logger->expects($this->once())->method('error')->with(
+			$this->stringContains('the mail server is unreachable'),
+			$this->callback(fn (array $context): bool => $context['signRequestId'] === 2
+				&& $context['identifyMethod'] === 'email'),
+		);
+
+		$this->service->notifyActivatedSigners($this->buildSignRequests([
+			[2, SignRequestStatus::ABLE_TO_SIGN, 2],
+			[3, SignRequestStatus::ABLE_TO_SIGN, 2],
+		]));
+	}
+
+	public function testNotifyActivatedSignersContinuesAfterAnIdentificationLookupFails(): void {
+		$delivered = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class);
+		$delivered->expects($this->once())->method('willNotifyUser')->with(true);
+		$delivered->expects($this->once())->method('notify');
+
+		$this->identifyMethodService->expects($this->exactly(2))
+			->method('getIdentifyMethodsFromSignRequestId')
+			->willReturnCallback(function (int $signRequestId) use ($delivered): array {
+				if ($signRequestId === 2) {
+					throw new \RuntimeException('identification lookup failed');
+				}
+				$this->assertSame(3, $signRequestId);
+				return [[$delivered]];
+			});
+		$this->logger->expects($this->once())->method('error')->with(
+			$this->stringContains('identification lookup failed'),
+			$this->callback(fn (array $context): bool => $context['signRequestId'] === 2
+				&& $context['exception'] instanceof \RuntimeException),
+		);
+
+		$this->service->notifyActivatedSigners($this->buildSignRequests([
+			[2, SignRequestStatus::ABLE_TO_SIGN, 2],
+			[3, SignRequestStatus::ABLE_TO_SIGN, 2],
+		]));
+	}
+
+	public function testNotifyActivatedSignersDoesNotSwallowIdentificationLookupProgrammingErrors(): void {
+		$this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId')
+			->willThrowException(new \TypeError('invalid identification lookup'));
+		$this->logger->expects($this->never())->method('error');
+		$this->expectException(\TypeError::class);
+		$this->expectExceptionMessage('invalid identification lookup');
+
+		$this->service->notifyActivatedSigners($this->buildSignRequests([
+			[2, SignRequestStatus::ABLE_TO_SIGN, 2],
+		]));
+	}
+
+	public function testNotifyActivatedSignersDoesNotSwallowProgrammingErrors(): void {
+		$this->service->setFile($this->createMock(FileEntity::class));
+
+		$broken = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class);
+		$broken->expects($this->once())->method('notify')
+			->willThrowException(new \TypeError('a listener is calling the wrong method'));
+
+		$this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId')
+			->willReturn([[$broken]]);
+
+		$this->logger->expects($this->never())->method('error');
+
+		$this->expectException(\TypeError::class);
+		$this->expectExceptionMessage('a listener is calling the wrong method');
+
+		$this->service->notifyActivatedSigners($this->buildSignRequests([
+			[2, SignRequestStatus::ABLE_TO_SIGN, 2],
+		]));
+	}
+
 	public function testReorderAfterDeletionSkipsWhenNotOrdered(): void {
 		$file = $this->createMock(FileEntity::class);
 		$file->method('getSignatureFlowEnum')