From 996534fbead8cf3787926e83ea10a40cf8348334 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Fri, 25 Mar 2022 16:48:00 +0800 Subject: [PATCH 1/4] MDL-74321 mod_forum: forum_check_throttling performance improvements * Return early if the forum's blockafter or blockperiod attributes are empty. * If $cm is not passed in forum_check_throttling(), try to fetch it using get_fast_modinfo() which avoids DB reads. Fetch it via get_coursemodule_from_instance() as a last resort (though it's unlikely to happen). --- mod/forum/lib.php | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/mod/forum/lib.php b/mod/forum/lib.php index ec24c8a800d..c3d54972101 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -5045,10 +5045,6 @@ function forum_check_throttling($forum, $cm = null) { return false; // This is broken. } - if (!$cm) { - $cm = get_coursemodule_from_instance('forum', $forum->id, $forum->course, false, MUST_EXIST); - } - if (empty($forum->blockafter)) { return false; } @@ -5057,6 +5053,22 @@ function forum_check_throttling($forum, $cm = null) { return false; } + if (!$cm) { + // Try to fetch the $cm object via get_fast_modinfo() so we don't incur DB reads. + $modinfo = get_fast_modinfo($forum->course); + $forumcms = $modinfo->get_instances_of('forum'); + foreach ($forumcms as $tmpcm) { + if ($tmpcm->instance == $forum->id) { + $cm = $tmpcm; + break; + } + } + // Last resort. Try to fetch via get_coursemodule_from_instance(). + if (!$cm) { + $cm = get_coursemodule_from_instance('forum', $forum->id, $forum->course, false, MUST_EXIST); + } + } + $modcontext = context_module::instance($cm->id); if (has_capability('mod/forum:postwithoutthrottling', $modcontext)) { return false; From 0a77eb4aced63337eb9c23064d0a3cad2de70b8e Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Fri, 25 Mar 2022 16:49:21 +0800 Subject: [PATCH 2/4] MDL-74321 mod_forum: Pass course module to forum_check_throttling() --- mod/forum/classes/local/managers/capability.php | 5 +++-- mod/forum/classes/local/renderers/discussion.php | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/mod/forum/classes/local/managers/capability.php b/mod/forum/classes/local/managers/capability.php index 5e440fb06db..76e347f1175 100644 --- a/mod/forum/classes/local/managers/capability.php +++ b/mod/forum/classes/local/managers/capability.php @@ -120,7 +120,8 @@ class capability { } // If the user reaches the number of posts equal to warning/blocking setting then return the value of canpost in $warningobj. - if ($warningobj = forum_check_throttling($this->forumrecord)) { + $cmrecord = $this->forum->get_course_module_record(); + if ($warningobj = forum_check_throttling($this->forumrecord, $cmrecord)) { return $warningobj->canpost; } @@ -330,7 +331,7 @@ class capability { $status = forum_user_can_post($forumrecord, $discussionrecord, $user, $coursemodule, $course, $context); // If the user reaches the number of posts equal to warning/blocking setting then logically and canpost value with $status. - if ($warningobj = forum_check_throttling($forumrecord)) { + if ($warningobj = forum_check_throttling($forumrecord, $coursemodule)) { return $status && $warningobj->canpost; } return $status; diff --git a/mod/forum/classes/local/renderers/discussion.php b/mod/forum/classes/local/renderers/discussion.php index 2ed95e04bec..ccf984fd421 100644 --- a/mod/forum/classes/local/renderers/discussion.php +++ b/mod/forum/classes/local/renderers/discussion.php @@ -236,7 +236,8 @@ class discussion { } $exporteddiscussion['throttlingwarningmsg'] = ''; - if (($warningobj = forum_check_throttling($this->forumrecord)) && $warningobj->canpost) { + $cmrecord = $this->forum->get_course_module_record(); + if (($warningobj = forum_check_throttling($this->forumrecord, $cmrecord)) && $warningobj->canpost) { $throttlewarnnotification = (new notification( get_string($warningobj->errorcode, $warningobj->module, $warningobj->additional) ))->set_show_closebutton(); From 86f97c625e8bb9d583f52f61b384f44ce3fdacd0 Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Mon, 28 Mar 2022 13:08:47 +0800 Subject: [PATCH 3/4] MDL-74321 mod_forum: Forum_check_throttling improvements and fixes * Fetch only the fields required by the function. * Quick validation of the passed forum parameter. If the forum parameter is an object, it should have the forum ID and the course ID. * Default return when there's no need to show a warning yet. --- mod/forum/lib.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/mod/forum/lib.php b/mod/forum/lib.php index c3d54972101..8bbdcfe0644 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -5038,11 +5038,16 @@ function forum_check_throttling($forum, $cm = null) { global $CFG, $DB, $USER; if (is_numeric($forum)) { - $forum = $DB->get_record('forum', array('id' => $forum), '*', MUST_EXIST); + $forum = $DB->get_record('forum', ['id' => $forum], 'id, course, blockperiod, blockafter, warnafter', MUST_EXIST); } - if (!is_object($forum)) { - return false; // This is broken. + if (!is_object($forum) || !isset($forum->id) || !isset($forum->course)) { + // The passed forum parameter is invalid. This can happen if: + // - a non-object and non-numeric forum is passed; or + // - the forum object does not have an ID or course attributes. + // This is unlikely to happen with properly formed forum record fetched from the database, + // so it's most likely a dev error if we hit such this case. + throw new coding_exception('Invalid forum parameter passed'); } if (empty($forum->blockafter)) { @@ -5108,6 +5113,9 @@ function forum_check_throttling($forum, $cm = null) { return $warning; } + + // No warning needs to be shown yet. + return false; } /** From 1a6bf2e5ffc790a456f4b586a6de8847e7af377c Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Mon, 28 Mar 2022 13:09:34 +0800 Subject: [PATCH 4/4] MDL-74321 mod_forum: Add forum_check_throttling unit tests --- mod/forum/tests/lib_test.php | 178 +++++++++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/mod/forum/tests/lib_test.php b/mod/forum/tests/lib_test.php index 60545e10566..1c13e3711c1 100644 --- a/mod/forum/tests/lib_test.php +++ b/mod/forum/tests/lib_test.php @@ -16,6 +16,8 @@ namespace mod_forum; +use mod_forum_generator; + defined('MOODLE_INTERNAL') || die(); global $CFG; @@ -4083,4 +4085,180 @@ class lib_test extends \advanced_testcase { $this->assertEquals($expectednormal, forum_get_layout_modes(false)); $this->assertEquals($expectedexperimental, forum_get_layout_modes(true)); } + + /** + * Provides data for tests that cause forum_check_throttling to return early. + * + * @return array + */ + public function forum_check_throttling_early_returns_provider() { + return [ + 'Empty blockafter' => [(object)['id' => 1, 'course' => SITEID, 'blockafter' => 0]], + 'Empty blockperiod' => [(object)['id' => 1, 'course' => SITEID, 'blockafter' => DAYSECS, 'blockperiod' => 0]], + ]; + } + + /** + * Tests the early return scenarios of forum_check_throttling. + * + * @dataProvider forum_check_throttling_early_returns_provider + * @covers ::forum_check_throttling + * @param \stdClass $forum The forum data. + */ + public function test_forum_check_throttling_early_returns(\stdClass $forum) { + $this->assertFalse(forum_check_throttling($forum)); + } + + /** + * Provides data for tests that cause forum_check_throttling to throw exceptions early. + * + * @return array + */ + public function forum_check_throttling_early_exceptions_provider() { + return [ + 'Non-object forum' => ['a'], + 'Forum ID not set' => [(object)['id' => false]], + 'Course ID not set' => [(object)['id' => 1]], + ]; + } + + /** + * Tests the early exception scenarios of forum_check_throttling. + * + * @dataProvider forum_check_throttling_early_exceptions_provider + * @covers ::forum_check_throttling + * @param mixed $forum The forum data. + */ + public function test_forum_check_throttling_early_exceptions($forum) { + $this->expectException(\coding_exception::class); + $this->assertFalse(forum_check_throttling($forum)); + } + + /** + * Tests forum_check_throttling when a non-existent numeric ID is passed for its forum parameter. + * + * @covers ::forum_check_throttling + */ + public function test_forum_check_throttling_nonexistent_numeric_id() { + $this->resetAfterTest(); + + $this->expectException(\moodle_exception::class); + forum_check_throttling(1); + } + + /** + * Tests forum_check_throttling when a non-existent forum record is passed for its forum parameter. + * + * @covers ::forum_check_throttling + */ + public function test_forum_check_throttling_nonexistent_forum_cm() { + $this->resetAfterTest(); + + $dummyforum = (object)[ + 'id' => 1, + 'course' => SITEID, + 'blockafter' => 2, + 'blockperiod' => DAYSECS, + ]; + $this->expectException(\moodle_exception::class); + forum_check_throttling($dummyforum); + } + + /** + * Tests forum_check_throttling when a user with the 'mod/forum:postwithoutthrottling' capability. + * + * @covers ::forum_check_throttling + */ + public function test_forum_check_throttling_teacher() { + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $teacher = $generator->create_and_enrol($course, 'teacher'); + + /** @var mod_forum_generator $forumgenerator */ + $forumgenerator = $generator->get_plugin_generator('mod_forum'); + // Forum that limits students from creating more than two posts per day. + $forum = $forumgenerator->create_instance( + [ + 'course' => $course->id, + 'blockafter' => 2, + 'blockperiod' => DAYSECS, + ] + ); + + $this->setUser($teacher); + $discussionrecord = [ + 'course' => $course->id, + 'forum' => $forum->id, + 'userid' => $teacher->id, + ]; + $discussion = $forumgenerator->create_discussion($discussionrecord); + // Create a forum post as the teacher. + $postrecord = [ + 'userid' => $teacher->id, + 'discussion' => $discussion->id, + ]; + $forumgenerator->create_post($postrecord); + // Create another forum post. + $forumgenerator->create_post($postrecord); + + $this->assertFalse(forum_check_throttling($forum)); + } + + /** + * Tests forum_check_throttling for students. + * + * @covers ::forum_check_throttling + */ + public function test_forum_check_throttling_student() { + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $student = $generator->create_and_enrol($course, 'student'); + + /** @var mod_forum_generator $forumgenerator */ + $forumgenerator = $generator->get_plugin_generator('mod_forum'); + // Forum that limits students from creating more than two posts per day. + $forum = $forumgenerator->create_instance( + [ + 'course' => $course->id, + 'blockafter' => 2, + 'blockperiod' => DAYSECS, + 'warnafter' => 1, + ] + ); + + $this->setUser($student); + + // Student hasn't posted yet so no warning will be shown. + $throttling = forum_check_throttling($forum); + $this->assertFalse($throttling); + + // Create a discussion. + $discussionrecord = [ + 'course' => $course->id, + 'forum' => $forum->id, + 'userid' => $student->id, + ]; + $discussion = $forumgenerator->create_discussion($discussionrecord); + + // A warning will be shown to the student, but they should still be able to post. + $throttling = forum_check_throttling($forum); + $this->assertIsObject($throttling); + $this->assertTrue($throttling->canpost); + + // Create another forum post as the student. + $postrecord = [ + 'userid' => $student->id, + 'discussion' => $discussion->id, + ]; + $forumgenerator->create_post($postrecord); + + // Student should now be unable to post after their second post. + $throttling = forum_check_throttling($forum); + $this->assertIsObject($throttling); + $this->assertFalse($throttling->canpost); + } }