From 2ba29a908833c8b0efd70372f1ba7fc75425db42 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= <daniel@klabbers.email>
Date: Mon, 10 Feb 2020 11:45:35 +0100
Subject: [PATCH] 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 +++++++++++++++++++
 src/Notification/Job/SendNotificationsJob.php | 30 ++++----------
 src/Notification/NotificationSyncer.php       | 23 ++++++++++-
 3 files changed, 68 insertions(+), 24 deletions(-)
 create mode 100644 src/Notification/Job/SendEmailNotificationJob.php

diff --git a/src/Notification/Job/SendEmailNotificationJob.php b/src/Notification/Job/SendEmailNotificationJob.php
new file mode 100644
index 000000000..68bffeb17
--- /dev/null
+++ b/src/Notification/Job/SendEmailNotificationJob.php
@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Notification\Job;
+
+use Flarum\Notification\MailableInterface;
+use Flarum\Notification\NotificationMailer;
+use Flarum\Queue\AbstractJob;
+use Flarum\User\User;
+
+class SendEmailNotificationJob extends AbstractJob
+{
+    /**
+     * @var MailableInterface
+     */
+    private $blueprint;
+
+    /**
+     * @var User
+     */
+    private $recipient;
+
+    public function __construct(MailableInterface $blueprint, User $recipient)
+    {
+        $this->blueprint = $blueprint;
+        $this->recipient = $recipient;
+    }
+
+    public function handle(NotificationMailer $mailer)
+    {
+        $mailer->send($this->blueprint, $this->recipient);
+    }
+}
diff --git a/src/Notification/Job/SendNotificationsJob.php b/src/Notification/Job/SendNotificationsJob.php
index c1d329e18..31207bac7 100644
--- a/src/Notification/Job/SendNotificationsJob.php
+++ b/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/src/Notification/NotificationSyncer.php b/src/Notification/NotificationSyncer.php
index 5fd2ead77..b6b6f96e1 100644
--- a/src/Notification/NotificationSyncer.php
+++ b/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.
      *