Add Migration to remove Notification + additional tests

Signed-off-by: Robin Windey <ro.windey@gmail.com>
R0Wi-DEV · Sep 8, 2023 · c464f6c · c464f6c
1 parent a06b746
commit c464f6c
Show file tree

Hide file tree

Showing 3 changed files with 144 additions and 7 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 <ro.windey@gmail.com>
+ *
+ *  @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
@@ -96,11 +96,11 @@ public function prepare(INotification $notification, string $languageCode): INot
 		$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);
-				}
+			($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

diff --git a/tests/Unit/Notification/NotifierTest.php b/tests/Unit/Notification/NotifierTest.php
@@ -215,7 +215,7 @@ public function testPrepareConstructsOcrErrorCorrectlyWithoutFile() {
 		$this->assertEquals('<translated> Workflow OCR error', $preparedNotification->getParsedSubject());
 	}
 
-	public function testSendsFallbackNotificationWithoutFileInfoIfFileCannotBeRead() {
+	public function testSendsFallbackNotificationWithoutFileInfoIfFileNotFoundWasThrown() {
 		/** @var IValidator|MockObject */
 		$validator = $this->createMock(IValidator::class);
 		/** @var IL10N|MockObject */
@@ -237,10 +237,11 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileCannotBeRead()
 
 		/** @var Folder|MockObject */
 		$userFolder = $this->createMock(Folder::class);
+		$ex = new \OCP\Files\NotFoundException('nope ... sorry');
 		$userFolder->expects($this->once())
 			->method('getById')
 			->with('123')
-			->willThrowException(new \OCP\Files\NotFoundException());
+			->willThrowException($ex); // This is what we want to test ...
 		$userFolder->expects($this->never())
 			->method('getRelativePath');
 		$this->rootFolder->expects($this->once())
@@ -251,6 +252,55 @@ public function testSendsFallbackNotificationWithoutFileInfoIfFileCannotBeRead()
 			->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 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('<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);
+		$userFolder->expects($this->once())
+			->method('getById')
+			->with('123')
+			->willReturn([]); // 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('warning')
+			->with('Could not find file with id {fileId} for user {uid}', ['fileId' => '123', 'uid' => 'user']);
 
 		$notification = $this->notifier->prepare($notification, 'en');