Skip to content

Fix: discard notifications for deleted files instead of logging warnings repeatedly #389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
R0Wi wants to merge 1 commit into
base: master
Choose a base branch
from
+60 −39
Open

Fix: discard notifications for deleted files instead of logging warnings repeatedly #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 36 additions & 17 deletions lib/Notification/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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 [
Expand Down
46 changes: 24 additions & 22 deletions tests/Unit/Notification/NotifierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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');
Expand All @@ -283,7 +283,10 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThr
$this->assertEquals('<translated> 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 */
Expand All @@ -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('<translated> Workflow OCR error', $notification->getParsedSubject());
$this->notifier->prepare($notification, 'en');
}

public function testFallbackToParsedSubjectIfMessageIsEmpty() {
Expand Down
Loading