472310 - When replies are made to older threads they do not appear on first page of the forum

Status: VERIFIED FIXED

(support.mozilla.org :: Forum, task)

Description

[Tony-E]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090105 Firefox/3.2a1pre
Build Identifier: 

I believe this occured with 0.8, the bug is best explained with an example.

If you are going through older forum threads and post a reply to a thread that is on page 5, the thread remains on page 5 of the list, though it will jump to the top of page 5.

Previously if you replied to an older thread, the thread would then appear on page 1 of the forum index.

This makes it hard to keep track of any new replies to old threads. 

Reproducible: Always

Comment 1

[Alice Wyman]

Firefox Contributors forum topic on this, posted 26 of Dec, 2008:
http://support.mozilla.com/tiki-view_forum_thread.php?comments_parentId=238679&forumId=3 

The problem is also mentioned here:
http://support.mozilla.com/tiki-view_forum_thread.php?forumId=3&comments_parentId=244829

Comment 2

[[:Cww]]

Confirmed.  Also if you increase the number of posts per page in the URL so that it'd include the thread in its original position (&comments_per_page=200), it ends up getting shown on the first page where it should be.

It seems like when finding what threads to display on a given page it uses the commentDate of the original poster not the latest reply.  But when it sorts the list it sorts correctly by lastPost.  (That's completely a guess...)

Comment 3

[E. Cooper [:ecooper]]

It's essentially what's happening. The first query gets the threads by commentDate, and those threadIds are then used in the second query to get the posts for each thread. Last post info isn't available until the second query.

The original query was a problem. This patch within Comments::get_forum_topics causes the regression: https://bugzilla.mozilla.org/attachment.cgi?id=352122&action=diff

(In reply to comment #2)
> Confirmed.  Also if you increase the number of posts per page in the URL so
> that it'd include the thread in its original position (&comments_per_page=200),
> it ends up getting shown on the first page where it should be.
> 
> It seems like when finding what threads to display on a given page it uses the
> commentDate of the original poster not the latest reply.  But when it sorts the
> list it sorts correctly by lastPost.  (That's completely a guess...)

Comment 4

[[:Cww]]

(In reply to comment #3)

Ok, found the bug but you are NOT asking me to patch it :P

in /webroot/lib/commentslib.php line 678, you have:

$query .= " where a.`object`=? and a.`objectType` = 'forum' and a.`parentId` = ? ";

and then set the second ? to 0.

Which means that you're only finding posts with parentId=0 which means when you sort by commentDate later in the query, you only sort by the commentDate of the parent post.

So... I can go on a long walkabout and waste a lot people's resources and time (thanks for trying laura, ecooper, zzxc, stephend) to make the patch myself or someone else can patch for me :)

Comment 5

[[:Cww]]

Attachment: patch with new query

With a lot of patience from a lot of people... here's a patch.

Note: this offsets some of the perf gain from splitting the queries since we now need to sort on a calculated entity again but it's just one sort.

There are other ways to do this that won't require an extra sort but they're messy at best.

Comment 6

[E. Cooper [:ecooper]]

Comment on attachment 358484 [details] [diff] [review]
patch with new query

>Index: commentslib.php
>===================================================================
>--- commentslib.php	(revision 21647)
>+++ commentslib.php	(working copy)
>@@ -671,13 +671,12 @@
> 	$params = array();
> 	$ret = array();
> 
>-        $query = "SELECT DISTINCT (IF(a.parentId = 0, a.threadId, a.parentId)) as threadId,
>+        $query = "SELECT (IF(a.parentId = 0, a.threadId, a.parentId)) as threadId, MAX(a.commentDate) as 

I think you may need to change threadId in the alias assignment to something like topicId. Otherwise, the "GROUP BY threadId" might end up being interpreted as "GROUP BY a.threadId". 

This will probably give the appearance of a working query in an active forum.

I'll test this locally tonight.

Comment 7

[E. Cooper [:ecooper]]

(In reply to comment #6)
> I think you may need to change threadId in the alias assignment to something
> like topicId. Otherwise, the "GROUP BY threadId" might end up being interpreted
> as "GROUP BY a.threadId". 
> 
> This will probably give the appearance of a working query in an active forum.
> 
> I'll test this locally tonight.

I don't know if this is a local nuance, but the current patch fails to get distinct parentIds and you can end up with duplicate ids being passed to the second query. Duplicate ids will result in less than 20 threads being displayed per page.

More specifically, the "group by" is failing and is including the parentId of the newest messages in the return set regardless if they are already present in the set.

Steps to reproduce:
1. Post in a topic off the first page or create a new one
2. Continue to post in that topic four or five times
3. Return to the forum index

If this isn't a local MySQL quark, less than 20 topics should be listed. I have an updated patch ready to post once someone can verify that this issue isn't something isolated to my local testing box.

Comment 8

[[:Cww]]

Ooops, missed the e-mail.  Yes you're right this is a problem (I didn't check it, sorry).

I can patch it to work (I probably SHOULD have used different column names but I was lazy... so much for that) or I can review your patch since you already have it written.

Comment 9

[[:Cww]]

I mean I did verify that it's a problem on my local copy but I didn't check that this was a problem when I wrote it.  Sigh, 3 AM bugmail sorting is not ideal)

Comment 10

[E. Cooper [:ecooper]]

Attachment: Cww's patch v2

Tested locally. Didn't load test.

Comment 11

[[:Cww]]

The code looks good here, I'll do some hardcore testing in a bit but it's a r+ for solving the problem.

Comment 12

[E. Cooper [:ecooper]]

Attachment: Basic Benchmarks

Okay, so some notes about these numbers. These times are actually the time it takes for TikiLib::query to execute and return. They are not the raw SQL execution time. The test was run locally on my laptop due to some maintenance happening on my dedicated test/sandbox server. And finally, the time control performance feature currently used on production wasn't used so I could get the queries to trudge through a decent sized dataset.

All in all, take look at these with a grain of salt. I figure, if anything, they can show the potential difference in performance, though.

Comment 13

[[:Cww]]

Comment on attachment 358959 [details] [diff] [review]
Cww's patch v2

r+

Comment 14

[E. Cooper [:ecooper]]

In r21980 / r21981

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; I made a post in http://support-stage.mozilla.org/tiki-view_forum_thread.php?locale=en-US&comments_parentId=233149&forumId=1 and it showed up on both the homepage and the top of the Firefox forum list, here:

http://support-stage.mozilla.org/tiki-view_forum.php?locale=en-US&forumId=1

Comment 16

[Cor vL (cor-el)]

I recently noticed that this bug still occurs if I use poster=cor-el (_me) or other posters like poster=the-edmeister who post a lot.
Subsequent pages still show posts that belong on page one (comments_offset=0).
I had wondered for a while why I missed so many answers to threads where I posted in.

Is this a new bug or is this bug not fixed for the poster Filter?

To speed up things:
[1] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=0
[2] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=50
[3] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=100
[4] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=150
[5] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=200
[6] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=250
[7] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=300
[8] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=350
[9] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=400
[10] https://support.mozilla.com/tiki-view_forum.php?forumId=1&comments_threshold=0&thread_sort_mode=lastPost_desc&comments_per_page=50&time_control=1209600&poster=cor-el&comments_offset=450

Comment 17

[[:Cww]]

Filed bug 498105