From 1ecf0397bbb9929c2c27824df7ee96b9bc128578 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Mudr=C3=A1k?= <david@moodle.com>
Date: Tue, 11 Jun 2019 09:45:21 +0200
Subject: [PATCH] MDL-65888 forum: Do not send notifications to unconfirmed
 users

Under certain conditions, users marked as not confirmed yet may be
returned as forum notification recipients. Such users are considered as
inactive though and the delivery adhoc task cannot be run as these
users, throwing an exception.

The solution is to make sure that fetch_subscribed_users() does not
include unconfirmed users, similarly to what was done for suspended
users and nologin users.

The added unit test also checks for deleted users. But these should
never be returned as subscribed because we consider enrolled users only
and deleted users are filtered out implicitly.
---
 mod/forum/classes/subscriptions.php |  6 +++---
 mod/forum/tests/mail_test.php       | 21 +++++++++++++++++++--
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/mod/forum/classes/subscriptions.php b/mod/forum/classes/subscriptions.php
index 74181b6e982..75583df3e2e 100644
--- a/mod/forum/classes/subscriptions.php
+++ b/mod/forum/classes/subscriptions.php
@@ -272,7 +272,7 @@ class subscriptions {
         $sql = "SELECT $fields
                 FROM {user} u
                 JOIN ($esql) je ON je.id = u.id
-               WHERE u.auth <> 'nologin' AND u.suspended = 0
+               WHERE u.auth <> 'nologin' AND u.suspended = 0 AND u.confirmed = 1
             ORDER BY $sort";
 
         return $DB->get_records_sql($sql, $params);
@@ -443,7 +443,7 @@ class subscriptions {
                         ) subscriptions
                         JOIN {user} u ON u.id = subscriptions.userid
                         JOIN ($esql) je ON je.id = u.id
-                        WHERE u.auth <> 'nologin' AND u.suspended = 0
+                        WHERE u.auth <> 'nologin' AND u.suspended = 0 AND u.confirmed = 1
                         ORDER BY u.email ASC";
 
             } else {
@@ -452,7 +452,7 @@ class subscriptions {
                         JOIN ($esql) je ON je.id = u.id
                         JOIN {forum_subscriptions} s ON s.userid = u.id
                         WHERE
-                          s.forum = :forumid AND u.auth <> 'nologin' AND u.suspended = 0
+                          s.forum = :forumid AND u.auth <> 'nologin' AND u.suspended = 0 AND u.confirmed = 1
                         ORDER BY u.email ASC";
             }
             $results = $DB->get_records_sql($sql, $params);
diff --git a/mod/forum/tests/mail_test.php b/mod/forum/tests/mail_test.php
index 4b1654ce868..7688acbf1df 100644
--- a/mod/forum/tests/mail_test.php
+++ b/mod/forum/tests/mail_test.php
@@ -137,6 +137,7 @@ class mod_forum_mail_testcase extends advanced_testcase {
     }
 
     public function test_forced_subscription() {
+        global $DB;
         $this->resetAfterTest(true);
 
         // Create a course, with a forum.
@@ -145,8 +146,14 @@ class mod_forum_mail_testcase extends advanced_testcase {
         $options = array('course' => $course->id, 'forcesubscribe' => FORUM_FORCESUBSCRIBE);
         $forum = $this->getDataGenerator()->create_module('forum', $options);
 
-        // Create two users enrolled in the course as students.
-        list($author, $recipient) = $this->helper_create_users($course, 2);
+        // Create users enrolled in the course as students.
+        list($author, $recipient, $unconfirmed, $deleted) = $this->helper_create_users($course, 4);
+
+        // Make the third user unconfirmed (thence inactive) to make sure it does not break the notifications.
+        $DB->set_field('user', 'confirmed', 0, ['id' => $unconfirmed->id]);
+
+        // Mark the fourth user as deleted to make sure it does not break the notifications.
+        $DB->set_field('user', 'deleted', 1, ['id' => $deleted->id]);
 
         // Post a discussion to the forum.
         list($discussion, $post) = $this->helper_post_to_forum($forum, $author);
@@ -160,11 +167,21 @@ class mod_forum_mail_testcase extends advanced_testcase {
                 'userid' => $recipient->id,
                 'messages' => 1,
             ],
+            (object) [
+                'userid' => $unconfirmed->id,
+                'messages' => 0,
+            ],
+            (object) [
+                'userid' => $deleted->id,
+                'messages' => 0,
+            ],
         ];
         $this->queue_tasks_and_assert($expect);
 
         $this->send_notifications_and_assert($author, [$post]);
         $this->send_notifications_and_assert($recipient, [$post]);
+        $this->send_notifications_and_assert($unconfirmed, []);
+        $this->send_notifications_and_assert($deleted, []);
     }
 
     /**