Adjustments from the code review

owncloud · Apr 14, 2021 · 4dfa6c2 · 4dfa6c2
1 parent f5e00cc
commit 4dfa6c2
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 82 deletions.
diff --git a/lib/Command/RepairNotifications.php b/lib/Command/RepairNotifications.php
@@ -22,27 +22,27 @@
 
 namespace OCA\Notifications\Command;
 
-use OCP\IDBConnection;
+use OCA\Notifications\Handler;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class RepairNotifications extends Command {
 
-	/** @var IDBConnection */
-	protected $connection;
+	/** @var Handler */
+	protected $handler;
 
 	public static $availableSubjects = [
 		'relativeLinks'
 	];
 
 	/**
-	 * @param IDBConnection $connection
+	 * @param Handler $handler
 	 */
-	public function __construct(IDBConnection $connection) {
+	public function __construct(Handler $handler) {
 		parent::__construct();
-		$this->connection = $connection;
+		$this->handler = $handler;
 	}
 
 	protected function configure() {
@@ -56,60 +56,19 @@ protected function configure() {
 	/**
 	 * @param InputInterface $input
 	 * @param OutputInterface $output
-	 * @return false|int
+	 * @return int
 	 */
 	protected function execute(InputInterface $input, OutputInterface $output) {
 		$subject = $input->getArgument('subject');
 
 		if (!\in_array($subject, self::$availableSubjects)) {
-			throw new \LogicException('Invalid subject');
+			$output->writeln('Invalid subject');
+			return 1;
 		}
 
-		$sql = $this->connection->getQueryBuilder();
-		$sql->select(['notification_id', 'link', 'actions'])
-			->from('notifications')
-			->where($sql->expr()->like('link', $sql->createPositionalParameter('http%')))
-			->orWhere($sql->expr()->like('actions', $sql->createPositionalParameter('%"link":"http%')));
+		$updatedNotificationsCount = $this->handler->removeBaseUrlFromAbsoluteLinks();
 
-		$result = $sql->execute()->fetchAll();
-
-		if (!$result) {
-			$output->writeln('No notifications found to repair.');
-			return 0;
-		}
-
-		$output->writeln(\sprintf('%s notification(s) found to repair', \count($result)));
-
-		foreach ($result as $row) {
-			$output->writeln(\sprintf('Repairing notification with ID %s...', $row['notification_id']));
-
-			$sql = $this->connection->getQueryBuilder();
-			$sql->update('notifications')
-				->where($sql->expr()->eq('notification_id', $sql->createNamedParameter($row['notification_id'])));
-
-			$linkUrlComponents = \parse_url($row['link']);
-			if (\array_key_exists('scheme', $linkUrlComponents)) {
-				$newLink = \parse_url($row['link'], PHP_URL_PATH);
-				$sql->set('link', $sql->createNamedParameter($newLink));
-			}
-
-			if (\strpos($row['actions'], 'http') !== false) {
-				$actions = \json_decode($row['actions'], true);
-
-				foreach ($actions as &$action) {
-					$actionUrlComponents = \parse_url($action['link']);
-					if (\array_key_exists('scheme', $actionUrlComponents)) {
-						$action['link'] = \parse_url($action['link'], PHP_URL_PATH);
-					}
-				}
-
-				$sql->set('actions', $sql->createNamedParameter(\json_encode($actions)));
-			}
-
-			$sql->execute();
-		}
-
-		$output->writeln('Done');
+		$output->writeln("$updatedNotificationsCount notifications were updated");
 		return 0;
 	}
 }
diff --git a/lib/Handler.php b/lib/Handler.php
@@ -310,4 +310,54 @@ protected function notificationFromRow(array $row) {
 
 		return $notification;
 	}
+
+	/**
+	 * Remove the base url from absolute links in the database.
+	 * This affects the columns 'link' and 'action'.
+	 * e.g: http://owncloud.com/test -> /test
+	 *
+	 * @return int number of updated notifications
+	 */
+	public function removeBaseUrlFromAbsoluteLinks() {
+		$sql = $this->connection->getQueryBuilder();
+		$sql->select(['notification_id', 'link', 'actions'])
+			->from('notifications')
+			->where($sql->expr()->like('link', $sql->createPositionalParameter('http%')))
+			->orWhere($sql->expr()->like('actions', $sql->createPositionalParameter('%"link":"http%')));
+
+		$statement = $sql->execute();
+		$counter = 0;
+
+		while ($row = $statement->fetch()) {
+			$sql = $this->connection->getQueryBuilder();
+			$sql->update('notifications')
+				->where($sql->expr()->eq('notification_id', $sql->createNamedParameter($row['notification_id'])));
+
+			$linkUrlComponents = \parse_url($row['link']);
+			if (isset($linkUrlComponents['scheme'])) {
+				$newLink = \parse_url($row['link'], PHP_URL_PATH);
+				$sql->set('link', $sql->createNamedParameter($newLink));
+			}
+
+			if (\strpos($row['actions'], 'http') !== false) {
+				$actions = \json_decode($row['actions'], true);
+
+				foreach ($actions as $index => $action) {
+					$actionUrlComponents = \parse_url($action['link']);
+					if (isset($actionUrlComponents['scheme'])) {
+						$actions[$index]['link'] = \parse_url($action['link'], PHP_URL_PATH);
+					}
+				}
+
+				$sql->set('actions', $sql->createNamedParameter(\json_encode($actions)));
+			}
+
+			$counter++;
+			$sql->execute();
+		}
+
+		$statement->closeCursor();
+
+		return $counter;
+	}
 }
diff --git a/lib/Mailer/NotificationMailer.php b/lib/Mailer/NotificationMailer.php
@@ -76,12 +76,11 @@ public function sendNotification(INotification $notification, $serverUrl, $email
 		$emailMessage->setTo([$emailAddress]);
 
 		$notificationLink = $notification->getLink();
-		$components = \parse_url($notificationLink);
-		$linkIsAbsolute = \array_key_exists('host', $components);
+		$urlComponents = \parse_url($notificationLink);
 
 		if ($notificationLink === '') {
 			$notificationLink = $serverUrl;
-		} elseif ($linkIsAbsolute !== true) {
+		} elseif (!isset($urlComponents['host'])) {
 			$notificationLink = $this->urlGenerator->getAbsoluteURL($notificationLink);
 		}
 

diff --git a/tests/Unit/Command/RepairNotificationsTest.php b/tests/Unit/Command/RepairNotificationsTest.php
@@ -22,19 +22,16 @@
 
 namespace OCA\Notifications\Tests\Unit\Command;
 
-use Doctrine\DBAL\Driver\Statement;
 use OCA\Notifications\Command\Generate;
 use OCA\Notifications\Command\RepairNotifications;
+use OCA\Notifications\Handler;
 use OCA\Notifications\Tests\Unit\TestCase;
-use OCP\DB\QueryBuilder\IExpressionBuilder;
-use OCP\DB\QueryBuilder\IQueryBuilder;
-use OCP\IDBConnection;
 use Symfony\Component\Console\Tester\CommandTester;
 
 class RepairNotificationsTest extends TestCase {
 
-	/** @var IDBConnection | \PHPUnit\Framework\MockObject\MockObject */
-	protected $connection;
+	/** @var Handler | \PHPUnit\Framework\MockObject\MockObject */
+	protected $handler;
 	/** @var Generate */
 	protected $command;
 	/** @var CommandTester */
@@ -43,39 +40,23 @@ class RepairNotificationsTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
-		$this->connection = $this->createMock(IDBConnection::class);
-		$this->command = new RepairNotifications($this->connection);
+		$this->handler = $this->createMock(Handler::class);
+		$this->command = new RepairNotifications($this->handler);
 		$this->tester = new CommandTester($this->command);
 	}
 
 	public function testInvalidSubject() {
-		$this->expectException(\LogicException::class);
-
 		$options = [];
 		$input = ['subject' => 'test'];
-		$this->tester->execute($input, $options);
+		$response = $this->tester->execute($input, $options);
+		$this->assertEquals(1, $response);
 	}
 
 	public function testRepairLinks() {
 		$options = [];
 		$input = ['subject' => RepairNotifications::$availableSubjects[0]];
 
-		$dbResult = [
-			['notification_id' => 1, 'link' => 'http://owncloud.com/test', 'actions' => '[]']
-		];
-
-		$exprBuilder = $this->createMock(IExpressionBuilder::class);
-		$statementMock = $this->createMock(Statement::class);
-		$statementMock->method('fetchAll')->willReturn($dbResult);
-		$qbMock = $this->createMock(IQueryBuilder::class);
-		$qbMock->method('select')->willReturnSelf();
-		$qbMock->method('from')->willReturnSelf();
-		$qbMock->method('update')->willReturnSelf();
-		$qbMock->method('where')->willReturnSelf();
-		$qbMock->method('expr')->willReturn($exprBuilder);
-		$qbMock->method('execute')->willReturn($statementMock);
-
-		$this->connection->method('getQueryBuilder')->willReturn($qbMock);
+		$this->handler->expects($this->once())->method('removeBaseUrlFromAbsoluteLinks');
 
 		$response = $this->tester->execute($input, $options);
 		$this->assertEquals(0, $response);