diff --git a/mod/forum/externallib.php b/mod/forum/externallib.php index 16b0c6d482d..e5669912a48 100644 --- a/mod/forum/externallib.php +++ b/mod/forum/externallib.php @@ -196,10 +196,6 @@ class mod_forum_external extends external_api { // Array to store the forum discussions to return. $arrdiscussions = array(); - // Keep track of the course ids we have performed a require_course_login check on to avoid repeating. - $arrcourseschecked = array(); - // Store the modinfo for the forums in an individual courses. - $arrcoursesforuminfo = array(); // Keep track of the users we have looked up in the DB. $arrusers = array(); @@ -207,52 +203,58 @@ class mod_forum_external extends external_api { foreach ($forumids as $id) { // Get the forum object. $forum = $DB->get_record('forum', array('id' => $id), '*', MUST_EXIST); - // Check that that user can view this course if check not performed yet. - if (!in_array($forum->course, $arrcourseschecked)) { - // Check the user can function in this context. - self::validate_context(context_course::instance($forum->course)); - // Add to the array. - $arrcourseschecked[] = $forum->course; - } - // Get the modinfo for the course if we haven't already. - if (!isset($arrcoursesforuminfo[$forum->course])) { - $modinfo = get_fast_modinfo($forum->course); - $arrcoursesforuminfo[$forum->course] = $modinfo->get_instances_of('forum'); - } - // Check if this forum does not exist in the modinfo array, should always be false unless DB is borked. - if (empty($arrcoursesforuminfo[$forum->course][$forum->id])) { - throw new moodle_exception('invalidmodule', 'error'); - } - // We now have the course module. - $cm = $arrcoursesforuminfo[$forum->course][$forum->id]; - // If the forum is not visible throw an exception. - if (!$cm->uservisible) { - throw new moodle_exception('nopermissiontoshow', 'error'); - } + $course = get_course($forum->course); + + $modinfo = get_fast_modinfo($course); + $forums = $modinfo->get_instances_of('forum'); + $cm = $forums[$forum->id]; + // Get the module context. $modcontext = context_module::instance($cm->id); - // Check they have the view forum capability. + + // Validate the context. + self::validate_context($modcontext); + require_capability('mod/forum:viewdiscussion', $modcontext); - // Check if they can view full names. - $canviewfullname = has_capability('moodle/site:viewfullnames', $modcontext); - // Get the unreads array, this takes a forum id and returns data for all discussions. - $unreads = array(); - if ($cantrack = forum_tp_can_track_forums($forum)) { - if ($forumtracked = forum_tp_is_tracked($forum)) { - $unreads = forum_get_discussions_unread($cm); + + // Get the discussions for this forum. + $params = array(); + + $groupselect = ""; + $groupmode = groups_get_activity_groupmode($cm, $course); + + if ($groupmode and $groupmode != VISIBLEGROUPS and !has_capability('moodle/site:accessallgroups', $modcontext)) { + // Get all the discussions from all the groups this user belongs to. + $usergroups = groups_get_user_groups($course->id); + if (!empty($usergroups['0'])) { + list($sql, $params) = $DB->get_in_or_equal($usergroups['0']); + $groupselect = "AND (groupid $sql OR groupid = -1)"; } } - // The forum function returns the replies for all the discussions in a given forum. - $replies = forum_count_discussion_replies($id); - // Get the discussions for this forum. - $order = 'timemodified DESC'; - if ($discussions = $DB->get_records('forum_discussions', array('forum' => $id), $order, '*', $limitfrom, $limitnum)) { - foreach ($discussions as $discussion) { - // If the forum is of type qanda and the user has not posted in the discussion - // we need to ensure that they have the required capability. - if ($forum->type == 'qanda' && !forum_user_has_posted($discussion->forum, $discussion->id, $USER->id)) { - require_capability('mod/forum:viewqandawithoutposting', $modcontext); + array_unshift($params, $id); + $select = "forum = ? $groupselect"; + + if ($discussions = $DB->get_records_select('forum_discussions', $select, $params, 'timemodified DESC', '*', + $limitfrom, $limitnum)) { + + // Check if they can view full names. + $canviewfullname = has_capability('moodle/site:viewfullnames', $modcontext); + // Get the unreads array, this takes a forum id and returns data for all discussions. + $unreads = array(); + if ($cantrack = forum_tp_can_track_forums($forum)) { + if ($forumtracked = forum_tp_is_tracked($forum)) { + $unreads = forum_get_discussions_unread($cm); } + } + // The forum function returns the replies for all the discussions in a given forum. + $replies = forum_count_discussion_replies($id); + + foreach ($discussions as $discussion) { + // This function checks capabilities, timed discussions, groups and qanda forums posting. + if (!forum_user_can_see_discussion($forum, $discussion, $modcontext)) { + continue; + } + $usernamefields = user_picture::fields(); // If we don't have the users details then perform DB call. if (empty($arrusers[$discussion->userid])) { @@ -290,10 +292,10 @@ class mod_forum_external extends external_api { if (!empty($replies[$discussion->id])) { $return->numreplies = (int) $replies[$discussion->id]->replies; $return->lastpost = (int) $replies[$discussion->id]->lastpostid; - } else { // No replies, so the last post will be the first post. + } else { // No replies, so the last post will be the first post. $return->numreplies = 0; $return->lastpost = (int) $discussion->firstpost; - } + } // Get the last post as well as the user who made it. $lastpost = $DB->get_record('forum_posts', array('id' => $return->lastpost), '*', MUST_EXIST); if (empty($arrusers[$lastpost->userid])) { diff --git a/mod/forum/tests/externallib_test.php b/mod/forum/tests/externallib_test.php index 21c46290817..b3f15ceeed9 100644 --- a/mod/forum/tests/externallib_test.php +++ b/mod/forum/tests/externallib_test.php @@ -177,11 +177,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { $record->trackingtype = FORUM_TRACKING_FORCED; $forum2 = self::getDataGenerator()->create_module('forum', $record); - // Third forum where we will only have one discussion with no replies. - $record = new stdClass(); - $record->course = $course2->id; - $forum3 = self::getDataGenerator()->create_module('forum', $record); - // Add discussions to the forums. $record = new stdClass(); $record->course = $course1->id; @@ -195,12 +190,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { $record->forum = $forum2->id; $discussion2 = self::getDataGenerator()->get_plugin_generator('mod_forum')->create_discussion($record); - $record = new stdClass(); - $record->course = $course2->id; - $record->userid = $user2->id; - $record->forum = $forum3->id; - $discussion3 = self::getDataGenerator()->get_plugin_generator('mod_forum')->create_discussion($record); - // Add three replies to the discussion 1 from different users. $record = new stdClass(); $record->discussion = $discussion1->id; @@ -227,12 +216,12 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { $discussion2reply2 = self::getDataGenerator()->get_plugin_generator('mod_forum')->create_post($record); // Check the forums were correctly created. - $this->assertEquals(3, $DB->count_records_select('forum', 'id = :forum1 OR id = :forum2 OR id = :forum3', - array('forum1' => $forum1->id, 'forum2' => $forum2->id, 'forum3' => $forum3->id))); + $this->assertEquals(2, $DB->count_records_select('forum', 'id = :forum1 OR id = :forum2', + array('forum1' => $forum1->id, 'forum2' => $forum2->id))); // Check the discussions were correctly created. - $this->assertEquals(3, $DB->count_records_select('forum_discussions', 'forum = :forum1 OR forum = :forum2 - OR id = :forum3', array('forum1' => $forum1->id, 'forum2' => $forum2->id, 'forum3' => $forum3->id))); + $this->assertEquals(2, $DB->count_records_select('forum_discussions', 'forum = :forum1 OR forum = :forum2', + array('forum1' => $forum1->id, 'forum2' => $forum2->id))); // Check the posts were correctly created, don't forget each discussion created also creates a post. $this->assertEquals(7, $DB->count_records_select('forum_posts', 'discussion = :discussion1 OR discussion = :discussion2', @@ -261,11 +250,6 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { $newrole = create_role('Role 2', 'role2', 'Role 2 description'); $this->assignUserCapability('mod/forum:viewdiscussion', $context->id, $newrole); - // Assign capabilities to view discussions for forum 3. - $cm = get_coursemodule_from_id('forum', $forum3->cmid, 0, false, MUST_EXIST); - $context = context_module::instance($cm->id); - $this->assignUserCapability('mod/forum:viewdiscussion', $context->id, $newrole); - // Create what we expect to be returned when querying the forums. $expecteddiscussions = array(); $expecteddiscussions[$discussion1->id] = array( @@ -322,47 +306,21 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { 'lastuserpicture' => $user3->picture, 'lastuseremail' => $user3->email ); - $expecteddiscussions[$discussion3->id] = array( - 'id' => $discussion3->id, - 'course' => $discussion3->course, - 'forum' => $discussion3->forum, - 'name' => $discussion3->name, - 'firstpost' => $discussion3->firstpost, - 'userid' => $discussion3->userid, - 'groupid' => $discussion3->groupid, - 'assessed' => $discussion3->assessed, - 'timemodified' => $discussion3->timemodified, - 'usermodified' => $discussion3->usermodified, - 'timestart' => $discussion3->timestart, - 'timeend' => $discussion3->timeend, - 'firstuserfullname' => fullname($user2), - 'firstuserimagealt' => $user2->imagealt, - 'firstuserpicture' => $user2->picture, - 'firstuseremail' => $user2->email, - 'subject' => $discussion3->name, - 'numreplies' => 0, - 'numunread' => 1, - 'lastpost' => $discussion3->firstpost, - 'lastuserid' => $user2->id, - 'lastuserfullname' => fullname($user2), - 'lastuserimagealt' => $user2->imagealt, - 'lastuserpicture' => $user2->picture, - 'lastuseremail' => $user2->email - ); // Call the external function passing forum ids. - $discussions = mod_forum_external::get_forum_discussions(array($forum1->id, $forum2->id, $forum3->id)); + $discussions = mod_forum_external::get_forum_discussions(array($forum1->id, $forum2->id)); external_api::clean_returnvalue(mod_forum_external::get_forum_discussions_returns(), $discussions); $this->assertEquals($expecteddiscussions, $discussions); + // Some debugging is going to be produced, this is because we switch PAGE contexts in the get_forum_discussions function, + // the switch happens when the validate_context function is called inside a foreach loop. + // See MDL-41746 for more information. + $this->assertDebuggingCalled(); // Remove the users post from the qanda forum and ensure they can not return the discussion. $DB->delete_records('forum_posts', array('id' => $discussion2reply1->id)); - try { - mod_forum_external::get_forum_discussions(array($forum2->id)); - $this->fail('Exception expected due to attempting to access qanda forum without posting.'); - } catch (moodle_exception $e) { - $this->assertEquals('nopermissions', $e->errorcode); - } + $discussions = mod_forum_external::get_forum_discussions(array($forum2->id)); + $discussions = external_api::clean_returnvalue(mod_forum_external::get_forum_discussions_returns(), $discussions); + $this->assertEquals(0, count($discussions)); // Call without required view discussion capability. $this->unassignUserCapability('mod/forum:viewdiscussion', null, null, $course1->id); @@ -372,6 +330,7 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { } catch (moodle_exception $e) { $this->assertEquals('nopermissions', $e->errorcode); } + $this->assertDebuggingCalled(); // Unenrol user from second course. $enrol->unenrol_user($instance2, $user1->id); @@ -383,6 +342,7 @@ class mod_forum_external_testcase extends externallib_advanced_testcase { } catch (moodle_exception $e) { $this->assertEquals('requireloginerror', $e->errorcode); } + $this->assertDebuggingCalled(); } /**