diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index e9b36b3fe5..01a7e71c56 100644 --- a/lib/Service/SequentialSigningService.php +++ b/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 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 $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 $signRequests + * @return list 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 { diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index f39fc7baff..f4a0627121 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/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,6 +93,8 @@ 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 Signers activated by the committed rejection. */ private function persistRejection( FileEntity $libreSignFile, @@ -99,7 +102,7 @@ private function persistRejection( ?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; } /** diff --git a/tests/php/Unit/Service/SequentialSigningServiceTest.php b/tests/php/Unit/Service/SequentialSigningServiceTest.php index 84de2b3a7c..d68d6a5a1d 100644 --- a/tests/php/Unit/Service/SequentialSigningServiceTest.php +++ b/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')