From 4238983e2a1693220945b6684a35d15c73e30be0 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 4 Sep 2014 20:06:55 +0800 Subject: [PATCH] MDL-47107 mod_forum Optimise forum subscription checks Supply a coursemodule where relevant to the forum subscription checks. --- mod/forum/classes/observer.php | 2 +- mod/forum/classes/subscriptions.php | 14 +++++--- mod/forum/discuss.php | 2 +- mod/forum/index.php | 2 +- mod/forum/lib.php | 2 +- mod/forum/post.php | 5 +-- mod/forum/subscribe.php | 4 +-- mod/forum/subscribe_ajax.php | 2 +- mod/forum/tests/subscriptions_test.php | 49 ++++++++++++++++++++++++++ 9 files changed, 68 insertions(+), 14 deletions(-) diff --git a/mod/forum/classes/observer.php b/mod/forum/classes/observer.php index b630ab7fcb0..cc43934ef7e 100644 --- a/mod/forum/classes/observer.php +++ b/mod/forum/classes/observer.php @@ -72,7 +72,7 @@ class mod_forum_observer { require_once($CFG->dirroot . '/mod/forum/lib.php'); $userid = $event->relateduserid; - $sql = "SELECT f.id, cm.id AS cmid, f.forcesubscribe + $sql = "SELECT f.id, f.course as course, cm.id AS cmid, f.forcesubscribe FROM {forum} f JOIN {course_modules} cm ON (cm.instance = f.id) JOIN {modules} m ON (m.id = cm.module) diff --git a/mod/forum/classes/subscriptions.php b/mod/forum/classes/subscriptions.php index de4253b7a7c..79156a680b2 100644 --- a/mod/forum/classes/subscriptions.php +++ b/mod/forum/classes/subscriptions.php @@ -104,14 +104,18 @@ class subscriptions { * @param int $userid The user ID * @param \stdClass $forum The record of the forum to test * @param int $discussionid The ID of the discussion to check + * @param \cm_info $cm The coursemodule record. If not supplied, this will be calculated using get_fast_modinfo instead. * @return boolean */ - public static function is_subscribed($userid, $forum, $discussionid = null) { + public static function is_subscribed($userid, $forum, $discussionid = null, cm_info $cm = null) { // If forum is force subscribed and has allowforcesubscribe, then user is subscribed. - $cm = get_coursemodule_from_instance('forum', $forum->id); - if ($cm && self::is_forcesubscribed($forum) && - has_capability('mod/forum:allowforcesubscribe', \context_module::instance($cm->id), $userid)) { - return true; + if (self::is_forcesubscribed($forum)) { + if (!$cm) { + $cm = get_fast_modinfo($forum->course)->instances['forum'][$forum->id]; + } + if (has_capability('mod/forum:allowforcesubscribe', \context_module::instance($cm->id), $userid)) { + return true; + } } if ($discussionid === null) { diff --git a/mod/forum/discuss.php b/mod/forum/discuss.php index dcded7a05eb..eb05d32907b 100644 --- a/mod/forum/discuss.php +++ b/mod/forum/discuss.php @@ -114,7 +114,7 @@ $subscriptionchanges = array(); foreach ($potentialsubscribers as $subuser) { $userid = $subuser->id; - $targetsubscription = \mod_forum\subscriptions::is_subscribed($userid, $forumto); + $targetsubscription = \mod_forum\subscriptions::is_subscribed($userid, $forumto, null, $cmto); if (\mod_forum\subscriptions::is_subscribed($userid, $forum, $discussion->id)) { if (!$targetsubscription) { $subscriptionchanges[$userid] = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; diff --git a/mod/forum/index.php b/mod/forum/index.php index 1d4ac71b640..b06f87efd89 100644 --- a/mod/forum/index.php +++ b/mod/forum/index.php @@ -192,7 +192,7 @@ if (!is_null($subscribe)) { $cansub = false; } if (!\mod_forum\subscriptions::is_forcesubscribed($forum)) { - $subscribed = \mod_forum\subscriptions::is_subscribed($USER->id, $forum); + $subscribed = \mod_forum\subscriptions::is_subscribed($USER->id, $forum, null, $cm); $canmanageactivities = has_capability('moodle/course:manageactivities', $coursecontext, $USER->id); if (($canmanageactivities || \mod_forum\subscriptions::is_subscribable($forum)) && $subscribe && !$subscribed && $cansub) { \mod_forum\subscriptions::subscribe_user($USER->id, $forum, $modcontext, true); diff --git a/mod/forum/lib.php b/mod/forum/lib.php index 7e578f3789e..b3a63b1a099 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -615,7 +615,7 @@ function forum_cron() { continue; } - if (!\mod_forum\subscriptions::is_subscribed($userto->id, $forum, $post->discussion)) { + if (!\mod_forum\subscriptions::is_subscribed($userto->id, $forum, $post->discussion, $coursemodules[$forum->id])) { // The user does not subscribe to this forum, or to this specific discussion. continue; } diff --git a/mod/forum/post.php b/mod/forum/post.php index 516128b418b..b1bdd9ff929 100644 --- a/mod/forum/post.php +++ b/mod/forum/post.php @@ -573,7 +573,8 @@ $mform_post = new mod_forum_post_form('post.php', array('course' => $course, 'modcontext' => $modcontext, 'forum' => $forum, 'post' => $post, - 'subscribe' => \mod_forum\subscriptions::is_subscribed($USER->id, $forum), + 'subscribe' => \mod_forum\subscriptions::is_subscribed($USER->id, $forum, + null, $cm), 'thresholdwarning' => $thresholdwarning, 'edit' => $edit), 'post', '', array('id' => 'mformforum')); @@ -616,7 +617,7 @@ $currenttext = file_prepare_draft_area($draftid_editor, $modcontext->id, 'mod_fo // which case use their existing preference. $discussionsubscribe = true; if (isset($discussion) && forum_user_has_posted($forum->id, $discussion->id, $USER->id)) { - $discussionsubscribe = \mod_forum\subscriptions::is_subscribed($USER->id, $forum, $discussion->id); + $discussionsubscribe = \mod_forum\subscriptions::is_subscribed($USER->id, $forum, $discussion->id, $cm); } $mform_post->set_data(array( 'attachments'=>$draftitemid, 'general'=>$heading, diff --git a/mod/forum/subscribe.php b/mod/forum/subscribe.php index 960b4439deb..b55616ac8c1 100644 --- a/mod/forum/subscribe.php +++ b/mod/forum/subscribe.php @@ -73,7 +73,7 @@ if (isset($cm->groupmode) && empty($course->groupmodeforce)) { } else { $groupmode = $course->groupmode; } -if ($groupmode && !\mod_forum\subscriptions::is_subscribed($user->id, $forum) && !has_capability('moodle/site:accessallgroups', $context)) { +if ($groupmode && !\mod_forum\subscriptions::is_subscribed($user->id, $forum, null, $cm) && !has_capability('moodle/site:accessallgroups', $context)) { if (!groups_get_all_groups($course->id, $USER->id)) { print_error('cannotsubscribe', 'forum'); } @@ -142,7 +142,7 @@ $info = new stdClass(); $info->name = fullname($user); $info->forum = format_string($forum->name); -if (\mod_forum\subscriptions::is_subscribed($user->id, $forum, $discussionid)) { +if (\mod_forum\subscriptions::is_subscribed($user->id, $forum, $discussionid, $cm)) { if (is_null($sesskey)) { // we came here via link in email $PAGE->set_title($course->shortname); $PAGE->set_heading($course->fullname); diff --git a/mod/forum/subscribe_ajax.php b/mod/forum/subscribe_ajax.php index ef2cbc7a6ab..d2a867a3255 100644 --- a/mod/forum/subscribe_ajax.php +++ b/mod/forum/subscribe_ajax.php @@ -46,7 +46,7 @@ if (!\mod_forum\subscriptions::is_subscribable($forum)) { die; } -if (\mod_forum\subscriptions::is_subscribed($USER->id, $forum, $discussion->id)) { +if (\mod_forum\subscriptions::is_subscribed($USER->id, $forum, $discussion->id, $cm)) { // The user is subscribed, unsubscribe them. \mod_forum\subscriptions::unsubscribe_user_from_discussion($USER->id, $discussion, $context); } else { diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php index 0d358fe5973..f161bdba495 100644 --- a/mod/forum/tests/subscriptions_test.php +++ b/mod/forum/tests/subscriptions_test.php @@ -1433,4 +1433,53 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { ))); } + /** + * Test that providing a context_module instance to is_subscribed does not result in additional lookups to retrieve + * the context_module. + */ + public function test_is_subscribed_cm() { + global $DB; + + $this->resetAfterTest(true); + + // Create a course, with a forum. + $course = $this->getDataGenerator()->create_course(); + + $options = array('course' => $course->id, 'forcesubscribe' => FORUM_FORCESUBSCRIBE); + $forum = $this->getDataGenerator()->create_module('forum', $options); + + // Create a user enrolled in the course as a student. + list($user) = $this->helper_create_users($course, 1); + + // Retrieve the $cm now. + $cm = get_fast_modinfo($forum->course)->instances['forum'][$forum->id]; + + // Reset get_fast_modinfo. + get_fast_modinfo(0, 0, true); + + // Call is_subscribed without passing the $cmid - this should result in a lookup and filling of some of the + // caches. This provides us with consistent data to start from. + $this->assertTrue(\mod_forum\subscriptions::is_subscribed($user->id, $forum)); + $this->assertTrue(\mod_forum\subscriptions::is_subscribed($user->id, $forum)); + + // Make a note of the number of DB calls. + $basecount = $DB->perf_get_reads(); + + // Call is_subscribed - it should give return the correct result (False), and result in no additional queries. + $this->assertTrue(\mod_forum\subscriptions::is_subscribed($user->id, $forum, null, $cm)); + + // The capability check does require some queries, so we don't test it directly. + // We don't assert here because this is dependant upon linked code which could change at any time. + $suppliedcmcount = $DB->perf_get_reads() - $basecount; + + // Call is_subscribed without passing the $cmid now - this should result in a lookup. + get_fast_modinfo(0, 0, true); + $basecount = $DB->perf_get_reads(); + $this->assertTrue(\mod_forum\subscriptions::is_subscribed($user->id, $forum)); + $calculatedcmcount = $DB->perf_get_reads() - $basecount; + + // There should be more queries than when we performed the same check a moment ago. + $this->assertGreaterThan($suppliedcmcount, $calculatedcmcount); + } + }