Skip to:
Content

BuddyPress.org

Opened 12 years ago

Closed 7 years ago

Last modified 7 years ago

#4429 closed defect (bug) (fixed)

User banned from public group can still reply to group activity items

Reported by: djpaul's profile DJPaul Owned by: rachelbaker's profile rachelbaker
Milestone: 3.0 Priority: normal
Severity: normal Version: 1.2
Component: Activity Keywords: has-patch
Cc:

Description

A user banned from a public group can still reply to group activity items. This should be changed as, in the Default theme at least, the activity items are visually represented as belonging inside of a group -- so the ban should apply across all content in a group regardless of which underlying BP component it belongs to.

Attachments (3)

4429.patch (720 bytes) - added by rachelbaker 12 years ago.
blocking activity comments by banned users
4229.01.patch (2.2 KB) - added by boonebgorges 12 years ago.
4229.02.patch (3.3 KB) - added by r-a-y 7 years ago.

Download all attachments as: .zip

Change History (23)

#1 @rachelbaker
12 years ago

  • Owner set to rachelbaker
  • Status changed from new to assigned

@rachelbaker
12 years ago

blocking activity comments by banned users

#2 @rachelbaker
12 years ago

  • Keywords has-patch needs-testing added

#3 @DJPaul
12 years ago

Thanks for the patch, Rachel. Some initial feedback (I haven't tested the patch yet): we need to check that the Groups component is active before calling the function, and that the activity itself is related to a group.

It might also be more elegant to hook into (for example) bp_activity_can_comment from inside the Groups component and do the logic there; it helps to keep components tidy and unentangled. I know there's lots of parts of core that don't do this, but it might be a nice thing to aim for going forward.

#4 @rachelbaker
12 years ago

Paul,

Yeah on second look, you are right. I will rework the patch.

#5 @rachelbaker
12 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

#6 @johnjamesjacoby
12 years ago

  • Keywords needs-refresh reporter-feedback added; needs-patch removed

Any updates here?

#7 @boonebgorges
12 years ago

  • Keywords 1.8-early added; needs-refresh removed

I just whipped up a patch along the lines of what rachelbaker and DJPaul have been suggesting here. See 4229.01.patch.

It works, but is not ready for primetime. In order to make it a truly general solution (one that works in the sitewide activity stream etc, as well as on a single group activity stream), you have to do this for each activity item:

groups_is_user_banned( bp_loggedin_user_id(), bp_get_activity_item_id() )

This results in an additional database hit for every single group-related activity item. The is_banned data is not cached at all. On activity pages, this could result in 20 extra queries per page, which is unacceptable.

We should be doing one or both of the following:

  • Functions like groups_is_user_banned() should cache their results somehow
  • When fetching the activity stream and building the $activities_template global, also slurp up whether the logged-in user is a member of each returned activity item.

There's no time for either in this cycle. Let's take a swing for the next cycle.

#8 @boonebgorges
12 years ago

  • Keywords 1.8-early removed
  • Milestone changed from 1.7 to 1.8

#9 @imath
12 years ago

hi,

When the user is not a group member, he can comment activity or reply to activity comment that "belongs" to the group. I think it can be confusing for group admins to see that non members can contribute to the group content (in this case activity). I wonder if it's a good idea to include a "groups_is_user_member()" check on your patch.. and add something to check for the membership in case the non member wants to reply to a comment. (made a gist to suits my need : https://gist.github.com/imath/4966936)

So in the same spirit, Boone, i think you should also filter 'bp_activity_can_comment_reply' to check for banned user, because i've noticed on my case that if a member of a group commented on a group activity, then a non member can reply to this comment.

#10 @boonebgorges
12 years ago

  • Milestone changed from 1.8 to 1.9

#11 @boonebgorges
11 years ago

  • Milestone changed from 1.9 to 2.0

Thanks for the feedback, imath. Disallowing non-members from making comments on group activity items is a more radical change in behavior than what was originally being suggested, and I fear that it might alter the dynamics of certain sites. I guess I'd like to hear what other people think too.

Bumping this because it needs more work to reduce database overhead.

#12 @boonebgorges
11 years ago

The database considerations should be mitigated by #3856, which implements some pre-fetching for groups data at the beginning of the activity loop.

#13 @boonebgorges
11 years ago

  • Milestone changed from 2.0 to 2.1

#14 @boonebgorges
11 years ago

  • Keywords reporter-feedback removed

There are still some performance issues that are causing bottlenecks here. (There is currently no good way to cache is_banned values, because of some confusion about whether the results of groups_get_extras belong in the cache.) See #5407 for more discussion.

#15 @DJPaul
10 years ago

  • Milestone changed from 2.1 to Future Release

#16 @r-a-y
8 years ago

The performance issues noted by Boone should no longer be a problem with BuddyPress 2.6.

Maybe time to revive this for v2.7?

@r-a-y
7 years ago

#17 @r-a-y
7 years ago

  • Keywords has-patch added

I've added an updated patch that also filters 'bp_activity_can_comment_reply'.

The performance improvements for groups_is_user_banned() should be addressed since #6327.

#18 @r-a-y
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11959:

Groups: Banned group members shouldn't be able to comment on activity items.

Previously, we did not check activity commenting capabilities against group
membership status.

Props rachelbaker, boonebgorges, imath, DJPaul, r-a-y.

Fixes #4429.

#19 @r-a-y
7 years ago

  • Milestone changed from Awaiting Contributions to 3.0
  • Version set to 1.2

This one is finally fixed!

Thanks everyone for their contributionsn to this ticket.

Version 0, edited 7 years ago by r-a-y (next)

#20 @r-a-y
7 years ago

In 12131:

Groups: Fix display of "Comment" button in the activity stream.

In r11959, the banned group members check is using the wrong conditional.
This caused legitimate group members to never see the "Comment" button in
the activity stream.

This commit reverses the check and adds some unit tests (which should have
been written beforehand!).

Anti-props r-a-y.

See #4429.

Fixes #7854 (3.x branch).

Note: See TracTickets for help on using tickets.