From 5f155af825fe5734b5cc057adf3f4bf2205e8004 Mon Sep 17 00:00:00 2001 From: Juan Leyva Date: Mon, 20 Apr 2015 12:23:20 +0200 Subject: [PATCH] MDL-49921 forum: Handle exceptions correctly in get_forums_by_courses --- mod/forum/externallib.php | 74 +++++++++++++++++++----------------- mod/forum/tests/externallib_test.php | 44 ++++++++++----------- 2 files changed, 60 insertions(+), 58 deletions(-) diff --git a/mod/forum/externallib.php b/mod/forum/externallib.php index 322bbcc0b81..ae16d7fdeb6 100644 --- a/mod/forum/externallib.php +++ b/mod/forum/externallib.php @@ -54,7 +54,7 @@ class mod_forum_external extends external_api { * @since Moodle 2.5 */ public static function get_forums_by_courses($courseids = array()) { - global $CFG, $DB, $USER; + global $CFG; require_once($CFG->dirroot . "/mod/forum/lib.php"); @@ -72,44 +72,48 @@ class mod_forum_external extends external_api { // Ensure there are courseids to loop through. if (!empty($courseids)) { + // Array of the courses we are going to retrieve the forums from. + $dbcourses = array(); + // Mod info for courses. + $modinfocourses = array(); + // Go through the courseids and return the forums. - foreach ($courseids as $cid) { - // Get the course context. - $context = context_course::instance($cid); + foreach ($courseids as $courseid) { // Check the user can function in this context. - self::validate_context($context); - // Get the forums in this course. - if ($forums = $DB->get_records('forum', array('course' => $cid))) { + try { + $context = context_course::instance($courseid); + self::validate_context($context); // Get the modinfo for the course. - $modinfo = get_fast_modinfo($cid); - // Get the course object. - $course = $modinfo->get_course(); - // Get the forum instances. - $foruminstances = $modinfo->get_instances_of('forum'); - // Loop through the forums returned by modinfo. - foreach ($foruminstances as $forumid => $cm) { - // If it is not visible or present in the forums get_records call, continue. - if (!$cm->uservisible || !isset($forums[$forumid])) { - continue; - } - // Set the forum object. - $forum = $forums[$forumid]; - // Get the module context. - $context = context_module::instance($cm->id); - // Check they have the view forum capability. - require_capability('mod/forum:viewdiscussion', $context); - // Format the intro before being returning using the format setting. - list($forum->intro, $forum->introformat) = external_format_text($forum->intro, $forum->introformat, - $context->id, 'mod_forum', 'intro', 0); - // Add the course module id to the object, this information is useful. - $forum->cmid = $cm->id; - - // Discussions count. This function does static request cache. - $forum->numdiscussions = forum_count_discussions($forum, $cm, $course); - - // Add the forum to the array to return. - $arrforums[$forum->id] = (array) $forum; + $modinfocourses[$courseid] = get_fast_modinfo($courseid); + $dbcourses[$courseid] = $modinfocourses[$courseid]->get_course(); + + } catch (Exception $e) { + continue; + } + } + + // Get the forums in this course. This function checks users visibility permissions. + if ($forums = get_all_instances_in_courses("forum", $dbcourses)) { + foreach ($forums as $forum) { + + $course = $dbcourses[$forum->course]; + $cm = $modinfocourses[$course->id]->get_cm($forum->coursemodule); + $context = context_module::instance($cm->id); + + // Skip forums we are not allowed to see discussions. + if (!has_capability('mod/forum:viewdiscussion', $context)) { + continue; } + + // Format the intro before being returning using the format setting. + list($forum->intro, $forum->introformat) = external_format_text($forum->intro, $forum->introformat, + $context->id, 'mod_forum', 'intro', 0); + // Discussions count. This function does static request cache. + $forum->numdiscussions = forum_count_discussions($forum, $cm, $course); + $forum->cmid = $forum->coursemodule; + + // Add the forum to the array to return. + $arrforums[$forum->id] = $forum; } } } diff --git a/mod/forum/tests/externallib_test.php b/mod/forum/tests/externallib_test.php index 6d0764001b6..c5a57fc4de5 100644 --- a/mod/forum/tests/externallib_test.php +++ b/mod/forum/tests/externallib_test.php @@ -124,19 +124,28 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { $roleid2 = $this->assignUserCapability('mod/forum:viewdiscussion', $context2->id, $newrole); // Create what we expect to be returned when querying the two courses. + unset($forum1->displaywordcount); + unset($forum2->displaywordcount); + $expectedforums = array(); $expectedforums[$forum1->id] = (array) $forum1; $expectedforums[$forum2->id] = (array) $forum2; // Call the external function passing course ids. $forums = mod_forum_external::get_forums_by_courses(array($course1->id, $course2->id)); - external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); - $this->assertEquals($expectedforums, $forums); + $forums = external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); + $this->assertCount(2, $forums); + foreach ($forums as $forum) { + $this->assertEquals($expectedforums[$forum['id']], $forum); + } // Call the external function without passing course id. $forums = mod_forum_external::get_forums_by_courses(); - external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); - $this->assertEquals($expectedforums, $forums); + $forums = external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); + $this->assertCount(2, $forums); + foreach ($forums as $forum) { + $this->assertEquals($expectedforums[$forum['id']], $forum); + } // Unenrol user from second course and alter expected forums. $enrol->unenrol_user($instance2, $user->id); @@ -144,25 +153,14 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { // Call the external function without passing course id. $forums = mod_forum_external::get_forums_by_courses(); - external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); - $this->assertEquals($expectedforums, $forums); - - // Call for the second course we unenrolled the user from, ensure exception thrown. - try { - mod_forum_external::get_forums_by_courses(array($course2->id)); - $this->fail('Exception expected due to being unenrolled from the course.'); - } catch (moodle_exception $e) { - $this->assertEquals('requireloginerror', $e->errorcode); - } - - // Call without required capability, ensure exception thrown. - $this->unassignUserCapability('mod/forum:viewdiscussion', null, null, $course1->id); - try { - $forums = mod_forum_external::get_forums_by_courses(array($course1->id)); - $this->fail('Exception expected due to missing capability.'); - } catch (moodle_exception $e) { - $this->assertEquals('nopermissions', $e->errorcode); - } + $forums = external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); + $this->assertCount(1, $forums); + $this->assertEquals($expectedforums[$forum1->id], $forums[0]); + + // Call for the second course we unenrolled the user from. + $forums = mod_forum_external::get_forums_by_courses(array($course2->id)); + $forums = external_api::clean_returnvalue(mod_forum_external::get_forums_by_courses_returns(), $forums); + $this->assertCount(0, $forums); } /** -- 2.11.4.GIT