From d65f46103a3c806d9d73727e016eb13c26c63ecd Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Sat, 14 Aug 2021 00:24:52 +0800 Subject: [PATCH] MDL-67610 mod_forum: Do not include private replies in unread count Fetching the counts of unread posts should only include unread regular posts or unread private reply posts directed to the user unless the user has the capability to read private replies. In order to retrieve the correct counts, we also need to loop through each forum instance in the course to check the capability of the user to read private replies in each forum. --- mod/forum/lib.php | 117 ++++++++--- mod/forum/tests/lib_test.php | 364 +++++++++++++++++++++++++++++++++++ 2 files changed, 450 insertions(+), 31 deletions(-) diff --git a/mod/forum/lib.php b/mod/forum/lib.php index e9089555fd1..25aa2438ca2 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -4561,14 +4561,28 @@ function forum_tp_is_post_old($post, $time=null) { function forum_tp_get_course_unread_posts($userid, $courseid) { global $CFG, $DB; - $now = floor(time() / 60) * 60; // DB cache friendliness. - $cutoffdate = $now - ($CFG->forum_oldpostdays * 24 * 60 * 60); - $params = array($userid, $userid, $courseid, $cutoffdate, $userid); + $modinfo = get_fast_modinfo($courseid); + $forumcms = $modinfo->get_instances_of('forum'); + if (empty($forumcms)) { + // Return early if the course doesn't have any forum. Will save us a DB query. + return []; + } + + $now = floor(time() / MINSECS) * MINSECS; // DB cache friendliness. + $cutoffdate = $now - ($CFG->forum_oldpostdays * DAYSECS); + $params = [ + 'privatereplyto' => $userid, + 'modified' => $cutoffdate, + 'readuserid' => $userid, + 'trackprefsuser' => $userid, + 'courseid' => $courseid, + 'trackforumuser' => $userid, + ]; if (!empty($CFG->forum_enabletimedposts)) { - $timedsql = "AND d.timestart < ? AND (d.timeend = 0 OR d.timeend > ?)"; - $params[] = $now; - $params[] = $now; + $timedsql = "AND d.timestart < :timestart AND (d.timeend = 0 OR d.timeend > :timeend)"; + $params['timestart'] = $now; + $params['timeend'] = $now; } else { $timedsql = ""; } @@ -4576,31 +4590,65 @@ function forum_tp_get_course_unread_posts($userid, $courseid) { if ($CFG->forum_allowforcedreadtracking) { $trackingsql = "AND (f.trackingtype = ".FORUM_TRACKING_FORCED." OR (f.trackingtype = ".FORUM_TRACKING_OPTIONAL." AND tf.id IS NULL - AND (SELECT trackforums FROM {user} WHERE id = ?) = 1))"; + AND (SELECT trackforums FROM {user} WHERE id = :trackforumuser) = 1))"; } else { $trackingsql = "AND ((f.trackingtype = ".FORUM_TRACKING_OPTIONAL." OR f.trackingtype = ".FORUM_TRACKING_FORCED.") AND tf.id IS NULL - AND (SELECT trackforums FROM {user} WHERE id = ?) = 1)"; + AND (SELECT trackforums FROM {user} WHERE id = :trackforumuser) = 1)"; } - $sql = "SELECT f.id, COUNT(p.id) AS unread - FROM {forum_posts} p + $sql = "SELECT f.id, COUNT(p.id) AS unread, + COUNT(p.privatereply) as privatereplies, + COUNT(p.privatereplytouser) as privaterepliestouser + FROM ( + SELECT + id, + discussion, + CASE WHEN privatereplyto <> 0 THEN 1 END privatereply, + CASE WHEN privatereplyto = :privatereplyto THEN 1 END privatereplytouser + FROM {forum_posts} + WHERE modified >= :modified + ) p JOIN {forum_discussions} d ON d.id = p.discussion JOIN {forum} f ON f.id = d.forum JOIN {course} c ON c.id = f.course - LEFT JOIN {forum_read} r ON (r.postid = p.id AND r.userid = ?) - LEFT JOIN {forum_track_prefs} tf ON (tf.userid = ? AND tf.forumid = f.id) - WHERE f.course = ? - AND p.modified >= ? AND r.id is NULL + LEFT JOIN {forum_read} r ON (r.postid = p.id AND r.userid = :readuserid) + LEFT JOIN {forum_track_prefs} tf ON (tf.userid = :trackprefsuser AND tf.forumid = f.id) + WHERE f.course = :courseid + AND r.id is NULL $trackingsql $timedsql GROUP BY f.id"; - if ($return = $DB->get_records_sql($sql, $params)) { - return $return; + $results = []; + if ($records = $DB->get_records_sql($sql, $params)) { + // Loop through each forum instance to check for capability and count the number of unread posts. + foreach ($forumcms as $cm) { + // Check that the forum instance exists in the query results. + if (!isset($records[$cm->instance])) { + continue; + } + + $record = $records[$cm->instance]; + $unread = $record->unread; + + // Check if the user has the capability to read private replies for this forum instance. + $forumcontext = context_module::instance($cm->id); + if (!has_capability('mod/forum:readprivatereplies', $forumcontext, $userid)) { + // The real unread count would be the total of unread count minus the number of unread private replies plus + // the total unread private replies to the user. + $unread = $record->unread - $record->privatereplies + $record->privaterepliestouser; + } + + // Build and add the object to the array of results to be returned. + $results[$record->id] = (object)[ + 'id' => $record->id, + 'unread' => $unread, + ]; + } } - return array(); + return $results; } /** @@ -4645,7 +4693,8 @@ function forum_tp_count_forum_unread_posts($cm, $course, $resetreadcache = false return $readcache[$course->id][$forumid]; } - if (has_capability('moodle/site:accessallgroups', context_module::instance($cm->id))) { + $forumcontext = context_module::instance($cm->id); + if (has_any_capability(['moodle/site:accessallgroups', 'mod/forum:readprivatereplies'], $forumcontext)) { return $readcache[$course->id][$forumid]; } @@ -4658,30 +4707,36 @@ function forum_tp_count_forum_unread_posts($cm, $course, $resetreadcache = false // add all groups posts $mygroups[-1] = -1; - list ($groups_sql, $groups_params) = $DB->get_in_or_equal($mygroups); + list ($groupssql, $groupsparams) = $DB->get_in_or_equal($mygroups, SQL_PARAMS_NAMED); - $now = floor(time() / 60) * 60; // DB Cache friendliness. - $cutoffdate = $now - ($CFG->forum_oldpostdays*24*60*60); - $params = array($USER->id, $forumid, $cutoffdate); + $now = floor(time() / MINSECS) * MINSECS; // DB Cache friendliness. + $cutoffdate = $now - ($CFG->forum_oldpostdays * DAYSECS); + $params = [ + 'readuser' => $USER->id, + 'forum' => $forumid, + 'cutoffdate' => $cutoffdate, + 'privatereplyto' => $USER->id, + ]; if (!empty($CFG->forum_enabletimedposts)) { - $timedsql = "AND d.timestart < ? AND (d.timeend = 0 OR d.timeend > ?)"; - $params[] = $now; - $params[] = $now; + $timedsql = "AND d.timestart < :timestart AND (d.timeend = 0 OR d.timeend > :timeend)"; + $params['timestart'] = $now; + $params['timeend'] = $now; } else { $timedsql = ""; } - $params = array_merge($params, $groups_params); + $params = array_merge($params, $groupsparams); $sql = "SELECT COUNT(p.id) FROM {forum_posts} p - JOIN {forum_discussions} d ON p.discussion = d.id - LEFT JOIN {forum_read} r ON (r.postid = p.id AND r.userid = ?) - WHERE d.forum = ? - AND p.modified >= ? AND r.id is NULL + JOIN {forum_discussions} d ON p.discussion = d.id + LEFT JOIN {forum_read} r ON (r.postid = p.id AND r.userid = :readuser) + WHERE d.forum = :forum + AND p.modified >= :cutoffdate AND r.id is NULL $timedsql - AND d.groupid $groups_sql"; + AND d.groupid $groupssql + AND (p.privatereplyto = 0 OR p.privatereplyto = :privatereplyto)"; return $DB->get_field_sql($sql, $params); } diff --git a/mod/forum/tests/lib_test.php b/mod/forum/tests/lib_test.php index e0635e89780..25bff5950b9 100644 --- a/mod/forum/tests/lib_test.php +++ b/mod/forum/tests/lib_test.php @@ -509,6 +509,370 @@ class mod_forum_lib_testcase extends advanced_testcase { $this->assertEquals(false, isset($result[$forumoptional->id])); } + /** + * Test the logic in the forum_tp_get_course_unread_posts() function when private replies are present. + * + * @covers ::forum_tp_get_course_unread_posts + */ + public function test_forum_tp_get_course_unread_posts_with_private_replies() { + global $DB; + + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + + // Create 3 students. + $s1 = $generator->create_user(['trackforums' => 1]); + $s2 = $generator->create_user(['trackforums' => 1]); + $s3 = $generator->create_user(['trackforums' => 1]); + // Editing teacher. + $t1 = $generator->create_user(['trackforums' => 1]); + // Non-editing teacher. + $t2 = $generator->create_user(['trackforums' => 1]); + + // Create our course. + $course = $generator->create_course(); + + // Enrol editing and non-editing teachers. + $generator->enrol_user($t1->id, $course->id, 'editingteacher'); + $generator->enrol_user($t2->id, $course->id, 'teacher'); + + // Create forums. + $forum1 = $generator->create_module('forum', ['course' => $course->id]); + $forum2 = $generator->create_module('forum', ['course' => $course->id]); + $forumgenerator = $generator->get_plugin_generator('mod_forum'); + + // Prevent the non-editing teacher from reading private replies in forum 2. + $teacherroleid = $DB->get_field('role', 'id', ['shortname' => 'teacher']); + $forum2cm = get_coursemodule_from_instance('forum', $forum2->id); + $forum2context = context_module::instance($forum2cm->id); + role_change_permission($teacherroleid, $forum2context, 'mod/forum:readprivatereplies', CAP_PREVENT); + + // Create discussion by s1. + $discussiondata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'userid' => $s1->id, + ]; + $discussion1 = $forumgenerator->create_discussion($discussiondata); + + // Create discussion by s2. + $discussiondata->userid = $s2->id; + $discussion2 = $forumgenerator->create_discussion($discussiondata); + + // Create discussion by s3. + $discussiondata->userid = $s3->id; + $discussion3 = $forumgenerator->create_discussion($discussiondata); + + // Post a normal reply to s1's discussion in forum 1 as the editing teacher. + $replydata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'discussion' => $discussion1->id, + 'userid' => $t1->id, + ]; + $forumgenerator->create_post($replydata); + + // Post a normal reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $forumgenerator->create_post($replydata); + + // Post a normal reply to s3's discussion as the editing teacher. + $replydata->discussion = $discussion3->id; + $forumgenerator->create_post($replydata); + + // Post a private reply to s1's discussion in forum 1 as the editing teacher. + $replydata->discussion = $discussion1->id; + $replydata->userid = $t1->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + // Post another private reply to s1 as the teacher. + $forumgenerator->create_post($replydata); + + // Post a private reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $replydata->privatereplyto = $s2->id; + $forumgenerator->create_post($replydata); + + // Create discussion by s1 in forum 2. + $discussiondata->forum = $forum2->id; + $discussiondata->userid = $s1->id; + $discussion21 = $forumgenerator->create_discussion($discussiondata); + + // Post a private reply to s1's discussion in forum 2 as the editing teacher. + $replydata->discussion = $discussion21->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + + // Let's count! + // S1 should see 8 unread posts 3 discussions posts + 2 private replies + 3 normal replies. + $result = forum_tp_get_course_unread_posts($s1->id, $course->id); + $unreadcounts = $result[$forum1->id]; + $this->assertEquals(8, $unreadcounts->unread); + + // S2 should see 7 unread posts 3 discussions posts + 1 private reply + 3 normal replies. + $result = forum_tp_get_course_unread_posts($s2->id, $course->id); + $unreadcounts = $result[$forum1->id]; + $this->assertEquals(7, $unreadcounts->unread); + + // S3 should see 6 unread posts 3 discussions posts + 3 normal replies. No private replies. + $result = forum_tp_get_course_unread_posts($s3->id, $course->id); + $unreadcounts = $result[$forum1->id]; + $this->assertEquals(6, $unreadcounts->unread); + + // The editing teacher should see 9 unread posts in forum 1: 3 discussions posts + 3 normal replies + 3 private replies. + $result = forum_tp_get_course_unread_posts($t1->id, $course->id); + $unreadcounts = $result[$forum1->id]; + $this->assertEquals(9, $unreadcounts->unread); + + // Same with the non-editing teacher, since they can read private replies by default. + $result = forum_tp_get_course_unread_posts($t2->id, $course->id); + $unreadcounts = $result[$forum1->id]; + $this->assertEquals(9, $unreadcounts->unread); + + // But for forum 2, the non-editing teacher should only see 1 unread which is s1's discussion post. + $unreadcounts = $result[$forum2->id]; + $this->assertEquals(1, $unreadcounts->unread); + } + + /** + * Test the logic in the forum_tp_count_forum_unread_posts() function when private replies are present but without + * separate group mode. This should yield the same results returned by forum_tp_get_course_unread_posts(). + * + * @covers ::forum_tp_count_forum_unread_posts + */ + public function test_forum_tp_count_forum_unread_posts_with_private_replies() { + global $DB; + + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + + // Create 3 students. + $s1 = $generator->create_user(['username' => 's1', 'trackforums' => 1]); + $s2 = $generator->create_user(['username' => 's2', 'trackforums' => 1]); + $s3 = $generator->create_user(['username' => 's3', 'trackforums' => 1]); + // Editing teacher. + $t1 = $generator->create_user(['username' => 't1', 'trackforums' => 1]); + // Non-editing teacher. + $t2 = $generator->create_user(['username' => 't2', 'trackforums' => 1]); + + // Create our course. + $course = $generator->create_course(); + + // Enrol editing and non-editing teachers. + $generator->enrol_user($t1->id, $course->id, 'editingteacher'); + $generator->enrol_user($t2->id, $course->id, 'teacher'); + + // Create forums. + $forum1 = $generator->create_module('forum', ['course' => $course->id]); + $forum2 = $generator->create_module('forum', ['course' => $course->id]); + $forumgenerator = $generator->get_plugin_generator('mod_forum'); + + // Prevent the non-editing teacher from reading private replies in forum 2. + $teacherroleid = $DB->get_field('role', 'id', ['shortname' => 'teacher']); + $forum2cm = get_coursemodule_from_instance('forum', $forum2->id); + $forum2context = context_module::instance($forum2cm->id); + role_change_permission($teacherroleid, $forum2context, 'mod/forum:readprivatereplies', CAP_PREVENT); + + // Create discussion by s1. + $discussiondata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'userid' => $s1->id, + ]; + $discussion1 = $forumgenerator->create_discussion($discussiondata); + + // Create discussion by s2. + $discussiondata->userid = $s2->id; + $discussion2 = $forumgenerator->create_discussion($discussiondata); + + // Create discussion by s3. + $discussiondata->userid = $s3->id; + $discussion3 = $forumgenerator->create_discussion($discussiondata); + + // Post a normal reply to s1's discussion in forum 1 as the editing teacher. + $replydata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'discussion' => $discussion1->id, + 'userid' => $t1->id, + ]; + $forumgenerator->create_post($replydata); + + // Post a normal reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $forumgenerator->create_post($replydata); + + // Post a normal reply to s3's discussion as the editing teacher. + $replydata->discussion = $discussion3->id; + $forumgenerator->create_post($replydata); + + // Post a private reply to s1's discussion in forum 1 as the editing teacher. + $replydata->discussion = $discussion1->id; + $replydata->userid = $t1->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + // Post another private reply to s1 as the teacher. + $forumgenerator->create_post($replydata); + + // Post a private reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $replydata->privatereplyto = $s2->id; + $forumgenerator->create_post($replydata); + + // Create discussion by s1 in forum 2. + $discussiondata->forum = $forum2->id; + $discussiondata->userid = $s1->id; + $discussion11 = $forumgenerator->create_discussion($discussiondata); + + // Post a private reply to s1's discussion in forum 2 as the editing teacher. + $replydata->discussion = $discussion11->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + + // Let's count! + // S1 should see 8 unread posts 3 discussions posts + 2 private replies + 3 normal replies. + $this->setUser($s1); + $forum1cm = get_coursemodule_from_instance('forum', $forum1->id); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(8, $result); + + // S2 should see 7 unread posts 3 discussions posts + 1 private reply + 3 normal replies. + $this->setUser($s2); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(7, $result); + + // S3 should see 6 unread posts 3 discussions posts + 3 normal replies. No private replies. + $this->setUser($s3); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(6, $result); + + // The editing teacher should see 9 unread posts in forum 1: 3 discussions posts + 3 normal replies + 3 private replies. + $this->setUser($t1); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(9, $result); + + // Same with the non-editing teacher, since they can read private replies by default. + $this->setUser($t2); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(9, $result); + + // But for forum 2, the non-editing teacher should only see 1 unread which is s1's discussion post. + $result = forum_tp_count_forum_unread_posts($forum2cm, $course); + $this->assertEquals(1, $result); + } + + /** + * Test the logic in the forum_tp_count_forum_unread_posts() function when private replies are present and group modes are set. + * + * @covers ::forum_tp_count_forum_unread_posts + */ + public function test_forum_tp_count_forum_unread_posts_with_private_replies_and_separate_groups() { + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + + // Create 3 students. + $s1 = $generator->create_user(['username' => 's1', 'trackforums' => 1]); + $s2 = $generator->create_user(['username' => 's2', 'trackforums' => 1]); + // Editing teacher. + $t1 = $generator->create_user(['username' => 't1', 'trackforums' => 1]); + + // Create our course. + $course = $generator->create_course(); + + // Enrol students, editing and non-editing teachers. + $generator->enrol_user($s1->id, $course->id, 'student'); + $generator->enrol_user($s2->id, $course->id, 'student'); + $generator->enrol_user($t1->id, $course->id, 'editingteacher'); + + // Create groups. + $g1 = $generator->create_group(['courseid' => $course->id]); + $g2 = $generator->create_group(['courseid' => $course->id]); + $generator->create_group_member(['groupid' => $g1->id, 'userid' => $s1->id]); + $generator->create_group_member(['groupid' => $g2->id, 'userid' => $s2->id]); + + // Create forums. + $forum1 = $generator->create_module('forum', ['course' => $course->id, 'groupmode' => SEPARATEGROUPS]); + $forum2 = $generator->create_module('forum', ['course' => $course->id, 'groupmode' => VISIBLEGROUPS]); + $forumgenerator = $generator->get_plugin_generator('mod_forum'); + + // Create discussion by s1. + $discussiondata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'userid' => $s1->id, + 'groupid' => $g1->id, + ]; + $discussion1 = $forumgenerator->create_discussion($discussiondata); + + // Create discussion by s2. + $discussiondata->userid = $s2->id; + $discussiondata->groupid = $g2->id; + $discussion2 = $forumgenerator->create_discussion($discussiondata); + + // Post a normal reply to s1's discussion in forum 1 as the editing teacher. + $replydata = (object)[ + 'course' => $course->id, + 'forum' => $forum1->id, + 'discussion' => $discussion1->id, + 'userid' => $t1->id, + ]; + $forumgenerator->create_post($replydata); + + // Post a normal reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $forumgenerator->create_post($replydata); + + // Post a private reply to s1's discussion in forum 1 as the editing teacher. + $replydata->discussion = $discussion1->id; + $replydata->userid = $t1->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + // Post another private reply to s1 as the teacher. + $forumgenerator->create_post($replydata); + + // Post a private reply to s2's discussion as the editing teacher. + $replydata->discussion = $discussion2->id; + $replydata->privatereplyto = $s2->id; + $forumgenerator->create_post($replydata); + + // Create discussion by s1 in forum 2. + $discussiondata->forum = $forum2->id; + $discussiondata->userid = $s1->id; + $discussiondata->groupid = $g1->id; + $discussion21 = $forumgenerator->create_discussion($discussiondata); + + // Post a private reply to s1's discussion in forum 2 as the editing teacher. + $replydata->discussion = $discussion21->id; + $replydata->privatereplyto = $s1->id; + $forumgenerator->create_post($replydata); + + // Let's count! + // S1 should see 4 unread posts in forum 1 (1 discussions post + 2 private replies + 1 normal reply). + $this->setUser($s1); + $forum1cm = get_coursemodule_from_instance('forum', $forum1->id); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(4, $result); + + // S2 should see 3 unread posts in forum 1 (1 discussions post + 1 private reply + 1 normal reply). + $this->setUser($s2); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(3, $result); + + // S2 should see 1 unread posts in forum 2 (visible groups, 1 discussion post from s1). + $forum2cm = get_coursemodule_from_instance('forum', $forum2->id); + $result = forum_tp_count_forum_unread_posts($forum2cm, $course, true); + $this->assertEquals(1, $result); + + // The editing teacher should still see 7 unread posts (2 discussions posts + 2 normal replies + 3 private replies) + // in forum 1 since they have the capability to view all groups by default. + $this->setUser($t1); + $result = forum_tp_count_forum_unread_posts($forum1cm, $course, true); + $this->assertEquals(7, $result); + } + /** * Test the logic in the test_forum_tp_get_untracked_forums() function. */