"Jump to" links are broken on multi-page issues

I think this is a limitation of core and there's not really anything that the project_issue module can do about this. Did you have a suggestion in mind?

For Drupal 7.x this is fixed in core. You can go to comment/CID#comment-CID and it will calculate where that is in the current pager and forward you there.

For Drupal 6.x I implemented basically that same feature in http://drupal.org/project/permalink

We could either use http://drupal.org/project/permalink or just add a similar feature to project_issue module.

Subscribe, and crud this is getting annoying, as the community is expanding and more and more issues are pushing past 300...

Closed in deference to this issue:

• #649250: "Jump to most recent comment" broken on issues with > 300 comments
• #1174760: "Your post" updated link to new comments doesn't work properly when issue has over 300 messages and is paginated

For reference, too, check out my notes on Scaling the Drupal Community from webchick's DrupalCon talk.

Also, after glancing at the issue that bumped the limit to 300 (see: #281641: Increase comments_per_page to 300), it looks like the intention was, at that time, to put the limit at 500 comments per page (which never happened):

killes has upped it to 300 already. I offered to write a patch for drupalorg.module to up it to 500.

... but that never happened. Of course, even if we up it to 500, that's only putting a band-aid over the issue at hand. It would be great to solve this by making the 'new' link actually link to the latest comment(s).

Adding a note here that contains the text "Most recent comment" so that people searching for that being broken will find this issue :)
--
When there are more than 300 comments on an issue, it would be particularly useful if that "Most recent comment" link on the right were to take you to the most recent comment, on the last page. But instead, it won't even take me to the most recent comment on page 1 (where I might at least find the pager).

For an example of an issue with a really long comment thread, see #538904: D8UX: Redesign Modules Page

sorry for jumping in on this old topic,
but maybe you could suggest a module/snippest that does exactly what "Jump To" does on d.org?

Thank you!

The "jump to" links are handled via custom code in project_issue module, it's not a separate module.

Meanwhile, see also #2083605: Disable pagination for comments on individual issue pages as another approach to "solving" this -- just disable comment-based paging on issues entirely.

...just wanted to note that this issue also breaks the "# new" replies link (the one shown in the "Replies" column when there is a red "new" tag in the issue lists) when the comments of an issue span across multiple pages.

I didn't realize http://api.drupal.org/comment_new_page_count existed. That can be used to fix the jump to links.

Rough patch ... needs some testing and verification.

Ooops ... query needs to be an array.

And ... fixing a fatal typo.

Tested on my dev site, and this kind of works ... with a few caveats:

1) Most recent comment link doesn't even appear, as the $node->cid value is empty. Out of scope for this particular issue/patch.
2) The 'First unread comment' jumplink takes you to the correct comment page for the first unread comment, but causes a reload of the issue ... and since you've now viewed the issue, the '#new' comment count gets reset to 0 and thus the #new anchor no longer exists on the page. Needs some way of getting the comment_id of the first unread comment.

'Most recent attachment' jump link works.

Seems like #14.2 is a pretty serious problem with deploying this, no? I think we need to solve that before we can commit this. Otherwise, people are going to continue to complain that these links are broken.

Also, I don't really understand #14.1. @jthorson: Can you explain?

Thanks,
-Derek

So to break down 14.2, the relevant sequence is:

1. We are viewing a page of comments in an issue node (but not the last page).

2. node_show() calls, in sequence, (a) node_view_multiple() (which displays the page, including the link to the #new comments); and (b) node_tag_new() (which updates the data that link is based upon).

2a. node_view_multiple() results in comment_node_view() (hook_node_view) being called, which generates the #new fragment link IFF comment_num_new(), which is based on node_last_viewed() and specifically the timestamp column in the history table for that node and user.

2b. node_tag_new() then updates the timestamp column in the history table.

3. We click the link to go to the new comments, and the sequence is repeated for the last page of comments, but this time with the updated timestamp which, barring any additional comments posted since we viewed the original page, is now greater than that of the most recent comment, so (2a) doesn't generate the link (or if it does, it points to an extremely recent comment, rather than the one we wanted).

4. Regarding the link destination, CommentController::attachLoad() sets the $comment->new property for each comment according to the same timestamp via node_mark(), and comment_prepare_thread() uses this to determine if/where to put the id="new" anchor in the thread markup.

So the options which spring to mind are:

A) We prevent node_show() from calling node_tag_new() in this situation. This should be feasible, as the node object is passed all the way through this sequence, and therefore comment_node_view() could set some kind of "do not update history timestamp" property which we subsequently check for back in node_show().

I don't really like that. The user has viewed the node, so the history should reflect that. The fact that they haven't read the latest comments to the node seems like a separate concern.

B) We add comment-specific tracking, analogous to the current history timestamp, so that we can establish the most-recent comment seen by the user independently of when they last viewed the node.

This could be a separate table, or just an additional column in history.

