Whatever dolt wrote the patch for issue #1044322 did not account for the fact that {history} is clobbered by cron and entries older than NODE_NEW_LIMIT are removed. This causes node_last_viewed() to return 0, which our previously mentioned myopic patcher assumed meant that the node had never been viewed; no, it means the node doesn't exist in {history} (potentially anymore). So, we have to also check node_last_changed() to see if it's older than NODE_NEW_LIMIT, and if so, we don't mark the node as unread.

The patch is attached. It's tested, and fixes the issue.

CommentFileSizeAuthor
#6 stupid_idiot_did_not_read_api_carefully_updated.patch830 bytesrcurtin
stupid_idiot_did_not_read_api_carefully.patch858 bytesrcurtin

Comments

rcurtin’s picture

rcurtin commented

Sorry, mistype. The referenced issue is #1044322: A new topic with no comments does not show new post icon (now you get a nice link).

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

LOL! I was about to take you to task for being so nasty to someone who was nice enough to write a patch... Maybe I should still do that, actually. Not nice to be so nasty to the poor fellow who made a simple mistake. :)

Thanks for the update. Having company for a week (my office is the guest room) has really put me behind and I'm not in a position to apply this right now since my dev environment is messy. Will get to it ASAP, though.

Michelle

mcdruid’s picture

mcdruid commented
Status: Needs review » Postponed (maintainer needs more info)

Thanks rcurtin - could you check that this patch really is necessary against the latest 6.x-2.x-dev though please?

It's taken me a little while to work out what's going on; I think Michelle has committed your patch from #1044322 but that it didn't include the stuff you're now removing in your new patch.

I could be wrong, but I don't actually think we need to worry about this as the code you want to remove is not actually there in the branch.

Let me know if I've got the wrong end of the stick.

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented

Yeah, I never did anything on this issue. I pretty much missed the entire month of May due to being ill and never really got back into AF before switching gears to Artesian.

Michelle

rcurtin’s picture

rcurtin commented

Sorry for the delay -- life happened (I hate that!). This is still necessary against the latest 6.x-2.x-dev; or, that is what I am experiencing, having upgraded to a clean 6.x-2.x-dev and seeing the same issues, with old posts being marked new.

rcurtin’s picture

rcurtin commented
StatusFileSize
new stupid_idiot_did_not_read_api_carefully_updated.patch830 bytes

Ah, the old patch doesn't work anymore. I've updated it for great justice.

mcdruid’s picture

mcdruid commented

@rcurtin - no problem, I've had quite a lot of life happening recently too!

I'm still a bit confused about what's going on here. AFAICS your (updated) patch is trying to remove lines which are not there in the 6.x-2.x branch:

patch:

--- advanced_forum.module	2011-08-10 00:37:27.000000000 -0400
+++ advanced_forum.module.orig	2011-08-10 00:31:57.000000000 -0400
@@ -936,16 +936,8 @@
     $viewed = 0; // If not told otherwise, it has been viewed before
     if ($user->uid) {
       $viewed = node_last_viewed($nid);
-      // Set it to 1 if it has not been viewed before, but only if it has been
-      // modified after NODE_NEW_LIMIT; that is, it wouldn't have been purged
-      // from {history}.
-      if ($viewed == 0) {
-         if (node_last_changed($nid) > NODE_NEW_LIMIT) {
-            $viewed = 1;
-         }
-      } else {
-         $viewed = 0;
-      }
+      // Set it to 1 if it has not been viewed before.
+      $viewed = ($viewed == 0 ? 1 : 0);
     }
 
     $comment_type = module_invoke('nodecomment', 'get_comment_type', $node->type);

6.x-2.x branch / current dev release:

 935     // We must also check the forum post itself to see if we have viewed it
 936     $viewed = 0; // If not told otherwise, it has been viewed before
 937     if ($user->uid) {
 938       $viewed = node_last_viewed($nid);
 939       // Set it to 1 if it has not been viewed before.
 940       $viewed = ($viewed == 0 ? 1 : 0);
 941     }
 942 
 943     $comment_type = module_invoke('nodecomment', 'get_comment_type', $node->type);

So unless I'm missing something, I don't see that your patch is necessary, nor will it actually apply to the branch or the dev release (dated 2011-Aug-02 at the moment).

rcurtin’s picture

rcurtin commented

The patch was created backwards by accident, but read it again with the +s and -s and vice versa. patch will happily take this and apply it correctly, though (try it if you like).

Here is a further description of the bug and why the patch is necessary. At some point in printing out all the topics (or forums), we must decide if each one has anything new in it or not, so we know which icon to use (no new posts / new posts). Before #1044322: A new topic with no comments does not show new post icon was fixed, the code simply checked to see if there were new comments or nodecomments, based on which the forum itself was using. However, the code did not check to see if the original forum post (not the comments) had been viewed, and #1044322: A new topic with no comments does not show new post icon fixed that.

Now, this introduced another issue. The node_last_viewed($nid) call will return 0 if there is no record in {history} of the node being viewed. {history} is clobbered by cron regularly, removing all history entries older than NODE_NEW_LIMIT. As a result, what happens is that posts older than NODE_NEW_LIMIT are marked as new (again because node_last_viewed($nid) return 0 since the record of its viewing in {history} has been removed).

So, in the end, the best decision is to check if the node was modified before NODE_NEW_LIMIT (meaning that a view could possibly have been deleted from {history}), and if it was modified before then, we say it was already viewed. Yes, it is possible that we could mark a very old post that a user has never seen as viewed, but, this is the price paid to keep {history} small. There is not a good, space-efficient solution to keeping completely accurate node viewing data, and inside of the current framework, this is the best we can do. Even if we did add another table to keep track of forum history losslessly, now we have another big table (because it's not going to be small).

Is this enough information or should I add more to the dissertation here? You can try this yourself but you need a forum with posts older than NODE_NEW_LIMIT and then you need to run cron to remove old things from {history}.

mcdruid’s picture

mcdruid commented
Assigned: rcurtin » Unassigned
Status: Postponed (maintainer needs more info) » Fixed

rcurtin, thanks for your patch and your excellent dissertation.

However, I can only give you an A- because of the backwards patch :-)

Committed to 6.x-2.x branch.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

finex’s picture

finex commented

I want to report that the patch is not available on http://drupal.org/node/438698 (current -dev version) and in the current git repo.

finex’s picture

finex commented
Status: Closed (fixed) » Active
mcdruid’s picture

mcdruid commented
Status: Active » Closed (fixed)

are you sure about that? I've just checked advanced_forum-6.x-2.x-dev which is currently dated Aug 12th 2011, and the git repo, and I believe the changes are there in both.