From 00f653d75686661807e97410e8f97751723c292d Mon Sep 17 00:00:00 2001 From: Andrew Gosali Date: Tue, 17 Dec 2024 15:50:03 +0700 Subject: [PATCH] MDL-80848 mod_forum: to have id as the backup sorting condition When the first sorting condition fails (falls on the same value), it should have another sorting as a backup. --- .../generator/tests/maketestcourse_test.php | 2 +- mod/forum/classes/local/vaults/post.php | 41 +++++++------- mod/forum/externallib.php | 7 ++- mod/forum/lib.php | 7 ++- mod/forum/tests/generator/lib.php | 2 +- .../tests/vaults_discussion_list_test.php | 54 ++++++++++--------- 6 files changed, 61 insertions(+), 52 deletions(-) diff --git a/admin/tool/generator/tests/maketestcourse_test.php b/admin/tool/generator/tests/maketestcourse_test.php index 433736a1827..975608fb29f 100644 --- a/admin/tool/generator/tests/maketestcourse_test.php +++ b/admin/tool/generator/tests/maketestcourse_test.php @@ -158,7 +158,7 @@ final class maketestcourse_test extends \advanced_testcase { // Users that started discussions are the same. $forums = $modinfo->get_instances_of('forum'); - $discussions = forum_get_discussions(reset($forums), 'd.timemodified ASC'); + $discussions = forum_get_discussions(reset($forums), 'd.timemodified ASC, d.id ASC'); $lastusernumber = 0; $discussionstarters = array(); foreach ($discussions as $discussion) { diff --git a/mod/forum/classes/local/vaults/post.php b/mod/forum/classes/local/vaults/post.php index d587fe35fd2..0a734ae5e0c 100644 --- a/mod/forum/classes/local/vaults/post.php +++ b/mod/forum/classes/local/vaults/post.php @@ -111,7 +111,7 @@ class post extends db_table_vault { stdClass $user, int $discussionid, bool $canseeprivatereplies, - string $orderby = 'created ASC' + string $orderby = 'created ASC, id ASC' ): array { return $this->get_from_discussion_ids($user, [$discussionid], $canseeprivatereplies, $orderby); } @@ -232,7 +232,7 @@ class post extends db_table_vault { stdClass $user, post_entity $post, bool $canseeprivatereplies, - string $orderby = 'created ASC' + string $orderby = 'created ASC, id ASC' ): array { $alias = $this->get_table_alias(); @@ -439,19 +439,15 @@ class post extends db_table_vault { ] = $this->get_private_reply_sql($user, $canseeprivatereplies, "mp"); $sql = " - SELECT posts.* - FROM {" . self::TABLE . "} posts + SELECT p.* + FROM {" . self::TABLE . "} p JOIN ( - SELECT p.discussion, MAX(p.id) as latestpostid - FROM {" . self::TABLE . "} p - JOIN ( - SELECT mp.discussion, MAX(mp.created) AS created - FROM {" . self::TABLE . "} mp - WHERE mp.discussion {$insql} {$privatewhere} - GROUP BY mp.discussion - ) lp ON lp.discussion = p.discussion AND lp.created = p.created - GROUP BY p.discussion - ) plp on plp.discussion = posts.discussion AND plp.latestpostid = posts.id"; + SELECT mp.discussion, mp.created, MAX(mp.id) AS latestpostid + FROM {" . self::TABLE . "} mp + WHERE mp.discussion {$insql} {$privatewhere} + GROUP BY mp.discussion, mp.created + HAVING mp.created = MAX(mp.created) + ) lp on lp.discussion = p.discussion AND lp.latestpostid = p.id"; $records = $this->get_db()->get_records_sql($sql, array_merge($params, $privateparams)); $entities = $this->transform_db_records_to_entities($records); @@ -501,13 +497,14 @@ class post extends db_table_vault { $sql = " SELECT p.* - FROM {" . self::TABLE . "} p - JOIN ( - SELECT mp.discussion, MIN(mp.created) AS created - FROM {" . self::TABLE . "} mp - WHERE mp.discussion {$insql} - GROUP BY mp.discussion - ) lp ON lp.discussion = p.discussion AND lp.created = p.created"; + FROM {" . self::TABLE . "} p + JOIN ( + SELECT mp.discussion, mp.created, MIN(mp.id) AS firstpostid + FROM {" . self::TABLE . "} mp + WHERE mp.discussion {$insql} + GROUP BY mp.discussion, mp.created + HAVING mp.created = MIN(mp.created) + ) fp ON fp.discussion = p.discussion AND fp.firstpostid = p.id"; $records = $this->get_db()->get_records_sql($sql, $params); return $this->transform_db_records_to_entities($records); @@ -526,7 +523,7 @@ class post extends db_table_vault { int $discussionid, int $userid, bool $canseeprivatereplies, - string $orderby = 'created ASC' + string $orderby = 'created ASC, id ASC' ): array { $user = $this->get_db()->get_record('user', ['id' => (int)$userid], '*', IGNORE_MISSING); diff --git a/mod/forum/externallib.php b/mod/forum/externallib.php index 61b19525c4d..26e632e7aac 100644 --- a/mod/forum/externallib.php +++ b/mod/forum/externallib.php @@ -226,6 +226,11 @@ class mod_forum_external extends external_api { 'allowed values are: ' . implode(',', $directionallowedvalues)); } + $orderbysql = "{$sortby} {$sortdirection}"; + if (!empty($sortby) && $sortby != 'id') { + $orderbysql .= ", id {$sortdirection}"; + } + $managerfactory = mod_forum\local\container::get_manager_factory(); $capabilitymanager = $managerfactory->get_capability_manager($forum); @@ -234,7 +239,7 @@ class mod_forum_external extends external_api { $USER, $discussion->get_id(), $capabilitymanager->can_view_any_private_reply($USER), - "{$sortby} {$sortdirection}" + $orderbysql ); $builderfactory = mod_forum\local\container::get_builder_factory(); diff --git a/mod/forum/lib.php b/mod/forum/lib.php index ff4921e9b61..2440b84041a 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -1702,6 +1702,9 @@ function forum_get_discussions($cm, $forumsort="", $fullpost=true, $unused=-1, $ if (empty($forumsort)) { $forumsort = forum_get_default_sort_order(); } + if (!str_contains($forumsort, 'id')) { + $forumsort .= ', d.id DESC'; + } if (empty($fullpost)) { $postdata = "p.id, p.subject, p.modified, p.discussion, p.userid, p.created"; } else { @@ -1737,7 +1740,7 @@ function forum_get_discussions($cm, $forumsort="", $fullpost=true, $unused=-1, $ $umtable WHERE d.forum = ? AND p.parent = 0 $timelimit $groupselect $updatedsincesql - ORDER BY $forumsort, d.id DESC"; + ORDER BY $forumsort"; return $DB->get_records_sql($sql, $params, $limitfrom, $limitnum); } @@ -4831,7 +4834,7 @@ function forum_discussion_update_last_post($discussionid) { $sql = "SELECT id, userid, modified FROM {forum_posts} WHERE discussion=? - ORDER BY modified DESC"; + ORDER BY modified DESC, id DESC"; // Lets go find the last post if (($lastposts = $DB->get_records_sql($sql, array($discussionid), 0, 1))) { diff --git a/mod/forum/tests/generator/lib.php b/mod/forum/tests/generator/lib.php index de276702cc5..db3881de072 100644 --- a/mod/forum/tests/generator/lib.php +++ b/mod/forum/tests/generator/lib.php @@ -254,7 +254,7 @@ class mod_forum_generator extends testing_module_generator { $this->forumpostcount++; // Variable to store time. - $time = time() + $this->forumpostcount; + $time = time(); $record = (array) $record; diff --git a/mod/forum/tests/vaults_discussion_list_test.php b/mod/forum/tests/vaults_discussion_list_test.php index aa9b1f93dc3..fa394adc70d 100644 --- a/mod/forum/tests/vaults_discussion_list_test.php +++ b/mod/forum/tests/vaults_discussion_list_test.php @@ -257,27 +257,31 @@ final class vaults_discussion_list_test extends \advanced_testcase { $forum = $datagenerator->create_module('forum', ['course' => $course->id]); $this->getDataGenerator()->enrol_user($user->id, $course->id, null, 'manual'); - $this->assertEquals([], $vault->get_from_forum_id($forum->id, true, true, - null, 0, 0, $user)); + $this->assertEquals([], $vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, $user->id, + null, 0, 0)); $now = time(); - [$discussion1, $post1] = $this->helper_post_to_forum($forum, $user, ['timestart' => $now - 10, 'timemodified' => 1]); - [$discussion2, $post2] = $this->helper_post_to_forum($forum, $user, ['timestart' => $now - 9, 'timemodified' => 2]); - [$hiddendiscussion, $post3] = $this->helper_post_to_forum($forum, $user, ['timestart' => $now + 10, 'timemodified' => 3]); + [$discussion1, $post1] = $this->helper_post_to_forum($forum, $user, ['timestart' => $now - 10, 'timemodified' => $now + 1]); + [$discussion2, $post2] = $this->helper_post_to_forum($forum, $user, ['timestart' => $now - 9, 'timemodified' => $now + 2]); + [$hiddendiscussion, $post3] = $this->helper_post_to_forum( + $forum, + $user, + ['timestart' => $now + 10, 'timemodified' => $now + 3] + ); [$groupdiscussion1, $post4] = $this->helper_post_to_forum( $forum, $user, - ['timestart' => $now - 8, 'timemodified' => 4, 'groupid' => 1] + ['timestart' => $now - 8, 'timemodified' => $now + 4, 'groupid' => 1] ); [$groupdiscussion2, $post5] = $this->helper_post_to_forum( $forum, $user, - ['timestart' => $now - 7, 'timemodified' => 5, 'groupid' => 2] + ['timestart' => $now - 7, 'timemodified' => $now + 5, 'groupid' => 2] ); [$hiddengroupdiscussion, $post6] = $this->helper_post_to_forum( $forum, $user, - ['timestart' => $now + 11, 'timemodified' => 6, 'groupid' => 3] + ['timestart' => $now + 11, 'timemodified' => $now + 6, 'groupid' => 3] ); $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, @@ -355,10 +359,10 @@ final class vaults_discussion_list_test extends \advanced_testcase { $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, $user->id, $vault::SORTORDER_LASTPOST_DESC, 0, 0)); $this->assertCount(6, $summaries); - $this->assertEquals($groupdiscussion2->id, $summaries[0]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion1->id, $summaries[1]->get_discussion()->get_id()); - $this->assertEquals($hiddendiscussion->id, $summaries[2]->get_discussion()->get_id()); - $this->assertEquals($hiddengroupdiscussion->id, $summaries[3]->get_discussion()->get_id()); + $this->assertEquals($hiddengroupdiscussion->id, $summaries[0]->get_discussion()->get_id()); + $this->assertEquals($hiddendiscussion->id, $summaries[1]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion2->id, $summaries[2]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion1->id, $summaries[3]->get_discussion()->get_id()); $this->assertEquals($discussion2->id, $summaries[4]->get_discussion()->get_id()); $this->assertEquals($discussion1->id, $summaries[5]->get_discussion()->get_id()); @@ -368,10 +372,10 @@ final class vaults_discussion_list_test extends \advanced_testcase { $this->assertCount(6, $summaries); $this->assertEquals($discussion1->id, $summaries[0]->get_discussion()->get_id()); $this->assertEquals($discussion2->id, $summaries[1]->get_discussion()->get_id()); - $this->assertEquals($hiddengroupdiscussion->id, $summaries[2]->get_discussion()->get_id()); - $this->assertEquals($hiddendiscussion->id, $summaries[3]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion1->id, $summaries[4]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion2->id, $summaries[5]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion1->id, $summaries[2]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion2->id, $summaries[3]->get_discussion()->get_id()); + $this->assertEquals($hiddendiscussion->id, $summaries[4]->get_discussion()->get_id()); + $this->assertEquals($hiddengroupdiscussion->id, $summaries[5]->get_discussion()->get_id()); // Sort discussions by replies DESC. $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, @@ -421,27 +425,27 @@ final class vaults_discussion_list_test extends \advanced_testcase { $this->pin_discussion($discussion1); $this->pin_discussion($hiddendiscussion); - $summaries = array_values($vault->get_from_forum_id($forum->id, true, null, + $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, null, $vault::SORTORDER_LASTPOST_DESC, 0, 0)); $this->assertCount(6, $summaries); $this->assertEquals($hiddendiscussion->id, $summaries[0]->get_discussion()->get_id()); $this->assertEquals($discussion1->id, $summaries[1]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion2->id, $summaries[2]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion1->id, $summaries[3]->get_discussion()->get_id()); - $this->assertEquals($hiddengroupdiscussion->id, $summaries[4]->get_discussion()->get_id()); + $this->assertEquals($hiddengroupdiscussion->id, $summaries[2]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion2->id, $summaries[3]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion1->id, $summaries[4]->get_discussion()->get_id()); $this->assertEquals($discussion2->id, $summaries[5]->get_discussion()->get_id()); - $summaries = array_values($vault->get_from_forum_id($forum->id, true, null, + $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, null, $vault::SORTORDER_LASTPOST_ASC, 0, 0)); $this->assertCount(6, $summaries); $this->assertEquals($discussion1->id, $summaries[0]->get_discussion()->get_id()); $this->assertEquals($hiddendiscussion->id, $summaries[1]->get_discussion()->get_id()); $this->assertEquals($discussion2->id, $summaries[2]->get_discussion()->get_id()); - $this->assertEquals($hiddengroupdiscussion->id, $summaries[3]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion1->id, $summaries[4]->get_discussion()->get_id()); - $this->assertEquals($groupdiscussion2->id, $summaries[5]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion1->id, $summaries[3]->get_discussion()->get_id()); + $this->assertEquals($groupdiscussion2->id, $summaries[4]->get_discussion()->get_id()); + $this->assertEquals($hiddengroupdiscussion->id, $summaries[5]->get_discussion()->get_id()); - $summaries = array_values($vault->get_from_forum_id($forum->id, true, null, + $summaries = array_values($vault->get_from_forum_id_and_group_id($forum->id, [1, 2, 3], true, null, $vault::SORTORDER_REPLIES_DESC, 0, 0)); $this->assertCount(6, $summaries); $this->assertEquals($discussion1->id, $summaries[0]->get_discussion()->get_id());