From 9b288cda6daf0ab82a910b3442fcc0e003471741 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 10 Apr 2019 14:12:10 +0200 Subject: [PATCH 01/12] Make all interfaces strict Signed-off-by: Joas Schilling --- lib/private/Notification/Action.php | 45 +++++---- lib/private/Notification/Manager.php | 14 +-- lib/private/Notification/Notification.php | 112 ++++++++++------------ lib/public/Notification/IAction.php | 63 ++++++++---- lib/public/Notification/IApp.php | 7 +- lib/public/Notification/IManager.php | 11 ++- lib/public/Notification/INotification.php | 78 +++++++-------- lib/public/Notification/INotifier.php | 3 +- 8 files changed, 173 insertions(+), 160 deletions(-) diff --git a/lib/private/Notification/Action.php b/lib/private/Notification/Action.php index 8dfeecb98c..f6d6a33358 100644 --- a/lib/private/Notification/Action.php +++ b/lib/private/Notification/Action.php @@ -1,4 +1,5 @@ label = ''; $this->labelParsed = ''; @@ -62,8 +60,8 @@ class Action implements IAction { * @throws \InvalidArgumentException if the label is invalid * @since 8.2.0 */ - public function setLabel($label) { - if (!is_string($label) || $label === '' || isset($label[32])) { + public function setLabel(string $label): IAction { + if ($label === '' || isset($label[32])) { throw new \InvalidArgumentException('The given label is invalid'); } $this->label = $label; @@ -74,7 +72,7 @@ class Action implements IAction { * @return string * @since 8.2.0 */ - public function getLabel() { + public function getLabel(): string { return $this->label; } @@ -84,8 +82,8 @@ class Action implements IAction { * @throws \InvalidArgumentException if the label is invalid * @since 8.2.0 */ - public function setParsedLabel($label) { - if (!is_string($label) || $label === '') { + public function setParsedLabel(string $label): IAction { + if ($label === '') { throw new \InvalidArgumentException('The given parsed label is invalid'); } $this->labelParsed = $label; @@ -96,21 +94,16 @@ class Action implements IAction { * @return string * @since 8.2.0 */ - public function getParsedLabel() { + public function getParsedLabel(): string { return $this->labelParsed; } /** * @param $primary bool * @return $this - * @throws \InvalidArgumentException if $primary is invalid * @since 9.0.0 */ - public function setPrimary($primary) { - if (!is_bool($primary)) { - throw new \InvalidArgumentException('The given primary option is invalid'); - } - + public function setPrimary(bool $primary): IAction { $this->primary = $primary; return $this; } @@ -119,7 +112,7 @@ class Action implements IAction { * @return bool * @since 9.0.0 */ - public function isPrimary() { + public function isPrimary(): bool { return $this->primary; } @@ -130,11 +123,17 @@ class Action implements IAction { * @throws \InvalidArgumentException if the link is invalid * @since 8.2.0 */ - public function setLink($link, $requestType) { - if (!is_string($link) || $link === '' || isset($link[256])) { + public function setLink(string $link, string $requestType): IAction { + if ($link === '' || isset($link[256])) { throw new \InvalidArgumentException('The given link is invalid'); } - if (!in_array($requestType, ['GET', 'POST', 'PUT', 'DELETE'], true)) { + if (!in_array($requestType, [ + self::TYPE_GET, + self::TYPE_POST, + self::TYPE_PUT, + self::TYPE_DELETE, + self::TYPE_WEB, + ], true)) { throw new \InvalidArgumentException('The given request type is invalid'); } $this->link = $link; @@ -146,7 +145,7 @@ class Action implements IAction { * @return string * @since 8.2.0 */ - public function getLink() { + public function getLink(): string { return $this->link; } @@ -154,21 +153,21 @@ class Action implements IAction { * @return string * @since 8.2.0 */ - public function getRequestType() { + public function getRequestType(): string { return $this->requestType; } /** * @return bool */ - public function isValid() { + public function isValid(): bool { return $this->label !== '' && $this->link !== ''; } /** * @return bool */ - public function isValidParsed() { + public function isValidParsed(): bool { return $this->labelParsed !== '' && $this->link !== ''; } } diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index 4c3f7a2453..876a191fb9 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -1,4 +1,5 @@ validator = $validator; $this->apps = []; @@ -179,7 +175,7 @@ class Manager implements IManager { * @param bool $preparingPushNotification * @since 14.0.0 */ - public function setPreparingPushNotification($preparingPushNotification) { + public function setPreparingPushNotification(bool $preparingPushNotification): void { $this->preparingPushNotification = $preparingPushNotification; } @@ -196,7 +192,7 @@ class Manager implements IManager { * @throws \InvalidArgumentException When the notification is not valid * @since 8.2.0 */ - public function notify(INotification $notification) { + public function notify(INotification $notification): void { if (!$notification->isValid()) { throw new \InvalidArgumentException('The given notification is invalid'); } @@ -218,7 +214,7 @@ class Manager implements IManager { * @throws \InvalidArgumentException When the notification was not prepared by a notifier * @since 8.2.0 */ - public function prepare(INotification $notification, $languageCode): INotification { + public function prepare(INotification $notification, string $languageCode): INotification { $notifiers = $this->getNotifiers(); foreach ($notifiers as $notifier) { @@ -243,7 +239,7 @@ class Manager implements IManager { /** * @param INotification $notification */ - public function markProcessed(INotification $notification) { + public function markProcessed(INotification $notification): void { $apps = $this->getApps(); foreach ($apps as $app) { diff --git a/lib/private/Notification/Notification.php b/lib/private/Notification/Notification.php index 47f415d15d..e64c059db2 100644 --- a/lib/private/Notification/Notification.php +++ b/lib/private/Notification/Notification.php @@ -1,5 +1,5 @@ richValidator = $richValidator; $this->app = ''; @@ -134,8 +129,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the app id is invalid * @since 8.2.0 */ - public function setApp(string $app) { - if (trim($app) === '' || isset($app[32])) { + public function setApp(string $app): INotification { + if ($app === '' || isset($app[32])) { throw new \InvalidArgumentException('The given app name is invalid'); } $this->app = $app; @@ -146,7 +141,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getApp() { + public function getApp(): string { return $this->app; } @@ -156,8 +151,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the user id is invalid * @since 8.2.0 */ - public function setUser(string $user) { - if (trim($user) === '' || isset($user[64])) { + public function setUser(string $user): INotification { + if ($user === '' || isset($user[64])) { throw new \InvalidArgumentException('The given user id is invalid'); } $this->user = $user; @@ -168,7 +163,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getUser() { + public function getUser(): string { return $this->user; } @@ -178,7 +173,7 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the $dateTime is invalid * @since 9.0.0 */ - public function setDateTime(\DateTime $dateTime) { + public function setDateTime(\DateTime $dateTime): INotification { if ($dateTime->getTimestamp() === 0) { throw new \InvalidArgumentException('The given date time is invalid'); } @@ -190,7 +185,7 @@ class Notification implements INotification { * @return \DateTime * @since 9.0.0 */ - public function getDateTime() { + public function getDateTime(): \DateTime { return $this->dateTime; } @@ -201,13 +196,13 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the object type or id is invalid * @since 8.2.0 - 9.0.0: Type of $id changed to string */ - public function setObject(string $type, $id) { - if (trim($type) === '' || isset($type[64])) { + public function setObject(string $type, string $id): INotification { + if ($type === '' || isset($type[64])) { throw new \InvalidArgumentException('The given object type is invalid'); } $this->objectType = $type; - if (!is_int($id) && (!is_string($id) || $id === '' || isset($id[64]))) { + if ($id === '' || isset($id[64])) { throw new \InvalidArgumentException('The given object id is invalid'); } $this->objectId = (string) $id; @@ -218,7 +213,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getObjectType() { + public function getObjectType(): string { return $this->objectType; } @@ -226,7 +221,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 - 9.0.0: Return type changed to string */ - public function getObjectId() { + public function getObjectId(): string { return $this->objectId; } @@ -237,8 +232,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the subject or parameters are invalid * @since 8.2.0 */ - public function setSubject(string $subject, array $parameters = []) { - if (trim($subject) === '' || isset($subject[64])) { + public function setSubject(string $subject, array $parameters = []): INotification { + if ($subject === '' || isset($subject[64])) { throw new \InvalidArgumentException('The given subject is invalid'); } @@ -252,15 +247,15 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getSubject() { + public function getSubject(): string { return $this->subject; } /** - * @return string[] + * @return array * @since 8.2.0 */ - public function getSubjectParameters() { + public function getSubjectParameters(): array { return $this->subjectParameters; } @@ -270,8 +265,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the subject is invalid * @since 8.2.0 */ - public function setParsedSubject(string $subject) { - if (trim($subject) === '') { + public function setParsedSubject(string $subject): INotification { + if ($subject === '') { throw new \InvalidArgumentException('The given parsed subject is invalid'); } $this->subjectParsed = $subject; @@ -282,7 +277,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getParsedSubject() { + public function getParsedSubject(): string { return $this->subjectParsed; } @@ -293,8 +288,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the subject or parameters are invalid * @since 11.0.0 */ - public function setRichSubject(string $subject, array $parameters = []) { - if (trim($subject) === '') { + public function setRichSubject(string $subject, array $parameters = []): INotification { + if ($subject === '') { throw new \InvalidArgumentException('The given parsed subject is invalid'); } @@ -308,7 +303,7 @@ class Notification implements INotification { * @return string * @since 11.0.0 */ - public function getRichSubject() { + public function getRichSubject(): string { return $this->subjectRich; } @@ -316,7 +311,7 @@ class Notification implements INotification { * @return array[] * @since 11.0.0 */ - public function getRichSubjectParameters() { + public function getRichSubjectParameters(): array { return $this->subjectRichParameters; } @@ -327,8 +322,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the message or parameters are invalid * @since 8.2.0 */ - public function setMessage(string $message, array $parameters = []) { - if (trim($message) === '' || isset($message[64])) { + public function setMessage(string $message, array $parameters = []): INotification { + if ($message === '' || isset($message[64])) { throw new \InvalidArgumentException('The given message is invalid'); } @@ -342,15 +337,15 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getMessage() { + public function getMessage(): string { return $this->message; } /** - * @return string[] + * @return array * @since 8.2.0 */ - public function getMessageParameters() { + public function getMessageParameters(): array { return $this->messageParameters; } @@ -360,8 +355,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the message is invalid * @since 8.2.0 */ - public function setParsedMessage(string $message) { - if (trim($message) === '') { + public function setParsedMessage(string $message): INotification { + if ($message === '') { throw new \InvalidArgumentException('The given parsed message is invalid'); } $this->messageParsed = $message; @@ -372,7 +367,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getParsedMessage() { + public function getParsedMessage(): string { return $this->messageParsed; } @@ -383,8 +378,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the message or parameters are invalid * @since 11.0.0 */ - public function setRichMessage(string $message, array $parameters = []) { - if (trim($message) === '') { + public function setRichMessage(string $message, array $parameters = []): INotification { + if ($message === '') { throw new \InvalidArgumentException('The given parsed message is invalid'); } @@ -398,7 +393,7 @@ class Notification implements INotification { * @return string * @since 11.0.0 */ - public function getRichMessage() { + public function getRichMessage(): string { return $this->messageRich; } @@ -406,7 +401,7 @@ class Notification implements INotification { * @return array[] * @since 11.0.0 */ - public function getRichMessageParameters() { + public function getRichMessageParameters(): array { return $this->messageRichParameters; } @@ -416,8 +411,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the link is invalid * @since 8.2.0 */ - public function setLink(string $link) { - if (trim($link) === '' || isset($link[4000])) { + public function setLink(string $link): INotification { + if ($link === '' || isset($link[4000])) { throw new \InvalidArgumentException('The given link is invalid'); } $this->link = $link; @@ -428,7 +423,7 @@ class Notification implements INotification { * @return string * @since 8.2.0 */ - public function getLink() { + public function getLink(): string { return $this->link; } @@ -438,8 +433,8 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the icon is invalid * @since 11.0.0 */ - public function setIcon(string $icon) { - if (trim($icon) === '' || isset($icon[4000])) { + public function setIcon(string $icon): INotification { + if ($icon === '' || isset($icon[4000])) { throw new \InvalidArgumentException('The given icon is invalid'); } $this->icon = $icon; @@ -450,7 +445,7 @@ class Notification implements INotification { * @return string * @since 11.0.0 */ - public function getIcon() { + public function getIcon(): string { return $this->icon; } @@ -458,7 +453,7 @@ class Notification implements INotification { * @return IAction * @since 8.2.0 */ - public function createAction() { + public function createAction(): IAction { return new Action(); } @@ -468,7 +463,7 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the action is invalid * @since 8.2.0 */ - public function addAction(IAction $action) { + public function addAction(IAction $action): INotification { if (!$action->isValid()) { throw new \InvalidArgumentException('The given action is invalid'); } @@ -489,7 +484,7 @@ class Notification implements INotification { * @return IAction[] * @since 8.2.0 */ - public function getActions() { + public function getActions(): array { return $this->actions; } @@ -499,7 +494,7 @@ class Notification implements INotification { * @throws \InvalidArgumentException if the action is invalid * @since 8.2.0 */ - public function addParsedAction(IAction $action) { + public function addParsedAction(IAction $action): INotification { if (!$action->isValidParsed()) { throw new \InvalidArgumentException('The given parsed action is invalid'); } @@ -524,7 +519,7 @@ class Notification implements INotification { * @return IAction[] * @since 8.2.0 */ - public function getParsedActions() { + public function getParsedActions(): array { return $this->actionsParsed; } @@ -532,7 +527,7 @@ class Notification implements INotification { * @return bool * @since 8.2.0 */ - public function isValid() { + public function isValid(): bool { return $this->isValidCommon() && @@ -544,7 +539,7 @@ class Notification implements INotification { * @return bool * @since 8.2.0 */ - public function isValidParsed() { + public function isValidParsed(): bool { if ($this->getRichSubject() !== '' || !empty($this->getRichSubjectParameters())) { try { $this->richValidator->validate($this->getRichSubject(), $this->getRichSubjectParameters()); @@ -568,10 +563,7 @@ class Notification implements INotification { ; } - /** - * @return bool - */ - protected function isValidCommon() { + protected function isValidCommon(): bool { return $this->getApp() !== '' && diff --git a/lib/public/Notification/IAction.php b/lib/public/Notification/IAction.php index 6f2b78e3a8..718a61b2f3 100644 --- a/lib/public/Notification/IAction.php +++ b/lib/public/Notification/IAction.php @@ -1,4 +1,5 @@ App Name * @since 9.0.0 */ - public function listNotifiers(); + public function listNotifiers(): array; /** * @return INotification * @since 9.0.0 */ - public function createNotification(); + public function createNotification(): INotification; /** * @return bool * @since 9.0.0 */ - public function hasNotifiers(); + public function hasNotifiers(): bool; /** * @param bool $preparingPushNotification * @since 14.0.0 */ - public function setPreparingPushNotification($preparingPushNotification); + public function setPreparingPushNotification(bool $preparingPushNotification): void; /** * @return bool * @since 14.0.0 */ - public function isPreparingPushNotification(); + public function isPreparingPushNotification(): bool; } diff --git a/lib/public/Notification/INotification.php b/lib/public/Notification/INotification.php index f7400e4778..7261409e35 100644 --- a/lib/public/Notification/INotification.php +++ b/lib/public/Notification/INotification.php @@ -1,5 +1,5 @@ Date: Wed, 10 Apr 2019 14:45:33 +0200 Subject: [PATCH 02/12] Change how Notifiers and Apps are registered Signed-off-by: Joas Schilling --- lib/private/Notification/Manager.php | 143 ++++++++++-------- .../AlreadyProcessedException.php | 32 ++++ lib/public/Notification/IManager.php | 18 +-- lib/public/Notification/INotifier.php | 18 +++ 4 files changed, 141 insertions(+), 70 deletions(-) create mode 100644 lib/public/Notification/AlreadyProcessedException.php diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index 876a191fb9..df54265a7a 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -26,6 +26,9 @@ declare(strict_types=1); namespace OC\Notification; +use OCP\AppFramework\QueryException; +use OCP\ILogger; +use OCP\Notification\AlreadyProcessedException; use OCP\Notification\IApp; use OCP\Notification\IManager; use OCP\Notification\INotification; @@ -35,6 +38,8 @@ use OCP\RichObjectStrings\IValidator; class Manager implements IManager { /** @var IValidator */ protected $validator; + /** @var ILogger */ + protected $logger; /** @var IApp[] */ protected $apps; @@ -45,11 +50,11 @@ class Manager implements IManager { /** @var array[] */ protected $notifiersInfo; - /** @var \Closure[] */ - protected $appsClosures; + /** @var string[] */ + protected $appClasses; - /** @var \Closure[] */ - protected $notifiersClosures; + /** @var string[] */ + protected $notifierClasses; /** @var \Closure[] */ protected $notifiersInfoClosures; @@ -57,55 +62,60 @@ class Manager implements IManager { /** @var bool */ protected $preparingPushNotification; - public function __construct(IValidator $validator) { + public function __construct(IValidator $validator, + ILogger $logger) { $this->validator = $validator; + $this->logger = $logger; $this->apps = []; $this->notifiers = []; - $this->notifiersInfo = []; - $this->appsClosures = []; - $this->notifiersClosures = []; - $this->notifiersInfoClosures = []; + $this->appClasses = []; + $this->notifierClasses = []; $this->preparingPushNotification = false; } - /** - * @param \Closure $service The service must implement IApp, otherwise a + * @param string $appClass The service must implement IApp, otherwise a * \InvalidArgumentException is thrown later - * @since 8.2.0 + * @since 9.0.0 */ - public function registerApp(\Closure $service) { - $this->appsClosures[] = $service; - $this->apps = []; + public function registerApp(string $appClass): void { + $this->appClasses[] = $appClass; } /** - * @param \Closure $service The service must implement INotifier, otherwise a + * @param string $notifierClass The service must implement INotifier, otherwise a * \InvalidArgumentException is thrown later - * @param \Closure $info An array with the keys 'id' and 'name' containing - * the app id and the app name - * @since 8.2.0 - Parameter $info was added in 9.0.0 + * @since 17.0.0 */ - public function registerNotifier(\Closure $service, \Closure $info) { - $this->notifiersClosures[] = $service; - $this->notifiersInfoClosures[] = $info; - $this->notifiers = []; - $this->notifiersInfo = []; + public function registerNotifier(string $notifierClass): void { + $this->notifierClasses[] = $notifierClass; } /** * @return IApp[] */ protected function getApps(): array { - if (!empty($this->apps)) { + if (empty($this->appClasses)) { return $this->apps; } - $this->apps = []; - foreach ($this->appsClosures as $closure) { - $app = $closure(); - if (!($app instanceof IApp)) { - throw new \InvalidArgumentException('The given notification app does not implement the IApp interface'); + foreach ($this->appClasses as $appClass) { + try { + $app = \OC::$server->query($appClass); + } catch (QueryException $e) { + $this->logger->logException($e, [ + 'message' => 'Failed to load notification app class: ' . $appClass, + 'app' => 'notifications', + ]); + continue; } + + if (!($app instanceof IApp)) { + $this->logger->error('Notification app class ' . $appClass . ' is not implementing ' . IApp::class, [ + 'app' => 'notifications', + ]); + continue; + } + $this->apps[] = $app; } @@ -115,46 +125,35 @@ class Manager implements IManager { /** * @return INotifier[] */ - protected function getNotifiers(): array { - if (!empty($this->notifiers)) { + public function getNotifiers(): array { + if (empty($this->notifierClasses)) { return $this->notifiers; } - $this->notifiers = []; - foreach ($this->notifiersClosures as $closure) { - $notifier = $closure(); - if (!($notifier instanceof INotifier)) { - throw new \InvalidArgumentException('The given notifier does not implement the INotifier interface'); + foreach ($this->notifierClasses as $notifierClass) { + try { + $notifier = \OC::$server->query($notifierClass); + } catch (QueryException $e) { + $this->logger->logException($e, [ + 'message' => 'Failed to load notification notifier class: ' . $notifierClass, + 'app' => 'notifications', + ]); + continue; } + + if (!($notifier instanceof INotifier)) { + $this->logger->error('Notification notifier class ' . $notifierClass . ' is not implementing ' . INotifier::class, [ + 'app' => 'notifications', + ]); + continue; + } + $this->notifiers[] = $notifier; } return $this->notifiers; } - /** - * @return array[] - */ - public function listNotifiers(): array { - if (!empty($this->notifiersInfo)) { - return $this->notifiersInfo; - } - - $this->notifiersInfo = []; - foreach ($this->notifiersInfoClosures as $closure) { - $notifier = $closure(); - if (!\is_array($notifier) || \count($notifier) !== 2 || !isset($notifier['id'], $notifier['name'])) { - throw new \InvalidArgumentException('The given notifier information is invalid'); - } - if (isset($this->notifiersInfo[$notifier['id']])) { - throw new \InvalidArgumentException('The given notifier ID ' . $notifier['id'] . ' is already in use'); - } - $this->notifiersInfo[$notifier['id']] = $notifier['name']; - } - - return $this->notifiersInfo; - } - /** * @return INotification * @since 8.2.0 @@ -207,11 +206,32 @@ class Manager implements IManager { } } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'core'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return 'core'; + } + /** * @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 * @since 8.2.0 */ public function prepare(INotification $notification, string $languageCode): INotification { @@ -222,6 +242,9 @@ class Manager implements IManager { $notification = $notifier->prepare($notification, $languageCode); } catch (\InvalidArgumentException $e) { continue; + } catch (AlreadyProcessedException $e) { + $this->markProcessed($notification); + throw new \InvalidArgumentException('The given notification has been processed'); } if (!($notification instanceof INotification) || !$notification->isValidParsed()) { diff --git a/lib/public/Notification/AlreadyProcessedException.php b/lib/public/Notification/AlreadyProcessedException.php new file mode 100644 index 0000000000..3cfe4667d6 --- /dev/null +++ b/lib/public/Notification/AlreadyProcessedException.php @@ -0,0 +1,32 @@ + + * + * @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 . + * + */ + +namespace OCP\Notification; + + +class AlreadyProcessedException extends \RuntimeException { + + public function __construct() { + parent::__construct('Notification is processed already'); + } + +} diff --git a/lib/public/Notification/IManager.php b/lib/public/Notification/IManager.php index 7feac76a5d..262bb12143 100644 --- a/lib/public/Notification/IManager.php +++ b/lib/public/Notification/IManager.php @@ -31,26 +31,24 @@ namespace OCP\Notification; */ interface IManager extends IApp, INotifier { /** - * @param \Closure $service The service must implement IApp, otherwise a + * @param string $appClass The service must implement IApp, otherwise a * \InvalidArgumentException is thrown later - * @since 9.0.0 + * @since 17.0.0 */ - public function registerApp(\Closure $service); + public function registerApp(string $appClass): void; /** - * @param \Closure $service The service must implement INotifier, otherwise a + * @param string $notifierClass The service must implement INotifier, otherwise a * \InvalidArgumentException is thrown later - * @param \Closure $info An array with the keys 'id' and 'name' containing - * the app id and the app name - * @since 9.0.0 + * @since 17.0.0 */ - public function registerNotifier(\Closure $service, \Closure $info); + public function registerNotifier(string $notifierClass): void; /** - * @return array App ID => App Name + * @return INotifier[] * @since 9.0.0 */ - public function listNotifiers(): array; + public function getNotifiers(): array; /** * @return INotification diff --git a/lib/public/Notification/INotifier.php b/lib/public/Notification/INotifier.php index 44066c035b..b730b1d801 100644 --- a/lib/public/Notification/INotifier.php +++ b/lib/public/Notification/INotifier.php @@ -30,11 +30,29 @@ namespace OCP\Notification; * @since 9.0.0 */ interface INotifier { + + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string; + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + 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 * @since 9.0.0 */ public function prepare(INotification $notification, string $languageCode): INotification; From f376b9fea7ba02fb3087ba2fb403c481a7053340 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 12 Apr 2019 13:44:01 +0200 Subject: [PATCH 03/12] Fix creation of the Manager Signed-off-by: Joas Schilling --- lib/private/Server.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/Server.php b/lib/private/Server.php index c716b996a3..28cd204a51 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -899,7 +899,8 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerService(\OCP\Notification\IManager::class, function (Server $c) { return new Manager( - $c->query(IValidator::class) + $c->query(IValidator::class), + $c->getLogger() ); }); $this->registerAlias('NotificationManager', \OCP\Notification\IManager::class); From a386ecec8c75677942f489e9e94ddcca6a29a05e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 12 Apr 2019 13:44:23 +0200 Subject: [PATCH 04/12] Update twofactor_backupcodes Notifier Signed-off-by: Joas Schilling --- .../lib/AppInfo/Application.php | 10 +------- .../lib/Notifications/Notifier.php | 23 +++++++++++++++++-- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/apps/twofactor_backupcodes/lib/AppInfo/Application.php b/apps/twofactor_backupcodes/lib/AppInfo/Application.php index 041af03706..c4e02aaa26 100644 --- a/apps/twofactor_backupcodes/lib/AppInfo/Application.php +++ b/apps/twofactor_backupcodes/lib/AppInfo/Application.php @@ -74,15 +74,7 @@ class Application extends App { $container = $this->getContainer(); /** @var IManager $manager */ $manager = $container->query(IManager::class); - $manager->registerNotifier( - function() use ($container) { - return $container->query(Notifier::class); - }, - function () use ($container) { - $l = $container->query(IL10N::class); - return ['id' => 'twofactor_backupcodes', 'name' => $l->t('Second-factor backup codes')]; - } - ); + $manager->registerNotifier(Notifier::class); } public function deleteUser($params) { diff --git a/apps/twofactor_backupcodes/lib/Notifications/Notifier.php b/apps/twofactor_backupcodes/lib/Notifications/Notifier.php index df92c7a9e0..5e53a32df7 100644 --- a/apps/twofactor_backupcodes/lib/Notifications/Notifier.php +++ b/apps/twofactor_backupcodes/lib/Notifications/Notifier.php @@ -42,7 +42,27 @@ class Notifier implements INotifier { $this->url = $url; } - public function prepare(INotification $notification, $languageCode) { + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'twofactor_backupcodes'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->factory->get('twofactor_backupcodes')->t('Second-factor backup codes'); + } + + public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'twofactor_backupcodes') { // Not my app => throw throw new \InvalidArgumentException(); @@ -67,5 +87,4 @@ class Notifier implements INotifier { throw new \InvalidArgumentException(); } } - } From 44bc697a93309522cd955be32b4f469c526d268c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 12 Apr 2019 13:48:53 +0200 Subject: [PATCH 05/12] Update Federated sharing notifier Signed-off-by: Joas Schilling --- apps/federatedfilesharing/appinfo/app.php | 10 +--------- apps/federatedfilesharing/lib/Notifier.php | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/apps/federatedfilesharing/appinfo/app.php b/apps/federatedfilesharing/appinfo/app.php index f941987365..bd46f9824f 100644 --- a/apps/federatedfilesharing/appinfo/app.php +++ b/apps/federatedfilesharing/appinfo/app.php @@ -29,15 +29,7 @@ $app = new \OCA\FederatedFileSharing\AppInfo\Application(); $eventDispatcher = \OC::$server->getEventDispatcher(); $manager = \OC::$server->getNotificationManager(); -$manager->registerNotifier(function() { - return \OC::$server->query(Notifier::class); -}, function() { - $l = \OC::$server->getL10N('files_sharing'); - return [ - 'id' => 'files_sharing', - 'name' => $l->t('Federated sharing'), - ]; -}); +$manager->registerNotifier(Notifier::class); $federatedShareProvider = $app->getFederatedShareProvider(); diff --git a/apps/federatedfilesharing/lib/Notifier.php b/apps/federatedfilesharing/lib/Notifier.php index 03491f20af..02a46d65bb 100644 --- a/apps/federatedfilesharing/lib/Notifier.php +++ b/apps/federatedfilesharing/lib/Notifier.php @@ -59,13 +59,33 @@ class Notifier implements INotifier { $this->cloudIdManager = $cloudIdManager; } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'federatedfilesharing'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->factory->get('federatedfilesharing')->t('Federated sharing'); + } + /** * @param INotification $notification * @param string $languageCode The code of the language that should be used to prepare the notification * @return INotification * @throws \InvalidArgumentException */ - public function prepare(INotification $notification, $languageCode) { + public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'files_sharing') { // Not my app => throw throw new \InvalidArgumentException(); From 865c12aa0e624d21d15b351ee5fcbedffa55f11c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 30 Apr 2019 12:08:10 +0200 Subject: [PATCH 06/12] Fix detection of Notifiers Signed-off-by: Joas Schilling --- lib/private/Notification/Manager.php | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index df54265a7a..2ed7aa7c17 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -43,22 +43,14 @@ class Manager implements IManager { /** @var IApp[] */ protected $apps; - - /** @var INotifier[] */ - protected $notifiers; - - /** @var array[] */ - protected $notifiersInfo; - /** @var string[] */ protected $appClasses; + /** @var INotifier[] */ + protected $notifiers; /** @var string[] */ protected $notifierClasses; - /** @var \Closure[] */ - protected $notifiersInfoClosures; - /** @var bool */ protected $preparingPushNotification; @@ -167,7 +159,7 @@ class Manager implements IManager { * @since 8.2.0 */ public function hasNotifiers(): bool { - return !empty($this->notifiersClosures); + return !empty($this->notifiers) || !empty($this->notifierClasses); } /** From 64f67818bcc6cc61cc49b1a7c032f3db85b73c91 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 30 Apr 2019 12:08:22 +0200 Subject: [PATCH 07/12] Fix new core notifier Signed-off-by: Joas Schilling --- core/Application.php | 20 ++-------------- .../Notification/RemoveLinkSharesNotifier.php | 23 +++++++++++++++++-- .../Authentication/Notifications/Notifier.php | 21 ++++++++++++++++- 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/core/Application.php b/core/Application.php index 9655a8e1a4..3a212f8547 100644 --- a/core/Application.php +++ b/core/Application.php @@ -65,24 +65,8 @@ class Application extends App { $eventDispatcher = $server->query(IEventDispatcher::class); $notificationManager = $server->getNotificationManager(); - $notificationManager->registerNotifier(function () use ($server) { - return new RemoveLinkSharesNotifier( - $server->getL10NFactory() - ); - }, function () { - return [ - 'id' => 'core', - 'name' => 'core', - ]; - }); - $notificationManager->registerNotifier(function () use ($server) { - return $server->query(AuthenticationNotifier::class); - }, function () { - return [ - 'id' => 'auth', - 'name' => 'authentication notifier', - ]; - }); + $notificationManager->registerNotifier(RemoveLinkSharesNotifier::class); + $notificationManager->registerNotifier(AuthenticationNotifier::class); $eventDispatcher->addListener(IDBConnection::CHECK_MISSING_INDEXES_EVENT, function (GenericEvent $event) use ($container) { diff --git a/core/Notification/RemoveLinkSharesNotifier.php b/core/Notification/RemoveLinkSharesNotifier.php index b77846c847..b2e73aee23 100644 --- a/core/Notification/RemoveLinkSharesNotifier.php +++ b/core/Notification/RemoveLinkSharesNotifier.php @@ -36,7 +36,27 @@ class RemoveLinkSharesNotifier implements INotifier { $this->l10nFactory = $factory; } - public function prepare(INotification $notification, $languageCode): INotification { + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'core'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->l10nFactory->get('core')->t('Nextcloud Server'); + } + + public function prepare(INotification $notification, string $languageCode): INotification { if($notification->getApp() !== 'core') { throw new \InvalidArgumentException(); } @@ -51,5 +71,4 @@ class RemoveLinkSharesNotifier implements INotifier { throw new \InvalidArgumentException('Invalid subject'); } - } diff --git a/lib/private/Authentication/Notifications/Notifier.php b/lib/private/Authentication/Notifications/Notifier.php index 0aafc115b2..f76bb077a3 100644 --- a/lib/private/Authentication/Notifications/Notifier.php +++ b/lib/private/Authentication/Notifications/Notifier.php @@ -42,7 +42,7 @@ class Notifier implements INotifier { /** * @inheritDoc */ - public function prepare(INotification $notification, $languageCode) { + public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'auth') { // Not my app => throw throw new InvalidArgumentException(); @@ -74,4 +74,23 @@ class Notifier implements INotifier { } } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'auth'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->factory->get('lib')->t('Authentication'); + } } From 6d71e471e166c30c0b9abe05d36240b9f1556d8e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 30 Apr 2019 14:29:30 +0200 Subject: [PATCH 08/12] Update shipped implementations of the INotifier Signed-off-by: Joas Schilling --- apps/comments/lib/AppInfo/Application.php | 10 +------ apps/comments/lib/Notification/Notifier.php | 27 +++++++++++++++-- .../lib/AppInfo/Application.php | 10 +------ .../lib/Notification/Notifier.php | 29 ++++++++++++++++--- apps/user_ldap/appinfo/app.php | 12 +------- apps/user_ldap/lib/Notification/Notifier.php | 22 +++++++++++++- 6 files changed, 74 insertions(+), 36 deletions(-) diff --git a/apps/comments/lib/AppInfo/Application.php b/apps/comments/lib/AppInfo/Application.php index b44c1c519c..3d20cbbc16 100644 --- a/apps/comments/lib/AppInfo/Application.php +++ b/apps/comments/lib/AppInfo/Application.php @@ -76,15 +76,7 @@ class Application extends App { } protected function registerNotifier() { - $this->getContainer()->getServer()->getNotificationManager()->registerNotifier( - function() { - return $this->getContainer()->query(Notifier::class); - }, - function () { - $l = $this->getContainer()->getServer()->getL10NFactory()->get('comments'); - return ['id' => 'comments', 'name' => $l->t('Comments')]; - } - ); + $this->getContainer()->getServer()->getNotificationManager()->registerNotifier(Notifier::class); } protected function registerCommentsEventHandler() { diff --git a/apps/comments/lib/Notification/Notifier.php b/apps/comments/lib/Notification/Notifier.php index 2132f05ef8..e8a7ade820 100644 --- a/apps/comments/lib/Notification/Notifier.php +++ b/apps/comments/lib/Notification/Notifier.php @@ -32,6 +32,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; use OCP\L10N\IFactory; +use OCP\Notification\AlreadyProcessedException; use OCP\Notification\INotification; use OCP\Notification\INotifier; @@ -66,13 +67,35 @@ class Notifier implements INotifier { $this->userManager = $userManager; } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'comments'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->l10nFactory->get('comments')->t('Comments'); + } + /** * @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 + * @since 9.0.0 */ - public function prepare(INotification $notification, $languageCode) { + public function prepare(INotification $notification, string $languageCode): INotification { if($notification->getApp() !== 'comments') { throw new \InvalidArgumentException(); } @@ -101,7 +124,7 @@ class Notifier implements INotifier { $userFolder = $this->rootFolder->getUserFolder($notification->getUser()); $nodes = $userFolder->getById((int)$parameters[1]); if(empty($nodes)) { - throw new \InvalidArgumentException('Cannot resolve file ID to node instance'); + throw new AlreadyProcessedException(); } $node = $nodes[0]; diff --git a/apps/updatenotification/lib/AppInfo/Application.php b/apps/updatenotification/lib/AppInfo/Application.php index 14512bae83..91b9020d82 100644 --- a/apps/updatenotification/lib/AppInfo/Application.php +++ b/apps/updatenotification/lib/AppInfo/Application.php @@ -71,14 +71,6 @@ class Application extends App { public function registerNotifier() { $notificationsManager = $this->getContainer()->getServer()->getNotificationManager(); - $notificationsManager->registerNotifier(function() { - return $this->getContainer()->query(Notifier::class); - }, function() { - $l = $this->getContainer()->getServer()->getL10N('updatenotification'); - return [ - 'id' => 'updatenotification', - 'name' => $l->t('Update notifications'), - ]; - }); + $notificationsManager->registerNotifier(Notifier::class); } } diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php index 4e3a30f225..44fe91c63d 100644 --- a/apps/updatenotification/lib/Notification/Notifier.php +++ b/apps/updatenotification/lib/Notification/Notifier.php @@ -31,6 +31,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; use OCP\L10N\IFactory; +use OCP\Notification\AlreadyProcessedException; use OCP\Notification\IManager; use OCP\Notification\INotification; use OCP\Notification\INotifier; @@ -79,14 +80,35 @@ class Notifier implements INotifier { $this->appVersions = $this->getAppVersions(); } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'updatenotification'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->l10NFactory->get('updatenotification')->t('Update notifications'); + } + /** * @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 * @since 9.0.0 */ - public function prepare(INotification $notification, $languageCode): INotification { + public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'updatenotification') { throw new \InvalidArgumentException('Unknown app id'); } @@ -142,12 +164,11 @@ class Notifier implements INotifier { * * @param INotification $notification * @param string $installedVersion - * @throws \InvalidArgumentException When the update is already installed + * @throws AlreadyProcessedException When the update is already installed */ protected function updateAlreadyInstalledCheck(INotification $notification, $installedVersion) { if (version_compare($notification->getObjectId(), $installedVersion, '<=')) { - $this->notificationManager->markProcessed($notification); - throw new \InvalidArgumentException('Update already installed'); + throw new AlreadyProcessedException(); } } diff --git a/apps/user_ldap/appinfo/app.php b/apps/user_ldap/appinfo/app.php index 5afd928301..34b850e655 100644 --- a/apps/user_ldap/appinfo/app.php +++ b/apps/user_ldap/appinfo/app.php @@ -42,17 +42,7 @@ if(count($configPrefixes) > 0) { $ldapWrapper = new OCA\User_LDAP\LDAP(); $ocConfig = \OC::$server->getConfig(); $notificationManager = \OC::$server->getNotificationManager(); - $notificationManager->registerNotifier(function() { - return new \OCA\User_LDAP\Notification\Notifier( - \OC::$server->getL10NFactory() - ); - }, function() { - $l = \OC::$server->getL10N('user_ldap'); - return [ - 'id' => 'user_ldap', - 'name' => $l->t('LDAP user and group backend'), - ]; - }); + $notificationManager->registerNotifier(\OCA\User_LDAP\Notification\Notifier::class); $userSession = \OC::$server->getUserSession(); $userPluginManager = \OC::$server->query('LDAPUserPluginManager'); diff --git a/apps/user_ldap/lib/Notification/Notifier.php b/apps/user_ldap/lib/Notification/Notifier.php index 34625a3bef..2c89743fa7 100644 --- a/apps/user_ldap/lib/Notification/Notifier.php +++ b/apps/user_ldap/lib/Notification/Notifier.php @@ -42,13 +42,33 @@ class Notifier implements INotifier { $this->l10nFactory = $l10nFactory; } + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + return 'user_ldap'; + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + return $this->l10nFactory->get('user_ldap')->t('LDAP User backend'); + } + /** * @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 */ - public function prepare(INotification $notification, $languageCode) { + public function prepare(INotification $notification, string $languageCode): INotification { if ($notification->getApp() !== 'user_ldap') { // Not my app => throw throw new \InvalidArgumentException(); From 55f5bc79a148f065cffa8c97e1867c5814047ff0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 16 Jul 2019 11:36:32 +0200 Subject: [PATCH 09/12] Keep the old method as a fallback and adjust the tests Signed-off-by: Joas Schilling --- apps/comments/lib/AppInfo/Application.php | 2 +- apps/federatedfilesharing/appinfo/app.php | 2 +- .../lib/AppInfo/Application.php | 2 +- .../lib/AppInfo/Application.php | 2 +- apps/user_ldap/appinfo/app.php | 2 +- core/Application.php | 4 +- lib/private/Notification/Manager.php | 25 +- .../AlreadyProcessedException.php | 6 + lib/public/Notification/IManager.php | 14 +- tests/lib/Notification/ActionTest.php | 45 --- tests/lib/Notification/DummyApp.php | 56 +++ tests/lib/Notification/DummyNotifier.php | 63 +++ tests/lib/Notification/ManagerTest.php | 372 ++++-------------- tests/lib/Notification/NotificationTest.php | 77 +--- 14 files changed, 240 insertions(+), 432 deletions(-) create mode 100644 tests/lib/Notification/DummyApp.php create mode 100644 tests/lib/Notification/DummyNotifier.php diff --git a/apps/comments/lib/AppInfo/Application.php b/apps/comments/lib/AppInfo/Application.php index 3d20cbbc16..916345e4a5 100644 --- a/apps/comments/lib/AppInfo/Application.php +++ b/apps/comments/lib/AppInfo/Application.php @@ -76,7 +76,7 @@ class Application extends App { } protected function registerNotifier() { - $this->getContainer()->getServer()->getNotificationManager()->registerNotifier(Notifier::class); + $this->getContainer()->getServer()->getNotificationManager()->registerNotifierService(Notifier::class); } protected function registerCommentsEventHandler() { diff --git a/apps/federatedfilesharing/appinfo/app.php b/apps/federatedfilesharing/appinfo/app.php index bd46f9824f..6b273faca8 100644 --- a/apps/federatedfilesharing/appinfo/app.php +++ b/apps/federatedfilesharing/appinfo/app.php @@ -29,7 +29,7 @@ $app = new \OCA\FederatedFileSharing\AppInfo\Application(); $eventDispatcher = \OC::$server->getEventDispatcher(); $manager = \OC::$server->getNotificationManager(); -$manager->registerNotifier(Notifier::class); +$manager->registerNotifierService(Notifier::class); $federatedShareProvider = $app->getFederatedShareProvider(); diff --git a/apps/twofactor_backupcodes/lib/AppInfo/Application.php b/apps/twofactor_backupcodes/lib/AppInfo/Application.php index c4e02aaa26..735ee091bd 100644 --- a/apps/twofactor_backupcodes/lib/AppInfo/Application.php +++ b/apps/twofactor_backupcodes/lib/AppInfo/Application.php @@ -74,7 +74,7 @@ class Application extends App { $container = $this->getContainer(); /** @var IManager $manager */ $manager = $container->query(IManager::class); - $manager->registerNotifier(Notifier::class); + $manager->registerNotifierService(Notifier::class); } public function deleteUser($params) { diff --git a/apps/updatenotification/lib/AppInfo/Application.php b/apps/updatenotification/lib/AppInfo/Application.php index 91b9020d82..6a1bf71946 100644 --- a/apps/updatenotification/lib/AppInfo/Application.php +++ b/apps/updatenotification/lib/AppInfo/Application.php @@ -71,6 +71,6 @@ class Application extends App { public function registerNotifier() { $notificationsManager = $this->getContainer()->getServer()->getNotificationManager(); - $notificationsManager->registerNotifier(Notifier::class); + $notificationsManager->registerNotifierService(Notifier::class); } } diff --git a/apps/user_ldap/appinfo/app.php b/apps/user_ldap/appinfo/app.php index 34b850e655..c6e1208e6e 100644 --- a/apps/user_ldap/appinfo/app.php +++ b/apps/user_ldap/appinfo/app.php @@ -42,7 +42,7 @@ if(count($configPrefixes) > 0) { $ldapWrapper = new OCA\User_LDAP\LDAP(); $ocConfig = \OC::$server->getConfig(); $notificationManager = \OC::$server->getNotificationManager(); - $notificationManager->registerNotifier(\OCA\User_LDAP\Notification\Notifier::class); + $notificationManager->registerNotifierService(\OCA\User_LDAP\Notification\Notifier::class); $userSession = \OC::$server->getUserSession(); $userPluginManager = \OC::$server->query('LDAPUserPluginManager'); diff --git a/core/Application.php b/core/Application.php index 3a212f8547..7949cfd663 100644 --- a/core/Application.php +++ b/core/Application.php @@ -65,8 +65,8 @@ class Application extends App { $eventDispatcher = $server->query(IEventDispatcher::class); $notificationManager = $server->getNotificationManager(); - $notificationManager->registerNotifier(RemoveLinkSharesNotifier::class); - $notificationManager->registerNotifier(AuthenticationNotifier::class); + $notificationManager->registerNotifierService(RemoveLinkSharesNotifier::class); + $notificationManager->registerNotifierService(AuthenticationNotifier::class); $eventDispatcher->addListener(IDBConnection::CHECK_MISSING_INDEXES_EVENT, function (GenericEvent $event) use ($container) { diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index 2ed7aa7c17..9cca86a4bd 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -74,12 +74,27 @@ class Manager implements IManager { } /** - * @param string $notifierClass The service must implement INotifier, otherwise a + * @param \Closure $service The service must implement INotifier, otherwise a + * \InvalidArgumentException is thrown later + * @param \Closure $info An array with the keys 'id' and 'name' containing + * the app id and the app name + * @deprecated 17.0.0 use registerNotifierService instead. + * @since 8.2.0 - Parameter $info was added in 9.0.0 + */ + public function registerNotifier(\Closure $service, \Closure $info) { + $infoData = $info(); + $this->logger->logException(new \InvalidArgumentException( + 'Notifier ' . $infoData['name'] . ' (id: ' . $infoData['id'] . ') is not considered because it is using the old way to register.' + )); + } + + /** + * @param string $notifierService The service must implement INotifier, otherwise a * \InvalidArgumentException is thrown later * @since 17.0.0 */ - public function registerNotifier(string $notifierClass): void { - $this->notifierClasses[] = $notifierClass; + public function registerNotifierService(string $notifierService): void { + $this->notifierClasses[] = $notifierService; } /** @@ -111,6 +126,8 @@ class Manager implements IManager { $this->apps[] = $app; } + $this->appClasses = []; + return $this->apps; } @@ -143,6 +160,8 @@ class Manager implements IManager { $this->notifiers[] = $notifier; } + $this->notifierClasses = []; + return $this->notifiers; } diff --git a/lib/public/Notification/AlreadyProcessedException.php b/lib/public/Notification/AlreadyProcessedException.php index 3cfe4667d6..0ccac25f0f 100644 --- a/lib/public/Notification/AlreadyProcessedException.php +++ b/lib/public/Notification/AlreadyProcessedException.php @@ -23,8 +23,14 @@ declare(strict_types=1); namespace OCP\Notification; +/** + * @since 17.0.0 + */ class AlreadyProcessedException extends \RuntimeException { + /** + * @since 17.0.0 + */ public function __construct() { parent::__construct('Notification is processed already'); } diff --git a/lib/public/Notification/IManager.php b/lib/public/Notification/IManager.php index 262bb12143..af890594d4 100644 --- a/lib/public/Notification/IManager.php +++ b/lib/public/Notification/IManager.php @@ -38,11 +38,21 @@ interface IManager extends IApp, INotifier { public function registerApp(string $appClass): void; /** - * @param string $notifierClass The service must implement INotifier, otherwise a + * @param \Closure $service The service must implement INotifier, otherwise a + * \InvalidArgumentException is thrown later + * @param \Closure $info An array with the keys 'id' and 'name' containing + * the app id and the app name + * @deprecated 17.0.0 use registerNotifierService instead. + * @since 8.2.0 - Parameter $info was added in 9.0.0 + */ + public function registerNotifier(\Closure $service, \Closure $info); + + /** + * @param string $notifierService The service must implement INotifier, otherwise a * \InvalidArgumentException is thrown later * @since 17.0.0 */ - public function registerNotifier(string $notifierClass): void; + public function registerNotifierService(string $notifierService): void; /** * @return INotifier[] diff --git a/tests/lib/Notification/ActionTest.php b/tests/lib/Notification/ActionTest.php index 74c995280c..9160ea7a7c 100644 --- a/tests/lib/Notification/ActionTest.php +++ b/tests/lib/Notification/ActionTest.php @@ -55,14 +55,8 @@ class ActionTest extends TestCase { public function dataSetLabelInvalid() { return [ - [true], - [false], - [0], - [1], [''], [str_repeat('a', 33)], - [[]], - [[str_repeat('a', 33)]], ]; } @@ -96,13 +90,7 @@ class ActionTest extends TestCase { public function dataSetParsedLabelInvalid() { return [ - [true], - [false], - [0], - [1], [''], - [[]], - [[str_repeat('a', 33)]], ]; } @@ -140,23 +128,11 @@ class ActionTest extends TestCase { public function dataSetLinkInvalid() { return [ // Invalid link - [true, 'GET'], - [false, 'GET'], - [0, 'GET'], - [1, 'GET'], ['', 'GET'], [str_repeat('a', 257), 'GET'], - [[], 'GET'], - [[str_repeat('a', 257)], 'GET'], // Invalid type ['url', 'notGET'], - ['url', true], - ['url', false], - ['url', 0], - ['url', 1], - ['url', []], - ['url', ['GET']], ]; } @@ -188,27 +164,6 @@ class ActionTest extends TestCase { $this->assertSame($primary, $this->action->isPrimary()); } - public function dataSetPrimaryInvalid() { - return [ - [0], - [1], - [''], - [str_repeat('a', 257)], - [[]], - [[str_repeat('a', 257)]], - ]; - } - - /** - * @dataProvider dataSetPrimaryInvalid - * @param mixed $primary - * - * @expectedException \InvalidArgumentException - */ - public function testSetPrimaryInvalid($primary) { - $this->action->setPrimary($primary); - } - public function testIsValid() { $this->assertFalse($this->action->isValid()); $this->assertFalse($this->action->isValidParsed()); diff --git a/tests/lib/Notification/DummyApp.php b/tests/lib/Notification/DummyApp.php new file mode 100644 index 0000000000..680fb44d00 --- /dev/null +++ b/tests/lib/Notification/DummyApp.php @@ -0,0 +1,56 @@ + + * + * @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 . + * + */ + +namespace Test\Notification; + + +use OCP\Notification\IApp; +use OCP\Notification\INotification; + +class DummyApp implements IApp { + + /** + * @param INotification $notification + * @throws \InvalidArgumentException When the notification is not valid + * @since 9.0.0 + */ + public function notify(INotification $notification): void { + // TODO: Implement notify() method. + } + + /** + * @param INotification $notification + * @since 9.0.0 + */ + public function markProcessed(INotification $notification): void { + // TODO: Implement markProcessed() method. + } + + /** + * @param INotification $notification + * @return int + * @since 9.0.0 + */ + public function getCount(INotification $notification): int { + // TODO: Implement getCount() method. + } +} diff --git a/tests/lib/Notification/DummyNotifier.php b/tests/lib/Notification/DummyNotifier.php new file mode 100644 index 0000000000..849fb3c275 --- /dev/null +++ b/tests/lib/Notification/DummyNotifier.php @@ -0,0 +1,63 @@ + + * + * @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 . + * + */ + +namespace Test\Notification; + + +use OCP\Notification\AlreadyProcessedException; +use OCP\Notification\INotification; +use OCP\Notification\INotifier; + +class DummyNotifier implements INotifier { + + /** + * Identifier of the notifier, only use [a-z0-9_] + * + * @return string + * @since 17.0.0 + */ + public function getID(): string { + // TODO: Implement getID() method. + } + + /** + * Human readable name describing the notifier + * + * @return string + * @since 17.0.0 + */ + public function getName(): string { + // TODO: Implement getName() method. + } + + /** + * @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 + * @since 9.0.0 + */ + public function prepare(INotification $notification, string $languageCode): INotification { + // TODO: Implement prepare() method. + } +} diff --git a/tests/lib/Notification/ManagerTest.php b/tests/lib/Notification/ManagerTest.php index cb6504b67e..259ac0beef 100644 --- a/tests/lib/Notification/ManagerTest.php +++ b/tests/lib/Notification/ManagerTest.php @@ -22,162 +22,72 @@ namespace Test\Notification; use OC\Notification\Manager; +use OCP\ILogger; use OCP\Notification\IApp; use OCP\Notification\IManager; use OCP\Notification\INotification; use OCP\Notification\INotifier; use OCP\RichObjectStrings\IValidator; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class ManagerTest extends TestCase { /** @var IManager */ protected $manager; + /** @var IValidator|MockObject */ + protected $validator; + /** @var ILogger|MockObject */ + protected $logger; + public function setUp() { parent::setUp(); - $validator = $this->createMock(IValidator::class); - $this->manager = new Manager($validator); + $this->validator = $this->createMock(IValidator::class); + $this->logger = $this->createMock(ILogger::class); + $this->manager = new Manager($this->validator, $this->logger); } public function testRegisterApp() { - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - $closure = function() use ($app) { - return $app; - }; + $this->assertEquals([], self::invokePrivate($this->manager, 'getApps')); - $this->assertEquals([], $this->invokePrivate($this->manager, 'getApps')); + $this->manager->registerApp(DummyApp::class); - $this->manager->registerApp($closure); + $this->assertCount(1, self::invokePrivate($this->manager, 'getApps')); + $this->assertCount(1, self::invokePrivate($this->manager, 'getApps')); - $this->assertEquals([$app], $this->invokePrivate($this->manager, 'getApps')); - $this->assertEquals([$app], $this->invokePrivate($this->manager, 'getApps')); + $this->manager->registerApp(DummyApp::class); - $this->manager->registerApp($closure); - - $this->assertEquals([$app, $app], $this->invokePrivate($this->manager, 'getApps')); + $this->assertCount(2, self::invokePrivate($this->manager, 'getApps')); } - /** - * @expectedException \InvalidArgumentException - */ public function testRegisterAppInvalid() { - $notifier = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); + $this->manager->registerApp(DummyNotifier::class); - $closure = function() use ($notifier) { - return $notifier; - }; - - $this->manager->registerApp($closure); - - $this->invokePrivate($this->manager, 'getApps'); + $this->logger->expects($this->once()) + ->method('error'); + self::invokePrivate($this->manager, 'getApps'); } public function testRegisterNotifier() { - $notifier = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); + $this->assertEquals([], self::invokePrivate($this->manager, 'getNotifiers')); - $closure = function() use ($notifier) { - return $notifier; - }; + $this->manager->registerNotifierService(DummyNotifier::class); - $this->assertEquals([], $this->invokePrivate($this->manager, 'getNotifiers')); - $this->assertEquals([], $this->invokePrivate($this->manager, 'listNotifiers')); + $this->assertCount(1, self::invokePrivate($this->manager, 'getNotifiers')); + $this->assertCount(1, self::invokePrivate($this->manager, 'getNotifiers')); - $this->manager->registerNotifier($closure, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); + $this->manager->registerNotifierService(DummyNotifier::class); - $this->assertEquals([$notifier], $this->invokePrivate($this->manager, 'getNotifiers')); - $this->assertEquals(['test1' => 'Test One'], $this->invokePrivate($this->manager, 'listNotifiers')); - $this->assertEquals([$notifier], $this->invokePrivate($this->manager, 'getNotifiers')); - $this->assertEquals(['test1' => 'Test One'], $this->invokePrivate($this->manager, 'listNotifiers')); - - $this->manager->registerNotifier($closure, function() { - return ['id' => 'test2', 'name' => 'Test Two']; - }); - - $this->assertEquals([$notifier, $notifier], $this->invokePrivate($this->manager, 'getNotifiers')); - $this->assertEquals(['test1' => 'Test One', 'test2' => 'Test Two'], $this->invokePrivate($this->manager, 'listNotifiers')); + $this->assertCount(2, self::invokePrivate($this->manager, 'getNotifiers')); } - /** - * @expectedException \InvalidArgumentException - */ public function testRegisterNotifierInvalid() { - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); + $this->manager->registerNotifierService(DummyApp::class); - $closure = function() use ($app) { - return $app; - }; - - $this->manager->registerNotifier($closure, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - - $this->invokePrivate($this->manager, 'getNotifiers'); - } - - public function dataRegisterNotifierInfoInvalid() { - return [ - [null], - ['No array'], - [['id' => 'test1', 'name' => 'Test One', 'size' => 'Invalid']], - [['no-id' => 'test1', 'name' => 'Test One']], - [['id' => 'test1', 'no-name' => 'Test One']], - ]; - } - - /** - * @dataProvider dataRegisterNotifierInfoInvalid - * @expectedException \InvalidArgumentException - * @param mixed $data - */ - public function testRegisterNotifierInfoInvalid($data) { - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - - $closure = function() use ($app) { - return $app; - }; - - $this->manager->registerNotifier($closure, function() use ($data) { - return $data; - }); - - $this->manager->listNotifiers(); - } - - /** - * @expectedException \InvalidArgumentException - * @expectedExceptionMessage The given notifier ID test1 is already in use - */ - public function testRegisterNotifierInfoDuplicate() { - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - - $closure = function() use ($app) { - return $app; - }; - - $this->manager->registerNotifier($closure, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - - $this->manager->registerNotifier($closure, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - - $this->manager->listNotifiers(); + $this->logger->expects($this->once()) + ->method('error'); + self::invokePrivate($this->manager, 'getNotifiers'); } public function testCreateNotification() { @@ -194,30 +104,19 @@ class ManagerTest extends TestCase { ->method('isValid') ->willReturn(true); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app */ - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() + $manager = $this->getMockBuilder(Manager::class) + ->setConstructorArgs([ + $this->validator, + $this->logger, + ]) + ->setMethods(['getApps']) ->getMock(); - $app->expects($this->once()) - ->method('notify') - ->with($notification); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app2 */ - $app2 = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - $app2->expects($this->once()) - ->method('notify') - ->with($notification); + $manager->expects($this->once()) + ->method('getApps') + ->willReturn([]); - $this->manager->registerApp(function() use ($app) { - return $app; - }); - $this->manager->registerApp(function() use ($app2) { - return $app2; - }); - - $this->manager->notify($notification); + $manager->notify($notification); } /** @@ -232,127 +131,18 @@ class ManagerTest extends TestCase { ->method('isValid') ->willReturn(false); - $this->manager->notify($notification); - } - - public function testPrepare() { - /** @var \OCP\Notification\INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ - $notification = $this->getMockBuilder(INotification::class) - ->disableOriginalConstructor() + $manager = $this->getMockBuilder(Manager::class) + ->setConstructorArgs([ + $this->validator, + $this->logger, + ]) + ->setMethods(['getApps']) ->getMock(); - $notification->expects($this->once()) - ->method('isValidParsed') - ->willReturn(true); - /** @var \OCP\Notification\INotification|\PHPUnit_Framework_MockObject_MockObject $notification2 */ - $notification2 = $this->getMockBuilder(INotification::class) - ->disableOriginalConstructor() - ->getMock(); - $notification2->expects($this->exactly(2)) - ->method('isValidParsed') - ->willReturn(true); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $notifier */ - $notifier = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); - $notifier->expects($this->once()) - ->method('prepare') - ->with($notification, 'en') - ->willReturnArgument(0); + $manager->expects($this->never()) + ->method('getApps'); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $notifier2 */ - $notifier2 = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); - $notifier2->expects($this->once()) - ->method('prepare') - ->with($notification, 'en') - ->willReturn($notification2); - - $this->manager->registerNotifier(function() use ($notifier) { - return $notifier; - }, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - $this->manager->registerNotifier(function() use ($notifier2) { - return $notifier2; - }, function() { - return ['id' => 'test2', 'name' => 'Test Two']; - }); - - $this->assertEquals($notification2, $this->manager->prepare($notification, 'en')); - } - - /** - * @expectedException \InvalidArgumentException - */ - public function testPrepareInvalid() { - /** @var \OCP\Notification\INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ - $notification = $this->getMockBuilder(INotification::class) - ->disableOriginalConstructor() - ->getMock(); - $notification->expects($this->once()) - ->method('isValidParsed') - ->willReturn(false); - - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $notifier */ - $notifier = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); - $notifier->expects($this->once()) - ->method('prepare') - ->with($notification, 'de') - ->willReturnArgument(0); - - $this->manager->registerNotifier(function() use ($notifier) { - return $notifier; - }, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - - $this->manager->prepare($notification, 'de'); - } - - public function testPrepareNotifierThrows() { - /** @var \OCP\Notification\INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ - $notification = $this->getMockBuilder(INotification::class) - ->disableOriginalConstructor() - ->getMock(); - $notification->expects($this->once()) - ->method('isValidParsed') - ->willReturn(true); - - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $notifier */ - $notifier = $this->getMockBuilder(INotifier::class) - ->disableOriginalConstructor() - ->getMock(); - $notifier->expects($this->once()) - ->method('prepare') - ->with($notification, 'de') - ->willThrowException(new \InvalidArgumentException); - - $this->manager->registerNotifier(function() use ($notifier) { - return $notifier; - }, function() { - return ['id' => 'test1', 'name' => 'Test One']; - }); - - $this->assertEquals($notification, $this->manager->prepare($notification, 'de')); - } - - /** - * @expectedException \InvalidArgumentException - */ - public function testPrepareNoNotifier() { - /** @var \OCP\Notification\INotification|\PHPUnit_Framework_MockObject_MockObject $notification */ - $notification = $this->getMockBuilder(INotification::class) - ->disableOriginalConstructor() - ->getMock(); - $notification->expects($this->once()) - ->method('isValidParsed') - ->willReturn(false); - - $this->manager->prepare($notification, 'en'); + $manager->notify($notification); } public function testMarkProcessed() { @@ -361,30 +151,19 @@ class ManagerTest extends TestCase { ->disableOriginalConstructor() ->getMock(); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app */ - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() + $manager = $this->getMockBuilder(Manager::class) + ->setConstructorArgs([ + $this->validator, + $this->logger, + ]) + ->setMethods(['getApps']) ->getMock(); - $app->expects($this->once()) - ->method('markProcessed') - ->with($notification); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app2 */ - $app2 = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - $app2->expects($this->once()) - ->method('markProcessed') - ->with($notification); + $manager->expects($this->once()) + ->method('getApps') + ->willReturn([]); - $this->manager->registerApp(function() use ($app) { - return $app; - }); - $this->manager->registerApp(function() use ($app2) { - return $app2; - }); - - $this->manager->markProcessed($notification); + $manager->markProcessed($notification); } public function testGetCount() { @@ -393,31 +172,18 @@ class ManagerTest extends TestCase { ->disableOriginalConstructor() ->getMock(); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app */ - $app = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() + $manager = $this->getMockBuilder(Manager::class) + ->setConstructorArgs([ + $this->validator, + $this->logger, + ]) + ->setMethods(['getApps']) ->getMock(); - $app->expects($this->once()) - ->method('getCount') - ->with($notification) - ->willReturn(21); - /** @var \OCP\Notification\IApp|\PHPUnit_Framework_MockObject_MockObject $app2 */ - $app2 = $this->getMockBuilder(IApp::class) - ->disableOriginalConstructor() - ->getMock(); - $app2->expects($this->once()) - ->method('getCount') - ->with($notification) - ->willReturn(42); + $manager->expects($this->once()) + ->method('getApps') + ->willReturn([]); - $this->manager->registerApp(function() use ($app) { - return $app; - }); - $this->manager->registerApp(function() use ($app2) { - return $app2; - }); - - $this->assertSame(63, $this->manager->getCount($notification)); + $manager->getCount($notification); } } diff --git a/tests/lib/Notification/NotificationTest.php b/tests/lib/Notification/NotificationTest.php index 7517be715e..d72c9f62ae 100644 --- a/tests/lib/Notification/NotificationTest.php +++ b/tests/lib/Notification/NotificationTest.php @@ -63,29 +63,6 @@ class NotificationTest extends TestCase { return $dataSets; } - protected function dataInvalidStringType() { - return [ - [true], - [false], - [16412], - [[]], - [null], - ]; - } - - protected function dataInvalidInt() { - return [ - [true], - [false], - [''], - ['a'], - [str_repeat('a', 256)], - [[]], - [['a']], - [[str_repeat('a', 256)]], - ]; - } - public function dataSetApp() { return $this->dataValidString(32); } @@ -104,10 +81,6 @@ class NotificationTest extends TestCase { return $this->dataInvalidString(32); } - public function dataSetAppInvalidType() { - return $this->dataInvalidStringType(); - } - /** * @dataProvider dataSetAppInvalid * @param mixed $app @@ -118,16 +91,6 @@ class NotificationTest extends TestCase { $this->notification->setApp($app); } - /** - * @dataProvider dataSetAppInvalidType - * @param mixed $app - * - * @expectedException \TypeError - */ - public function testSetAppInvalidType($app) { - $this->notification->setApp($app); - } - public function dataSetUser() { return $this->dataValidString(64); @@ -147,10 +110,6 @@ class NotificationTest extends TestCase { return $this->dataInvalidString(64); } - public function dataSetUserInvalidType() { - return $this->dataInvalidStringType(); - } - /** * @dataProvider dataSetUserInvalid * @param mixed $user @@ -161,16 +120,6 @@ class NotificationTest extends TestCase { $this->notification->setUser($user); } - /** - * @dataProvider dataSetUserInvalidType - * @param mixed $user - * - * @expectedException \TypeError - */ - public function testSetUserInvalidType($user) { - $this->notification->setUser($user); - } - public function dataSetDateTime() { $past = new \DateTime(); $past->sub(new \DateInterval('P1Y')); @@ -216,48 +165,32 @@ class NotificationTest extends TestCase { public function dataSetObject() { return [ - ['a', '21', '21'], - [str_repeat('a', 64), 42, '42'], + ['a', '21'], + [str_repeat('a', 64), '42'], ]; } /** * @dataProvider dataSetObject * @param string $type - * @param int|string $id - * @param string $exptectedId + * @param string $id */ - public function testSetObject($type, $id, $exptectedId) { + public function testSetObject($type, $id) { $this->assertSame('', $this->notification->getObjectType()); $this->assertSame('', $this->notification->getObjectId()); $this->assertSame($this->notification, $this->notification->setObject($type, $id)); $this->assertSame($type, $this->notification->getObjectType()); - $this->assertSame($exptectedId, $this->notification->getObjectId()); + $this->assertSame($id, $this->notification->getObjectId()); } public function dataSetObjectTypeInvalid() { return $this->dataInvalidString(64); } - /** - * @dataProvider dataSetObjectTypeInvalid - * @param mixed $type - * - * @expectedException \InvalidArgumentException - * @expectedMessage 'The given object type is invalid' - */ - public function testSetObjectTypeInvalid($type) { - $this->notification->setObject($type, 1337); - } - public function dataSetObjectIdInvalid() { return [ - [true], - [false], [''], [str_repeat('a', 64 + 1)], - [[]], - [[str_repeat('a', 64 + 1)]], ]; } From c3ef1cd90d215f36edf7236456578a233bc5f3bd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 16 Jul 2019 13:17:51 +0200 Subject: [PATCH 10/12] Update autoloader Signed-off-by: Joas Schilling --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index d04a186036..fa38232afc 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -347,6 +347,7 @@ return array( 'OCP\\Migration\\IOutput' => $baseDir . '/lib/public/Migration/IOutput.php', 'OCP\\Migration\\IRepairStep' => $baseDir . '/lib/public/Migration/IRepairStep.php', 'OCP\\Migration\\SimpleMigrationStep' => $baseDir . '/lib/public/Migration/SimpleMigrationStep.php', + 'OCP\\Notification\\AlreadyProcessedException' => $baseDir . '/lib/public/Notification/AlreadyProcessedException.php', 'OCP\\Notification\\IAction' => $baseDir . '/lib/public/Notification/IAction.php', 'OCP\\Notification\\IApp' => $baseDir . '/lib/public/Notification/IApp.php', 'OCP\\Notification\\IManager' => $baseDir . '/lib/public/Notification/IManager.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 8cd007f513..4cea0319a0 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -381,6 +381,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Migration\\IOutput' => __DIR__ . '/../../..' . '/lib/public/Migration/IOutput.php', 'OCP\\Migration\\IRepairStep' => __DIR__ . '/../../..' . '/lib/public/Migration/IRepairStep.php', 'OCP\\Migration\\SimpleMigrationStep' => __DIR__ . '/../../..' . '/lib/public/Migration/SimpleMigrationStep.php', + 'OCP\\Notification\\AlreadyProcessedException' => __DIR__ . '/../../..' . '/lib/public/Notification/AlreadyProcessedException.php', 'OCP\\Notification\\IAction' => __DIR__ . '/../../..' . '/lib/public/Notification/IAction.php', 'OCP\\Notification\\IApp' => __DIR__ . '/../../..' . '/lib/public/Notification/IApp.php', 'OCP\\Notification\\IManager' => __DIR__ . '/../../..' . '/lib/public/Notification/IManager.php', From 565838da9c279fe071a9cdfbfd9cf574893a61f5 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 16 Jul 2019 13:32:44 +0200 Subject: [PATCH 11/12] Update unit tests Signed-off-by: Joas Schilling --- .../tests/Unit/Controller/NotificationsTest.php | 6 ++++++ .../tests/Unit/Notification/ListenerTest.php | 6 ++++++ .../tests/Unit/Notification/NotifierTest.php | 5 ++++- .../tests/Notification/NotifierTest.php | 12 ++---------- .../Listeners/RemoteWipeNotificationsListener.php | 2 +- .../RemoteWipeNotificationsListenerTest.php | 4 ++-- 6 files changed, 21 insertions(+), 14 deletions(-) diff --git a/apps/comments/tests/Unit/Controller/NotificationsTest.php b/apps/comments/tests/Unit/Controller/NotificationsTest.php index c5209b9f9d..9df81f01b0 100644 --- a/apps/comments/tests/Unit/Controller/NotificationsTest.php +++ b/apps/comments/tests/Unit/Controller/NotificationsTest.php @@ -117,6 +117,9 @@ class NotificationsTest extends TestCase { $comment->expects($this->any()) ->method('getObjectType') ->willReturn('files'); + $comment->expects($this->any()) + ->method('getId') + ->willReturn('1234'); $this->commentsManager->expects($this->any()) ->method('get') @@ -192,6 +195,9 @@ class NotificationsTest extends TestCase { $comment->expects($this->any()) ->method('getObjectType') ->willReturn('files'); + $comment->expects($this->any()) + ->method('getId') + ->willReturn('1234'); $this->commentsManager->expects($this->any()) ->method('get') diff --git a/apps/comments/tests/Unit/Notification/ListenerTest.php b/apps/comments/tests/Unit/Notification/ListenerTest.php index d6f83262f3..e31b227bce 100644 --- a/apps/comments/tests/Unit/Notification/ListenerTest.php +++ b/apps/comments/tests/Unit/Notification/ListenerTest.php @@ -91,6 +91,9 @@ class ListenerTest extends TestCase { [ 'type' => 'user', 'id' => '23452-4333-54353-2342'], [ 'type' => 'user', 'id' => 'yolo'], ]); + $comment->expects($this->atLeastOnce()) + ->method('getId') + ->willReturn('1234'); /** @var CommentsEvent|\PHPUnit_Framework_MockObject_MockObject $event */ $event = $this->getMockBuilder(CommentsEvent::class) @@ -186,6 +189,9 @@ class ListenerTest extends TestCase { $comment->expects($this->once()) ->method('getMentions') ->willReturn([[ 'type' => 'user', 'id' => 'foobar']]); + $comment->expects($this->atLeastOnce()) + ->method('getId') + ->willReturn('1234'); /** @var CommentsEvent|\PHPUnit_Framework_MockObject_MockObject $event */ $event = $this->getMockBuilder(CommentsEvent::class) diff --git a/apps/comments/tests/Unit/Notification/NotifierTest.php b/apps/comments/tests/Unit/Notification/NotifierTest.php index 6eceed4491..d008c4c312 100644 --- a/apps/comments/tests/Unit/Notification/NotifierTest.php +++ b/apps/comments/tests/Unit/Notification/NotifierTest.php @@ -195,6 +195,9 @@ class NotifierTest extends TestCase { ->expects($this->any()) ->method('getMentions') ->willReturn([['type' => 'user', 'id' => 'you']]); + $this->comment->expects($this->atLeastOnce()) + ->method('getId') + ->willReturn('1234'); $this->commentsManager ->expects($this->once()) @@ -539,7 +542,7 @@ class NotifierTest extends TestCase { } /** - * @expectedException \InvalidArgumentException + * @expectedException \OCP\Notification\AlreadyProcessedException */ public function testPrepareUnresolvableFileID() { $displayName = 'Huraga'; diff --git a/apps/updatenotification/tests/Notification/NotifierTest.php b/apps/updatenotification/tests/Notification/NotifierTest.php index b1ddf7b478..f97ff776c9 100644 --- a/apps/updatenotification/tests/Notification/NotifierTest.php +++ b/apps/updatenotification/tests/Notification/NotifierTest.php @@ -30,6 +30,7 @@ use OCP\IGroupManager; use OCP\IURLGenerator; use OCP\IUserSession; use OCP\L10N\IFactory; +use OCP\Notification\AlreadyProcessedException; use OCP\Notification\IManager; use OCP\Notification\INotification; use Test\TestCase; @@ -112,21 +113,12 @@ class NotifierTest extends TestCase { ->method('getObjectId') ->willReturn($versionNotification); - if ($exception) { - $this->notificationManager->expects($this->once()) - ->method('markProcessed') - ->with($notification); - } else { - $this->notificationManager->expects($this->never()) - ->method('markProcessed'); - } - try { self::invokePrivate($notifier, 'updateAlreadyInstalledCheck', [$notification, $versionInstalled]); $this->assertFalse($exception); } catch (\Exception $e) { $this->assertTrue($exception); - $this->assertInstanceOf('InvalidArgumentException', $e); + $this->assertInstanceOf(AlreadyProcessedException::class, $e); } } } diff --git a/lib/private/Authentication/Listeners/RemoteWipeNotificationsListener.php b/lib/private/Authentication/Listeners/RemoteWipeNotificationsListener.php index ffddddff1d..57213a8a19 100644 --- a/lib/private/Authentication/Listeners/RemoteWipeNotificationsListener.php +++ b/lib/private/Authentication/Listeners/RemoteWipeNotificationsListener.php @@ -61,7 +61,7 @@ class RemoteWipeNotificationsListener implements IEventListener { $notification->setApp('auth') ->setUser($token->getUID()) ->setDateTime($this->timeFactory->getDateTime()) - ->setObject('token', $token->getId()) + ->setObject('token', (string) $token->getId()) ->setSubject($event, [ 'name' => $token->getName(), ]); diff --git a/tests/lib/Authentication/Listeners/RemoteWipeNotificationsListenerTest.php b/tests/lib/Authentication/Listeners/RemoteWipeNotificationsListenerTest.php index 27d386ca5b..e3e526e401 100644 --- a/tests/lib/Authentication/Listeners/RemoteWipeNotificationsListenerTest.php +++ b/tests/lib/Authentication/Listeners/RemoteWipeNotificationsListenerTest.php @@ -92,7 +92,7 @@ class RemoteWipeNotificationListenerTests extends TestCase { $token->method('getId')->willReturn(123); $notification->expects($this->once()) ->method('setObject') - ->with('token', 123) + ->with('token', '123') ->willReturnSelf(); $token->method('getName')->willReturn('Token 1'); $notification->expects($this->once()) @@ -132,7 +132,7 @@ class RemoteWipeNotificationListenerTests extends TestCase { $token->method('getId')->willReturn(123); $notification->expects($this->once()) ->method('setObject') - ->with('token', 123) + ->with('token', '123') ->willReturnSelf(); $token->method('getName')->willReturn('Token 1'); $notification->expects($this->once()) From 594efca1e3b936d0d86f2d80ebf366980a425713 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 16 Jul 2019 16:58:38 +0200 Subject: [PATCH 12/12] Update since to the correct version Signed-off-by: Joas Schilling --- lib/private/Notification/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Notification/Manager.php b/lib/private/Notification/Manager.php index 9cca86a4bd..fae0a66491 100644 --- a/lib/private/Notification/Manager.php +++ b/lib/private/Notification/Manager.php @@ -67,7 +67,7 @@ class Manager implements IManager { /** * @param string $appClass The service must implement IApp, otherwise a * \InvalidArgumentException is thrown later - * @since 9.0.0 + * @since 17.0.0 */ public function registerApp(string $appClass): void { $this->appClasses[] = $appClass;