fix ORDER BY for flat comment display setting

I'm getting the same issue - if an admin edits a post, the date stamp remains the same, if the user edits it it changes the datestamp. Since we're using forums it can bring posts in front of all of the replies.

Well... we really want that the comment date should be updated if it was edited.
Other time we want to maintain the old date because was an admin edition.

I don't have sure that this is a bug... anyway, more feedback is needed.

This is an example of what happens with a flat forums setup:

as you can see, edited comments jump after the original ones, disrupting the logic of the discussion.

my suggestion is that, it should be like below

Forum post.....

comment 2: sdfdfsf
comment 1: sdfjflsdf

after changes in the comment 1 for the above post, it should be like below

Forum post....
comment 1: sldfsfwsdf (recently updated)
comment 2: sdfsdf

in that case, we can know that comment 1 itself have been updated recently. does this make sense?

Thanks catch. This way it is really a bug and mbchandar gave a possible solution.

The question is: how do we want for comments order?

can we make the comment order by 'timestamp' field in the comments table?

I think comments should be ordered the way they were originally posted.

At the moment we get the following:

Forum Topic
Post 1
Post 2 (replying to post 1)
Post 3

Then post 1 is edited for a typo and you get:

Forum Topic
Post 2 (replying to post 1)
Post 3
Post 1

Although there are variations depending on threading (which is also arbitrary in a flat setup).

If someone really wants to add new content, surely they'd post a new comment rather than update an existing one? (unless they specifically wanted the new content to be in the location of the original post).

If some people really still want "last updated time" to be the arbiter, then maybe there could be an option in comments admistration where you choose between posted and last updated.

The other possibility would be to have an option on flat forums, where each comment is automatically a reply to the previous comment - this came up here: http://drupal.org/node/84012 and this related issue: http://drupal.org/node/81373

http://drupal.org/node/73399
and http://drupal.org/node/170724

were duplicates

OK this patch is by Slartibartfast from the above duplicate issue which I've just marked as such. It's in the wrong format and the solution isn't clean, but here for reference.

Also confirmed that there's no change to comment_save in Drupal 6 so upping version.

Another option compared to the above patch would be to simply not update the timestamp when comments are edited. That'd be a two line patch which I'll roll later on this evening with a bit of luck.

For a full explanation of the bug in comment display, see #63 (note the second reply appears before the first).

The timestamp change only affects the display of flat comments - not threaded, so not reproducable on drupal.org

As far as I can tell, this will also stop updated comments from being marked as new if they've simply been edited for typos, and stop them appearing in the new comments block for the same reason - which I'd also consider a bug rather than a feature.

Note there's no "updated" column, just the one timestamp available, so regardless of these two bugs I think it makes sense.

I don't see anywhere obvious where fixing this bug would break something else or change functionality.

Attached patch simply removes the timestamp column from the update db_query in comment_save when a comment is being edited - leaving the original timestamp intact as it should be.

ack!

same ol' hacked_with_an_axe patch against 5.1 code if that matters, only in appropriate format this time... no time (nor drupal know-how) to do something smarter. I guess that nice thing to do would be to add hidden field with timestamp for non-admin users. That way admin can change timestamp if necessary, and normal users would have same timestamp when edit their post...

As far as I remember/know, changing the timestamp when a comment is edited is in fact a *feature*. If someone included new information in their comment, then it should be marked as new. The built in comment views list comments by their original submission order (thread field in the db), not their last edited dates, so there this should not be a problem.

On flat expanded there is very much a bug, not a feature - with comments appearing before the posts they reply to in many cases if the original comment is updated. I think this may be down to the fact that many people who reply to flat expanded comments don't care about 'threading' since it's not visually represented at all. Either way it's a very confusing bug, and having comments appear in the wrong place is in my mind much more a problem than whether they get an updated mark for a typo or not.

Either an 'updated' timestamp, or a 'truly flat comments' setting which removes all threading for 'flat expanded' would be better obviously, but that won't get into D6.

To see it in action, I refer again to: http://libcom.org/forums/feedback/comment-jumping-test-again-please-dont...

and the png attached.

As this problem also exists in Drupal 5.2 trying to make a real flat forum, I´ve digged through the code. The best solution I can think of is not to change the update query as I really want the timestamp for normal users to be updated to now, but instead to change the order in the SELECT for flat comments.

For Drupal 5.2:

Change line 984 from
$query .= ' ORDER BY c.timestamp DESC';

to
$query .= ' ORDER BY c.cid DESC';

Change line 992 from
$query .= ' ORDER BY c.timestamp';

to
$query .= ' ORDER BY c.cid';

This way flat comments will always be in chronological order, but also save an admin changed timestamp or the actual timestamp for normal users.

Would be great to see this in CVS for Druapal 5.2 and all newer versions.

Greats from Germany
LeisureLarry

This change has to be made in comment.module in the directory of your comments module.

Leisure Larry, that's a much better suggestion.

I've rolled that fix into a new patch against 6.x - this is a minor change and works as expected. If it's acceptable for Drupal 6 I think a backport to 5.x would be worthwhile as well.

