Project

General

Profile

Actions

Bug #1649

closed

BuddyPress Doc Discussion

Added by Michael Smith about 12 years ago. Updated over 9 years ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Daniel Jones
Category name:
BuddyPress Docs
Target version:
Start date:
2012-02-23
Due date:
% Done:

0%

Estimated time:
Deployment actions:

Description

Currently the most recent two messages of a 52 message discussion thread in the Commons Email Doc are visible:

http://commons.gc.cuny.edu/groups/cac-community-team-project-planning/docs/cuny-academic-commons-email?nocache=1

When attempting to review any of the first 50 message, the 'Previous' & '1' links lead to a 'Your status is all email' page – no longer on the particular doc page.

I attempted to access the first 50 messages in three browsers in OS 10.6.8 – Safari Version 5.1.2, Firefox 7.0.1, & Chrome 17.0.963.56.

(Of course it would be the community team that finds this bug!)

Actions #1

Updated by Boone Gorges about 12 years ago

  • Status changed from New to Assigned
  • Priority name changed from High to Normal
  • Target version set to 1.4

There's a bug in BP Docs that is making it not work properly with comment pagination. I've disabled this pagination on the Commons for the moment, and will fix the underlying problem in an upcoming version.

Actions #2

Updated by Matt Gold about 12 years ago

(Of course it would be the community team that finds this bug!)

LOL -- indeed.

Actions #3

Updated by Boone Gorges over 11 years ago

  • Target version changed from 1.4 to Future release

This is a bigger job to fix than I would like to take on at the moment. I'd like to wait until after BP Docs has had its 1.2 rewrite, which will be done by the end of the summer.

Actions #4

Updated by Boone Gorges almost 10 years ago

  • Target version changed from Future release to 1.7

I'll try to work on a proper fix for this for 1.7 (though it won't matter for the Commons, because we're not using the distribution version of the plugin).

Actions #5

Updated by Boone Gorges over 9 years ago

  • Category name set to BuddyPress Docs
  • Assignee changed from Boone Gorges to Daniel Jones

Dan, you want to have a look at this? Before looking at the Commons, dig into the distribution version of BuddyPress Docs. Use this branch: https://github.com/boonebgorges/buddypress-docs/tree/1.8.x

I don't know off the top of my head how this might be fixed. I'd think that it'd work automatically because Docs uses WP's standard `comments_template()` function. So something must be funky somewhere. To test, I'd suggest turning down the comments-per-page setting on your local (Dashboard > Settings > Discussion) and then creating a dummy Doc with a bunch of dummy comments.

Actions #6

Updated by Daniel Jones over 9 years ago

After a quick look the problem that I'm getting is that when I click on any of the comment pages, the page loads but there's no content. I get the header, nav, sidebar, name of the group, and the sidebar, but nothing where the doc itself and the comments should show up.

On the doc that's linked to as an example it looks like the setting has been changed to just show all the comments and not paginate them, which means I can't test to see if it's a different problem on the live site. I think they're probably the same though, as the "Your status is all email" line would still show up with the problem I'm having locally. And since it was only 52 comments on a 52-per-page setting, there was only a Previous and 1 link, which I think means it's likely that even if there were more pages they would have had the same issue.

Some debugging showed me that the problem is with the query-builder.php file in the includes folder in the plugin. Specifically, these lines:

$template_path = bp_docs_locate_template( $template );
if ( !empty( $template ) )
include( apply_filters( 'bp_docs_template', $template_path, $this ) );

The issue is that $template is never set, which happens because all the template setting lines rely on $this->current_view being set. On the first page I land on when I click on a doc, the view is "single" so it works out, but for the comment pages, with urls like ...comment-page-2/#comments, the view doesn't seem to be set at all, so it doesn't know what template to pull in.

Does this sound right? Figured I wouldn't spend a bunch of time trying to fix it myself before getting your thoughts.

Actions #7

Updated by Daniel Jones over 9 years ago

Oops sorry just saw your note that you disabled pagination on the Commons for the time being!

Actions #8

Updated by Boone Gorges over 9 years ago

Thanks, Dan. Yes, I think this sounds right so far as it goes. So it sounds like the right tack is to get $this->current_view to be set to 'single' when looking at one of these comment pages. Can you investigate a bit more how to do this?

Actions #9

Updated by Daniel Jones over 9 years ago

