MDL-49921 forum: Handle exceptions correctly in get_forums_by_courses
authorJuan Leyva <juanleyvadelgado@gmail.com>
Mon, 20 Apr 2015 10:23:20 +0000 (12:23 +0200)
committerJuan Leyva <juanleyvadelgado@gmail.com>
Wed, 22 Apr 2015 07:47:18 +0000 (09:47 +0200)
mod/forum/externallib.php
mod/forum/tests/externallib_test.php

index 03a6618..1a357ea 100644 (file)
@@ -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;
                 }
             }
         }
index ba57cd4..aa78d36 100644 (file)
@@ -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);
     }
 
     /**