From 8d8d0bfaae202de8568eab9672b8a7d30bd937c9 Mon Sep 17 00:00:00 2001 From: tjhunt Date: Thu, 11 Dec 2008 07:01:57 +0000 Subject: [PATCH] forum subscriptions: MDL-14876 user of deprecated functions was breaking managing subscribers. This was visible when, for example, unusual role definitions meant that someone could access a forum without being enrolled in a course. Note that one of the places that was previously broken was front page forums. Since subscribers.php does not do paging at all, the fact that I have fixed this bug makes this page dangerous on large sites. A proper solution will have to wait until bug 17550 is fixed in HEAD. This also fixes a minor problem introduced by MDL-12979. --- mod/forum/lib.php | 41 ++++++++++++++++++++++++------ mod/forum/subscriber.html | 20 ++++----------- mod/forum/subscribers.php | 53 +++++++++++++++++++++------------------ 3 files changed, 66 insertions(+), 48 deletions(-) diff --git a/mod/forum/lib.php b/mod/forum/lib.php index 0ab2070bd2..7942bb51a8 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -83,9 +83,13 @@ function forum_add_instance($forum) { } if ($forum->forcesubscribe == FORUM_INITIALSUBSCRIBE) { - // all users should be subscribed initially - $users = get_users_by_capability(get_context_instance(CONTEXT_COURSE, $forum->course), - 'mod/forum:initialsubscriptions', 'u.id', '', '','','',null, false); + /// all users should be subscribed initially + /// Note: forum_get_potential_subscribers should take the forum context, + /// but that does not exist yet, becuase the forum is only half build at this + /// stage. However, because the forum is brand new, we know that there are + /// no role assignments or overrides in the forum context, so using the + /// course context gives the same list of users. + $users = forum_get_potential_subscribers(get_context_instance(CONTEXT_COURSE, $forum->course), 0, 'id, email', ''); foreach ($users as $user) { forum_subscribe($user->id, $forum->id); } @@ -386,7 +390,8 @@ function forum_cron() { // caching subscribed users of each forum if (!isset($subscribedusers[$forumid])) { - if ($subusers = forum_subscribed_users($courses[$courseid], $forums[$forumid], 0, false)) { + if ($subusers = forum_subscribed_users($courses[$courseid], $forums[$forumid], 0, + get_context_instance(CONTEXT_MODULE, $coursemodules[$forumid]))) { foreach ($subusers as $postuser) { // do not try to mail users with stopped email if ($postuser->emailstop) { @@ -2661,10 +2666,28 @@ function forum_get_user_discussions($courseid, $userid, $groupid=0) { } /** - * Returns list of user objects that are subscribed to this forum + * Get the list of potential subscribers to a forum. + * + * @param object $forumcontext the forum context. + * @param integer $groupid the id of a group, or 0 for all groups. + * @param string $fields the list of fields to return for each user. As for get_users_by_capability. + * @param string $sort sort order. As for get_users_by_capability. + * @return array list of users. */ -function forum_subscribed_users($course, $forum, $groupid=0) { +function forum_get_potential_subscribers($forumcontext, $groupid, $fields, $sort) { + return get_users_by_capability($forumcontext, 'mod/forum:initialsubscriptions', $fields, $sort, '', '', $groupid, '', false, true); +} +/** + * Returns list of user objects that are subscribed to this forum + * + * @param object $course the course + * @param forum $forum the forum + * @param integer $groupid group id, or 0 for all. + * @param object $context the forum context, to save re-fetching it where possible. + * @return array list of users. + */ +function forum_subscribed_users($course, $forum, $groupid=0, $context = NULL) { global $CFG, $DB; $params = array($forum->id); @@ -2699,9 +2722,11 @@ function forum_subscribed_users($course, $forum, $groupid=0) { u.mnethostid"; if (forum_is_forcesubscribed($forum)) { - $context = get_context_instance(CONTEXT_COURSE, $course->id); + if (is_null($context)) { + $context = get_context_instance(CONTEXT_MODULE, get_coursemodule_from_instance('forum', $forum->id, $course->id)); + } $sort = "u.email ASC"; - $results = get_users_by_capability($context, 'mod/forum:initialsubscriptions', $fields, $sort, '','','','', false, true); + $results = forum_get_potential_subscribers($context, $groupid, $fields, $sort); } else { $results = $DB->get_records_sql("SELECT $fields FROM {user} u, diff --git a/mod/forum/subscriber.html b/mod/forum/subscriber.html index f100e4914f..1023dc0185 100644 --- a/mod/forum/subscriber.html +++ b/mod/forum/subscriber.html @@ -1,6 +1,5 @@
- @@ -14,7 +13,7 @@
- -
- + '."\n"; } ?> diff --git a/mod/forum/subscribers.php b/mod/forum/subscribers.php index 2952ed6be8..e00a8713a7 100644 --- a/mod/forum/subscribers.php +++ b/mod/forum/subscribers.php @@ -31,10 +31,10 @@ add_to_log($course->id, "forum", "view subscribers", "subscribers.php?id=$forum->id", $forum->id, $cm->id); - $strsubscribeall = get_string("subscribeall", "forum"); + $strsubscribeall = get_string("subscribeall", "forum"); $strsubscribenone = get_string("subscribenone", "forum"); - $strsubscribers = get_string("subscribers", "forum"); - $strforums = get_string("forums", "forum"); + $strsubscribers = get_string("subscribers", "forum"); + $strforums = get_string("forums", "forum"); $navigation = build_navigation($strsubscribers, $cm); @@ -56,7 +56,7 @@ if (empty($USER->subscriptionsediting)) { /// Display an overview of subscribers - if (! $users = forum_subscribed_users($course, $forum, $currentgroup) ) { + if (! $users = forum_subscribed_users($course, $forum, $currentgroup, $context) ) { print_heading(get_string("nosubscribers", "forum")); @@ -93,12 +93,11 @@ $strsubscribers = get_string("subscribers", "forum"); $strforums = get_string("forums", "forum"); + $searchtext = optional_param('searchtext', '', PARAM_RAW); if ($frm = data_submitted()) { /// A form was submitted so process the input - if (!empty($frm->add) and !empty($frm->addselect)) { - foreach ($frm->addselect as $addsubscriber) { if (! forum_subscribe($addsubscriber, $id)) { print_error('cannotaddsubscriber', 'forum', '', $addsubscriber); @@ -111,36 +110,40 @@ } } } else if (!empty($frm->showall)) { - unset($frm->searchtext); - $frm->previoussearch = 0; + $searchtext = ''; } } - $previoussearch = (!empty($frm->search) or (!empty($frm->previoussearch) && $frm->previoussearch == 1)) ; - /// Get all existing subscribers for this forum. - if (!$subscribers = forum_subscribed_users($course, $forum, $currentgroup)) { + if (!$subscribers = forum_subscribed_users($course, $forum, $currentgroup, $context)) { $subscribers = array(); } - $subscriberarray = array(); - foreach ($subscribers as $subscriber) { - $subscriberarray[] = $subscriber->id; +/// Get all the potential subscribers excluding users already subscribed + $users = forum_get_potential_subscribers($context, $currentgroup, 'id,email,firstname,lastname', 'firstname ASC, lastname ASC'); + if (!$users) { + $users = array(); } - $subscriberlist = implode(',', $subscriberarray); - -/// Get search results excluding any users already subscribed - - if (!empty($frm->searchtext) and $previoussearch) { - $searchusers = search_users($course->id, $currentgroup, $frm->searchtext, 'firstname ASC, lastname ASC', $subscriberarray); + foreach ($subscribers as $subscriber) { + unset($users[$subscriber->id]); } -/// If no search results then get potential subscribers for this forum excluding users already subscribed - if (empty($searchusers)) { - $users = get_users_by_capability($context, 'moodle/course:view', '', 'firstname ASC, lastname ASC', '','',$currentgroup,$subscriberlist, false); +/// This is yucky, but do the search in PHP, becuase the list we are using comes from get_users_by_capability, +/// which does not allow searching in the database. Fortunately the list is only this list of users in this +/// course, which is normally OK, except on the site course of a big site. But before you can enter a search +/// term, you have already seen a page that lists everyone, since this code never does paging, so you have probably +/// already crashed your server if you are going to. This will be fixed properly for Moodle 2.0: MDL-17550. + if ($searchtext) { + $searchusers = array(); + $lcsearchtext = moodle_strtolower($searchtext); + foreach ($users as $userid => $user) { + if (strpos(moodle_strtolower($user->email), $lcsearchtext) !== false || + strpos(moodle_strtolower($user->firstname . ' ' . $user->lastname), $lcsearchtext) !== false) { + $searchusers[$userid] = $user; + } + unset($users[$userid]); + } } - $searchtext = (isset($frm->searchtext)) ? $frm->searchtext : ""; - $previoussearch = ($previoussearch) ? '1' : '0'; print_simple_box_start('center'); -- 2.39.5