diff --git a/lib/App.php b/lib/App.php index ab2c02c0c..16ac4058d 100644 --- a/lib/App.php +++ b/lib/App.php @@ -61,16 +61,18 @@ public function getCount(INotification $notification): int { * @since 8.2.0 */ public function markProcessed(INotification $notification): void { - $deleted = $this->handler->delete($notification); + $notifications = $this->handler->delete($notification); $isAlreadyDeferring = $this->push->isDeferring(); if (!$isAlreadyDeferring) { $this->push->deferPayloads(); } - foreach ($deleted as $user => $notifications) { - foreach ($notifications as $data) { - $this->push->pushDeleteToDevice((string)$user, [$data['id']], $data['app']); - } + foreach ($notifications as $notificationId => $notification) { + $this->push->pushDeleteToDevice( + $notification->getUser(), + [ $notificationId ], + $notification->getApp() + ); } if (!$isAlreadyDeferring) { $this->push->flushPayloads(); diff --git a/lib/Controller/EndpointController.php b/lib/Controller/EndpointController.php index 2da411929..b482acc83 100644 --- a/lib/Controller/EndpointController.php +++ b/lib/Controller/EndpointController.php @@ -226,11 +226,9 @@ public function deleteNotification(int $id): DataResponse { try { $notification = $this->handler->getById($id, $this->getCurrentUser()); - $deleted = $this->handler->deleteById($id, $this->getCurrentUser(), $notification); - if ($deleted) { - $this->push->pushDeleteToDevice($this->getCurrentUser(), [$id], $notification->getApp()); - } + // markProcessed() does the actual deletion + $this->manager->markProcessed($notification); } catch (NotificationNotFoundException) { } @@ -251,15 +249,13 @@ public function deleteAllNotifications(): DataResponse { return new DataResponse(null, Http::STATUS_FORBIDDEN); } - $shouldFlush = $this->manager->defer(); - - $deletedSomething = $this->handler->deleteByUser($this->getCurrentUser()); - if ($deletedSomething) { - $this->push->pushDeleteToDevice($this->getCurrentUser(), null); - } + try { + $notification = $this->manager->createNotification(); + $notification->setUser($this->getCurrentUser()); - if ($shouldFlush) { - $this->manager->flush(); + // markProcessed() does the actual deletion + $this->manager->markProcessed($notification); + } catch (NotificationNotFoundException) { } return new DataResponse(); diff --git a/lib/Handler.php b/lib/Handler.php index b69246d76..7adc6ce2f 100644 --- a/lib/Handler.php +++ b/lib/Handler.php @@ -59,28 +59,7 @@ public function count(INotification $notification): int { * @return array A Map with all deleted notifications [user => [notifications]] */ public function delete(INotification $notification): array { - $sql = $this->connection->getQueryBuilder(); - $sql->select('*') - ->from('notifications'); - - $this->sqlWhere($sql, $notification); - $statement = $sql->executeQuery(); - - $deleted = []; - $notifications = []; - while ($row = $statement->fetch()) { - if (!isset($deleted[$row['user']])) { - $deleted[$row['user']] = []; - } - - $deleted[$row['user']][] = [ - 'id' => (int)$row['notification_id'], - 'app' => $row['app'], - ]; - $notifications[(int)$row['notification_id']] = $this->notificationFromRow($row); - } - $statement->closeCursor(); - + $notifications = $this->get($notification, PHP_INT_MAX); if (count($notifications) === 0) { return []; } @@ -93,9 +72,17 @@ public function delete(INotification $notification): array { $this->manager->dismissNotification($n); } + // delete notifications from db in chunks $notificationIds = array_keys($notifications); - foreach (array_chunk($notificationIds, 1000) as $chunk) { - $this->deleteIds($chunk); + foreach (array_chunk($notificationIds, 1000) as $ids) { + $sql = $this->connection->getQueryBuilder(); + $sql->delete('notifications') + ->where($sql->expr()->in( + 'notification_id', + $sql->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY) + ) + ); + $sql->executeStatement(); } if ($shouldFlush) { @@ -107,51 +94,7 @@ public function delete(INotification $notification): array { } $this->connection->commit(); - return $deleted; - } - - /** - * Delete the notification of a given user - */ - public function deleteByUser(string $user): bool { - $notification = $this->manager->createNotification(); - try { - $notification->setUser($user); - } catch (\InvalidArgumentException) { - return false; - } - return !empty($this->delete($notification)); - } - - /** - * Delete the notification matching the given id - * - * @throws NotificationNotFoundException - */ - public function deleteById(int $id, string $user, ?INotification $notification = null): bool { - if (!$notification instanceof INotification) { - $notification = $this->getById($id, $user); - } - - $this->manager->dismissNotification($notification); - - $sql = $this->connection->getQueryBuilder(); - $sql->delete('notifications') - ->where($sql->expr()->eq('notification_id', $sql->createNamedParameter($id))) - ->andWhere($sql->expr()->eq('user', $sql->createNamedParameter($user))); - return (bool)$sql->executeStatement(); - } - - /** - * Delete the notification matching the given ids - * - * @param int[] $ids - */ - public function deleteIds(array $ids): void { - $sql = $this->connection->getQueryBuilder(); - $sql->delete('notifications') - ->where($sql->expr()->in('notification_id', $sql->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); - $sql->executeStatement(); + return $notifications; } /** diff --git a/lib/Listener/UserDeletedListener.php b/lib/Listener/UserDeletedListener.php index 06cd3494d..13033e6de 100644 --- a/lib/Listener/UserDeletedListener.php +++ b/lib/Listener/UserDeletedListener.php @@ -9,7 +9,7 @@ namespace OCA\Notifications\Listener; -use OCA\Notifications\Handler; +use OCP\Notification\IManager as INotificationManager; use OCA\Notifications\Model\SettingsMapper; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; @@ -20,7 +20,7 @@ */ class UserDeletedListener implements IEventListener { public function __construct( - private Handler $handler, + private INotificationManager $notificationManager, private SettingsMapper $settingsMapper, ) { } @@ -32,7 +32,11 @@ public function handle(Event $event): void { } $user = $event->getUser(); - $this->handler->deleteByUser($user->getUID()); + + $notification = $this->notificationManager->createNotification(); + $notification->setUser($user->getUID()); + $this->notificationManager->markProcessed($notification); + $this->settingsMapper->deleteSettingsByUser($user->getUID()); } } diff --git a/tests/Unit/Controller/EndpointControllerTest.php b/tests/Unit/Controller/EndpointControllerTest.php index 24897edd1..d30f3c0a9 100644 --- a/tests/Unit/Controller/EndpointControllerTest.php +++ b/tests/Unit/Controller/EndpointControllerTest.php @@ -376,52 +376,6 @@ public static function dataDeleteNotification(): array { ]; } - /** - * @dataProvider dataDeleteNotification - */ - public function testDeleteNotification(int $id, string $username): void { - $controller = $this->getController([], $username); - - $this->handler->expects($this->once()) - ->method('deleteById') - ->with($id, $username); - - $response = $controller->deleteNotification($id); - $this->assertInstanceOf(DataResponse::class, $response); - $this->assertSame(Http::STATUS_OK, $response->getStatus()); - } - - public function testDeleteNotificationNoId(): void { - $controller = $this->getController(); - - $this->handler->expects($this->never()) - ->method('deleteById'); - - $response = $controller->deleteNotification(0); - $this->assertInstanceOf(DataResponse::class, $response); - $this->assertSame(Http::STATUS_NOT_FOUND, $response->getStatus()); - } - - /** - * @dataProvider dataDeleteNotification - */ - public function testDeleteAllNotifications(int $_, string $username): void { - $controller = $this->getController([], $username); - - $this->handler->expects($this->once()) - ->method('deleteByUser') - ->with($username); - $this->manager->expects($this->once()) - ->method('defer') - ->willReturn(true); - $this->manager->expects($this->once()) - ->method('flush'); - - $response = $controller->deleteAllNotifications(); - $this->assertInstanceOf(DataResponse::class, $response); - $this->assertSame(Http::STATUS_OK, $response->getStatus()); - } - public static function dataNotificationToArray(): array { return [ ['v1', 42, 'app1', 'user1', 1234, 'type1', '42', 'subject1', '', [], 'message1', 'richMessage 1', ['richMessage param'], 'link1', 'icon1', 0, []], diff --git a/tests/Unit/HandlerTest.php b/tests/Unit/HandlerTest.php index b9f66c74b..d002e98f2 100644 --- a/tests/Unit/HandlerTest.php +++ b/tests/Unit/HandlerTest.php @@ -153,68 +153,6 @@ public function testFullEmptyMessageForOracle(): void { $this->assertSame(0, $this->handler->count($limitedNotification2), 'Wrong notification count for user2 after deleting'); } - public function testDeleteById(): void { - $notification = $this->getNotification([ - 'getApp' => 'testing_notifications', - 'getUser' => 'test_user1', - 'getDateTime' => new \DateTime(), - 'getObjectType' => 'notification', - 'getObjectId' => '1337', - 'getSubject' => 'subject', - 'getSubjectParameters' => [], - 'getMessage' => 'message', - 'getMessageParameters' => [], - 'getLink' => 'link', - 'getIcon' => 'icon', - 'getActions' => [ - [ - 'getLabel' => 'action_label', - 'getLink' => 'action_link', - 'getRequestType' => 'GET', - 'isPrimary' => true, - ] - ], - ]); - $limitedNotification = $this->getNotification([ - 'getApp' => 'testing_notifications', - 'getUser' => 'test_user1', - ]); - - // Make sure there is no notification - $this->assertSame(0, $this->handler->count($limitedNotification)); - $notifications = $this->handler->get($limitedNotification); - $this->assertCount(0, $notifications); - - // Add and count - $this->handler->add($notification); - $this->assertSame(1, $this->handler->count($limitedNotification)); - - // Get and count - $notifications = $this->handler->get($limitedNotification); - $this->assertCount(1, $notifications); - $notificationId = array_key_first($notifications); - - // Get with wrong user - try { - $this->handler->getById($notificationId, 'test_user2'); - $this->fail('Exception of type NotificationNotFoundException expected'); - } catch (\Exception $e) { - $this->assertInstanceOf(NotificationNotFoundException::class, $e); - } - - // Delete with wrong user - $this->handler->deleteById($notificationId, 'test_user2', $notification); - $this->assertSame(1, $this->handler->count($limitedNotification), 'Wrong notification count for user1 after trying to delete for user2'); - - // Get with correct user - $getNotification = $this->handler->getById($notificationId, 'test_user1'); - $this->assertInstanceOf(INotification::class, $getNotification); - - // Delete and count - $this->handler->deleteById($notificationId, 'test_user1'); - $this->assertSame(0, $this->handler->count($limitedNotification), 'Wrong notification count for user1 after deleting'); - } - protected function getNotification(array $values = []): INotification&MockObject { $notification = $this->getMockBuilder(INotification::class) ->getMock();