From 8381ac52cd6f609ad00639d263c2ad108d9f4dde Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 11 Aug 2016 14:29:29 +0800 Subject: [PATCH] MDL-46654 mod_forum: Remove irrelevant digest/subscribe options If the user cannot subscribe, there is no point showing the digest options. --- mod/forum/classes/subscriptions.php | 3 +- mod/forum/index.php | 195 ++++++++++++++----------- mod/forum/tests/subscriptions_test.php | 102 +++++++++++++ 3 files changed, 212 insertions(+), 88 deletions(-) diff --git a/mod/forum/classes/subscriptions.php b/mod/forum/classes/subscriptions.php index d0672f39309..8f7aa2fea65 100644 --- a/mod/forum/classes/subscriptions.php +++ b/mod/forum/classes/subscriptions.php @@ -164,7 +164,8 @@ class subscriptions { * @return bool */ public static function is_subscribable($forum) { - return (!\mod_forum\subscriptions::is_forcesubscribed($forum) && + return (isloggedin() && !isguestuser() && + !\mod_forum\subscriptions::is_forcesubscribed($forum) && !\mod_forum\subscriptions::subscription_disabled($forum)); } diff --git a/mod/forum/index.php b/mod/forum/index.php index 3a8ba37e3b1..3f9e1ce3a9c 100644 --- a/mod/forum/index.php +++ b/mod/forum/index.php @@ -29,7 +29,7 @@ require_once($CFG->libdir . '/rsslib.php'); $id = optional_param('id', 0, PARAM_INT); // Course id $subscribe = optional_param('subscribe', null, PARAM_INT); // Subscribe/Unsubscribe all forums -$url = new moodle_url('/mod/forum/index.php', array('id'=>$id)); +$url = new moodle_url('/mod/forum/index.php', array('id' => $id)); if ($subscribe !== null) { require_sesskey(); $url->param('subscribe', $subscribe); @@ -37,7 +37,7 @@ if ($subscribe !== null) { $PAGE->set_url($url); if ($id) { - if (! $course = $DB->get_record('course', array('id' => $id))) { + if (!$course = $DB->get_record('course', array('id' => $id))) { print_error('invalidcourseid'); } } else { @@ -96,34 +96,6 @@ if ($usetracking = forum_tp_can_track_forums()) { // Fill the subscription cache for this course and user combination. \mod_forum\subscriptions::fill_subscription_cache_for_course($course->id, $USER->id); -$can_subscribe = is_enrolled($coursecontext); -if ($can_subscribe) { - $generaltable->head[] = $strsubscribed; - $generaltable->align[] = 'center'; - - $generaltable->head[] = $stremaildigest . ' ' . $OUTPUT->help_icon('emaildigesttype', 'mod_forum'); - $generaltable->align[] = 'center'; - - // Retrieve the list of forum digest options for later. - $digestoptions = forum_get_user_digest_options(); - $digestoptions_selector = new single_select(new moodle_url('/mod/forum/maildigest.php', - array( - 'backtoindex' => 1, - )), - 'maildigest', - $digestoptions, - null, - ''); - $digestoptions_selector->method = 'post'; -} - -if ($show_rss = (($can_subscribe || $course->id == SITEID) && - isset($CFG->enablerssfeeds) && isset($CFG->forum_enablerssfeeds) && - $CFG->enablerssfeeds && $CFG->forum_enablerssfeeds)) { - $generaltable->head[] = $strrss; - $generaltable->align[] = 'center'; -} - $usesections = course_format_uses_sections($course->format); $table = new html_table(); @@ -143,8 +115,9 @@ $forums = $DB->get_records_sql(" $generalforums = array(); $learningforums = array(); $modinfo = get_fast_modinfo($course); +$showsubscriptioncolumns = false; -foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) { +foreach ($modinfo->get_instances_of('forum') as $forumid => $cm) { if (!$cm->uservisible or !isset($forums[$forumid])) { continue; } @@ -152,14 +125,23 @@ foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) { $forum = $forums[$forumid]; if (!$context = context_module::instance($cm->id, IGNORE_MISSING)) { - continue; // Shouldn't happen - } - - if (!has_capability('mod/forum:viewdiscussion', $context)) { + // Shouldn't happen. continue; } - // fill two type array - order in modinfo is the same as in course + if (!has_capability('mod/forum:viewdiscussion', $context)) { + // User can't view this one - skip it. + continue; + } + + // Determine whether subscription options should be displayed. + $forum->cansubscribe = mod_forum\subscriptions::is_subscribable($forum); + $forum->cansubscribe = $forum->cansubscribe || has_capability('mod/forum:managesubscriptions', $context); + $forum->issubscribed = mod_forum\subscriptions::is_subscribed($USER->id, $forum, null, $cm); + + $showsubscriptioncolumns = $showsubscriptioncolumns || $forum->issubscribed || $forum->cansubscribe; + + // Fill two type array - order in modinfo is the same as in course. if ($forum->type == 'news' or $forum->type == 'social') { $generalforums[$forum->id] = $forum; @@ -171,9 +153,62 @@ foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) { } } +if ($showsubscriptioncolumns) { + // The user can subscribe to at least one forum. + $generaltable->head[] = $strsubscribed; + $generaltable->align[] = 'center'; + + $generaltable->head[] = $stremaildigest . ' ' . $OUTPUT->help_icon('emaildigesttype', 'mod_forum'); + $generaltable->align[] = 'center'; + + // Retrieve the list of forum digest options for later. + $digestoptions = forum_get_user_digest_options(); + $digestoptionsselector = new single_select(new moodle_url('/mod/forum/maildigest.php', + array( + 'backtoindex' => 1, + )), + 'maildigest', + $digestoptions, + null, + ''); + $digestoptionsselector->method = 'post'; + + $forumsubscriptionhelper = function($forum, $row, $context) use ($digestoptionsselector, $stryes, $strno) { + global $OUTPUT; + + $row[] = forum_get_subscribe_link($forum, $context, array('subscribed' => $stryes, + 'unsubscribed' => $strno, 'forcesubscribed' => $stryes, + 'cantsubscribe' => '-'), false, false, true); + + $digestoptionsselector->url->param('id', $forum->id); + if ($forum->maildigest === null) { + $digestoptionsselector->selected = -1; + } else { + $digestoptionsselector->selected = $forum->maildigest; + } + + if ($forum->cansubscribe || $forum->issubscribed) { + $row[] = $OUTPUT->render($digestoptionsselector); + } else { + // This user can subscribe to some forums. Add the empty fields. + $row[] = ''; + } + + return $row; + }; +} + +if ($show_rss = (($showsubscriptioncolumns || $course->id == SITEID) && + isset($CFG->enablerssfeeds) && isset($CFG->forum_enablerssfeeds) && + $CFG->enablerssfeeds && $CFG->forum_enablerssfeeds)) { + $generaltable->head[] = $strrss; + $generaltable->align[] = 'center'; +} + + // Do course wide subscribe/unsubscribe if requested if (!is_null($subscribe)) { - if (isguestuser() or !$can_subscribe) { + if (isguestuser() or !$showsubscriptioncolumns) { // There should not be any links leading to this place, just redirect. redirect( new moodle_url('/mod/forum/index.php', array('id' => $id)), @@ -183,7 +218,7 @@ if (!is_null($subscribe)) { ); } // Can proceed now, the user is not guest and is enrolled - foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) { + foreach ($modinfo->get_instances_of('forum') as $forumid => $cm) { $forum = $forums[$forumid]; $modcontext = context_module::instance($cm->id); $cansub = false; @@ -225,9 +260,8 @@ if (!is_null($subscribe)) { } } -/// First, let's process the general forums and build up a display - if ($generalforums) { + // Process general forums. foreach ($generalforums as $forum) { $cm = $modinfo->instances['forum'][$forum->id]; $context = context_module::instance($cm->id); @@ -260,9 +294,9 @@ if ($generalforums) { 'sesskey' => sesskey(), )); if (!isset($untracked[$forum->id])) { - $trackedlink = $OUTPUT->single_button($aurl, $stryes, 'post', array('title'=>$strnotrackforum)); + $trackedlink = $OUTPUT->single_button($aurl, $stryes, 'post', array('title' => $strnotrackforum)); } else { - $trackedlink = $OUTPUT->single_button($aurl, $strno, 'post', array('title'=>$strtrackforum)); + $trackedlink = $OUTPUT->single_button($aurl, $strno, 'post', array('title' => $strtrackforum)); } } } @@ -285,21 +319,11 @@ if ($generalforums) { $row[] = $trackedlink; // Tracking. } - if ($can_subscribe) { - $row[] = forum_get_subscribe_link($forum, $context, array('subscribed' => $stryes, - 'unsubscribed' => $strno, 'forcesubscribed' => $stryes, - 'cantsubscribe' => '-'), false, false, true); - - $digestoptions_selector->url->param('id', $forum->id); - if ($forum->maildigest === null) { - $digestoptions_selector->selected = -1; - } else { - $digestoptions_selector->selected = $forum->maildigest; - } - $row[] = $OUTPUT->render($digestoptions_selector); + if ($showsubscriptioncolumns) { + $row = $forumsubscriptionhelper($forum, $row, $context); } - //If this forum has RSS activated, calculate it + // If this forum has RSS activated, calculate it. if ($show_rss) { if ($forum->rsstype and $forum->rssarticles) { //Calculate the tooltip text @@ -339,7 +363,7 @@ if ($usetracking) { $learningtable->align[] = 'center'; } -if ($can_subscribe) { +if ($showsubscriptioncolumns) { $learningtable->head[] = $strsubscribed; $learningtable->align[] = 'center'; @@ -347,15 +371,14 @@ if ($can_subscribe) { $learningtable->align[] = 'center'; } -if ($show_rss = (($can_subscribe || $course->id == SITEID) && +if ($show_rss = (($showsubscriptioncolumns || $course->id == SITEID) && isset($CFG->enablerssfeeds) && isset($CFG->forum_enablerssfeeds) && $CFG->enablerssfeeds && $CFG->forum_enablerssfeeds)) { $learningtable->head[] = $strrss; $learningtable->align[] = 'center'; } -/// Now let's process the learning forums - +// Now let's process the learning forums. if ($course->id != SITEID) { // Only real courses have learning forums // 'format_.'$course->format only applicable when not SITEID (format_site is not a format) $strsectionname = get_string('sectionname', 'format_'.$course->format); @@ -393,11 +416,11 @@ if ($course->id != SITEID) { // Only real courses have learning forums } else if ($forum->trackingtype === FORUM_TRACKING_OFF || ($USER->trackforums == 0)) { $trackedlink = '-'; } else { - $aurl = new moodle_url('/mod/forum/settracking.php', array('id'=>$forum->id)); + $aurl = new moodle_url('/mod/forum/settracking.php', array('id' => $forum->id)); if (!isset($untracked[$forum->id])) { - $trackedlink = $OUTPUT->single_button($aurl, $stryes, 'post', array('title'=>$strnotrackforum)); + $trackedlink = $OUTPUT->single_button($aurl, $stryes, 'post', array('title' => $strnotrackforum)); } else { - $trackedlink = $OUTPUT->single_button($aurl, $strno, 'post', array('title'=>$strtrackforum)); + $trackedlink = $OUTPUT->single_button($aurl, $strno, 'post', array('title' => $strtrackforum)); } } } @@ -431,18 +454,8 @@ if ($course->id != SITEID) { // Only real courses have learning forums $row[] = $trackedlink; // Tracking. } - if ($can_subscribe) { - $row[] = forum_get_subscribe_link($forum, $context, array('subscribed' => $stryes, - 'unsubscribed' => $strno, 'forcesubscribed' => $stryes, - 'cantsubscribe' => '-'), false, false, true); - - $digestoptions_selector->url->param('id', $forum->id); - if ($forum->maildigest === null) { - $digestoptions_selector->selected = -1; - } else { - $digestoptions_selector->selected = $forum->maildigest; - } - $row[] = $OUTPUT->render($digestoptions_selector); + if ($showsubscriptioncolumns) { + $row = $forumsubscriptionhelper($forum, $row, $context); } //If this forum has RSS activated, calculate it @@ -466,25 +479,34 @@ if ($course->id != SITEID) { // Only real courses have learning forums } } - -/// Output the page +// Output the page. $PAGE->navbar->add($strforums); $PAGE->set_title("$course->shortname: $strforums"); $PAGE->set_heading($course->fullname); $PAGE->set_button($searchform); echo $OUTPUT->header(); -// Show the subscribe all options only to non-guest, enrolled users -if (!isguestuser() && isloggedin() && $can_subscribe) { +if (!isguestuser() && isloggedin() && $showsubscriptioncolumns) { + // Show the subscribe all options only to non-guest, enrolled users. echo $OUTPUT->box_start('subscription'); - echo html_writer::tag('div', - html_writer::link(new moodle_url('/mod/forum/index.php', array('id'=>$course->id, 'subscribe'=>1, 'sesskey'=>sesskey())), - get_string('allsubscribe', 'forum')), - array('class'=>'helplink')); - echo html_writer::tag('div', - html_writer::link(new moodle_url('/mod/forum/index.php', array('id'=>$course->id, 'subscribe'=>0, 'sesskey'=>sesskey())), - get_string('allunsubscribe', 'forum')), - array('class'=>'helplink')); + + $subscriptionlink = new moodle_url('/mod/forum/index.php', [ + 'id' => $course->id, + 'sesskey' => sesskey(), + ]); + + // Subscribe all. + $subscriptionlink->param('subscribe', 1); + echo html_writer::tag('div', html_writer::link($subscriptionlink, get_string('allsubscribe', 'forum')), [ + 'class' => 'helplink', + ]); + + // Unsubscribe all. + $subscriptionlink->param('subscribe', 0); + echo html_writer::tag('div', html_writer::link($subscriptionlink, get_string('allunsubscribe', 'forum')), [ + 'class' => 'helplink', + ]); + echo $OUTPUT->box_end(); echo $OUTPUT->box(' ', 'clearer'); } @@ -500,4 +522,3 @@ if ($learningforums) { } echo $OUTPUT->footer(); - diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php index cdd98acefc3..f435f00b982 100644 --- a/mod/forum/tests/subscriptions_test.php +++ b/mod/forum/tests/subscriptions_test.php @@ -106,6 +106,12 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $options = array('course' => $course->id); $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); + + // Must be logged in as the current user. + $this->setUser($user); + \mod_forum\subscriptions::set_subscription_mode($forum->id, FORUM_FORCESUBSCRIBE); $forum = $DB->get_record('forum', array('id' => $forum->id)); $this->assertEquals(FORUM_FORCESUBSCRIBE, \mod_forum\subscriptions::get_subscription_mode($forum)); @@ -1318,4 +1324,100 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $this->assertGreaterThan($suppliedcmcount, $calculatedcmcount); } + public function is_subscribable_forums() { + return [ + [ + 'forcesubscribe' => FORUM_DISALLOWSUBSCRIBE, + ], + [ + 'forcesubscribe' => FORUM_CHOOSESUBSCRIBE, + ], + [ + 'forcesubscribe' => FORUM_INITIALSUBSCRIBE, + ], + [ + 'forcesubscribe' => FORUM_FORCESUBSCRIBE, + ], + ]; + } + + public function is_subscribable_provider() { + $data = []; + foreach ($this->is_subscribable_forums() as $forum) { + $data[] = [$forum]; + } + + return $data; + } + + /** + * @dataProvider is_subscribable_provider + */ + public function test_is_subscribable_logged_out($options) { + $this->resetAfterTest(true); + + // Create a course, with a forum. + $course = $this->getDataGenerator()->create_course(); + $options['course'] = $course->id; + $forum = $this->getDataGenerator()->create_module('forum', $options); + + $this->assertFalse(\mod_forum\subscriptions::is_subscribable($forum)); + } + + /** + * @dataProvider is_subscribable_provider + */ + public function test_is_subscribable_is_guest($options) { + global $DB; + $this->resetAfterTest(true); + + $guest = $DB->get_record('user', array('username'=>'guest')); + $this->setUser($guest); + + // Create a course, with a forum. + $course = $this->getDataGenerator()->create_course(); + $options['course'] = $course->id; + $forum = $this->getDataGenerator()->create_module('forum', $options); + + $this->assertFalse(\mod_forum\subscriptions::is_subscribable($forum)); + } + + public function is_subscribable_loggedin_provider() { + return [ + [ + ['forcesubscribe' => FORUM_DISALLOWSUBSCRIBE], + false, + ], + [ + ['forcesubscribe' => FORUM_CHOOSESUBSCRIBE], + true, + ], + [ + ['forcesubscribe' => FORUM_INITIALSUBSCRIBE], + true, + ], + [ + ['forcesubscribe' => FORUM_FORCESUBSCRIBE], + false, + ], + ]; + } + + /** + * @dataProvider is_subscribable_loggedin_provider + */ + public function test_is_subscribable_loggedin($options, $expect) { + $this->resetAfterTest(true); + + // Create a course, with a forum. + $course = $this->getDataGenerator()->create_course(); + $options['course'] = $course->id; + $forum = $this->getDataGenerator()->create_module('forum', $options); + + $user = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user->id, $course->id); + $this->setUser($user); + + $this->assertEquals($expect, \mod_forum\subscriptions::is_subscribable($forum)); + } }