From f098a5dc96f4fb232c81aa821705cad23ba6c3d7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 11 Apr 2024 15:01:45 +0200 Subject: [PATCH] techdebt(exceptions): Migrate to new exceptions Signed-off-by: Joas Schilling --- docs/notification-workflow.md | 26 ++++++++------- lib/App.php | 18 +++++------ lib/Notifier/AdminNotifications.php | 33 ++++++------------- tests/Unit/AppTest.php | 30 +++++++++--------- tests/Unit/Notifier/NotifierTest.php | 47 +++++++++++----------------- 5 files changed, 64 insertions(+), 90 deletions(-) diff --git a/docs/notification-workflow.md b/docs/notification-workflow.md index c63943b27..b791520dd 100644 --- a/docs/notification-workflow.md +++ b/docs/notification-workflow.md @@ -11,7 +11,7 @@ with it, we want to remove the notification again. 1. Grab a new notification object (`\OCP\Notification\INotification`) from the manager (`\OCP\Notification\IManager`): ```php -$manager = \OC::$server->get(\OCP\Notification\IManager::class); +$manager = \OCP\Server::get(\OCP\Notification\IManager::class); $notification = $manager->createNotification(); ``` @@ -51,16 +51,18 @@ $manager->notify($notification); ### Preparing a notification for display - 1. In `app.php` register your Notifier (`\OCP\Notification\INotifier`) interface to the manager, +1. In `OCA\MyApp\Application::register(IRegistrationContext $context)` register your Notifier (implementing the `\OCP\Notification\INotifier` interface) to the manager, using a `\Closure` returning the Notifier and a `\Closure` returning an array of the id and name: ```php -$manager = \OC::$server->get(\OCP\Notification\IManager::class); -$manager->registerNotifierService(\OCA\Files_Sharing\Notification\Notifier::class); + public function register(IRegistrationContext $context): void { + $context->registerNotifierService(\OCA\Files_Sharing\Notification\Notifier::class); + } ``` - 2. The manager will execute the closure and then call the `prepare()` method on your notifier. - If the notification is not known by your app, just throw an `\InvalidArgumentException`, - but if it is actually from your app, you must set the parsed subject, message and action labels: +2. The manager will execute the closure and then call the `prepare()` method on your notifier. + If the notification is not known by your app, just throw an `OCP\Notification\UnknownNotificationException` (*Added in Nextcloud 30, before throw `\InvalidArgumentException`*), + but if it is actually from your app, you must set the parsed subject, message and action labels. + If the notification is obsolete by now, e.g. because the share was revoked already, you can throw a `OCP\Notification\AlreadyProcessedException` which will remove the notification also from the devices. ```php class Notifier implements \OCP\Notification\INotifier { @@ -96,7 +98,7 @@ class Notifier implements \OCP\Notification\INotifier { public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'files_sharing') { // Not my app => throw - throw new \InvalidArgumentException(); + throw new \OCP\Notification\UnknownNotificationException(); } // Read the language from the notification @@ -150,7 +152,7 @@ class Notifier implements \OCP\Notification\INotifier { default: // Unknown subject => Unknown notification => throw - throw new \InvalidArgumentException(); + throw new \OCP\Notification\UnknownNotificationException(); } } @@ -186,7 +188,7 @@ call the `markProcessed()` method on the manager with the necessary information notification object: ```php -$manager = \OC::$server->get(\OCP\Notification\IManager::class); +$manager = \OCP\Server::get(\OCP\Notification\IManager::class); $notification->setApp('files_sharing') ->setObject('remote', 1337) ->setUser('recipient1'); @@ -198,7 +200,7 @@ will be marked as processed for all users that have it. So the following example remove all notifications for the app files_sharing on the object "remote #1337": ```php -$manager = \OC::$server->get(\OCP\Notification\IManager::class); +$manager = \OCP\Server::get(\OCP\Notification\IManager::class); $notification->setApp('files_sharing') ->setObject('remote', 1337); $manager->markProcessed($notification); @@ -210,7 +212,7 @@ Sometimes you might send multiple notifications in one request. In that case it makes sense to defer the sending, so in the end only one connection is done to the push server instead of 1 per notification. ```php -$manager = \OC::$server->get(\OCP\Notification\IManager::class); +$manager = \OCP\Server::get(\OCP\Notification\IManager::class); $shouldFlush = $manager->defer(); // Your application code generating notifications … diff --git a/lib/App.php b/lib/App.php index fc1d2cf97..5a6019e35 100644 --- a/lib/App.php +++ b/lib/App.php @@ -28,18 +28,15 @@ use OCA\Notifications\Exceptions\NotificationNotFoundException; use OCP\Notification\IDeferrableApp; use OCP\Notification\INotification; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Output\OutputInterface; class App implements IDeferrableApp { - /** @var Handler */ - protected $handler; - /** @var Push */ - protected $push; - - public function __construct(Handler $handler, - Push $push) { - $this->handler = $handler; - $this->push = $push; + public function __construct( + protected Handler $handler, + protected Push $push, + protected LoggerInterface $logger, + ) { } public function setOutput(OutputInterface $output): void { @@ -48,7 +45,6 @@ public function setOutput(OutputInterface $output): void { /** * @param INotification $notification - * @throws \InvalidArgumentException When the notification is not valid * @since 8.2.0 */ public function notify(INotification $notification): void { @@ -57,7 +53,7 @@ public function notify(INotification $notification): void { try { $this->push->pushToDevice($notificationId, $notification); } catch (NotificationNotFoundException $e) { - throw new \InvalidArgumentException('Error while preparing push notification'); + $this->logger->error('Error while preparing push notification', ['exception' => $e]); } } diff --git a/lib/Notifier/AdminNotifications.php b/lib/Notifier/AdminNotifications.php index dfc489d8b..3d7e061d8 100644 --- a/lib/Notifier/AdminNotifications.php +++ b/lib/Notifier/AdminNotifications.php @@ -32,30 +32,18 @@ use OCP\IUser; use OCP\IUserManager; use OCP\L10N\IFactory; -use OCP\Notification\AlreadyProcessedException; use OCP\Notification\IAction; use OCP\Notification\INotification; use OCP\Notification\INotifier; +use OCP\Notification\UnknownNotificationException; class AdminNotifications implements INotifier { - /** @var IFactory */ - protected $l10nFactory; - - /** @var IURLGenerator */ - protected $urlGenerator; - /** @var IUserManager */ - protected $userManager; - /** @var IRootFolder */ - protected $rootFolder; - - public function __construct(IFactory $l10nFactory, - IURLGenerator $urlGenerator, - IUserManager $userManager, - IRootFolder $rootFolder) { - $this->l10nFactory = $l10nFactory; - $this->urlGenerator = $urlGenerator; - $this->userManager = $userManager; - $this->rootFolder = $rootFolder; + public function __construct( + protected IFactory $l10nFactory, + protected IURLGenerator $urlGenerator, + protected IUserManager $userManager, + protected IRootFolder $rootFolder, + ) { } /** @@ -82,12 +70,11 @@ public function getName(): string { * @param INotification $notification * @param string $languageCode The code of the language that should be used to prepare the notification * @return INotification - * @throws \InvalidArgumentException When the notification was not prepared by a notifier - * @throws AlreadyProcessedException When the notification is not needed anymore and should be deleted + * @throws UnknownNotificationException When the notification was not prepared by a notifier */ public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'admin_notifications' && $notification->getApp() !== 'admin_notification_talk') { - throw new \InvalidArgumentException('Unknown app'); + throw new UnknownNotificationException('app'); } switch ($notification->getSubject()) { @@ -216,7 +203,7 @@ public function prepare(INotification $notification, string $languageCode): INot return $notification; default: - throw new \InvalidArgumentException('Unknown subject'); + throw new UnknownNotificationException('subject'); } } } diff --git a/tests/Unit/AppTest.php b/tests/Unit/AppTest.php index 26d540d41..bfc47ed09 100644 --- a/tests/Unit/AppTest.php +++ b/tests/Unit/AppTest.php @@ -26,17 +26,15 @@ use OCA\Notifications\Handler; use OCA\Notifications\Push; use OCP\Notification\INotification; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; class AppTest extends TestCase { - /** @var Handler|\PHPUnit_Framework_MockObject_MockObject */ - protected $handler; - /** @var Push|\PHPUnit_Framework_MockObject_MockObject */ - protected $push; - /** @var INotification|\PHPUnit_Framework_MockObject_MockObject */ - protected $notification; - - /** @var App */ - protected $app; + protected Handler|MockObject $handler; + protected Push|MockObject $push; + protected INotification|MockObject $notification; + protected LoggerInterface|MockObject $logger; + protected App $app; protected function setUp(): void { parent::setUp(); @@ -44,14 +42,16 @@ protected function setUp(): void { $this->handler = $this->createMock(Handler::class); $this->push = $this->createMock(Push::class); $this->notification = $this->createMock(INotification::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->app = new App( $this->handler, - $this->push + $this->push, + $this->logger, ); } - public function dataNotify() { + public static function dataNotify(): array { return [ [23], [42], @@ -63,7 +63,7 @@ public function dataNotify() { * * @param int $id */ - public function testNotify($id) { + public function testNotify(int $id): void { $this->handler->expects($this->once()) ->method('add') ->with($this->notification) @@ -75,7 +75,7 @@ public function testNotify($id) { $this->app->notify($this->notification); } - public function dataGetCount() { + public static function dataGetCount(): array { return [ [23], [42], @@ -87,7 +87,7 @@ public function dataGetCount() { * * @param int $count */ - public function testGetCount($count) { + public function testGetCount(int $count): void { $this->handler->expects($this->once()) ->method('count') ->with($this->notification) @@ -96,7 +96,7 @@ public function testGetCount($count) { $this->assertSame($count, $this->app->getCount($this->notification)); } - public function testMarkProcessed() { + public function testMarkProcessed(): void { $this->handler->expects($this->once()) ->method('delete') ->with($this->notification); diff --git a/tests/Unit/Notifier/NotifierTest.php b/tests/Unit/Notifier/NotifierTest.php index 2688ab184..93fb7f3a9 100644 --- a/tests/Unit/Notifier/NotifierTest.php +++ b/tests/Unit/Notifier/NotifierTest.php @@ -30,22 +30,16 @@ use OCP\IUserManager; use OCP\L10N\IFactory; use OCP\Notification\INotification; +use OCP\Notification\UnknownNotificationException; use PHPUnit\Framework\MockObject\MockObject; class NotifierTest extends \Test\TestCase { - /** @var AdminNotifications */ - protected $notifier; - - /** @var IFactory|MockObject */ - protected $factory; - /** @var IURLGenerator|MockObject */ - protected $urlGenerator; - /** @var IUserManager|MockObject */ - protected $userManager; - /** @var IRootFolder|MockObject */ - protected $rootFolder; - /** @var IL10N|MockObject */ - protected $l; + protected IFactory|MockObject $factory; + protected IURLGenerator|MockObject $urlGenerator; + protected IUserManager|MockObject $userManager; + protected IRootFolder|MockObject $rootFolder; + protected IL10N|MockObject $l; + protected AdminNotifications $notifier; protected function setUp(): void { parent::setUp(); @@ -72,8 +66,8 @@ protected function setUp(): void { ); } - public function testPrepareWrongApp() { - /** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ + public function testPrepareWrongApp(): void { + /** @var INotification|MockObject $notification */ $notification = $this->createMock(INotification::class); $notification->expects($this->exactly(2)) @@ -82,13 +76,13 @@ public function testPrepareWrongApp() { $notification->expects($this->never()) ->method('getSubject'); - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Unknown app'); + $this->expectException(UnknownNotificationException::class); + $this->expectExceptionMessage('app'); $this->notifier->prepare($notification, 'en'); } - public function testPrepareWrongSubject() { - /** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ + public function testPrepareWrongSubject(): void { + /** @var INotification|MockObject $notification */ $notification = $this->createMock(INotification::class); $notification->expects($this->once()) @@ -98,12 +92,12 @@ public function testPrepareWrongSubject() { ->method('getSubject') ->willReturn('wrong subject'); - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('Unknown subject'); + $this->expectException(UnknownNotificationException::class); + $this->expectExceptionMessage('subject'); $this->notifier->prepare($notification, 'en'); } - public function dataPrepare() { + public static function dataPrepare(): array { return [ ['ocs', ['subject'], ['message'], true], ]; @@ -111,14 +105,9 @@ public function dataPrepare() { /** * @dataProvider dataPrepare - * - * @param string $subject - * @param array $subjectParams - * @param array $messageParams - * @param bool $setMessage */ - public function testPrepare($subject, $subjectParams, $messageParams, $setMessage) { - /** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ + public function testPrepare(string $subject, array $subjectParams, array $messageParams, bool $setMessage): void { + /** @var INotification|MockObject $notification */ $notification = $this->createMock(INotification::class); $notification->expects($this->once())