diff --git a/lib/Migration/Version2702Date20230908170345.php b/lib/Migration/Version2702Date20230908170345.php new file mode 100644 index 0000000..aa0bc51 --- /dev/null +++ b/lib/Migration/Version2702Date20230908170345.php @@ -0,0 +1,87 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OCA\WorkflowOcr\Migration; + +use Closure; +use OCA\WorkflowOcr\AppInfo\Application; +use OCP\DB\ISchemaWrapper; +use OCP\IDBConnection; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +class Version2702Date20230908170345 extends SimpleMigrationStep { + /** @var IDBConnection */ + private $db; + + public function __construct(IDBConnection $db) { + $this->db = $db; + } + + /** + * {@inheritDoc} + */ + public function name(): string { + return 'delete old notifications'; + } + + /** + * {@inheritDoc} + */ + public function description(): string { + return 'Delete old notifications which could not be processed #221'; + } + + /** + * {@inheritDoc} + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + try { + $this->deleteNonDeliverableNotifications(); + } catch(\Throwable $e) { + $output->warning('Could not delete non-deliverable notifications: ' . $e->getMessage() . '. Please see https://github.com/R0Wi-DEV/workflow_ocr/issues/221.'); + } + + return null; + } + + private function deleteNonDeliverableNotifications() { + /* + * See https://github.com/R0Wi-DEV/workflow_ocr/issues/221 + * We need to delete notifications which could not be delivered + * (for example because the file has been deleted in the meantime) + * because they are not deleted automatically. This is due to the fact + * that in our Notifier we set characteristics of the notification + * too early, which will lead to the problem, that the + * thrown AlreadyProcessedException won't let the framework delete + * the notification (the framework itself doesn't find any matching + * notifications). Therefore, such a notification will be processed + * over and over again. + */ + $builder = $this->db->getQueryBuilder(); + $builder->delete('notifications') + ->where($builder->expr()->eq('app', $builder->createNamedParameter(Application::APP_NAME))) + ->andWhere($builder->expr()->eq('subject', $builder->createNamedParameter('ocr_error'))) + ->executeStatement(); + } +} diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 46035e7..39463ba 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -81,37 +81,56 @@ public function prepare(INotification $notification, string $languageCode): INot throw new \InvalidArgumentException(); } - $notification->setIcon($this->urlGenerator->imagePath(Application::APP_NAME, 'app-dark.svg')); + // Currently we only support sending notifications for ocr_error + $subject = $notification->getSubject(); + if ($subject !== 'ocr_error') { + $this->logger->warning('Unsupported notification subject {subject}', ['subject' => $subject]); + // Note:: AlreadyProcessedException has be be thrown before any call to $notification->set... + // otherwise notification won't be removed from the database + throw new AlreadyProcessedException(); + } + $l = $this->l10nFactory->get(Application::APP_NAME, $languageCode); - // Currently we only support sending notifications for ocr_error - if ($notification->getSubject() !== 'ocr_error') { - throw new \InvalidArgumentException(); + // Only add file info if we have some ... + $richParams = false; + if ($notification->getObjectType() === 'file' && + ($fileId = $notification->getObjectId()) && + ($uid = $notification->getUser())) { + $richParams = $this->tryGetRichParamForFile($uid, intval($fileId)); + if ($richParams !== false) { + $notification->setRichSubject($l->t('Workflow OCR error for file {file}'), $richParams); + } + } + + // Fallback to generic error message without file link + if ($richParams === false) { + $notification->setParsedSubject($l->t('Workflow OCR error')); } $message = $notification->getSubjectParameters()['message']; $notification - ->setParsedSubject($l->t('Workflow OCR error')) - ->setParsedMessage($message); - // Only add file info if we have one ... - if ($notification->getObjectType() === 'file' && $notification->getObjectId()) { - $richParams = $this->getRichParamForFile($notification); - $notification->setRichSubject($l->t('Workflow OCR error for file {file}'), $richParams); - } + ->setParsedMessage($message) + ->setIcon($this->urlGenerator->imagePath(Application::APP_NAME, 'app-dark.svg')); + return $notification; } - private function getRichParamForFile(INotification $notification) : array { + private function tryGetRichParamForFile(string $uid, int $fileId) : array | bool { try { - $userFolder = $this->rootFolder->getUserFolder($notification->getUser()); + $userFolder = $this->rootFolder->getUserFolder($uid); /** @var File[] */ - $files = $userFolder->getById($notification->getObjectId()); + $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()); } catch (\Throwable $th) { $this->logger->error($th->getMessage(), ['exception' => $th]); - throw new AlreadyProcessedException(); + return false; } return [ @@ -120,7 +139,7 @@ private function getRichParamForFile(INotification $notification) : array { 'id' => $file->getId(), 'name' => $file->getName(), 'path' => $relativePath, - 'link' => $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $notification->getObjectId()]) + 'link' => $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $fileId]) ] ]; } diff --git a/tests/Unit/Notification/NotifierTest.php b/tests/Unit/Notification/NotifierTest.php index b5df259..3708c9e 100644 --- a/tests/Unit/Notification/NotifierTest.php +++ b/tests/Unit/Notification/NotifierTest.php @@ -108,7 +108,7 @@ public function testPrepareThrowsInvalidArgumentExceptionIfNotificationSubjectNo $notification->expects($this->once()) ->method('getSubject') ->willReturn('not_ocr_error'); - $this->expectException(\InvalidArgumentException::class); + $this->expectException(AlreadyProcessedException::class); $this->notifier->prepare($notification, 'en'); } @@ -117,16 +117,10 @@ public function testPrepareConstructsOcrErrorCorrectlyWithFileId() { $validator = $this->createMock(IValidator::class); /** @var IL10N|MockObject */ $l10n = $this->createMock(IL10N::class); - $l10n->expects($this->exactly(2)) + $l10n->expects($this->once()) ->method('t') - ->withConsecutive( - ['Workflow OCR error'], - ['Workflow OCR error for file {file}'] - ) - ->willReturnOnConsecutiveCalls( - 'Workflow OCR', - 'Workflow OCR error for file {file}' - ); + ->with('Workflow OCR error for file {file}') + ->willReturn(' Workflow OCR error for file {file}'); $this->l10nFactory->expects($this->once()) ->method('get') ->with('workflow_ocr') @@ -177,7 +171,7 @@ public function testPrepareConstructsOcrErrorCorrectlyWithFileId() { $richSubject = $preparedNotification->getRichSubject(); $richSubjectParams = $preparedNotification->getRichSubjectParameters(); - $this->assertEquals('Workflow OCR error for file {file}', $richSubject); + $this->assertEquals(' Workflow OCR error for file {file}', $richSubject); $this->assertEquals(['file' => [ 'type' => 'file', 'id' => '123', @@ -195,7 +189,7 @@ public function testPrepareConstructsOcrErrorCorrectlyWithoutFile() { $l10n->expects($this->once()) ->method('t') ->with('Workflow OCR error') - ->willReturn('Workflow OCR error'); + ->willReturn(' Workflow OCR error'); $this->l10nFactory->expects($this->once()) ->method('get') ->with('workflow_ocr') @@ -216,14 +210,59 @@ public function testPrepareConstructsOcrErrorCorrectlyWithoutFile() { $preparedNotification = $this->notifier->prepare($notification, 'en'); - $richSubject = $preparedNotification->getRichSubject(); - $richSubjectParams = $preparedNotification->getRichSubjectParameters(); + $this->assertEmpty($preparedNotification->getRichSubject()); + $this->assertEmpty($preparedNotification->getRichSubjectParameters()); + $this->assertEquals(' Workflow OCR error', $preparedNotification->getParsedSubject()); + } - $this->assertEquals('', $richSubject); - $this->assertEmpty($richSubjectParams); + public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThrown() { + /** @var IValidator|MockObject */ + $validator = $this->createMock(IValidator::class); + /** @var IL10N|MockObject */ + $l10n = $this->createMock(IL10N::class); + $l10n->expects($this->once()) + ->method('t') + ->with('Workflow OCR error') + ->willReturn(' Workflow OCR error'); + $this->l10nFactory->expects($this->once()) + ->method('get') + ->with('workflow_ocr') + ->willReturn($l10n); + + $notification = new Notification($validator); + $notification->setUser('user'); + $notification->setApp('workflow_ocr'); + $notification->setSubject('ocr_error', ['message' => 'mymessage']); + $notification->setObject('file', '123'); + + /** @var Folder|MockObject */ + $userFolder = $this->createMock(Folder::class); + $ex = new \OCP\Files\NotFoundException('nope ... sorry'); + $userFolder->expects($this->once()) + ->method('getById') + ->with('123') + ->willThrowException($ex); // 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('http://localhost/index.php/apps/workflow_ocr/app-dark.svg'); + $this->logger->expects($this->once()) + ->method('error') + ->with('nope ... sorry', ['exception' => $ex]); + + $notification = $this->notifier->prepare($notification, 'en'); + + $this->assertEmpty($notification->getRichSubject()); + $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); } - public function testThrowsAlreadyProcessedExceptionIfFileCannotBeRead() { + public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayWasEmpty() { /** @var IValidator|MockObject */ $validator = $this->createMock(IValidator::class); /** @var IL10N|MockObject */ @@ -231,7 +270,7 @@ public function testThrowsAlreadyProcessedExceptionIfFileCannotBeRead() { $l10n->expects($this->once()) ->method('t') ->with('Workflow OCR error') - ->willReturn('Workflow OCR'); + ->willReturn(' Workflow OCR error'); $this->l10nFactory->expects($this->once()) ->method('get') ->with('workflow_ocr') @@ -248,7 +287,7 @@ public function testThrowsAlreadyProcessedExceptionIfFileCannotBeRead() { $userFolder->expects($this->once()) ->method('getById') ->with('123') - ->willThrowException(new \OCP\Files\NotFoundException()); + ->willReturn([]); // This is what we want to test ... $userFolder->expects($this->never()) ->method('getRelativePath'); $this->rootFolder->expects($this->once()) @@ -259,8 +298,13 @@ public function testThrowsAlreadyProcessedExceptionIfFileCannotBeRead() { ->method('imagePath') ->with('workflow_ocr', 'app-dark.svg') ->willReturn('http://localhost/index.php/apps/workflow_ocr/app-dark.svg'); + $this->logger->expects($this->once()) + ->method('warning') + ->with('Could not find file with id {fileId} for user {uid}', ['fileId' => '123', 'uid' => 'user']); - $this->expectException(AlreadyProcessedException::class); - $this->notifier->prepare($notification, 'en'); + $notification = $this->notifier->prepare($notification, 'en'); + + $this->assertEmpty($notification->getRichSubject()); + $this->assertEquals(' Workflow OCR error', $notification->getParsedSubject()); } }