So I've made a little progress here - I went into integration-groups.php and found where current_view gets set and made it so that comment pages also get their view set to 'single'. A very small change - https://github.com/cuny-academic-commons/cac/commit/8dd04b3ece170961bf2504fcfec8d651776d5dd6

However after that a new problem came up - when I click on the other comment pages the URL changes but nothing else on the page does. The same comments (from the last page) show, and the list of comment page links stays the same, so that the link for the last page is still un-linked. So for some reason [doc url]/comment-page-2 is returning the same thing as [doc url]/comment-page-1 and comment-page-3. Could this be caused by how I messed with the view?

I spent a long time trying to figure out what was going on - since the problem is showing up with both the list of comments and the paginated comment links, my first bet was that something was going wrong with the cpage query var that keeps track of the comment page, but I couldn't get it for some reason.

Then I thought it could also be some kind of caching issue - so I deleted the comments.php file entirely and reloaded the page, and the comments were still there and the links were still (not) working like before. I don't know a whole lot about caching though so I'm not sure if that could really be the issue. Anyone have thoughts on where to go next?

Actions #10

Updated by Boone Gorges over 9 years ago

So I've made a little progress here - I went into integration-groups.php and found where current_view gets set and made it so that comment pages also get their view set to 'single'.

Cool, this looks right.

Anyone have thoughts on where to go next?

This is not a caching issue. I think what's happening is that we're using WP's comment-page-x URL structure, but there's nothing in buddypress-docs's URL parsing logic to take care of this. To see what I mean, look at the following on a relevant comments-page-x page:

var_dump( buddypress()->current_action );
var_dump( buddypress()->action_variables() );

I should stop here and note that we're running a 3-year-old version of BuddyPress Docs. The same problem does not happen in newer versions, because newer versions use WP's native custom post type URL routing. It's only this old version that uses BP's action_variables() etc (not to mention the current_view stuff you've already stumbled upon). So we should be careful how much time we sink into coming up with a solution.

Here's what I'd try to do. After verifying what I say above about action_variables() etc, see if there's something you can do to trick the comment template into displaying comments from the "correct" page, given the information found in action_variables(). I'm not sure off the top of my head how to do that, but I assume it's possible by filtering comments_array or something like that. Take a bit of time to muck around in there and see if you can find something. If not, let's abandon this ticket and wait to inherit the fix when we upgrade someday. Thanks!

Actions #11

Updated by Daniel Jones over 9 years ago

Thanks for the direction here - I think I got it to work!

I had to make three changes:
1- I think the comments_template() function is mostly designed for use with themes not plugins. So how we had it in wasn't actually loading our template file, just the default one from the theme. I added a filter on WP's "comments_template" hook so that when the action is the one for BP Docs (stole the test for that from elsewhere in the plugin) it loads up our comments template instead.
2- I added an extra argument to the call to wp_list_comments() to make it manually fetch the right page of comments based on the URL - I did this by pulling in the action_variables and then using str_replace to isolate the number.
3 - I added an action to the 'bp_before_blog_comment_list' hook to manually update the 'cpage' query var that WP uses to generate the paginated comment links. Used the same technique as for the wp_list_comments() argument. Strangely enough even with updating this query var before the comments are listed, I still needed to pass wp_list_comments() the 'page' argument - it wasn't able to just take it from the query var automatically.

I'm realizing now it'd probably be better to pull in the 'page' argument there from my newly updated 'cpage' query var instead of pulling it in again from action_variables- do you think it's worth going in and doing that?

I added the filters to bp-docs.php, which I doubt is the right place for them. Where do you think I should put them?

Actions #12

Updated by Daniel Jones over 9 years ago

Actions #13

Updated by Boone Gorges over 9 years ago

  • Status changed from Assigned to Resolved

Dan - Really nice! This is exactly the kind of fix I had in mind, and I'm pleasantly surprised to see that it took such a small number of lines of code to make it work.

I'm realizing now it'd probably be better to pull in the 'page' argument there from my newly updated 'cpage' query var instead of pulling it in again from action_variables- do you think it's worth going in and doing that?

Seems like it's working fine now. If it ain't broke :)

I added the filters to bp-docs.php, which I doubt is the right place for them. Where do you think I should put them?

We're essentially running a fork of BP Docs, so I think it's OK to leave them right in the plugin - in particular because these changes will become a non-issue after we upgrade to a newer version. So let's leave as-is.

Thanks so much! Marking resolved.

Actions

Also available in: Atom PDF