diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 3d12aaa8..055f9315 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -103,18 +103,20 @@ public function prepare(INotification $notification, string $languageCode): INot } // Only add file info if we have some ... - $richParams = false; + $richParams = null; if ($notification->getObjectType() === 'file' && ($fileId = $notification->getObjectId()) && ($uid = $notification->getUser())) { + // Note:: This might throw an AlreadyProcessedException if the file doesn't exist anymore. + // It has to be thrown before any call to $notification->set... otherwise the notification + // won't be removed from the database. $richParams = $this->tryGetRichParamForFile($uid, intval($fileId)); - if ($richParams !== false) { - $notification->setRichSubject($richSubject, $richParams); - } } - // Fallback to generic error message without file link - if ($richParams === false) { + if ($richParams !== null) { + $notification->setRichSubject($richSubject, $richParams); + } else { + // Fallback to generic error message without file link $notification->setParsedSubject($parsedSubject); } @@ -129,21 +131,38 @@ public function prepare(INotification $notification, string $languageCode): INot return $notification; } - private function tryGetRichParamForFile(string $uid, int $fileId) : array|bool { + /** + * Tries to build the rich notification parameters pointing to the file the + * notification was created for. + * + * @return array|null The rich parameters or `null` if they could not be determined + * because of an unexpected error. + * @throws AlreadyProcessedException If the file cannot be found for the given user + * anymore. In that case the notification is obsolete + * and gets removed from the database instead of being + * re-rendered on every notification poll (see #382). + */ + private function tryGetRichParamForFile(string $uid, int $fileId) : ?array { try { $userFolder = $this->rootFolder->getUserFolder($uid); - /** @var File[] */ - $files = $userFolder->getById($fileId); - /** @var File $file */ - $file = array_shift($files); - if ($file === null) { - $this->logger->warning('Could not find file with id {fileId} for user {uid}', ['fileId' => $fileId, 'uid' => $uid]); - return false; - } - $relativePath = $userFolder->getRelativePath($file->getPath()); + /** @var File|null $file */ + $file = $userFolder->getFirstNodeById($fileId); + $relativePath = $file !== null ? $userFolder->getRelativePath($file->getPath()) : null; } catch (\Throwable $th) { $this->logger->error($th->getMessage(), ['exception' => $th]); - return false; + return null; + } + + if ($file === null) { + // Nothing unusual: the user might have deleted the file (or moved it to the + // trashbin) after the OCR process has finished. Since we cannot render a link + // to the file anymore, the notification is dropped. This also prevents the + // message from being logged over and over again, because the notifications app + // re-renders every stored notification on each poll. + $this->logger->debug('Could not find file with id {fileId} for user {uid}, discarding obsolete notification', ['fileId' => $fileId, 'uid' => $uid]); + // Note:: AlreadyProcessedException has to be thrown before any call to $notification->set... + // otherwise notification won't be removed from the database + throw new AlreadyProcessedException(); } return [ diff --git a/tests/Unit/Notification/NotifierTest.php b/tests/Unit/Notification/NotifierTest.php index 644ca43f..73f48de5 100644 --- a/tests/Unit/Notification/NotifierTest.php +++ b/tests/Unit/Notification/NotifierTest.php @@ -156,9 +156,9 @@ public function testPrepareConstructsOcrErrorCorrectlyWithFileId() { /** @var Folder|MockObject */ $userFolder = $this->createMock(Folder::class); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') - ->willReturn(['file' => $file]); + ->method('getFirstNodeById') + ->with(123) + ->willReturn($file); $userFolder->expects($this->once()) ->method('getRelativePath') ->with('admin/files/file.txt') @@ -256,8 +256,8 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThr $userFolder = $this->createMock(Folder::class); $ex = new \OCP\Files\NotFoundException('nope ... sorry'); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') + ->method('getFirstNodeById') + ->with(123) ->willThrowException($ex); // This is what we want to test ... $userFolder->expects($this->never()) ->method('getRelativePath'); @@ -283,7 +283,10 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThr $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); } - public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayWasEmpty() { + /** + * @see https://github.com/R0Wi-DEV/workflow_ocr/issues/382 + */ + public function testThrowsAlreadyProcessedExceptionIfFileCannotBeFoundAnymore() { /** @var IValidator|MockObject */ $validator = $this->createMock(IValidator::class); /** @var IRichTextFormatter|MockObject */ @@ -305,31 +308,30 @@ public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayW /** @var Folder|MockObject */ $userFolder = $this->createMock(Folder::class); $userFolder->expects($this->once()) - ->method('getById') - ->with('123') - ->willReturn([]); // This is what we want to test ... + ->method('getFirstNodeById') + ->with(123) + ->willReturn(null); // This is what we want to test ... $userFolder->expects($this->never()) ->method('getRelativePath'); $this->rootFolder->expects($this->once()) ->method('getUserFolder') ->with('user') ->willReturn($userFolder); - $this->urlGenerator->expects($this->once()) - ->method('imagePath') - ->with('workflow_ocr', 'app-dark.svg') - ->willReturn('apps/workflow_ocr/app-dark.svg'); - $this->urlGenerator->expects($this->once()) - ->method('getAbsoluteURL') - ->with('apps/workflow_ocr/app-dark.svg') - ->willReturn('http://localhost/index.php/apps/workflow_ocr/app-dark.svg'); + // The notification is dropped, so it's never rendered + $this->urlGenerator->expects($this->never()) + ->method('imagePath'); + $this->urlGenerator->expects($this->never()) + ->method('linkToRouteAbsolute'); + // A missing file is expected (e.g. user deleted it), so no warning should be logged + $this->logger->expects($this->never()) + ->method('warning'); $this->logger->expects($this->once()) - ->method('warning') - ->with('Could not find file with id {fileId} for user {uid}', ['fileId' => '123', 'uid' => 'user']); + ->method('debug') + ->with('Could not find file with id {fileId} for user {uid}, discarding obsolete notification', ['fileId' => 123, 'uid' => 'user']); - $notification = $this->notifier->prepare($notification, 'en'); + $this->expectException(AlreadyProcessedException::class); - $this->assertEmpty($notification->getRichSubject()); - $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); + $this->notifier->prepare($notification, 'en'); } public function testFallbackToParsedSubjectIfMessageIsEmpty() {