From 7b7f44e0ab21734d2d575b9c549348f49af0a23c Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Thu, 18 Jan 2024 11:04:42 +0100 Subject: [PATCH 01/12] first attempt to fix bug --- classes/task/send_daily_mail.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/classes/task/send_daily_mail.php b/classes/task/send_daily_mail.php index 7c2fa1cda9..3f4913a961 100644 --- a/classes/task/send_daily_mail.php +++ b/classes/task/send_daily_mail.php @@ -57,6 +57,11 @@ public function execute() { // Fill the $mail array. foreach ($userdata as $row) { $currentcourse = $DB->get_record('course', array('id' => $row->courseid), 'fullname, id'); + // Check if the user is enrolled in the course, if not, go to the next row. + if (!is_enrolled(\context_course::instance($row->courseid), $user->userid, '')) { + continue; + } + $currentforum = $DB->get_record('moodleoverflow', array('id' => $row->forumid), 'name, id'); $coursemoduleid = get_coursemodule_from_instance('moodleoverflow', $row->forumid); $discussion = $DB->get_record('moodleoverflow_discussions', array('id' => $row->forumdiscussionid), 'name, id'); From 2eeb0f7ebf2747f33f4f458070094e9785fb9fdf Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Mon, 22 Jan 2024 16:41:01 +0100 Subject: [PATCH 02/12] add sequence=false in install.xml --- classes/task/send_daily_mail.php | 2 +- db/install.xml | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/classes/task/send_daily_mail.php b/classes/task/send_daily_mail.php index 3f4913a961..67e6c2eee1 100644 --- a/classes/task/send_daily_mail.php +++ b/classes/task/send_daily_mail.php @@ -82,7 +82,7 @@ public function execute() { 'linktoforum' => $linktoforum, 'linktodiscussion' => $linktodiscussion, 'unreadposts' => $unreadposts)); - array_push($mail, $string); + $mail[] = $string; } // Build the final message and send it to user. Then remove the sent records. $message = implode('
', $mail); diff --git a/db/install.xml b/db/install.xml index e4651fdc18..f7f28d27fb 100644 --- a/db/install.xml +++ b/db/install.xml @@ -179,11 +179,11 @@ - - - - - + + + + + From b606320a375e1595bd8b4611c0ffc68350a58a84 Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 24 Jan 2024 11:19:26 +0100 Subject: [PATCH 03/12] extra check in enrolled check --- classes/task/send_daily_mail.php | 2 +- locallib.php | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/classes/task/send_daily_mail.php b/classes/task/send_daily_mail.php index 67e6c2eee1..7dfbe02c98 100644 --- a/classes/task/send_daily_mail.php +++ b/classes/task/send_daily_mail.php @@ -58,7 +58,7 @@ public function execute() { foreach ($userdata as $row) { $currentcourse = $DB->get_record('course', array('id' => $row->courseid), 'fullname, id'); // Check if the user is enrolled in the course, if not, go to the next row. - if (!is_enrolled(\context_course::instance($row->courseid), $user->userid, '')) { + if (!is_enrolled(\context_course::instance($row->courseid), $user->userid, '', true)) { continue; } diff --git a/locallib.php b/locallib.php index 30e751657d..1141753535 100644 --- a/locallib.php +++ b/locallib.php @@ -928,12 +928,7 @@ function moodleoverflow_print_discussion($course, $cm, $moodleoverflow, $discuss // Retrieve all posts of the discussion. $posts = moodleoverflow_get_all_discussion_posts($discussion->id, $istracked, $modulecontext); - /*$newpost = []; - foreach($posts as $posti) { - $newpost[] = $posti->message; - } - var_dump($newpost); - */$usermapping = anonymous::get_userid_mapping($moodleoverflow, $discussion->id); + $usermapping = anonymous::get_userid_mapping($moodleoverflow, $discussion->id); // Start with the parent post. $post = $posts[$post->id]; From f459edcd327ab53aa3cd36df5dd355746171f7d9 Mon Sep 17 00:00:00 2001 From: Nina Herrmann Date: Tue, 19 Mar 2024 10:10:24 +0100 Subject: [PATCH 04/12] Update send_daily_mail.php Forgot bracket :disappointed: --- classes/task/send_daily_mail.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/classes/task/send_daily_mail.php b/classes/task/send_daily_mail.php index 1d2723cdde..75f110ab3a 100644 --- a/classes/task/send_daily_mail.php +++ b/classes/task/send_daily_mail.php @@ -81,7 +81,7 @@ public function execute() { $string = get_string('digestunreadpost', 'mod_moodleoverflow', ['linktocourse' => $linktocourse, 'linktoforum' => $linktoforum, 'linktodiscussion' => $linktodiscussion, - 'unreadposts' => $unreadposts)); + 'unreadposts' => $unreadposts, ]); $mail[] = $string; } // Build the final message and send it to user. Then remove the sent records. From 47c336cc72311b6a8c44e3d5d42eb6cd04d7b080 Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 14:31:25 +0100 Subject: [PATCH 05/12] add phpunit test for bugfix --- tests/dailymail_test.php | 53 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 5 deletions(-) diff --git a/tests/dailymail_test.php b/tests/dailymail_test.php index 922050e033..9be1c336e6 100644 --- a/tests/dailymail_test.php +++ b/tests/dailymail_test.php @@ -59,6 +59,9 @@ class dailymail_test extends \advanced_testcase { /** @var \stdClass discussion instance */ private $discussion; + /** @var moodleoverflow generator */ + private $generator; + /** * Test setUp. */ @@ -94,12 +97,13 @@ public function tearDown(): void { */ public function helper_create_user_and_discussion($maildigest) { // Create a user enrolled in the course as student. - $this->user = $this->getDataGenerator()->create_user(['firstname' => 'Tamaro', 'maildigest' => $maildigest]); + $this->user = $this->getDataGenerator()->create_user(['firstname' => 'Tamaro', 'email' => 'tamaromail@example.com', + 'maildigest' => $maildigest]); $this->getDataGenerator()->enrol_user($this->user->id, $this->course->id, 'student'); // Create a new discussion and post within the moodleoverflow. - $generator = $this->getDataGenerator()->get_plugin_generator('mod_moodleoverflow'); - $this->discussion = $generator->post_to_forum($this->moodleoverflow, $this->user); + $this->generator = $this->getDataGenerator()->get_plugin_generator('mod_moodleoverflow'); + $this->discussion = $this->generator->post_to_forum($this->moodleoverflow, $this->user); } /** @@ -136,8 +140,7 @@ private function helper_run_send_mails() { * Test if the task send_daily_mail sends a mail to the user. * @covers \send_daily_mail::execute */ - public function test_mail_delivery() { - + public function test_mail_delivery(): void { // Create user with maildigest = on. $this->helper_create_user_and_discussion('1'); @@ -149,6 +152,46 @@ public function test_mail_delivery() { $this->assertEquals(1, $messages); } + /** + * Test if the task send_daily_mail does not sends email from posts that are not in the course of the user. + * @return void + */ + public function test_delivery_not_enrolled(): void { + // Create user with maildigest = on. + $this->helper_create_user_and_discussion('1'); + + // Create another user, course and a moodleoverflow post. + $course = $this->getDataGenerator()->create_course(); + $location = ['course' => $course->id, 'forcesubscribe' => MOODLEOVERFLOW_FORCESUBSCRIBE]; + $moodleoverflow = $this->getDataGenerator()->create_module('moodleoverflow', $location); + $coursemodule = get_coursemodule_from_instance('moodleoverflow', $moodleoverflow->id); + $student = $this->getDataGenerator()->create_user(['firstname' => 'Ethan', 'email' => 'ethanmail@example.com', + 'maildigest' => '1']); + $this->getDataGenerator()->enrol_user($student->id, $course->id, 'teacher'); + $discussion = $this->generator->post_to_forum($moodleoverflow, $student); + + // Send the mails. + $this->helper_run_send_mails(); + $this->helper_run_send_daily_mail(); + $messages = $this->sink->count(); + $content = $this->sink->get_messages(); + var_dump($content); + // There should be 2 mails. + $this->assertEquals(2, $messages); + + // Check the recipient of the mails and the discussion that is addressed. There should be false addressed mails. + $firstmail = $content[0]; + $secondmail = $content[1]; + if ($firstmail->to == "tamaromail@example.com") { + $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); + $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); + } else { + $this->assertEquals('petermail@example.com', $secondmail->to); + $this->assertStringContainsString($discussion[0]->name, $secondmail->body); + $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); + } + } + /** * Test if the content of the mail matches the supposed content. From 3b284880fe7c71001243045b92c65a4faa959fcd Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 14:35:26 +0100 Subject: [PATCH 06/12] delete unused commands --- tests/dailymail_test.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/dailymail_test.php b/tests/dailymail_test.php index 9be1c336e6..50680e29b3 100644 --- a/tests/dailymail_test.php +++ b/tests/dailymail_test.php @@ -164,7 +164,6 @@ public function test_delivery_not_enrolled(): void { $course = $this->getDataGenerator()->create_course(); $location = ['course' => $course->id, 'forcesubscribe' => MOODLEOVERFLOW_FORCESUBSCRIBE]; $moodleoverflow = $this->getDataGenerator()->create_module('moodleoverflow', $location); - $coursemodule = get_coursemodule_from_instance('moodleoverflow', $moodleoverflow->id); $student = $this->getDataGenerator()->create_user(['firstname' => 'Ethan', 'email' => 'ethanmail@example.com', 'maildigest' => '1']); $this->getDataGenerator()->enrol_user($student->id, $course->id, 'teacher'); @@ -186,7 +185,7 @@ public function test_delivery_not_enrolled(): void { $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); } else { - $this->assertEquals('petermail@example.com', $secondmail->to); + $this->assertEquals('ethanmail@example.com', $secondmail->to); $this->assertStringContainsString($discussion[0]->name, $secondmail->body); $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); } From f94d028c02430b3ec85dfac5c9b9e9c2de914472 Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 14:41:14 +0100 Subject: [PATCH 07/12] delete var_dump --- tests/dailymail_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dailymail_test.php b/tests/dailymail_test.php index 50680e29b3..9cfe81d46d 100644 --- a/tests/dailymail_test.php +++ b/tests/dailymail_test.php @@ -174,7 +174,7 @@ public function test_delivery_not_enrolled(): void { $this->helper_run_send_daily_mail(); $messages = $this->sink->count(); $content = $this->sink->get_messages(); - var_dump($content); + // There should be 2 mails. $this->assertEquals(2, $messages); From 81efb5392c1d77fb0678e2fc3817bb45868d468b Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 15:34:48 +0100 Subject: [PATCH 08/12] remove if statement --- tests/dailymail_test.php | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tests/dailymail_test.php b/tests/dailymail_test.php index 9cfe81d46d..df50fbbddb 100644 --- a/tests/dailymail_test.php +++ b/tests/dailymail_test.php @@ -178,17 +178,16 @@ public function test_delivery_not_enrolled(): void { // There should be 2 mails. $this->assertEquals(2, $messages); - // Check the recipient of the mails and the discussion that is addressed. There should be false addressed mails. + // Check the recipient of the mails and the discussion that is addressed. There should be no false addressed discussions. $firstmail = $content[0]; $secondmail = $content[1]; - if ($firstmail->to == "tamaromail@example.com") { - $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); - $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); - } else { - $this->assertEquals('ethanmail@example.com', $secondmail->to); - $this->assertStringContainsString($discussion[0]->name, $secondmail->body); - $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); - } + $this->assertEquals('tamaromail@example.com', $firstmail->to); + $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); + $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); + + $this->assertEquals('ethanmail@example.com', $secondmail->to); + $this->assertStringContainsString($discussion[0]->name, $secondmail->body); + $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); } From f4d67aa84879a6b5b00e0400c33aadad6b3c9ae8 Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 15:59:56 +0100 Subject: [PATCH 09/12] adapt ci to M404 --- .github/workflows/moodle-ci.yml | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/.github/workflows/moodle-ci.yml b/.github/workflows/moodle-ci.yml index c020ab22d4..60efa86508 100644 --- a/.github/workflows/moodle-ci.yml +++ b/.github/workflows/moodle-ci.yml @@ -8,8 +8,8 @@ jobs: strategy: matrix: - php: ['8.1'] - moodle-branch: ['MOODLE_402_STABLE'] + php: ['8.2'] + moodle-branch: ['MOODLE_403_STABLE'] database: ['pgsql'] steps: @@ -110,27 +110,21 @@ jobs: strategy: fail-fast: false matrix: - php: ['8.0', '8.1'] - moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE'] + php: [8.1'] + moodle-branch: ['MOODLE_401_STABLE', 'MOODLE_402_STABLE', 'MOODLE_403_STABLE'] database: ['mariadb', 'pgsql'] include: - - php: '7.4' - moodle-branch: 'MOODLE_39_STABLE' + - php: '8.2' + moodle-branch: 'MOODLE_402_STABLE' database: 'mariadb' - - php: '7.4' - moodle-branch: 'MOODLE_39_STABLE' + - php: '8.2' + moodle-branch: 'MOODLE_402_STABLE' database: 'pgsql' - - php: '8.0' - moodle-branch: 'MOODLE_311_STABLE' + - php: '8.2' + moodle-branch: 'MOODLE_403_STABLE' database: 'mariadb' - - php: '8.0' - moodle-branch: 'MOODLE_311_STABLE' - database: 'pgsql' - - php: '8.0' - moodle-branch: 'MOODLE_400_STABLE' - database: 'mariadb' - - php: '8.0' - moodle-branch: 'MOODLE_400_STABLE' + - php: '8.2' + moodle-branch: 'MOODLE_403_STABLE' database: 'pgsql' steps: @@ -193,4 +187,4 @@ jobs: - name: Behat features if: ${{ always() }} - run: moodle-plugin-ci behat --auto-rerun 0 + run: moodle-plugin-ci behat --auto-rerun 0 \ No newline at end of file From 0943f5256c41480c36bdcc84668a9347e4f501af Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Wed, 20 Mar 2024 16:08:59 +0100 Subject: [PATCH 10/12] make assertions indepent from mail order --- tests/dailymail_test.php | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/dailymail_test.php b/tests/dailymail_test.php index df50fbbddb..5fc8526ab1 100644 --- a/tests/dailymail_test.php +++ b/tests/dailymail_test.php @@ -181,13 +181,21 @@ public function test_delivery_not_enrolled(): void { // Check the recipient of the mails and the discussion that is addressed. There should be no false addressed discussions. $firstmail = $content[0]; $secondmail = $content[1]; - $this->assertEquals('tamaromail@example.com', $firstmail->to); - $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); - $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); - - $this->assertEquals('ethanmail@example.com', $secondmail->to); - $this->assertStringContainsString($discussion[0]->name, $secondmail->body); - $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); + // Depending on the order of the mails, check the recipient and the discussion that is addressed. + if ($firstmail->to == "tamaromail@example.com") { + $this->assertStringContainsString($this->discussion[0]->name, $firstmail->body); + $this->assertStringNotContainsString($discussion[0]->name, $firstmail->body); + $this->assertEquals('ethanmail@example.com', $secondmail->to); + $this->assertStringContainsString($discussion[0]->name, $secondmail->body); + $this->assertStringNotContainsString($this->discussion[0]->name, $secondmail->body); + } else { + $this->assertEquals('ethanmail@example.com', $firstmail->to); + $this->assertStringContainsString($discussion[0]->name, $firstmail->body); + $this->assertStringNotContainsString($this->discussion[0]->name, $firstmail->body); + $this->assertEquals('tamaromail@example.com', $secondmail->to); + $this->assertStringContainsString($this->discussion[0]->name, $secondmail->body); + $this->assertStringNotContainsString($discussion[0]->name, $secondmail->body); + } } From 331836b8d0765ef1cd56161c6de982cf45fa9112 Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Tue, 16 Apr 2024 16:31:53 +0200 Subject: [PATCH 11/12] fix wrong use of behat command --- tests/behat/add_moodleoverflow.feature | 2 +- tests/behat/delete_file.feature | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/behat/add_moodleoverflow.feature b/tests/behat/add_moodleoverflow.feature index 939fdd84cd..84555e63f5 100644 --- a/tests/behat/add_moodleoverflow.feature +++ b/tests/behat/add_moodleoverflow.feature @@ -19,7 +19,7 @@ Feature: Add moodleoverflow activities and discussions And I log in as "teacher1" And I am on "Course 1" course homepage And I turn editing mode on - And I add a "Moodleoverflow" to section "1" and I fill the form with: + And I add a "Moodleoverflow" activity to course "C1" section "1" and I fill the form with: | Moodleoverflow name | Test moodleoverflow name | | Description | Test forum description | And I add a new discussion to "Test moodleoverflow name" moodleoverflow with: diff --git a/tests/behat/delete_file.feature b/tests/behat/delete_file.feature index 24b9de2c3f..62fb50b7c3 100644 --- a/tests/behat/delete_file.feature +++ b/tests/behat/delete_file.feature @@ -17,7 +17,7 @@ Feature: Delete attachments And I log in as "teacher1" And I am on "Course 1" course homepage And I turn editing mode on - And I add a "Moodleoverflow" to section "1" and I fill the form with: + And I add a "Moodleoverflow" activity to course "C1" section "1" and I fill the form with: | Moodleoverflow name | Test moodleoverflow name | | Description | Test forum description | And I add a new discussion to "Test moodleoverflow name" moodleoverflow with: From e1f997ebf756ac5f9491b921bb01c0cc0214a33f Mon Sep 17 00:00:00 2001 From: TamaroWalter Date: Tue, 16 Apr 2024 17:25:06 +0200 Subject: [PATCH 12/12] change for pgsql test --- tests/behat/add_moodleoverflow.feature | 2 +- tests/behat/delete_file.feature | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/behat/add_moodleoverflow.feature b/tests/behat/add_moodleoverflow.feature index 84555e63f5..6e56d9b252 100644 --- a/tests/behat/add_moodleoverflow.feature +++ b/tests/behat/add_moodleoverflow.feature @@ -19,7 +19,7 @@ Feature: Add moodleoverflow activities and discussions And I log in as "teacher1" And I am on "Course 1" course homepage And I turn editing mode on - And I add a "Moodleoverflow" activity to course "C1" section "1" and I fill the form with: + And I add a "moodleoverflow" activity to course "C1" section "1" and I fill the form with: | Moodleoverflow name | Test moodleoverflow name | | Description | Test forum description | And I add a new discussion to "Test moodleoverflow name" moodleoverflow with: diff --git a/tests/behat/delete_file.feature b/tests/behat/delete_file.feature index 62fb50b7c3..8917ef0950 100644 --- a/tests/behat/delete_file.feature +++ b/tests/behat/delete_file.feature @@ -17,7 +17,7 @@ Feature: Delete attachments And I log in as "teacher1" And I am on "Course 1" course homepage And I turn editing mode on - And I add a "Moodleoverflow" activity to course "C1" section "1" and I fill the form with: + And I add a "moodleoverflow" activity to course "C1" section "1" and I fill the form with: | Moodleoverflow name | Test moodleoverflow name | | Description | Test forum description | And I add a new discussion to "Test moodleoverflow name" moodleoverflow with: