Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

show doc counts in group and user tabs, fixes #423 #442

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JonathanReeve
Copy link
Contributor

Hi Boone,

Here's my fix for our issue at mlaa#1, which may help out with #423 and #261. My understanding of the problem was:

  • update_doc_count() wasn't working, because it relies on using bp_is_group to check whether we're creating or deleting a group document, and bp_is_group returned 0, because, judging from the slug /docs at least, BP didn't know that it was in a group context.
  • it's possible that the hooks for update_doc_count() weren't working, since they were hooked in using add_filter instead of add_action, and the hooks bp_docs_doc_saved and bp_docs_doc_deleted appear to be action hooks instead of filter hooks.

This fixes the issue by rewriting update_doc_count() so that it can tell, through a variety of methods, what group the doc is associated with (if any), and update the groupmeta accordingly. It's probably not the most elegant solution, but seems to be working so far.

Let me know what you think.

@boonebgorges
Copy link
Owner

Thanks very much for the PR, @JonathanReeve !

Looking back, I do think that you've correctly identified why I originally pulled this stuff out - Docs 1.3 is when group Docs were moved out of the group context, which would explain why the bp_is_group() check fails.

I'm still wary of your solution (and the original architecture). Trusting $_GET params, even in a relatively benign way like you're doing here, is not a great way to ensure security and data integrity. I would say that the most reliable thing here is to take the current Doc ID (which is passed to the 'bp_docs_doc_saved' hook, and look up associated-group info based on that. Thus function update_doc_count( $doc_id = false ) etc.

I also am not a huge fan of the way that these counts are stored in groupmeta/usermeta. It was done this way for performance, but I seem to remember that there were lurking invalidation problems, so that saving/deleting didn't always properly update counts. I suppose if there are problems of this nature, they'll pop up on MLA Commons soon enough :)

If you want to have another swing at the PR based on the above, that'd be great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants