From 6ee711fb6caba157b06f63f42e35ce848bc93eba Mon Sep 17 00:00:00 2001 From: waleedhassan Date: Mon, 17 Jun 2024 11:44:41 +0100 Subject: [PATCH] MDL-80484 mod_forum : Add a check for an empty email address Added a check for empty email address so that if the adhoc task for sending email notifcation to users for forum update fails because of empty email address an exception is not thrown as the exception was causing the the task for user with empty email address to keep requeuing again and again after failing. Wrote two test cases for testing if the fix has now prevented the task for empty email address to be requeued after failing and to test that if its still requeuing for other cases when the adhoc task fails for some other reason in this cause we are testing for bounce threshold. --- .../classes/task/send_user_notifications.php | 10 +- .../task/send_user_notifications_test.php | 176 ++++++++++++++++++ 2 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 mod/forum/tests/task/send_user_notifications_test.php diff --git a/mod/forum/classes/task/send_user_notifications.php b/mod/forum/classes/task/send_user_notifications.php index 1190f66d1fc..d3a140ad08d 100644 --- a/mod/forum/classes/task/send_user_notifications.php +++ b/mod/forum/classes/task/send_user_notifications.php @@ -184,9 +184,15 @@ class send_user_notifications extends \core\task\adhoc_task { } } - if ($errorcount > 0 and $sentcount === 0) { + if ($errorcount > 0 && $sentcount === 0) { // All messages errored. So fail. - throw new \moodle_exception('Error sending posts.'); + // Checking if the task failed because of empty email address so that it doesn't get rescheduled. + if (!empty($this->recipient->email)) { + throw new \moodle_exception('Error sending posts.'); + } else { + mtrace("Failed to send emails for the user with ID ". + $this->recipient->id ." due to an empty email address. Skipping re-queuing of the task."); + } } else if ($errorcount > 0) { // Requeue failed messages as a new task. $task = new send_user_notifications(); diff --git a/mod/forum/tests/task/send_user_notifications_test.php b/mod/forum/tests/task/send_user_notifications_test.php new file mode 100644 index 00000000000..8e6b385402f --- /dev/null +++ b/mod/forum/tests/task/send_user_notifications_test.php @@ -0,0 +1,176 @@ +. + +namespace mod_forum\task; + +use Exception; +use InvalidArgumentException; +use RuntimeException; + +/** + * Unit tests for the send_user_notifications task in the forum module. + * + * This class contains test cases to ensure that the forum module's + * send_user_notifications task functions as expected, particularly + * when handling email notifications to users after forum posts. + * + * It tests different scenarios related to user email configurations, + * such as when a user has an empty email address, when a user has exceeded + * the bounce threshold, and how the system behaves when posts are attempted + * to be sent under these conditions. + * + * Each test verifies that the appropriate exceptions are thrown, that + * messages are correctly sent (or skipped), and that the task requeues + * appropriately based on the user's email settings and other related conditions. + * + * @package mod_forum + * @copyright 2024 Waleed ul hassan + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +final class send_user_notifications_test extends \advanced_testcase { + /** + * Testcase to check send notification for post via email + * + * @covers \mod_forum\task\send_user_notifications + * @dataProvider send_user_notifications_cases + * @param array $userdata Test user for the case. + * @param string $expectedstring Expected string during the test case. + * @param array $expecteddebuggingstrings Expected debugging strings array. + * @param bool $expectedassertion Expected adhoc task to be re queued or not. + * @param array $userpreferences (optional) User preferences for the test case. + * @throws InvalidArgumentException If the user data is invalid. + * @throws RuntimeException If the notification fails to send. + * @throws Exception For any other general errors. + */ + public function test_send_user_notifications( + array $userdata, + string $expectedstring, + array $expecteddebuggingstrings, + bool $expectedassertion, + array $userpreferences = [], + ): void { + global $CFG; + require_once($CFG->dirroot . '/mod/forum/lib.php'); + $CFG->handlebounces = true; + $this->resetAfterTest(true); + $this->preventResetByRollback(); + $this->redirectEmails(); + + // Creating a user. + $user = $this->getDataGenerator()->create_user($userdata); + // Set user preferences. + foreach ($userpreferences as $name => $value) { + set_user_preference($name, $value, $user); + } + + // Create a course and a forum. + $course = $this->getDataGenerator()->create_course(); + $forum = $this->getDataGenerator()->create_module('forum', [ + 'course' => $course->id, + 'forcesubscribe' => \FORUM_FORCESUBSCRIBE, + ]); + + // Create a discussion in the forum. + $discussion = $this->getDataGenerator()->get_plugin_generator('mod_forum')->create_discussion([ + 'course' => $course->id, + 'forum' => $forum->id, + 'userid' => $user->id, + 'message' => 'Test discussion', + ]); + // Create a post in the discussion. + $post = $this->getDataGenerator()->get_plugin_generator('mod_forum')->create_post([ + 'course' => $course->id, + 'discussion' => $discussion->id, + 'userid' => $user->id, + 'message' => 'Test post', + ]); + + // Setting placeholders for user id and post id. + $expectedstring = sprintf($expectedstring, $user->id, $post->id, $user->id); + $expecteddebuggingstrings = array_map(function($expecteddebuggingstring) use ($user) { + return sprintf($expecteddebuggingstring, $user->id, $user->firstname . " " . $user->lastname); + }, $expecteddebuggingstrings); + + // Enroll the user in the course. + $this->getDataGenerator()->enrol_user($user->id, $course->id); + + // Trigger the send_user_notifications task. + $task = new send_user_notifications(); + $task->set_userid($user->id); + $task->set_custom_data($post->id); + $this->expectOutputString($expectedstring); + + // Testing if an exception is thrown because the task is re queued if an exception is thrown in the adhoc task. + $expectedexception = 'Error sending posts.'; + try { + $task->execute(); + } catch (\Exception $ex) { + $this->assertEquals($expectedexception, $ex->errorcode); + } + if (count($expecteddebuggingstrings)) { + $this->assertdebuggingcalledcount(count($expecteddebuggingstrings), $expecteddebuggingstrings); + } + } + /** + * Data provider for test cases related to sending user notifications. + * + * This data provider generates various test cases for the `test_send_user_notifications` function. + * Each test case consists of a user configuration, expected output strings, debugging messages, and assertions. + * + * @return array[] Array of test cases. + */ + public static function send_user_notifications_cases(): array { + + return [ + [ + // Create a user with an empty email address. + [ + 'email' => '', + 'username' => 'testuser', + ], + "Sending messages to testuser (%d)\n" . + " Failed to send post %d\n" . + "Sent 0 messages with 1 failures\n" . + "Failed to send emails for the user with ID %d" . + " due to an empty email address. Skipping re-queuing of the task.\n", + [ + "Can not send email to user without email: %d", + "Error calling message processor email", + ], + false, + ], + [ + // Create a user with bounce threshold. + [ + 'email' => 'bounce@example.com', + 'username' => 'bounceuser', + ], + "Sending messages to bounceuser (%d)\n" . + " Failed to send post %d\n" . + "Sent 0 messages with 1 failures\n", + [ + "email_to_user: User %d (%s) is over bounce threshold! Not sending.", + "Error calling message processor email", + ], + true, + [ + 'email_bounce_count' => 20, + 'email_send_count' => 20, + ], + ], + ]; + } +}