Personally I think this is the way to go. The down sides are the larger changes to the code, and all the additional history data for nodes with comments; however we could actually leave the new column NULL unless it differs from history.timestamp, in which case -- given how few issues ever reach 300 comments -- the data usage is relatively lightweight. I imagine a separate table would actually use the least data, as it would be populated by comparatively few rows. Using a new column in history means no joining, OTOH. I'm not certain which approach would be best.

C) We probably could track this (for affected nodes only) in session data? That seems too ugly, although I haven't considered it in detail.

Are there other options? What do people feel the correct solution is?

p.s. By far the biggest annoyance for me is that the "new" links generated on my dashboard for any of these large issues is broken, and I would imagine that the existing patch actually does deal to that (because the user hasn't yet viewed the issue node directly).

As such, I think there could still be a significant benefit in committing the proposed fix for that, despite 14.2 (or rather, while a solution to 14.2 is being worked out, as that might take a while).

Personally I don't think I ever actually use the link discussed in 14.2; whereas I'm looking at my dashboard virtually every day. I think there's some low-hanging fruit to be grabbed here.

So if we look at numbers, by either db query or going here:

https://drupal.org/project/issues?text=&projects=&status=All&priorities=...

There are ~50 issues with 300+ comments.

Outside of ~719000 of them.

https://drupal.org/project/issues?text=&projects=&status=All&priorities=...

So how about we simply bump limit to say 700 comments per page
(as suggested in #2083605: Disable pagination for comments on individual issue pages and elsewhere).

Considering there is also a discussion about going back to the D6 comment number urls #2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks

I'd be happy to see any quick fix. I don't think it precludes a better fix later on.

Long term, I that that user-controlled pagination would be a good thing, such that it's there by default, and users can not only disable it if they prefer, but also set a custom per-page threshold value (potentially to something much smaller than 300, should they -- and their internet connection -- decide that it would be preferable).

With the appropriate bug fixes and configuration options, it could be a nice feature to have. In the interim, because 300 is already such a large number, removing pagination doesn't make much difference in practice, so I'd be very happy with that as a short-term solution.

Tagging to get feedback from the Dev tools team.

Drupal core only goes up to 300, https://api.drupal.org/_comment_per_page, so we can't configure it to be larger without hacking core or making custom overrides. I don't think that is worth it for so few issues.

We can still try fixing the links which drupalorg module already generates on the dashboard, such as:

              $comments .= ', ' . l(format_plural($new, '1 new', '@count new'), 'node/' . $node->nid, array('fragment' => 'new'));

in drupalorg_block_view('tracker_user').

Setting the priority to normal because only ~50 issues are affected, and there is not data loss.

I will check this

I created a patch for the module. It changes the "first_unread_link" and "most_recent_link" links to the comment/cid version. Please let me know if I should change something.

That looks like a good strategy.

Let's use entity_uri() instead of hard-coding the comment URIs. Then #2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks will be able to change these URIs too.

Good point. I attached a new patch which uses entity_uri() instead of the hardcoded comment/cid url.

Tested on dev ... the next page jump link works great. (The 'new' tags disappear, but it takes you to the correct comment).

Committed with removing some variables that were only used once. Deploying to Drupal.org later today.

Now deployed.

FWIW, this causes a complete new page load on all issue pages now, if you happen to click those links:

#2239171: Regression: "Most recent comment" jump links navigate to an entirely new page

That's a major UX regression for the ordinary 99.9% case in which an issue has less than 300 comments.

Could we revert this change and only commit it with the additional change of #2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks ...?

#2239369: "Comments per page" setting cannot be configured to be larger than 300

I created a new version. It will generate new page load only if the comment will be displayed in a new page. What do you think about it?

kalman.hosszu: Does that resolve the issue 2 from comment #14 ? (also see breakdown in comment #18).

I created a new version which uses "comment-CID" fragment instead of "new".

#2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks does fix this well, and I think is nearly ready to deploy. (I've been traveling for NYCCamp & vacation, so deployments have slowed down for a couple days.)

Let's leave open for #2125397-37: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks

This query (calculates the page # for comment) is executed for each comment that rendered, so it makes sense to optimize it a bit.

Untagging since this isn't a D7 regression. The code has been deployed on Drupal.org and works.

The query is done once for the page, not once per comment. It has been deployed on Drupal.org without any major performance impact. Have you run any benchmarks or explain to see how bad the query is specifically?

The query I pointed in #37 runs in project_issue_preprocess_comment() so per comment entity

+++ b/project_issue.module
@@ -1044,21 +1044,44 @@ function template_preprocess_project_issue_issue_jump_links(&$variables) {
+    if (comment_get_display_page($node->cid, $node->type) > 0) {

this query runs once per page here

Sure, pages run plenty of queries. What exactly makes this problematic? Do you have an alternative which doesn't run a query?

I'm confused -- are we talking about a patch or about something that's been committed?

I've just done a git pull of the 7.x-2.x branch and I can't find what's being referred to.

If there really is that query run once per comment then that does seem a lot, and surely we can find a way to optimize it to only run once for the whole page, especially since we know that in our circumstance the comments are unthreaded.

Aha, this refers to the patch in #36, which does not need to be committed, as explained in #37. Resetting the status.