Fix Notifications not being processed #221 (#228)

* Fix Notifications not being processed #221 * Add Migration to remove Notification + additional tests Signed-off-by: Robin Windey <[email protected]> --------- Signed-off-by: Robin Windey <[email protected]>
R0Wi-DEV · Sep 8, 2023 · 06c6e6f · 06c6e6f
1 parent 8a6387f
commit 06c6e6f
Show file tree

Hide file tree

Showing 3 changed files with 187 additions and 37 deletions.
diff --git a/lib/Migration/Version2702Date20230908170345.php b/lib/Migration/Version2702Date20230908170345.php
@@ -0,0 +1,87 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2023 Robin Windey <[email protected]>
+ *
+ *  @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 <http://www.gnu.org/licenses/>.
+ */
+
+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
@@ -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
@@ -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('<translated> 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('<translated> 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('<translated> Workflow OCR error');
 		$this->l10nFactory->expects($this->once())
 			->method('get')
 			->with('workflow_ocr')
@@ -216,22 +210,67 @@ 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('<translated> 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('<translated> 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('<translated> Workflow OCR error', $notification->getParsedSubject());
 	}
 
-	public function testThrowsAlreadyProcessedExceptionIfFileCannotBeRead() {
+	public function testSendsFallbackNotificationWithoutFileInfoIfReturnedFileArrayWasEmpty() {
 		/** @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');
+			->willReturn('<translated> 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('<translated> Workflow OCR error', $notification->getParsedSubject());
 	}
 }