New comment marker does not work

OK I reproduce it on my side with 7.x-dev
Indeed, when I read a node, I can't see which comments are new since my previous visit... (regardless the theme...)

7.x-dev is the current development version (=the most recent/up to date). So we need to put this in the version field if the issue still occurs on the cvs branch

When I change (in the file comment.module, CommentController::attachLoad()) the line :
$comment->new = node_mark($comment->nid, $comment->changed);
to
$comment->new = 1;
then, all the comments are marked as "new". Quite a good news, isn't it?

After doing some quick tests with the node_mark function, I don't find any issue, so I'm wondering if $comment->nid and $comment->changed are correctly set... I'll check this evening. Until this, if someone has a piece of advice...

Wow. There are multiple layers of broken here. I wonder if/how this ever worked in d6? Maybe themes used to take care of the "new" tag?

Broken #1:
node_mark will only return MARK_NEW if the node has never been viewed. But this is not what we want for comments. For comments, the logic, I think, should be:
a) If a node has already been viewed at least once by the user, and the creation time of the comment is higher than the time the node was last viewed, mark the comment as NEW
b) If a node has already been viewed at least once by the user, and the last changed time of the comment is higher than the time the node was last viewed, mark the comment as UPDATED.
This can be solved introducing an entirely new function, but...

Broken #2:
The problem is, that once we get to the comment viewing logic, the last viewed timestamp of the node has already been updated (this happens in node_show), so the system will always think that the user has already viewed the node after the comment was created/updated. In d6, the timestamp used to be updated after the node output was built, which makes much more sense to me.
The proposed solution is to move the updating of the last viewed timestamp at the end of node_show. But I don't know if this could cause problems elsewhere (like, no last viewed timestamp in db when a node has never been viewed before? or worse?).

Broken #3:
This is just a presentation issue: in template_preprocess_comment, new comments markers are not passed through theme_mark. So, there is no themeing and no difference between MARK_NEW and MARK_UPDATED: the output always shows just a plain 'new'.

The patch here: http://pastebin.com/NB8CP3hQ attempts to solve all three issues. I would attach it in the issue but I keep getting validation errors for any file that I try to upload. Maybe has something to do with the problems on drupal.org today?

Note: I changed the component to node.module to stress the importance of this issue. It's not so much as being critical, but the problem in node_show makes so that the 'history' table is updated in a wrong way, that affects not only comments but every system that relies on it.

Here's the patch.
About tests: I don't have much experience with tests but I can try. Just need an advice: should I write a new test case or just add to testCommentInterface()?

Patch looks good on first glance, however I don't think we need the static cache in there, since node_last_viewed() already has one.

Also this could do with tests so it doesn't regress again.

Rerolled, removed static cache in commen_mark, added test written by drunken monkey here: #6371: All comments marked as read when viewing any page.

Corrected wrong variable name in comment_mark.

The above patch was checking against nid and not the timestamp as the patch in #9 was this should resolve that issue.

This is major at best.

This patch should hopefully work and pass the tests for new comments.

Minor change to add a comment to explain why we work out timestamp the way we do.

Reroll as comment changes have hit head.

Rework comment creation to ensure we don't have permissions issue when creating with the admin user.

Is it possible to make this a positive assertion somehow? If we ever change this text the assertion will always pass even if it breaks.

+    $this->assertNoLink(t('@count comments', array('@count' => 0)));
+    $this->assertNoLink(t('@count new comments', array('@count' => 0)));

Otherwise this looks good to me.

The only possible solution that code use a postive assertion is to check that we only have 1 li elements present (Read more link) alternative version of the patch with this approach attached. It leaves in the existing asserts and makes sure that the n links are correct - so 1 link which is read more.

Test pass locally so not sure what the issue is here.

#29: 983632-comment-mark-7.patch queued for re-testing.

Try as I might I can't get the tests to fail locally can someone else try running the tests on their localhost or server and see if they can get it to fail. And if that happens let me know what they are seeing when things do fail.

#29: 983632-comment-mark-7.patch queued for re-testing.

subscribe

subscribe

Subscribe.

A missing or not properly working new/updated marker is an extreme stretch of the major priority. Definitely not major.

+++ modules/node/node.module	5 Jan 2011 04:07:18 -0000
@@ -1403,11 +1403,13 @@
+  // For markup consistency with other pages, use node_view_multiple() rather than node_view().
+  $nodes = node_view_multiple(array($node->nid => $node), 'full');
+
   // Update the history table, stating that this user viewed this node.
   node_tag_new($node);
 
-  // For markup consistency with other pages, use node_view_multiple() rather than node_view().
-  return node_view_multiple(array($node->nid => $node), 'full');
+  return $nodes;

I suspect that this is the only required change for this issue.

Powered by Dreditor.

This is an obvious, site-visitor facing, regression from Drupal 6 and is causing duplicate reports in the issue queue such as http://drupal.org/node/6371#comment-4692102

Any site with a lot of comments and regular visitors (i.e. drupal.org or groups.drupal.org) is unusable without being able to see which comments are new or not. If Drupal.org or g.d.o upgraded to D7 without this bug being fixed it would be a serious pain in the arse, same for any other community site that is registered user + comment heavy. So I think it qualifies as major for those reasons.

I couldn't reproduce the test bot fail locally, I also agree with sun that the hunk he posted looks like the only necessary bug fix here, previous patches are introducing a markup change which we should try to avoid for D7 if at all possible now anyway.

So re-rolled with just the tests and that hunk, all comment tests pass locally for me, the only other change I made was tidying some of the code comments in the tests.

Happy to rtbc this.

Awesome! Thanks a lot for the fix and the tests! What a nasty bug. :\

Committed and pushed to 8.x and 7.x. Thanks!

Patch is good work, but #new marker is lost, if I use comment pager.
http://drupal.org/node/1215574

Note that the tests added by this issue wrongly assume that the "Read more" link will always be present for teaser nodes.

This is blocking: #823380: Read More link is always visible on teaser.

See also: #1300568: Fix tests that wrongly assume all teasers have a Read more link

Let's fix it in those issues rather than re-opening this one that was done three months ago then.

The problem is, I don't understand the tests well enough to rewrite them. Presumably the authors who contributed to this issue would have that understanding.

That's fine. People know of the other issue by now. This bug here has been fixed, case closed.