From eb451c798163378aa5f5e9e63d486b3e18bb87c2 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Tue, 28 Oct 2014 08:31:31 +0800 Subject: [PATCH] MDL-47903 mod_forum: Add discussion subscription time This helps us to only send notifications for messages the user should have already seen. --- mod/forum/classes/subscriptions.php | 31 ++++----- mod/forum/db/install.xml | 4 +- mod/forum/db/upgrade.php | 17 ++++- mod/forum/discuss.php | 5 +- mod/forum/lib.php | 7 ++ mod/forum/tests/events_test.php | 18 ++--- mod/forum/tests/mail_test.php | 93 ++++++++++++++++++++++++++ mod/forum/tests/subscriptions_test.php | 12 ++-- mod/forum/version.php | 2 +- 9 files changed, 148 insertions(+), 41 deletions(-) diff --git a/mod/forum/classes/subscriptions.php b/mod/forum/classes/subscriptions.php index 554e65f6437..f431f2d2715 100644 --- a/mod/forum/classes/subscriptions.php +++ b/mod/forum/classes/subscriptions.php @@ -34,13 +34,6 @@ defined('MOODLE_INTERNAL') || die(); */ class subscriptions { - /** - * The status value for a subscribed discussion. - * - * @const int - */ - const FORUM_DISCUSSION_SUBSCRIBED = 1; - /** * The status value for an unsubscribed discussion. * @@ -126,7 +119,7 @@ class subscriptions { // Check whether there is a record for this discussion subscription. if (isset($subscriptions[$discussionid])) { - return ($subscriptions[$discussionid] == self::FORUM_DISCUSSION_SUBSCRIBED); + return ($subscriptions[$discussionid] != self::FORUM_DISCUSSION_UNSUBSCRIBED); } return self::is_subscribed_to_forum($userid, $forum); @@ -605,14 +598,18 @@ class subscriptions { if ($userrequest) { $discussionsubscriptions = $DB->get_recordset('forum_discussion_subs', array('userid' => $userid, 'forum' => $forum->id)); - $DB->delete_records('forum_discussion_subs', - array('userid' => $userid, 'forum' => $forum->id, 'preference' => self::FORUM_DISCUSSION_SUBSCRIBED)); + $DB->delete_records_select('forum_discussion_subs', + 'userid = :userid AND forum = :forumid AND preference <> :preference', array( + 'userid' => $userid, + 'forumid' => $forum->id, + 'preference' => self::FORUM_DISCUSSION_UNSUBSCRIBED, + )); // Reset the subscription caches for this forum. // We know that the there were previously entries and there aren't any more. if (isset(self::$forumdiscussioncache[$userid]) && isset(self::$forumdiscussioncache[$userid][$forum->id])) { foreach (self::$forumdiscussioncache[$userid][$forum->id] as $discussionid => $preference) { - if ($preference == self::FORUM_DISCUSSION_SUBSCRIBED) { + if ($preference != self::FORUM_DISCUSSION_UNSUBSCRIBED) { unset(self::$forumdiscussioncache[$userid][$forum->id][$discussionid]); } } @@ -716,7 +713,7 @@ class subscriptions { // First check whether the user is subscribed to the discussion already. $subscription = $DB->get_record('forum_discussion_subs', array('userid' => $userid, 'discussion' => $discussion->id)); if ($subscription) { - if ($subscription->preference == self::FORUM_DISCUSSION_SUBSCRIBED) { + if ($subscription->preference != self::FORUM_DISCUSSION_UNSUBSCRIBED) { // The user is already subscribed to the discussion. Ignore. return false; } @@ -733,17 +730,17 @@ class subscriptions { } } else { if ($subscription) { - $subscription->preference = self::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $DB->update_record('forum_discussion_subs', $subscription); } else { $subscription = new \stdClass(); $subscription->userid = $userid; $subscription->forum = $discussion->forum; $subscription->discussion = $discussion->id; - $subscription->preference = self::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); - self::$forumdiscussioncache[$userid][$discussion->forum][$discussion->id] = true; + self::$forumdiscussioncache[$userid][$discussion->forum][$discussion->id] = $subscription->preference; } } @@ -785,7 +782,7 @@ class subscriptions { } // No discussion-level preference. Check for a forum level subscription. if (!$DB->record_exists('forum_subscriptions', array('userid' => $userid, 'forum' => $discussion->forum))) { - if ($subscription && $subscription->preference == self::FORUM_DISCUSSION_SUBSCRIBED) { + if ($subscription && $subscription->preference != self::FORUM_DISCUSSION_UNSUBSCRIBED) { // The user is not subscribed to the forum, but subscribed from the discussion, delete the discussion subscription. $DB->delete_records('forum_discussion_subs', array('id' => $subscription->id)); unset(self::$forumdiscussioncache[$userid][$discussion->forum][$discussion->id]); @@ -806,7 +803,7 @@ class subscriptions { $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); } - self::$forumdiscussioncache[$userid][$discussion->forum][$discussion->id] = false; + self::$forumdiscussioncache[$userid][$discussion->forum][$discussion->id] = $subscription->preference; } $context = forum_get_context($discussion->forum, $context); diff --git a/mod/forum/db/install.xml b/mod/forum/db/install.xml index 10867e81c8a..73537586623 100644 --- a/mod/forum/db/install.xml +++ b/mod/forum/db/install.xml @@ -1,5 +1,5 @@ - @@ -173,7 +173,7 @@ - + diff --git a/mod/forum/db/upgrade.php b/mod/forum/db/upgrade.php index c241541f201..52edb2e16f0 100644 --- a/mod/forum/db/upgrade.php +++ b/mod/forum/db/upgrade.php @@ -182,7 +182,7 @@ function xmldb_forum_upgrade($oldversion) { $table->add_field('forum', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); $table->add_field('userid', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); $table->add_field('discussion', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); - $table->add_field('preference', XMLDB_TYPE_INTEGER, '1', null, XMLDB_NOTNULL, null, '1'); + $table->add_field('preference', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '1'); // Adding keys to table forum_discussion_subs. $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); @@ -197,7 +197,20 @@ function xmldb_forum_upgrade($oldversion) { } // Forum savepoint reached. - upgrade_mod_savepoint(true, 2014081900, 'forum'); + upgrade_mod_savepoint(true, 2014102800, 'forum'); + } + + if ($oldversion > 2014081900 && $oldversion < 2014102800) { + + // Changing precision of field preference on table forum_discussion_subs to (10). + $table = new xmldb_table('forum_discussion_subs'); + $field = new xmldb_field('preference', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, '1', 'discussion'); + + // Launch change of precision for field preference. + $dbman->change_field_precision($table, $field); + + // Forum savepoint reached. + upgrade_mod_savepoint(true, 2014102800, 'forum'); } return true; diff --git a/mod/forum/discuss.php b/mod/forum/discuss.php index 30bed3c2177..000f445d63e 100644 --- a/mod/forum/discuss.php +++ b/mod/forum/discuss.php @@ -114,12 +114,13 @@ // And also for the discussion being moved. \mod_forum\subscriptions::fill_subscription_cache($forum->id); $subscriptionchanges = array(); + $subscriptiontime = time(); foreach ($potentialsubscribers as $subuser) { $userid = $subuser->id; $targetsubscription = \mod_forum\subscriptions::is_subscribed($userid, $forumto, null, $cmto); if (\mod_forum\subscriptions::is_subscribed($userid, $forum, $discussion->id)) { if (!$targetsubscription) { - $subscriptionchanges[$userid] = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscriptionchanges[$userid] = $subscriptiontime; } } else { if ($targetsubscription) { @@ -136,7 +137,7 @@ $newdiscussion = clone $discussion; $newdiscussion->forum = $forumto->id; foreach ($subscriptionchanges as $userid => $preference) { - if ($preference === \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED) { + if ($preference != \mod_forum\subscriptions::FORUM_DISCUSSION_UNSUBSCRIBED) { // Users must have viewdiscussion to a discussion. if (has_capability('mod/forum:viewdiscussion', $destinationctx, $userid)) { \mod_forum\subscriptions::subscribe_user_to_discussion($userid, $newdiscussion, $destinationctx); diff --git a/mod/forum/lib.php b/mod/forum/lib.php index 1ecd6c33623..a21b2248874 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -630,6 +630,13 @@ function forum_cron() { continue; } + if ($subscriptiontime = \mod_forum\subscriptions::fetch_discussion_subscription($forum->id, $userto->id)) { + // Skip posts if the user subscribed to the discussion after it was created. + if (isset($subscriptiontime[$post->discussion]) && ($subscriptiontime[$post->discussion] > $post->created)) { + continue; + } + } + // Don't send email if the forum is Q&A and the user has not posted. // Initial topics are still mailed. if ($forum->type == 'qanda' && !forum_get_user_posted_time($discussion->id, $userto->id) && $pid != $discussion->firstpost) { diff --git a/mod/forum/tests/events_test.php b/mod/forum/tests/events_test.php index 0c017bc5d3e..22edf3bc9ea 100644 --- a/mod/forum/tests/events_test.php +++ b/mod/forum/tests/events_test.php @@ -1914,7 +1914,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -1973,7 +1973,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2027,7 +2027,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2078,7 +2078,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2129,7 +2129,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2317,7 +2317,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2371,7 +2371,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2422,7 +2422,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); @@ -2473,7 +2473,7 @@ class mod_forum_events_testcase extends advanced_testcase { $subscription->userid = $user->id; $subscription->forum = $forum->id; $subscription->discussion = $discussion->id; - $subscription->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $subscription->preference = time(); $subscription->id = $DB->insert_record('forum_discussion_subs', $subscription); diff --git a/mod/forum/tests/mail_test.php b/mod/forum/tests/mail_test.php index 29239ecf430..959c4413df1 100644 --- a/mod/forum/tests/mail_test.php +++ b/mod/forum/tests/mail_test.php @@ -132,6 +132,7 @@ class mod_forum_mail_testcase extends advanced_testcase { * @return array An array of the messages caught by the message sink */ protected function helper_run_cron_check_count($post, $expected) { + // Clear the sinks before running cron. $this->helper->messagesink->clear(); $this->helper->mailsink->clear(); @@ -149,6 +150,35 @@ class mod_forum_mail_testcase extends advanced_testcase { return $messages; } + /** + * Run the forum cron, and check that the specified posts were sent the + * specified number of times. + * + * @param stdClass $post The forum post object + * @param integer $expected The number of times that the post should have been sent + * @return array An array of the messages caught by the message sink + */ + protected function helper_run_cron_check_counts($posts, $expected) { + + // Clear the sinks before running cron. + $this->helper->messagesink->clear(); + $this->helper->mailsink->clear(); + + // Cron daily uses mtrace, turn on buffering to silence output. + foreach ($posts as $post) { + $this->expectOutputRegex("/{$post['count']} users were sent post {$post['id']}, '{$post['subject']}'/"); + } + forum_cron(); + + // Now check the results in the message sink. + $messages = $this->helper->messagesink->get_messages(); + + // There should be the expected number of messages. + $this->assertEquals($expected, count($messages)); + + return $messages; + } + public function test_forced_subscription() { $this->resetAfterTest(true); @@ -409,6 +439,9 @@ class mod_forum_mail_testcase extends advanced_testcase { // Unsubscribe the 'author' user from the discussion. \mod_forum\subscriptions::unsubscribe_user_from_discussion($author->id, $discussion); + $this->assertFalse(\mod_forum\subscriptions::is_subscribed($author->id, $forum, $discussion->id)); + $this->assertTrue(\mod_forum\subscriptions::is_subscribed($recipient->id, $forum, $discussion->id)); + // We expect only one user to receive this post. $expected = 1; @@ -547,4 +580,64 @@ class mod_forum_mail_testcase extends advanced_testcase { // Run cron and check that the expected number of users received the notification. $messages = $this->helper_run_cron_check_count($post, $expected); } + + /** + * Test that a user unsubscribed from a forum who has subscribed to a discussion, only receives posts made after + * they subscribed to the discussion. + */ + public function test_forum_discussion_subscription_forum_unsubscribed_discussion_subscribed_after_post() { + $this->resetAfterTest(true); + global $DB; + + // Create a course, with a forum. + $course = $this->getDataGenerator()->create_course(); + + $options = array('course' => $course->id, 'forcesubscribe' => FORUM_CHOOSESUBSCRIBE); + $forum = $this->getDataGenerator()->create_module('forum', $options); + + $expectedmessages = array(); + + // Create a user enrolled in the course as a student. + list($author) = $this->helper_create_users($course, 1); + + // Post a discussion to the forum. + list($discussion, $post) = $this->helper_post_to_forum($forum, $author); + + // Update the original post to have a timecreated in the past by a few minutes. + $post->created = $post->created - 600; + $DB->update_record('forum_posts', $post); + + $expectedmessages[] = array( + 'id' => $post->id, + 'subject' => $post->subject, + 'count' => 0, + ); + + // Then subscribe the user to the discussion. + $this->assertTrue(\mod_forum\subscriptions::subscribe_user_to_discussion($author->id, $discussion)); + + // Then post a reply to the first discussion. + $record = new stdClass(); + $record->course = $forum->course; + $record->userid = $author->id; + $record->forum = $forum->id; + $record->discussion = $discussion->id; + $record->mailnow = 1; + $record->created = time(); + $generator = $this->getDataGenerator()->get_plugin_generator('mod_forum'); + $reply = $generator->create_post($record); + + $expectedmessages[] = array( + 'id' => $reply->id, + 'subject' => $reply->subject, + 'count' => 1, + ); + + // We expect there to be two notifications, and not three. + $expectedcount = 1; + + // Run cron and check that the expected number of users received the notification. + $messages = $this->helper_run_cron_check_counts($expectedmessages, $expectedcount); + + } } diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php index f161bdba495..8f57abc1106 100644 --- a/mod/forum/tests/subscriptions_test.php +++ b/mod/forum/tests/subscriptions_test.php @@ -79,6 +79,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { * @param array An array containing the discussion object, and the post object */ protected function helper_post_to_forum($forum, $author) { + global $DB; $generator = $this->getDataGenerator()->get_plugin_generator('mod_forum'); // Create a discussion in the forum, and then add a post to that discussion. @@ -88,13 +89,8 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $record->forum = $forum->id; $discussion = $generator->create_discussion($record); - $record = new stdClass(); - $record->course = $forum->course; - $record->userid = $author->id; - $record->forum = $forum->id; - $record->discussion = $discussion->id; - $record->mailnow = 1; - $post = $generator->create_post($record); + // Retrieve the post which was created by create_discussion. + $post = $DB->get_record('forum_posts', array('discussion' => $discussion->id)); return array($discussion, $post); } @@ -1046,7 +1042,7 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $record->userid = $users[2]->id; $record->forum = $forum->id; $record->discussion = $discussion->id; - $record->preference = \mod_forum\subscriptions::FORUM_DISCUSSION_SUBSCRIBED; + $record->preference = time(); $DB->insert_record('forum_discussion_subs', $record); // The discussion count should not have changed. diff --git a/mod/forum/version.php b/mod/forum/version.php index fc107f3ff91..095752571ae 100644 --- a/mod/forum/version.php +++ b/mod/forum/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2014082101; // The current module version (Date: YYYYMMDDXX) +$plugin->version = 2014102800; // The current module version (Date: YYYYMMDDXX) $plugin->requires = 2014050800; // Requires this Moodle version $plugin->component = 'mod_forum'; // Full name of the plugin (used for diagnostics)