Skip to content

Commit

Permalink
MDL-50993 forum: Display timed posts/discussions in a logical order
Browse files Browse the repository at this point in the history 
Users that don't have permission to view timed posts outside of the release
time frame will have discussions that have entered the visible frame appear
in an odd order from their point of view on the discussion list.

Example:
Discussion 1, modified 2015-01-01, hidden till 2015-01-03
Discussion 2, modified 2015-01-02, not hidden

The standard 'modified descending' order means that D2 is listed at the top
even after D1 becomes visible. When scanning the list of discussions for new
posts, the user could be tricked into thinking they'd already read it.

This fix instead takes into account the release time of the discussion when
timed forum posts are enabled.

I opted to use CASE statements to handle this instead of GREATEST as the
latter is not supported by MSSQL.
  • Loading branch information
@aolley @ryanwyllie
aolley authored and ryanwyllie committed Oct 19, 2015
1 parent 1b7bf43 commit 1e36665
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 18 deletions.
7 changes: 6 additions & 1 deletion blocks/news_items/block_news_items.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@ function get_content() {

/// Get all the recent discussions we're allowed to see

if (! $discussions = forum_get_discussions($cm, 'p.modified DESC', false,
// This block displays the most recent posts in a forum in
// descending order. The call to default sort order here will use
// that unless the discussion that post is in has a timestart set
// in the future.
$sort = forum_get_default_sort_order(true, 'p.modified');
if (! $discussions = forum_get_discussions($cm, $sort, false,
$currentgroup, $this->page->course->newsitems) ) {
$text .= '('.get_string('nonews', 'forum').')';
$this->content->text = $text;
Expand Down
69 changes: 62 additions & 7 deletions mod/forum/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2473,7 +2473,7 @@ function forum_count_discussions($forum, $cm, $course) {
* Use FORUM_POSTS_ALL_USER_GROUPS for all the user groups
* @return array
*/
function forum_get_discussions($cm, $forumsort="d.timemodified DESC", $fullpost=true, $unused=-1, $limit=-1,
function forum_get_discussions($cm, $forumsort="", $fullpost=true, $unused=-1, $limit=-1,
$userlastmodified=false, $page=-1, $perpage=0, $groupid = -1) {
global $CFG, $DB, $USER;

Expand Down Expand Up @@ -2568,7 +2568,7 @@ function forum_get_discussions($cm, $forumsort="d.timemodified DESC", $fullpost=
$groupselect = "";
}
if (empty($forumsort)) {
$forumsort = "d.timemodified DESC";
$forumsort = forum_get_default_sort_order();
}
if (empty($fullpost)) {
$postdata = "p.id,p.subject,p.modified,p.discussion,p.userid";
Expand Down Expand Up @@ -2707,11 +2707,37 @@ function forum_get_discussion_neighbours($cm, $discussion, $forum) {
$timelimit
$groupselect";

$prevsql = $sql . " AND d.timemodified < :disctimemodified
ORDER BY d.timemodified DESC";
if (empty($CFG->forum_enabletimedposts)) {
$prevsql = $sql . " AND d.timemodified < :disctimemodified";
$nextsql = $sql . " AND d.timemodified > :disctimemodified";

$nextsql = $sql . " AND d.timemodified > :disctimemodified
ORDER BY d.timemodified ASC";
} else {
// Normally we would just use the timemodified for sorting
// discussion posts. However, when timed discussions are enabled,
// then posts need to be sorted base on the later of timemodified
// or the release date of the post (timestart).
$params['disctimecompare'] = $discussion->timemodified;
if ($discussion->timemodified < $discussion->timestart) {
$params['disctimecompare'] = $discussion->timestart;
}
$params['disctimecompare2'] = $params['disctimecompare'];

// Here we need to take into account the release time (timestart)
// if one is set, of the neighbouring posts and compare it to the
// timestart or timemodified of *this* post depending on if the
// release date of this post is in the future or not.
// This stops discussions that appear later because of the
// timestart value from being buried under discussions that were
// made afterwards.
$prevsql = $sql . " AND CASE WHEN d.timemodified < d.timestart
THEN d.timestart < :disctimecompare
ELSE d.timemodified < :disctimecompare2 END";
$nextsql = $sql . " AND CASE WHEN d.timemodified < d.timestart
THEN d.timestart > :disctimecompare
ELSE d.timemodified > :disctimecompare2 END";
}
$prevsql .= ' ORDER BY '.forum_get_default_sort_order();
$nextsql .= ' ORDER BY '.forum_get_default_sort_order(false);

$neighbours['prev'] = $DB->get_record_sql($prevsql, $params, IGNORE_MULTIPLE);
$neighbours['next'] = $DB->get_record_sql($nextsql, $params, IGNORE_MULTIPLE);
Expand All @@ -2720,6 +2746,35 @@ function forum_get_discussion_neighbours($cm, $discussion, $forum) {
return $neighbours;
}

/**
* Get the sql to use in the ORDER BY clause for forum discussions.
*
* This has the ordering take timed discussion windows into account.
*
* @param bool $desc True for DESC, False for ASC.
* @param string $compare The field in the SQL to compare to normally sort by.
* @param string $prefix The prefix being used for the discussion table.
* @return string
*/
function forum_get_default_sort_order($desc = true, $compare = 'd.timemodified', $prefix = 'd') {
global $CFG;

if (!empty($prefix)) {
$prefix .= '.';
}

$dir = $desc ? 'DESC' : 'ASC';

$sort = "{$prefix}timemodified";
if (!empty($CFG->forum_enabletimedposts)) {
$sort = "CASE WHEN {$compare} < {$prefix}timestart
THEN {$prefix}timestart
ELSE {$compare}
END";
}
return "$sort $dir";
}

/**
*
* @global object
Expand Down Expand Up @@ -5133,7 +5188,7 @@ function forum_print_latest_discussions($course, $forum, $maxdiscussions = -1, $
$context = context_module::instance($cm->id);

if (empty($sort)) {
$sort = "d.timemodified DESC";
$sort = forum_get_default_sort_order();
}

$olddiscussionlink = false;
Expand Down
39 changes: 29 additions & 10 deletions mod/forum/tests/lib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -903,19 +903,30 @@ public function test_forum_get_neighbours() {
$record->timestart = $past;
$record->timeend = $future;
$disc12 = $forumgen->create_discussion($record);
sleep(1);
$record->timestart = $future + 1; // Should be last post for those that can see it.
$record->timeend = 0;
$disc13 = $forumgen->create_discussion($record);

// Admin user ignores the timed settings of discussions.
// Post ordering taking into account timestart:
// 8 = t
// 10 = t+3
// 11 = t+4
// 12 = t+5
// 9 = t+60
// 13 = t+61.
$this->setAdminUser();
$neighbours = forum_get_discussion_neighbours($cm, $disc8, $forum);
$this->assertEquals($disc7->id, $neighbours['prev']->id);
$this->assertEquals($disc9->id, $neighbours['next']->id);
$this->assertEquals($disc10->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc9, $forum);
$this->assertEquals($disc8->id, $neighbours['prev']->id);
$this->assertEquals($disc10->id, $neighbours['next']->id);
$this->assertEquals($disc12->id, $neighbours['prev']->id);
$this->assertEquals($disc13->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc10, $forum);
$this->assertEquals($disc9->id, $neighbours['prev']->id);
$this->assertEquals($disc8->id, $neighbours['prev']->id);
$this->assertEquals($disc11->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc11, $forum);
Expand All @@ -924,20 +935,24 @@ public function test_forum_get_neighbours() {

$neighbours = forum_get_discussion_neighbours($cm, $disc12, $forum);
$this->assertEquals($disc11->id, $neighbours['prev']->id);
$this->assertEquals($disc9->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc13, $forum);
$this->assertEquals($disc9->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, $forum);
$this->assertEquals($disc7->id, $neighbours['prev']->id);
$this->assertEquals($disc9->id, $neighbours['next']->id);
$this->assertEquals($disc10->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc9, $forum);
$this->assertEquals($disc8->id, $neighbours['prev']->id);
$this->assertEquals($disc10->id, $neighbours['next']->id);
$this->assertEquals($disc12->id, $neighbours['prev']->id);
$this->assertEquals($disc13->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc10, $forum);
$this->assertEquals($disc9->id, $neighbours['prev']->id);
$this->assertEquals($disc8->id, $neighbours['prev']->id);
$this->assertEquals($disc11->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc11, $forum);
Expand All @@ -946,6 +961,10 @@ public function test_forum_get_neighbours() {

$neighbours = forum_get_discussion_neighbours($cm, $disc12, $forum);
$this->assertEquals($disc11->id, $neighbours['prev']->id);
$this->assertEquals($disc9->id, $neighbours['next']->id);

$neighbours = forum_get_discussion_neighbours($cm, $disc13, $forum);
$this->assertEquals($disc9->id, $neighbours['prev']->id);
$this->assertEmpty($neighbours['next']);

// Normal user does not ignore timed settings.
Expand Down Expand Up @@ -975,11 +994,11 @@ public function test_forum_get_neighbours() {
$disc3 = $DB->get_record('forum_discussions', array('id' => $disc3->id));

$neighbours = forum_get_discussion_neighbours($cm, $disc2, $forum);
$this->assertEquals($disc12->id, $neighbours['prev']->id);
$this->assertEquals($disc13->id, $neighbours['prev']->id);
$this->assertEmpty($neighbours['next']);

$neighbours = forum_get_discussion_neighbours($cm, $disc3, $forum);
$this->assertEquals($disc12->id, $neighbours['prev']->id);
$this->assertEquals($disc13->id, $neighbours['prev']->id);
$this->assertEmpty($neighbours['next']);
}

Expand Down

0 comments on commit 1e36665

Please sign in to comment.