From 9e381efd1f744a028c46cca1700e7b86f908925e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 14 Aug 2015 01:21:57 +0200 Subject: [PATCH] MDL-50109 forum: Respect chronological order of blog posts in prev/next The function forum_get_discussion_neighbours() is used to generate links to neighbour discussions. I had to change the signature of the function to have access to the forum's type without the need to re-read it from the database in additional query. The SQL query for blogs is a variant of the existing query, just using the first post's creation time instead of the discussion's timemodified time. --- mod/forum/discuss.php | 2 +- mod/forum/lib.php | 67 ++++++++++++++++------ mod/forum/tests/lib_test.php | 106 +++++++++++++++++------------------ 3 files changed, 104 insertions(+), 71 deletions(-) diff --git a/mod/forum/discuss.php b/mod/forum/discuss.php index d5fed0bfd60..0fd6e771a41 100644 --- a/mod/forum/discuss.php +++ b/mod/forum/discuss.php @@ -266,7 +266,7 @@ if (!$canreply and $forum->type !== 'news') { } // Output the links to neighbour discussions. -$neighbours = forum_get_discussion_neighbours($cm, $discussion); +$neighbours = forum_get_discussion_neighbours($cm, $discussion, $forum); $neighbourlinks = $renderer->neighbouring_discussion_navigation($neighbours['prev'], $neighbours['next']); echo $neighbourlinks; diff --git a/mod/forum/lib.php b/mod/forum/lib.php index 05fc7017e83..577ce6279eb 100644 --- a/mod/forum/lib.php +++ b/mod/forum/lib.php @@ -2704,20 +2704,24 @@ function forum_get_discussions($cm, $forumsort="d.timemodified DESC", $fullpost= * other mean to sort the records, e.g. we cannot use IDs as a greater ID can have a lower * timemodified. * + * For blog-style forums, the calculation is based on the original creation time of the + * blog post. + * * Please note that this does not check whether or not the discussion passed is accessible * by the user, it simply uses it as a reference to find the neighbours. On the other hand, * the returned neighbours are checked and are accessible to the current user. * * @param object $cm The CM record. * @param object $discussion The discussion record. + * @param object $forum The forum instance record. * @return array That always contains the keys 'prev' and 'next'. When there is a result * they contain the record with minimal information such as 'id' and 'name'. * When the neighbour is not found the value is false. */ -function forum_get_discussion_neighbours($cm, $discussion) { +function forum_get_discussion_neighbours($cm, $discussion, $forum) { global $CFG, $DB, $USER; - if ($cm->instance != $discussion->forum) { + if ($cm->instance != $discussion->forum or $discussion->forum != $forum->id or $forum->id != $cm->instance) { throw new coding_exception('Discussion is not part of the same forum.'); } @@ -2762,25 +2766,54 @@ function forum_get_discussion_neighbours($cm, $discussion) { } } - $params['forumid'] = $cm->instance; - $params['discid'] = $discussion->id; - $params['disctimemodified'] = $discussion->timemodified; + if ($forum->type === 'blog') { + $params['forumid'] = $cm->instance; + $params['discid1'] = $discussion->id; + $params['discid2'] = $discussion->id; - $sql = "SELECT d.id, d.name, d.timemodified, d.groupid, d.timestart, d.timeend - FROM {forum_discussions} d - WHERE d.forum = :forumid - AND d.id <> :discid - $timelimit - $groupselect"; + $sql = "SELECT d.id, d.name, d.timemodified, d.groupid, d.timestart, d.timeend + FROM {forum_discussions} d + JOIN {forum_posts} p ON d.firstpost = p.id + WHERE d.forum = :forumid + AND d.id <> :discid1 + $timelimit + $groupselect"; - $prevsql = $sql . " AND d.timemodified < :disctimemodified - ORDER BY d.timemodified DESC"; + $sub = "SELECT pp.created + FROM {forum_discussions} dd + JOIN {forum_posts} pp ON dd.firstpost = pp.id + WHERE dd.id = :discid2"; - $nextsql = $sql . " AND d.timemodified > :disctimemodified - ORDER BY d.timemodified ASC"; + $prevsql = $sql . " AND p.created < ($sub) + ORDER BY p.created DESC"; - $neighbours['prev'] = $DB->get_record_sql($prevsql, $params, IGNORE_MULTIPLE); - $neighbours['next'] = $DB->get_record_sql($nextsql, $params, IGNORE_MULTIPLE); + $nextsql = $sql . " AND p.created > ($sub) + ORDER BY p.created ASC"; + + $neighbours['prev'] = $DB->get_record_sql($prevsql, $params, IGNORE_MULTIPLE); + $neighbours['next'] = $DB->get_record_sql($nextsql, $params, IGNORE_MULTIPLE); + + } else { + $params['forumid'] = $cm->instance; + $params['discid'] = $discussion->id; + $params['disctimemodified'] = $discussion->timemodified; + + $sql = "SELECT d.id, d.name, d.timemodified, d.groupid, d.timestart, d.timeend + FROM {forum_discussions} d + WHERE d.forum = :forumid + AND d.id <> :discid + $timelimit + $groupselect"; + + $prevsql = $sql . " AND d.timemodified < :disctimemodified + ORDER BY d.timemodified DESC"; + + $nextsql = $sql . " AND d.timemodified > :disctimemodified + ORDER BY d.timemodified ASC"; + + $neighbours['prev'] = $DB->get_record_sql($prevsql, $params, IGNORE_MULTIPLE); + $neighbours['next'] = $DB->get_record_sql($nextsql, $params, IGNORE_MULTIPLE); + } return $neighbours; } diff --git a/mod/forum/tests/lib_test.php b/mod/forum/tests/lib_test.php index 19419b79377..ce39bd1f80b 100644 --- a/mod/forum/tests/lib_test.php +++ b/mod/forum/tests/lib_test.php @@ -821,23 +821,23 @@ class mod_forum_lib_testcase extends advanced_testcase { $disc5 = $forumgen->create_discussion($record); // Getting the neighbours. - $neighbours = forum_get_discussion_neighbours($cm, $disc1); + $neighbours = forum_get_discussion_neighbours($cm, $disc1, $forum); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc2->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc2); + $neighbours = forum_get_discussion_neighbours($cm, $disc2, $forum); $this->assertEquals($disc1->id, $neighbours['prev']->id); $this->assertEquals($disc3->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc3); + $neighbours = forum_get_discussion_neighbours($cm, $disc3, $forum); $this->assertEquals($disc2->id, $neighbours['prev']->id); $this->assertEquals($disc4->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc4); + $neighbours = forum_get_discussion_neighbours($cm, $disc4, $forum); $this->assertEquals($disc3->id, $neighbours['prev']->id); $this->assertEquals($disc5->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc5); + $neighbours = forum_get_discussion_neighbours($cm, $disc5, $forum); $this->assertEquals($disc4->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -847,28 +847,28 @@ class mod_forum_lib_testcase extends advanced_testcase { $disc1->timemodified = time(); $DB->update_record('forum_discussions', $disc1); - $neighbours = forum_get_discussion_neighbours($cm, $disc5); + $neighbours = forum_get_discussion_neighbours($cm, $disc5, $forum); $this->assertEquals($disc4->id, $neighbours['prev']->id); $this->assertEquals($disc1->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc2); + $neighbours = forum_get_discussion_neighbours($cm, $disc2, $forum); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc3->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc1); + $neighbours = forum_get_discussion_neighbours($cm, $disc1, $forum); $this->assertEquals($disc5->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); // After some discussions were created. sleep(1); $disc6 = $forumgen->create_discussion($record); - $neighbours = forum_get_discussion_neighbours($cm, $disc6); + $neighbours = forum_get_discussion_neighbours($cm, $disc6, $forum); $this->assertEquals($disc1->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); sleep(1); $disc7 = $forumgen->create_discussion($record); - $neighbours = forum_get_discussion_neighbours($cm, $disc7); + $neighbours = forum_get_discussion_neighbours($cm, $disc7, $forum); $this->assertEquals($disc6->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -905,59 +905,59 @@ class mod_forum_lib_testcase extends advanced_testcase { // Admin user ignores the timed settings of discussions. $this->setAdminUser(); - $neighbours = forum_get_discussion_neighbours($cm, $disc8); + $neighbours = forum_get_discussion_neighbours($cm, $disc8, $forum); $this->assertEquals($disc7->id, $neighbours['prev']->id); $this->assertEquals($disc9->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc9); + $neighbours = forum_get_discussion_neighbours($cm, $disc9, $forum); $this->assertEquals($disc8->id, $neighbours['prev']->id); $this->assertEquals($disc10->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc10); + $neighbours = forum_get_discussion_neighbours($cm, $disc10, $forum); $this->assertEquals($disc9->id, $neighbours['prev']->id); $this->assertEquals($disc11->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc11); + $neighbours = forum_get_discussion_neighbours($cm, $disc11, $forum); $this->assertEquals($disc10->id, $neighbours['prev']->id); $this->assertEquals($disc12->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc12); + $neighbours = forum_get_discussion_neighbours($cm, $disc12, $forum); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); // Normal user can see their own timed discussions. $this->setUser($user); - $neighbours = forum_get_discussion_neighbours($cm, $disc8); + $neighbours = forum_get_discussion_neighbours($cm, $disc8, $forum); $this->assertEquals($disc7->id, $neighbours['prev']->id); $this->assertEquals($disc9->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc9); + $neighbours = forum_get_discussion_neighbours($cm, $disc9, $forum); $this->assertEquals($disc8->id, $neighbours['prev']->id); $this->assertEquals($disc10->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc10); + $neighbours = forum_get_discussion_neighbours($cm, $disc10, $forum); $this->assertEquals($disc9->id, $neighbours['prev']->id); $this->assertEquals($disc11->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc11); + $neighbours = forum_get_discussion_neighbours($cm, $disc11, $forum); $this->assertEquals($disc10->id, $neighbours['prev']->id); $this->assertEquals($disc12->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc12); + $neighbours = forum_get_discussion_neighbours($cm, $disc12, $forum); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); // Normal user does not ignore timed settings. $this->setUser($user2); - $neighbours = forum_get_discussion_neighbours($cm, $disc8); + $neighbours = forum_get_discussion_neighbours($cm, $disc8, $forum); $this->assertEquals($disc7->id, $neighbours['prev']->id); $this->assertEquals($disc10->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc10); + $neighbours = forum_get_discussion_neighbours($cm, $disc10, $forum); $this->assertEquals($disc8->id, $neighbours['prev']->id); $this->assertEquals($disc12->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm, $disc12); + $neighbours = forum_get_discussion_neighbours($cm, $disc12, $forum); $this->assertEquals($disc10->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -973,11 +973,11 @@ class mod_forum_lib_testcase extends advanced_testcase { $disc2 = $DB->get_record('forum_discussions', array('id' => $disc2->id)); $disc3 = $DB->get_record('forum_discussions', array('id' => $disc3->id)); - $neighbours = forum_get_discussion_neighbours($cm, $disc2); + $neighbours = forum_get_discussion_neighbours($cm, $disc2, $forum); $this->assertEquals($disc12->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); - $neighbours = forum_get_discussion_neighbours($cm, $disc3); + $neighbours = forum_get_discussion_neighbours($cm, $disc3, $forum); $this->assertEquals($disc12->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); } @@ -1050,38 +1050,38 @@ class mod_forum_lib_testcase extends advanced_testcase { // Admin user can see all groups. $this->setAdminUser(); - $neighbours = forum_get_discussion_neighbours($cm1, $disc11); + $neighbours = forum_get_discussion_neighbours($cm1, $disc11, $forum1); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc12->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc21); + $neighbours = forum_get_discussion_neighbours($cm2, $disc21, $forum2); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc22->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc12); + $neighbours = forum_get_discussion_neighbours($cm1, $disc12, $forum1); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEquals($disc13->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc22); + $neighbours = forum_get_discussion_neighbours($cm2, $disc22, $forum2); $this->assertEquals($disc21->id, $neighbours['prev']->id); $this->assertEquals($disc23->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc13); + $neighbours = forum_get_discussion_neighbours($cm1, $disc13, $forum1); $this->assertEquals($disc12->id, $neighbours['prev']->id); $this->assertEquals($disc14->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc23); + $neighbours = forum_get_discussion_neighbours($cm2, $disc23, $forum2); $this->assertEquals($disc22->id, $neighbours['prev']->id); $this->assertEquals($disc24->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc14); + $neighbours = forum_get_discussion_neighbours($cm1, $disc14, $forum1); $this->assertEquals($disc13->id, $neighbours['prev']->id); $this->assertEquals($disc15->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc24); + $neighbours = forum_get_discussion_neighbours($cm2, $disc24, $forum2); $this->assertEquals($disc23->id, $neighbours['prev']->id); $this->assertEquals($disc25->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc15); + $neighbours = forum_get_discussion_neighbours($cm1, $disc15, $forum1); $this->assertEquals($disc14->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); - $neighbours = forum_get_discussion_neighbours($cm2, $disc25); + $neighbours = forum_get_discussion_neighbours($cm2, $disc25, $forum2); $this->assertEquals($disc24->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -1090,24 +1090,24 @@ class mod_forum_lib_testcase extends advanced_testcase { $this->assertEquals($group1->id, groups_get_activity_group($cm1, true)); $this->assertEquals($group1->id, groups_get_activity_group($cm2, true)); - $neighbours = forum_get_discussion_neighbours($cm1, $disc11); + $neighbours = forum_get_discussion_neighbours($cm1, $disc11, $forum1); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc13->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc21); + $neighbours = forum_get_discussion_neighbours($cm2, $disc21, $forum2); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc23->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc13); + $neighbours = forum_get_discussion_neighbours($cm1, $disc13, $forum1); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEquals($disc15->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc23); + $neighbours = forum_get_discussion_neighbours($cm2, $disc23, $forum2); $this->assertEquals($disc21->id, $neighbours['prev']->id); $this->assertEquals($disc25->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc15); + $neighbours = forum_get_discussion_neighbours($cm1, $disc15, $forum1); $this->assertEquals($disc13->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); - $neighbours = forum_get_discussion_neighbours($cm2, $disc25); + $neighbours = forum_get_discussion_neighbours($cm2, $disc25, $forum2); $this->assertEquals($disc23->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -1117,10 +1117,10 @@ class mod_forum_lib_testcase extends advanced_testcase { $this->assertEquals(0, groups_get_activity_group($cm1, true)); // They can see anything in visible groups. - $neighbours = forum_get_discussion_neighbours($cm1, $disc12); + $neighbours = forum_get_discussion_neighbours($cm1, $disc12, $forum1); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEquals($disc13->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc13); + $neighbours = forum_get_discussion_neighbours($cm1, $disc13, $forum1); $this->assertEquals($disc12->id, $neighbours['prev']->id); $this->assertEquals($disc14->id, $neighbours['next']->id); @@ -1129,15 +1129,15 @@ class mod_forum_lib_testcase extends advanced_testcase { $_POST['group'] = 0; $this->assertEquals(0, groups_get_activity_group($cm2, true)); - $neighbours = forum_get_discussion_neighbours($cm2, $disc23); + $neighbours = forum_get_discussion_neighbours($cm2, $disc23, $forum2); $this->assertEmpty($neighbours['prev']); $this->assertEmpty($neighbours['next']); - $neighbours = forum_get_discussion_neighbours($cm2, $disc22); + $neighbours = forum_get_discussion_neighbours($cm2, $disc22, $forum2); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc23->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc24); + $neighbours = forum_get_discussion_neighbours($cm2, $disc24, $forum2); $this->assertEquals($disc23->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); @@ -1148,30 +1148,30 @@ class mod_forum_lib_testcase extends advanced_testcase { $this->assertEquals($group1->id, groups_get_activity_group($cm2, true)); // They can see non-grouped or same group. - $neighbours = forum_get_discussion_neighbours($cm1, $disc11); + $neighbours = forum_get_discussion_neighbours($cm1, $disc11, $forum1); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc13->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc21); + $neighbours = forum_get_discussion_neighbours($cm2, $disc21, $forum2); $this->assertEmpty($neighbours['prev']); $this->assertEquals($disc23->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc13); + $neighbours = forum_get_discussion_neighbours($cm1, $disc13, $forum1); $this->assertEquals($disc11->id, $neighbours['prev']->id); $this->assertEquals($disc15->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm2, $disc23); + $neighbours = forum_get_discussion_neighbours($cm2, $disc23, $forum2); $this->assertEquals($disc21->id, $neighbours['prev']->id); $this->assertEquals($disc25->id, $neighbours['next']->id); - $neighbours = forum_get_discussion_neighbours($cm1, $disc15); + $neighbours = forum_get_discussion_neighbours($cm1, $disc15, $forum1); $this->assertEquals($disc13->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); - $neighbours = forum_get_discussion_neighbours($cm2, $disc25); + $neighbours = forum_get_discussion_neighbours($cm2, $disc25, $forum2); $this->assertEquals($disc23->id, $neighbours['prev']->id); $this->assertEmpty($neighbours['next']); // Querying the neighbours of a discussion passing the wrong CM. $this->setExpectedException('coding_exception'); - forum_get_discussion_neighbours($cm2, $disc11); + forum_get_discussion_neighbours($cm2, $disc11, $forum2); } public function test_count_discussion_replies_basic() {