diff --git a/lib/db/install.xml b/lib/db/install.xml old mode 100644 new mode 100755 index 581582675139b..5214ee3447339 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -541,6 +541,7 @@ + @@ -565,6 +566,7 @@ +
diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 2b03cf69b5e9d..26a0cd0fb80c8 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -4177,5 +4177,29 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2015021100.00); } + if ($oldversion < 2015022401.00) { + + // Define index useridfromto (not unique) to be added to message. + $table = new xmldb_table('message'); + $index = new xmldb_index('useridfromto', XMLDB_INDEX_NOTUNIQUE, array('useridfrom', 'useridto')); + + // Conditionally launch add index useridfromto. + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Define index useridfromto (not unique) to be added to message_read. + $table = new xmldb_table('message_read'); + $index = new xmldb_index('useridfromto', XMLDB_INDEX_NOTUNIQUE, array('useridfrom', 'useridto')); + + // Conditionally launch add index useridfromto. + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2015022401.00); + } + return true; } diff --git a/message/lib.php b/message/lib.php index d2041e2b7fea0..dd0ecb13ccca8 100644 --- a/message/lib.php +++ b/message/lib.php @@ -708,32 +708,41 @@ function message_get_recent_conversations($user, $limitfrom=0, $limitto=100) { // There is a separate query for read and unread messages as they are stored // in different tables. They were originally retrieved in one query but it // was so large that it was difficult to be confident in its correctness. - $sql = "SELECT $userfields, + $uniquefield = $DB->sql_concat_join("'-'", array('message.useridfrom', 'message.useridto')); + $sql = "SELECT $uniquefield, $userfields, message.id as mid, message.notification, message.smallmessage, message.fullmessage, message.fullmessagehtml, message.fullmessageformat, message.timecreated, contact.id as contactlistid, contact.blocked - FROM {message_read} message - JOIN {user} otheruser ON otheruser.id = CASE - WHEN message.useridto = :userid1 THEN message.useridfrom - ELSE message.useridto END - LEFT JOIN {message_contacts} contact ON contact.userid = :userid2 AND contact.contactid = otheruser.id - - WHERE otheruser.deleted = 0 - AND (message.useridto = :userid3 OR message.useridfrom = :userid4) - AND message.notification = 0 - AND NOT EXISTS ( - SELECT 1 - FROM {message_read} othermessage - WHERE ((othermessage.useridto = :userid5 AND othermessage.useridfrom = otheruser.id) OR - (othermessage.useridfrom = :userid6 AND othermessage.useridto = otheruser.id)) - AND (othermessage.timecreated > message.timecreated OR ( - othermessage.timecreated = message.timecreated AND othermessage.id > message.id)) - ) - + JOIN ( + SELECT MAX(id) AS messageid, + matchedmessage.useridto, + matchedmessage.useridfrom + FROM {message_read} matchedmessage + INNER JOIN ( + SELECT MAX(recentmessages.timecreated) timecreated, + recentmessages.useridfrom, + recentmessages.useridto + FROM {message_read} recentmessages + WHERE (recentmessages.useridfrom = :userid1 OR recentmessages.useridto = :userid2) + GROUP BY recentmessages.useridfrom, recentmessages.useridto + ) recent ON matchedmessage.useridto = recent.useridto + AND matchedmessage.useridfrom = recent.useridfrom + AND matchedmessage.timecreated = recent.timecreated + GROUP BY matchedmessage.useridto, matchedmessage.useridfrom + ) messagesubset ON messagesubset.messageid = message.id + JOIN {user} otheruser ON (message.useridfrom = :userid4 AND message.useridto = otheruser.id) + OR (message.useridto = :userid5 AND message.useridfrom = otheruser.id) + LEFT JOIN {message_contacts} contact ON contact.userid = :userid3 AND contact.userid = otheruser.id + WHERE otheruser.deleted = 0 AND message.notification = 0 ORDER BY message.timecreated DESC"; - $params = array('userid1' => $user->id, 'userid2' => $user->id, 'userid3' => $user->id, - 'userid4' => $user->id, 'userid5' => $user->id, 'userid6' => $user->id); + $params = array( + 'userid1' => $user->id, + 'userid2' => $user->id, + 'userid3' => $user->id, + 'userid4' => $user->id, + 'userid5' => $user->id, + ); $read = $DB->get_records_sql($sql, $params, $limitfrom, $limitto); // We want to get the messages that have not been read. These are stored in the 'message' table. It is the @@ -742,16 +751,23 @@ function message_get_recent_conversations($user, $limitfrom=0, $limitto=100) { $sql = str_replace('{message_read}', '{message}', $sql); $unread = $DB->get_records_sql($sql, $params, $limitfrom, $limitto); - $conversations = array(); - // Union the 2 result sets together looking for the message with the most // recent timecreated for each other user. // $conversation->id (the array key) is the other user's ID. $conversation_arrays = array($unread, $read); foreach ($conversation_arrays as $conversation_array) { foreach ($conversation_array as $conversation) { - if (empty($conversations[$conversation->id]) || $conversations[$conversation->id]->timecreated < $conversation->timecreated ) { + if (!isset($conversations[$conversation->id])) { $conversations[$conversation->id] = $conversation; + } else { + $current = $conversations[$conversation->id]; + if ($current->timecreated < $conversation->timecreated) { + $conversations[$conversation->id] = $conversation; + } else if ($current->timecreated == $conversation->timecreated) { + if ($current->mid < $conversation->mid) { + $conversations[$conversation->id] = $conversation; + } + } } } } diff --git a/message/tests/messagelib_test.php b/message/tests/messagelib_test.php index 57006d0613250..38fc3fc6ce5bd 100644 --- a/message/tests/messagelib_test.php +++ b/message/tests/messagelib_test.php @@ -72,6 +72,7 @@ protected function send_fake_message($userfrom, $userto, $message = 'Hello world $record->useridto = $userto->id; $record->subject = 'No subject'; $record->fullmessage = $message; + $record->smallmessage = $message; $record->timecreated = time(); return $DB->insert_record('message', $record); @@ -362,68 +363,261 @@ public function test_message_search() { } /** - * Test message_get_recent_conversations. + * The data provider for message_get_recent_conversations. + * + * This provides sets of data to for testing. + * @return array */ - public function test_message_get_recent_conversations() { - global $DB, $USER; - - // Set this user as the admin. - $this->setAdminUser(); - - // Create user's to send messages to/from. - $user1 = $this->getDataGenerator()->create_user(array('firstname' => 'Test1', 'lastname' => 'user1')); - $user2 = $this->getDataGenerator()->create_user(array('firstname' => 'Test2', 'lastname' => 'user2')); - - // Add a few messages that have been read and some that are unread. - $m1 = $this->send_fake_message($USER, $user1, 'Message 1'); // An unread message. - $m2 = $this->send_fake_message($user1, $USER, 'Message 2'); // An unread message. - $m3 = $this->send_fake_message($USER, $user1, 'Message 3'); // An unread message. - $m4 = message_post_message($USER, $user2, 'Message 4', FORMAT_PLAIN); - $m5 = message_post_message($user2, $USER, 'Message 5', FORMAT_PLAIN); - $m6 = message_post_message($USER, $user2, 'Message 6', FORMAT_PLAIN); - - // We want to alter the timecreated values so we can ensure message_get_recent_conversations orders - // by timecreated, not the max id, to begin with. However, we also want more than one message to have - // the same timecreated value to ensure that when this happens we retrieve the one with the maximum id. - - // Store the current time. - $time = time(); - - // Set the first and second unread messages to have the same timecreated value. - $updatemessage = new stdClass(); - $updatemessage->id = $m1; - $updatemessage->timecreated = $time; - $DB->update_record('message', $updatemessage); - - $updatemessage->id = $m2; - $updatemessage->timecreated = $time; - $DB->update_record('message', $updatemessage); - - // Set the third unread message to have a timecreated value of 0. - $updatemessage->id = $m3; - $updatemessage->timecreated = 0; - $DB->update_record('message', $updatemessage); - - // Set the first and second read messages to have the same timecreated value. - $updatemessage->id = $m4; - $updatemessage->timecreated = $time + 1; - $DB->update_record('message', $updatemessage); - - $updatemessage->id = $m5; - $updatemessage->timecreated = $time + 1; - $DB->update_record('message', $updatemessage); - - // Set the third read message to have a timecreated value of 0. - $updatemessage->id = $m6; - $updatemessage->timecreated = 0; - $DB->update_record('message_read', $updatemessage); + public function message_get_recent_conversations_provider() { + return array( + array( + // Test that conversations with multiple contacts is correctly ordered. + 'users' => array( + 'user1', + 'user2', + 'user3', + ), + 'messages' => array( + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'subject' => 'S1', + ), + array( + 'from' => 'user2', + 'to' => 'user1', + 'state' => 'unread', + 'subject' => 'S2', + ), + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'timecreated' => 0, + 'subject' => 'S3', + ), + array( + 'from' => 'user1', + 'to' => 'user3', + 'state' => 'read', + 'timemodifier' => 1, + 'subject' => 'S4', + ), + array( + 'from' => 'user3', + 'to' => 'user1', + 'state' => 'read', + 'timemodifier' => 1, + 'subject' => 'S5', + ), + array( + 'from' => 'user1', + 'to' => 'user3', + 'state' => 'read', + 'timecreated' => 0, + 'subject' => 'S6', + ), + ), + 'expectations' => array( + 'user1' => array( + // User1 has conversed most recently with user3. The most recent message is M5. + array( + 'messageposition' => 0, + 'with' => 'user3', + 'subject' => 'S5', + ), + // User1 has also conversed with user2. The most recent message is S2. + array( + 'messageposition' => 1, + 'with' => 'user2', + 'subject' => 'S2', + ), + ), + 'user2' => array( + // User2 has only conversed with user1. Their most recent shared message was S2. + array( + 'messageposition' => 0, + 'with' => 'user1', + 'subject' => 'S2', + ), + ), + 'user3' => array( + // User3 has only conversed with user1. Their most recent shared message was S5. + array( + 'messageposition' => 0, + 'with' => 'user1', + 'subject' => 'S5', + ), + ), + ), + ), + array( + // Test conversations with a single user, where some messages are read and some are not. + 'users' => array( + 'user1', + 'user2', + ), + 'messages' => array( + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'read', + 'subject' => 'S1', + ), + array( + 'from' => 'user2', + 'to' => 'user1', + 'state' => 'read', + 'subject' => 'S2', + ), + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'timemodifier' => 1, + 'subject' => 'S3', + ), + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'timemodifier' => 1, + 'subject' => 'S4', + ), + ), + 'expectations' => array( + // The most recent message between user1 and user2 was S4. + 'user1' => array( + array( + 'messageposition' => 0, + 'with' => 'user2', + 'subject' => 'S4', + ), + ), + 'user2' => array( + // The most recent message between user1 and user2 was S4. + array( + 'messageposition' => 0, + 'with' => 'user1', + 'subject' => 'S4', + ), + ), + ), + ), + array( + // Test conversations with a single user, where some messages are read and some are not, and messages + // are out of order. + // This can happen through a combination of factors including multi-master DB replication with messages + // read somehow (e.g. API). + 'users' => array( + 'user1', + 'user2', + ), + 'messages' => array( + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'read', + 'subject' => 'S1', + 'timemodifier' => 1, + ), + array( + 'from' => 'user2', + 'to' => 'user1', + 'state' => 'read', + 'subject' => 'S2', + 'timemodifier' => 2, + ), + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'subject' => 'S3', + ), + array( + 'from' => 'user1', + 'to' => 'user2', + 'state' => 'unread', + 'subject' => 'S4', + ), + ), + 'expectations' => array( + // The most recent message between user1 and user2 was S2, even though later IDs have not been read. + 'user1' => array( + array( + 'messageposition' => 0, + 'with' => 'user2', + 'subject' => 'S2', + ), + ), + 'user2' => array( + array( + 'messageposition' => 0, + 'with' => 'user1', + 'subject' => 'S2', + ), + ), + ), + ), + ); + } - // Get the recent conversations for the current user. - $conversations = message_get_recent_conversations($USER); + /** + * Test message_get_recent_conversations with a mixture of messages. + * + * @dataProvider message_get_recent_conversations_provider + * @param array $usersdata The list of users to create for this test. + * @param array $messagesdata The list of messages to create. + * @param array $expectations The list of expected outcomes. + */ + public function test_message_get_recent_conversations($usersdata, $messagesdata, $expectations) { + global $DB; - // Confirm that we have received the messages with the maximum timecreated, rather than the max id. - $this->assertEquals('Message 2', $conversations[0]->fullmessage); - $this->assertEquals('Message 5', $conversations[1]->smallmessage); + // Create all of the users. + $users = array(); + foreach ($usersdata as $username) { + $users[$username] = $this->getDataGenerator()->create_user(array('username' => $username)); + } + + $defaulttimecreated = time(); + foreach ($messagesdata as $messagedata) { + $from = $users[$messagedata['from']]; + $to = $users[$messagedata['to']]; + $subject = $messagedata['subject']; + + if (isset($messagedata['state']) && $messagedata['state'] == 'unread') { + $table = 'message'; + $messageid = $this->send_fake_message($from, $to, $subject, FORMAT_PLAIN); + } else { + // If there is no state, or the state is not 'unread', assume the message is read. + $table = 'message_read'; + $messageid = message_post_message($from, $to, $subject, FORMAT_PLAIN); + } + + $updatemessage = new stdClass(); + $updatemessage->id = $messageid; + if (isset($messagedata['timecreated'])) { + $updatemessage->timecreated = $messagedata['timecreated']; + } else if (isset($messagedata['timemodifier'])) { + $updatemessage->timecreated = $defaulttimecreated + $messagedata['timemodifier']; + } else { + $updatemessage->timecreated = $defaulttimecreated; + } + $DB->update_record($table, $updatemessage); + } + + foreach ($expectations as $username => $data) { + // Get the recent conversations for the specified user. + $user = $users[$username]; + $conversations = message_get_recent_conversations($user); + foreach ($data as $expectation) { + $otheruser = $users[$expectation['with']]; + $conversation = $conversations[$expectation['messageposition']]; + $this->assertEquals($otheruser->id, $conversation->id); + $this->assertEquals($expectation['subject'], $conversation->smallmessage); + } + } } /** diff --git a/version.php b/version.php index 70ac5aa2d75bd..5ec837deccadf 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2015022300.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2015022401.00; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.