From 8cdcb25fccdd2ad9a973cc3de87ed353b73e000e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= Date: Wed, 13 Nov 2019 11:58:52 +0100 Subject: [PATCH 1/6] Notifications into the queue Forces notifications into a dedicated SendNotificationsJob and passed to the queue. - One static method re-used in the job ::getAttributes, is that okay or use a trait? - Do we want to use this solution and refactor into a better Hub after stable, postpone this implementation or use it in b11? --- .../Blueprint/AbstractBlueprint.php | 23 +++++ .../Blueprint/BlueprintInterface.php | 7 +- .../core/src/Notification/Event/Notifying.php | 39 +++++++++ .../core/src/Notification/Event/Sending.php | 2 +- .../Notification/Job/SendNotificationsJob.php | 73 ++++++++++++++++ .../src/Notification/NotificationSyncer.php | 87 +++---------------- framework/core/src/Queue/AbstractJob.php | 22 +++++ 7 files changed, 174 insertions(+), 79 deletions(-) create mode 100644 framework/core/src/Notification/Blueprint/AbstractBlueprint.php create mode 100644 framework/core/src/Notification/Event/Notifying.php create mode 100644 framework/core/src/Notification/Job/SendNotificationsJob.php create mode 100644 framework/core/src/Queue/AbstractJob.php diff --git a/framework/core/src/Notification/Blueprint/AbstractBlueprint.php b/framework/core/src/Notification/Blueprint/AbstractBlueprint.php new file mode 100644 index 000000000..23eb893d5 --- /dev/null +++ b/framework/core/src/Notification/Blueprint/AbstractBlueprint.php @@ -0,0 +1,23 @@ + static::getType(), + 'from_user_id' => ($fromUser = $this->getFromUser()) ? $fromUser->id : null, + 'subject_id' => ($subject = $this->getSubject()) ? $subject->getKey() : null, + 'data' => ($data = $this->getData()) ? json_encode($data) : null + ]; + } +} diff --git a/framework/core/src/Notification/Blueprint/BlueprintInterface.php b/framework/core/src/Notification/Blueprint/BlueprintInterface.php index 68b1d8553..38f5ed060 100644 --- a/framework/core/src/Notification/Blueprint/BlueprintInterface.php +++ b/framework/core/src/Notification/Blueprint/BlueprintInterface.php @@ -9,6 +9,9 @@ namespace Flarum\Notification\Blueprint; +use Flarum\Database\AbstractModel; +use Flarum\User\User; + /** * A notification BlueprintInterface, when instantiated, represents a notification about * something. The blueprint is used by the NotificationSyncer to commit the @@ -19,14 +22,14 @@ interface BlueprintInterface /** * Get the user that sent the notification. * - * @return \Flarum\User\User|null + * @return User|null */ public function getFromUser(); /** * Get the model that is the subject of this activity. * - * @return \Flarum\Database\AbstractModel|null + * @return AbstractModel|null */ public function getSubject(); diff --git a/framework/core/src/Notification/Event/Notifying.php b/framework/core/src/Notification/Event/Notifying.php new file mode 100644 index 000000000..5fb3ad5b2 --- /dev/null +++ b/framework/core/src/Notification/Event/Notifying.php @@ -0,0 +1,39 @@ +blueprint = $blueprint; + $this->users = &$users; + } +} diff --git a/framework/core/src/Notification/Event/Sending.php b/framework/core/src/Notification/Event/Sending.php index 4484cddc2..de2e1ca6e 100644 --- a/framework/core/src/Notification/Event/Sending.php +++ b/framework/core/src/Notification/Event/Sending.php @@ -23,7 +23,7 @@ class Sending /** * The users that the notification will be sent to. * - * @var array + * @var array|int[] */ public $users; diff --git a/framework/core/src/Notification/Job/SendNotificationsJob.php b/framework/core/src/Notification/Job/SendNotificationsJob.php new file mode 100644 index 000000000..c1d329e18 --- /dev/null +++ b/framework/core/src/Notification/Job/SendNotificationsJob.php @@ -0,0 +1,73 @@ +blueprint = $blueprint; + $this->recipientIds = $recipientIds; + } + + public function handle(NotificationMailer $mailer) + { + $now = Carbon::now('utc')->toDateTimeString(); + $recipients = $this->recipientIds; + + event(new Sending($this->blueprint, $recipients)); + + $attributes = $this->blueprint->getAttributes(); + + Notification::insert( + array_map(function (User $user) use ($attributes, $now) { + return $attributes + [ + 'user_id' => $user->id, + 'created_at' => $now + ]; + }, $recipients) + ); + + event(new Notifying($this->blueprint, $recipients)); + + if ($this->blueprint instanceof MailableInterface) { + $this->email($mailer, $this->blueprint, $recipients); + } + } + + protected function email(NotificationMailer $mailer, MailableInterface $blueprint, array $recipients) + { + foreach ($recipients as $user) { + if ($user->shouldEmail($blueprint::getType())) { + $mailer->send($blueprint, $user); + } + } + } +} diff --git a/framework/core/src/Notification/NotificationSyncer.php b/framework/core/src/Notification/NotificationSyncer.php index 12a987fec..5fd2ead77 100644 --- a/framework/core/src/Notification/NotificationSyncer.php +++ b/framework/core/src/Notification/NotificationSyncer.php @@ -9,10 +9,10 @@ namespace Flarum\Notification; -use Carbon\Carbon; use Flarum\Notification\Blueprint\BlueprintInterface; -use Flarum\Notification\Event\Sending; +use Flarum\Notification\Job\SendNotificationsJob; use Flarum\User\User; +use Illuminate\Contracts\Queue\Queue; /** * The Notification Syncer commits notification blueprints to the database, and @@ -42,20 +42,16 @@ class NotificationSyncer protected $notifications; /** - * @var NotificationMailer + * @var Queue */ - protected $mailer; + protected $queue; - /** - * @param NotificationRepository $notifications - * @param NotificationMailer $mailer - */ public function __construct( NotificationRepository $notifications, - NotificationMailer $mailer + Queue $queue ) { $this->notifications = $notifications; - $this->mailer = $mailer; + $this->queue = $queue; } /** @@ -69,7 +65,7 @@ class NotificationSyncer */ public function sync(Blueprint\BlueprintInterface $blueprint, array $users) { - $attributes = $this->getAttributes($blueprint); + $attributes = $blueprint->getAttributes(); // Find all existing notification records in the database matching this // blueprint. We will begin by assuming that they all need to be @@ -87,7 +83,7 @@ class NotificationSyncer continue; } - $existing = $toDelete->first(function ($notification, $i) use ($user) { + $existing = $toDelete->first(function ($notification) use ($user) { return $notification->user_id === $user->id; }); @@ -115,7 +111,7 @@ class NotificationSyncer // receiving this notification for the first time (we know because they // didn't have a record in the database). if (count($newRecipients)) { - $this->sendNotifications($blueprint, $newRecipients); + $this->queue->push(new SendNotificationsJob($blueprint, $newRecipients)); } } @@ -127,7 +123,7 @@ class NotificationSyncer */ public function delete(BlueprintInterface $blueprint) { - Notification::where($this->getAttributes($blueprint))->update(['is_deleted' => true]); + Notification::where($blueprint->getAttributes())->update(['is_deleted' => true]); } /** @@ -138,7 +134,7 @@ class NotificationSyncer */ public function restore(BlueprintInterface $blueprint) { - Notification::where($this->getAttributes($blueprint))->update(['is_deleted' => false]); + Notification::where($blueprint->getAttributes())->update(['is_deleted' => false]); } /** @@ -158,50 +154,6 @@ class NotificationSyncer static::$onePerUser = false; } - /** - * Create a notification record and send an email (depending on user - * preference) from a blueprint to a list of recipients. - * - * @param \Flarum\Notification\Blueprint\BlueprintInterface $blueprint - * @param User[] $recipients - */ - protected function sendNotifications(Blueprint\BlueprintInterface $blueprint, array $recipients) - { - $now = Carbon::now('utc')->toDateTimeString(); - - event(new Sending($blueprint, $recipients)); - - $attributes = $this->getAttributes($blueprint); - - Notification::insert( - array_map(function (User $user) use ($attributes, $now) { - return $attributes + [ - 'user_id' => $user->id, - 'created_at' => $now - ]; - }, $recipients) - ); - - if ($blueprint instanceof MailableInterface) { - $this->mailNotifications($blueprint, $recipients); - } - } - - /** - * Mail a notification to a list of users. - * - * @param MailableInterface $blueprint - * @param User[] $recipients - */ - protected function mailNotifications(MailableInterface $blueprint, array $recipients) - { - foreach ($recipients as $user) { - if ($user->shouldEmail($blueprint::getType())) { - $this->mailer->send($blueprint, $user); - } - } - } - /** * Set the deleted status of a list of notification records. * @@ -212,21 +164,4 @@ class NotificationSyncer { Notification::whereIn('id', $ids)->update(['is_deleted' => $isDeleted]); } - - /** - * Construct an array of attributes to be stored in a notification record in - * the database, given a notification blueprint. - * - * @param \Flarum\Notification\Blueprint\BlueprintInterface $blueprint - * @return array - */ - protected function getAttributes(Blueprint\BlueprintInterface $blueprint) - { - return [ - 'type' => $blueprint::getType(), - 'from_user_id' => ($fromUser = $blueprint->getFromUser()) ? $fromUser->id : null, - 'subject_id' => ($subject = $blueprint->getSubject()) ? $subject->id : null, - 'data' => ($data = $blueprint->getData()) ? json_encode($data) : null - ]; - } } diff --git a/framework/core/src/Queue/AbstractJob.php b/framework/core/src/Queue/AbstractJob.php new file mode 100644 index 000000000..6ec8235a8 --- /dev/null +++ b/framework/core/src/Queue/AbstractJob.php @@ -0,0 +1,22 @@ + Date: Mon, 10 Feb 2020 11:45:35 +0100 Subject: [PATCH 2/6] Moved sending emails to the syncer This separates sending each individual mail, thus hardening the app. There are still many improvements possible in this code, e.g. chaining these commands, making emails just another notification type and listening to the Notify event instead. We can postpone this to a later stable release. --- .../Job/SendEmailNotificationJob.php | 39 +++++++++++++++++++ .../Notification/Job/SendNotificationsJob.php | 30 ++++---------- .../src/Notification/NotificationSyncer.php | 23 ++++++++++- 3 files changed, 68 insertions(+), 24 deletions(-) create mode 100644 framework/core/src/Notification/Job/SendEmailNotificationJob.php diff --git a/framework/core/src/Notification/Job/SendEmailNotificationJob.php b/framework/core/src/Notification/Job/SendEmailNotificationJob.php new file mode 100644 index 000000000..68bffeb17 --- /dev/null +++ b/framework/core/src/Notification/Job/SendEmailNotificationJob.php @@ -0,0 +1,39 @@ +blueprint = $blueprint; + $this->recipient = $recipient; + } + + public function handle(NotificationMailer $mailer) + { + $mailer->send($this->blueprint, $this->recipient); + } +} diff --git a/framework/core/src/Notification/Job/SendNotificationsJob.php b/framework/core/src/Notification/Job/SendNotificationsJob.php index c1d329e18..31207bac7 100644 --- a/framework/core/src/Notification/Job/SendNotificationsJob.php +++ b/framework/core/src/Notification/Job/SendNotificationsJob.php @@ -13,9 +13,7 @@ use Carbon\Carbon; use Flarum\Notification\Blueprint\BlueprintInterface; use Flarum\Notification\Event\Notifying; use Flarum\Notification\Event\Sending; -use Flarum\Notification\MailableInterface; use Flarum\Notification\Notification; -use Flarum\Notification\NotificationMailer; use Flarum\Queue\AbstractJob; use Flarum\User\User; @@ -27,22 +25,21 @@ class SendNotificationsJob extends AbstractJob private $blueprint; /** - * @var array + * @var User[] */ - private $recipientIds; + private $recipients; - public function __construct(BlueprintInterface $blueprint, array $recipientIds = []) + public function __construct(BlueprintInterface $blueprint, array $recipients = []) { $this->blueprint = $blueprint; - $this->recipientIds = $recipientIds; + $this->recipients = $recipients; } - public function handle(NotificationMailer $mailer) + public function handle() { $now = Carbon::now('utc')->toDateTimeString(); - $recipients = $this->recipientIds; - event(new Sending($this->blueprint, $recipients)); + event(new Sending($this->blueprint, $this->recipients)); $attributes = $this->blueprint->getAttributes(); @@ -52,22 +49,9 @@ class SendNotificationsJob extends AbstractJob 'user_id' => $user->id, 'created_at' => $now ]; - }, $recipients) + }, $this->recipients) ); event(new Notifying($this->blueprint, $recipients)); - - if ($this->blueprint instanceof MailableInterface) { - $this->email($mailer, $this->blueprint, $recipients); - } - } - - protected function email(NotificationMailer $mailer, MailableInterface $blueprint, array $recipients) - { - foreach ($recipients as $user) { - if ($user->shouldEmail($blueprint::getType())) { - $mailer->send($blueprint, $user); - } - } } } diff --git a/framework/core/src/Notification/NotificationSyncer.php b/framework/core/src/Notification/NotificationSyncer.php index 5fd2ead77..b6b6f96e1 100644 --- a/framework/core/src/Notification/NotificationSyncer.php +++ b/framework/core/src/Notification/NotificationSyncer.php @@ -10,6 +10,7 @@ namespace Flarum\Notification; use Flarum\Notification\Blueprint\BlueprintInterface; +use Flarum\Notification\Job\SendEmailNotificationJob; use Flarum\Notification\Job\SendNotificationsJob; use Flarum\User\User; use Illuminate\Contracts\Queue\Queue; @@ -109,10 +110,15 @@ class NotificationSyncer // Create a notification record, and send an email, for all users // receiving this notification for the first time (we know because they - // didn't have a record in the database). + // didn't have a record in the database). As both operations can be + // intensive on resources (database and mail server), we queue them. if (count($newRecipients)) { $this->queue->push(new SendNotificationsJob($blueprint, $newRecipients)); } + + if ($blueprint instanceof MailableInterface) { + $this->mailNotifications($blueprint, $newRecipients); + } } /** @@ -154,6 +160,21 @@ class NotificationSyncer static::$onePerUser = false; } + /** + * Mail a notification to a list of users. + * + * @param MailableInterface $blueprint + * @param User[] $recipients + */ + protected function mailNotifications(MailableInterface $blueprint, array $recipients) + { + foreach ($recipients as $user) { + if ($user->shouldEmail($blueprint::getType())) { + $this->queue->push(new SendEmailNotificationJob($blueprint, $user)); + } + } + } + /** * Set the deleted status of a list of notification records. * From 2277f26e84c2605784a6ddb2a4388fc7c939848d Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 27 Mar 2020 14:48:40 +0100 Subject: [PATCH 3/6] Remove Notifying event for now As discussed with @luceos, let's add this once the use case comes up. It might be a left-over from a previous state of this PR anyway. --- .../core/src/Notification/Event/Notifying.php | 39 ------------------- .../Notification/Job/SendNotificationsJob.php | 3 -- 2 files changed, 42 deletions(-) delete mode 100644 framework/core/src/Notification/Event/Notifying.php diff --git a/framework/core/src/Notification/Event/Notifying.php b/framework/core/src/Notification/Event/Notifying.php deleted file mode 100644 index 5fb3ad5b2..000000000 --- a/framework/core/src/Notification/Event/Notifying.php +++ /dev/null @@ -1,39 +0,0 @@ -blueprint = $blueprint; - $this->users = &$users; - } -} diff --git a/framework/core/src/Notification/Job/SendNotificationsJob.php b/framework/core/src/Notification/Job/SendNotificationsJob.php index 31207bac7..f3e8d0277 100644 --- a/framework/core/src/Notification/Job/SendNotificationsJob.php +++ b/framework/core/src/Notification/Job/SendNotificationsJob.php @@ -11,7 +11,6 @@ namespace Flarum\Notification\Job; use Carbon\Carbon; use Flarum\Notification\Blueprint\BlueprintInterface; -use Flarum\Notification\Event\Notifying; use Flarum\Notification\Event\Sending; use Flarum\Notification\Notification; use Flarum\Queue\AbstractJob; @@ -51,7 +50,5 @@ class SendNotificationsJob extends AbstractJob ]; }, $this->recipients) ); - - event(new Notifying($this->blueprint, $recipients)); } } From 4c363fe76c2d5260a465caab99181406da4a56d3 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 27 Mar 2020 15:13:53 +0100 Subject: [PATCH 4/6] Remove unnecessary constructor parameter --- .../core/src/Notification/NotificationSyncer.php | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/framework/core/src/Notification/NotificationSyncer.php b/framework/core/src/Notification/NotificationSyncer.php index b6b6f96e1..94d154893 100644 --- a/framework/core/src/Notification/NotificationSyncer.php +++ b/framework/core/src/Notification/NotificationSyncer.php @@ -37,21 +37,13 @@ class NotificationSyncer */ protected static $sentTo = []; - /** - * @var NotificationRepository - */ - protected $notifications; - /** * @var Queue */ protected $queue; - public function __construct( - NotificationRepository $notifications, - Queue $queue - ) { - $this->notifications = $notifications; + public function __construct(Queue $queue) + { $this->queue = $queue; } From cf3e0cc5bcb154d73ff613bff26f615975011204 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 27 Mar 2020 15:50:02 +0100 Subject: [PATCH 5/6] Add BC layer for notification blueprints This gives extension authors time to add the new `getAttributes()` method to their `BlueprintInterface` implementations. The layer itself is easy to remove in beta.14. --- .../Blueprint/AbstractBlueprint.php | 23 ------------ .../Blueprint/BlueprintInterface.php | 11 ++++++ .../core/src/Notification/BlueprintBC.php | 36 +++++++++++++++++++ .../Notification/Job/SendNotificationsJob.php | 3 +- .../src/Notification/NotificationSyncer.php | 6 ++-- 5 files changed, 52 insertions(+), 27 deletions(-) delete mode 100644 framework/core/src/Notification/Blueprint/AbstractBlueprint.php create mode 100644 framework/core/src/Notification/BlueprintBC.php diff --git a/framework/core/src/Notification/Blueprint/AbstractBlueprint.php b/framework/core/src/Notification/Blueprint/AbstractBlueprint.php deleted file mode 100644 index 23eb893d5..000000000 --- a/framework/core/src/Notification/Blueprint/AbstractBlueprint.php +++ /dev/null @@ -1,23 +0,0 @@ - static::getType(), - 'from_user_id' => ($fromUser = $this->getFromUser()) ? $fromUser->id : null, - 'subject_id' => ($subject = $this->getSubject()) ? $subject->getKey() : null, - 'data' => ($data = $this->getData()) ? json_encode($data) : null - ]; - } -} diff --git a/framework/core/src/Notification/Blueprint/BlueprintInterface.php b/framework/core/src/Notification/Blueprint/BlueprintInterface.php index 38f5ed060..422ba1784 100644 --- a/framework/core/src/Notification/Blueprint/BlueprintInterface.php +++ b/framework/core/src/Notification/Blueprint/BlueprintInterface.php @@ -22,6 +22,7 @@ interface BlueprintInterface /** * Get the user that sent the notification. * + * @deprecated Will be removed for beta.14 * @return User|null */ public function getFromUser(); @@ -29,6 +30,7 @@ interface BlueprintInterface /** * Get the model that is the subject of this activity. * + * @deprecated Will be removed for beta.14 * @return AbstractModel|null */ public function getSubject(); @@ -36,10 +38,19 @@ interface BlueprintInterface /** * Get the data to be stored in the notification. * + * @deprecated Will be removed for beta.14 * @return array|null */ public function getData(); + /** + * Get the attributes that uniquely identify a notification, plus metadata. + * TODO: Uncomment this for beta.14. + * + * @return array + */ + //public function getAttributes(): array; + /** * Get the serialized type of this activity. * diff --git a/framework/core/src/Notification/BlueprintBC.php b/framework/core/src/Notification/BlueprintBC.php new file mode 100644 index 000000000..b3a4209d0 --- /dev/null +++ b/framework/core/src/Notification/BlueprintBC.php @@ -0,0 +1,36 @@ +getAttributes(); + } else { + return [ + 'type' => $blueprint::getType(), + 'from_user_id' => ($fromUser = $blueprint->getFromUser()) ? $fromUser->id : null, + 'subject_id' => ($subject = $blueprint->getSubject()) ? $subject->id : null, + 'data' => ($data = $blueprint->getData()) ? json_encode($data) : null + ]; + } + } +} diff --git a/framework/core/src/Notification/Job/SendNotificationsJob.php b/framework/core/src/Notification/Job/SendNotificationsJob.php index f3e8d0277..93a08b8fd 100644 --- a/framework/core/src/Notification/Job/SendNotificationsJob.php +++ b/framework/core/src/Notification/Job/SendNotificationsJob.php @@ -11,6 +11,7 @@ namespace Flarum\Notification\Job; use Carbon\Carbon; use Flarum\Notification\Blueprint\BlueprintInterface; +use Flarum\Notification\BlueprintBC; use Flarum\Notification\Event\Sending; use Flarum\Notification\Notification; use Flarum\Queue\AbstractJob; @@ -40,7 +41,7 @@ class SendNotificationsJob extends AbstractJob event(new Sending($this->blueprint, $this->recipients)); - $attributes = $this->blueprint->getAttributes(); + $attributes = BlueprintBC::getAttributes($this->blueprint); Notification::insert( array_map(function (User $user) use ($attributes, $now) { diff --git a/framework/core/src/Notification/NotificationSyncer.php b/framework/core/src/Notification/NotificationSyncer.php index 94d154893..9ae014588 100644 --- a/framework/core/src/Notification/NotificationSyncer.php +++ b/framework/core/src/Notification/NotificationSyncer.php @@ -58,7 +58,7 @@ class NotificationSyncer */ public function sync(Blueprint\BlueprintInterface $blueprint, array $users) { - $attributes = $blueprint->getAttributes(); + $attributes = BlueprintBC::getAttributes($blueprint); // Find all existing notification records in the database matching this // blueprint. We will begin by assuming that they all need to be @@ -121,7 +121,7 @@ class NotificationSyncer */ public function delete(BlueprintInterface $blueprint) { - Notification::where($blueprint->getAttributes())->update(['is_deleted' => true]); + Notification::where(BlueprintBC::getAttributes($blueprint))->update(['is_deleted' => true]); } /** @@ -132,7 +132,7 @@ class NotificationSyncer */ public function restore(BlueprintInterface $blueprint) { - Notification::where($blueprint->getAttributes())->update(['is_deleted' => false]); + Notification::where(BlueprintBC::getAttributes($blueprint))->update(['is_deleted' => false]); } /** From 119662f6cee95cea11a72703203502ff4e67527e Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Fri, 27 Mar 2020 15:53:24 +0100 Subject: [PATCH 6/6] Add new method to DiscussionRenamedBlueprint --- .../Blueprint/DiscussionRenamedBlueprint.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/framework/core/src/Notification/Blueprint/DiscussionRenamedBlueprint.php b/framework/core/src/Notification/Blueprint/DiscussionRenamedBlueprint.php index 181d298ad..7e569490a 100644 --- a/framework/core/src/Notification/Blueprint/DiscussionRenamedBlueprint.php +++ b/framework/core/src/Notification/Blueprint/DiscussionRenamedBlueprint.php @@ -51,6 +51,19 @@ class DiscussionRenamedBlueprint implements BlueprintInterface return ['postNumber' => (int) $this->post->number]; } + /** + * {@inheritdoc} + */ + public function getAttributes(): array + { + return [ + 'type' => static::getType(), + 'from_user_id' => $this->post->user ? $this->post->user->id : null, + 'subject_id' => $this->post->discussion ? $this->post->discussion->id : null, + 'data' => json_encode(['postNumber' => (int) $this->post->number]), + ]; + } + /** * {@inheritdoc} */