Skip to:
Content

Opened 3 years ago

Last modified 7 months ago

#4429 assigned defect (bug)

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

Reported by: DJPaul Owned by: rachelbaker
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Component - Activity Keywords:
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 (2)

4429.patch (720 bytes) - added by rachelbaker 2 years ago.
blocking activity comments by banned users
4229.01.patch (2.2 KB) - added by boonebgorges 2 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 @rachelbaker3 years ago

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

@rachelbaker2 years ago

blocking activity comments by banned users

comment:2 @rachelbaker2 years ago

  • Keywords has-patch needs-testing added

comment:3 @DJPaul2 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.

comment:4 @rachelbaker2 years ago

Paul,

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

comment:5 @rachelbaker2 years ago

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

comment:6 @johnjamesjacoby2 years ago

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

Any updates here?

comment:7 @boonebgorges2 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.

comment:8 @boonebgorges2 years ago

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

@boonebgorges2 years ago

comment:9 @imath2 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.

comment:10 @boonebgorges21 months ago

  • Milestone changed from 1.8 to 1.9

comment:11 @boonebgorges16 months 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.

comment:12 @boonebgorges12 months ago

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

comment:13 @boonebgorges11 months ago

  • Milestone changed from 2.0 to 2.1

comment:14 @boonebgorges10 months 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.

comment:15 @DJPaul7 months ago

  • Milestone changed from 2.1 to Future Release
Note: See TracTickets for help on using tickets.