I confirm the bug, reproduced it on 6.x-dev updated real site, and I've also seen weird re-appearances of "new" comments on my site before, only didn't have time to examine it properly. Turns out it's this issue. (BTW - beside other unrelated ones: flat forum is very common case, and Drupal have a lot of problems with it.)

Patch didn't apply to recent 6.x-dev, but I recreated it manually and works as expected.

I consider this a bug-fix for 6.x, and rather important one: The ordering in flat-mode is a way of threading, in the sense of determining which post should be shown below which, and so order by last update is plain wrong. Think of it this way: A question must be always shown ABOVE answer, no matter how many times it was corrected. Otherwise the whole thread is one big mess. So I agree that sort by CREATED time is the correct one, and since 'cid' is AUTO INCREMENT, I believe it's safe to use it for this purpose.

Another problem is with the "Recent comments" block, where the order is also incorrect IMO (the whole block is a kind of comments-flat-view too) - I believe this should show recently ADDED comments, not all typo-fixes. This part may be discussed, feel free to remove it from the patch if agreed upon.

New patch attached, against 6.x-dev and with the "Recent comments" block fix.

Another story is, why updated comments are marked as "New", which is very confusing. But that's stuff for a new issue, unrelated to this sort-problem.

Hmm, can't see why the previous patch failed for me, when it's identical to the one finally generated (let alone the add-on). Perhaps I've to find some problem with my Patch utility... :-/

This is a re-roll of JirkaRybka's patch in #22 which should apply cleanly. Wasn't created from the drupal root originally hence the issues applying it.

I think if this is fix is accepted for comment display on nodes, it's good for the comment block too.

ack!

Actually it's this way :)

Let me do a little summary, because I really want to have this in, or at least considered:

- There's a nasty bug, disrupting FLAT-comment discussions if the posts are edited by non-admin users (i.e. just fixing typos), as demonstrated in #10 and #13. (I confirm)

- First discussed solution was to avoid changing the timestamp on edited comments, to keep them in right place. (The latest patch for this approach seems to be #15.) But #16 and #17 stated that updating timestamps is a *feature* for threaded mode, which anyway does sort the comments on different basis (threading) and so doesn't exhibit the bug (so not reproducible on Drupal.org). Still, as #17 confirmed, the bug exists for flat mode and needs to be solved.

- That said, we left the timestamp-behavior unchanged in database, to avoid affecting the threaded view, and focused only just to the flat DISPLAYING code, using the (auto-increment) cid field to retrieve correct (original) order of the comments. This is the #20 patch.

- I proposed to do the same also for "Recent comments" block, which I believe is of similar nature (and certainly exhibits the bug too). If this is accepted - the resulting patch is #24. Otherwise #20.

An addition to this:

This is a non-api breaking change for Drupal 6. There looks to be possible work going on in comment module for Drupal 7, and I think an extra field in the database for "updated" would fix this issue more satisfactorily. At this point in the release cycle I can't see this breaking anything else, but it would make this particular bug go away with a couple of lines changed. Any future work on comment module to handle flat comments more satisfactorily is going to have to touch those lines again anyway, so it won't hold up more significant changes later on.

subscribing

Re-rolled #24 for today's 6.x-dev (fixing just an offset, almost the same file). No changes to the patch.

Any chance to get some review/progress here?

One thing I thought of is that it'd make it impossible for an admin to re-order a comment by changing the timestamp - however you can't do this in threaded mode anyway, and it's more of an edge case than the steps to reproduce this bug which are pretty common with the particular setting it affects.

This patch's code looks good (not tested) and might actually be a small performance improvement. The "ORDER BY c.cid" might be redundant though; I think SQL sorts using the primary key by default. It doesn't hurt to make it explicit though.

FWIW, I applied #24 to my Drupal 5.2 site and it works as expected. I'd like to see this make it for Drupal 6 as this issue is a big problem for flat forums. Before finding this patched I had patched comment.module to not allow users to edit their comments so as to prevent them from being moved out of order. This patch handles the problem in a much better way.

I'm not marking RTBC since I haven't tested on a Drupal 6 site, but it's a simple patch that fixes a nasty bug. The code changes look good. I think there's more that could be done to fix flat forums (many of the things have been mentioned in this issue already), but this patch goes a long way towards making flat display of comments more useful and error free.

Please, pretty please... Test if you can, and hit the RTBC if you like. I don't feel comfortable about doing it myself, as the last significant change was added by me, but I'm very close to that all the time. Seems we quite agreed on the solution here.

Given the comments by Dries and aclight I'm going to bump this to RTBC (I'm also deassigning myself since it isn't my patch!).

Yes, looks good indeed. Thanks, committed.

this bug is going to cause major problems for ifac if it's not fixed first. ifac is set to deploy very shortly, so i'm attaching a re-rolled patch for D5, tested as working (only difference was the offset).

Applies cleanly, patch is good, and fixes what will quickly become a serious problem on drupal.org once we deploy the changes to make issue followups true core comments. Definitely RTBC. Thanks.

Committed to 5.x.