fix(rejection): notify released signers only after the rejection commits

lib/Service/SequentialSigningService.php

@@ -9,9 +9,11 @@
 namespace OCA\Libresign\Service;
 
 use OCA\Libresign\Db\File as FileEntity;
+use OCA\Libresign\Db\SignRequest;
 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,76 @@ 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() without notifying anyone, for callers inside a
+	 * database transaction: a notification cannot be taken back if the
+	 * transaction is rolled back, so they notify with notifyActivatedSigners()
+	 * once it is committed.
+	 *
+	 * @return list<SignRequest> the signers that became 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);
+		return $this->activateSignersForOrder($allSignRequests, $nextOrder);
+	}
+
+	/**
+	 * @param list<SignRequest> $signers
+	 */
+	public function notifyActivatedSigners(array $signers): void {
+		foreach ($signers as $signer) {
+			$this->notifySigner($signer);
+		}
+	}
+
+	/**
+	 * The signer is already able to sign when this runs, so a failed delivery
+	 * must not keep the remaining identify methods and signers from being told.
+	 * Only \Exception is isolated: the channels behind notify() (mail,
+	 * notifications, gateways) raise different exception types, while an
+	 * \Error is a programming error and must stay visible.
+	 */
+	private function notifySigner(SignRequest $signer): void {
+		try {
+			$identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId());
+		} catch (\Exception $e) {
+			$this->logger->error('Failed to load the identify methods of an activated signer: ' . $e->getMessage(), [
+				'exception' => $e,
+				'signRequestId' => $signer->getId(),
+			]);
+			return;
+		}
+		foreach ($identifyMethods as $methodGroup) {
+			foreach ($methodGroup as $identifyMethod) {
+				$identifyMethod->willNotifyUser(true);
+				try {
+					$identifyMethod->notify();
+				} catch (\Exception $e) {
+					$this->logger->error('Failed to notify an activated signer: ' . $e->getMessage(), [
+						'exception' => $e,
+						'signRequestId' => $signer->getId(),
+						'identifyMethod' => $identifyMethod->getName(),
+					]);
+				}
+			}
+		}
 	}
 
 	/**
@@ -111,7 +168,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));
 				}
 			}
 		}
@@ -151,27 +208,23 @@ private function findNextOrder(array $signRequests, int $completedOrder): ?int {
 		return null;
 	}
 
-	private function activateSignersForOrder(array $signRequests, int $order): void {
+	/**
+	 * @return list<SignRequest>
+	 */
+	private function activateSignersForOrder(array $signRequests, int $order): array {
 		$signersToActivate = array_filter(
 			$signRequests,
 			fn ($sr) => $this->isSigningParticipant($sr)
 				&& $sr->getSigningOrder() === $order
+				&& $sr->getStatusEnum() === SignRequestStatus::DRAFT
 		);
 
 		foreach ($signersToActivate as $signer) {
-			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();
-					}
-				}
-			}
+			$signer->setStatusEnum(SignRequestStatus::ABLE_TO_SIGN);
+			$this->signRequestMapper->update($signer);
 		}
+
+		return array_values($signersToActivate);
 	}
 
 	private function getSignatureFlow(): SignatureFlow {

lib/Service/SignatureRejection/SignatureRejectionService.php

@@ -76,8 +76,11 @@ public function reject(
 		$commentIsPrivate = $normalizedComment !== null && $privateComment;
 
 		$workflowCanceled = $config->cancelsWorkflow();
-		$this->persistRejection($libreSignFile, $signRequest, $normalizedComment, $commentIsPrivate, $workflowCanceled);
+		$activatedSigners = $this->persistRejection($libreSignFile, $signRequest, $normalizedComment, $commentIsPrivate, $workflowCanceled);
+		// The event goes first so that nothing raised while notifying the released
+		// signers can keep the listeners from hearing about a committed rejection.
 		$this->dispatchRejectedEvent($signRequest, $libreSignFile, $workflowCanceled);
+		$this->sequentialSigningService->notifyActivatedSigners($activatedSigners);
 
 		return $signRequest;
 	}
@@ -94,14 +97,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> the signers released by the rejection, to be notified after the commit
 	 */
 	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(),
@@ -110,6 +115,7 @@ private function persistRejection(
 		$previousFileStatus = $libreSignFile->getStatus();
 		$rejectedAt = $this->timeFactory->getDateTime();
 
+		$activatedSigners = [];
 		$this->db->beginTransaction();
 		try {
 			foreach ($signRequests as $each) {
@@ -123,7 +129,7 @@ private function persistRejection(
 			if ($workflowCanceled) {
 				$this->cancelWorkflow($libreSignFile);
 			} else {
-				$this->releaseNextSigningOrders($libreSignFile, $signRequests);
+				$activatedSigners = $this->releaseNextSigningOrders($libreSignFile, $signRequests);
 			}
 
 			$this->db->commit();
@@ -138,6 +144,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;
 	}
 
 	/**
@@ -184,22 +192,28 @@ private function collectSignRequestsToReject(FileEntity $libreSignFile, SignRequ
 	 * When the workflow continues, a rejection ends the signer's turn just like a
 	 * signature does, so in a sequential flow the next order must be released on
 	 * every document the signer rejected, or the signers after them would wait
-	 * forever.
+	 * forever. The released signers are returned to be notified after the commit.
 	 *
 	 * @param list<SignRequestEntity> $rejectedSignRequests
+	 * @return list<SignRequestEntity> the signers that became able to sign
 	 */
-	private function releaseNextSigningOrders(FileEntity $libreSignFile, array $rejectedSignRequests): void {
+	private function releaseNextSigningOrders(FileEntity $libreSignFile, array $rejectedSignRequests): array {
+		$activatedSigners = [];
 		foreach ($rejectedSignRequests as $rejected) {
 			$document = $rejected->getFileId() === $libreSignFile->getId()
 				? $libreSignFile
 				: $this->fileMapper->getById($rejected->getFileId());
-			$this->sequentialSigningService
-				->setFile($document)
-				->releaseNextOrder(
-					$rejected->getFileId(),
-					$rejected->getSigningOrder()
-				);
+			$activatedSigners = [
+				...$activatedSigners,
+				...$this->sequentialSigningService
+					->setFile($document)
+					->activateNextOrder(
+						$rejected->getFileId(),
+						$rejected->getSigningOrder()
+					),
+			];
 		}
+		return $activatedSigners;
 	}
 
 	/**