Skip to:

Opened 5 months ago

Closed 9 days ago

#7608 closed enhancement (fixed)

Use BP_Groups_Group visibility and access properties.

Reported by: David Cavins Owned by: djpaul
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.0
Component: Groups Keywords: has-patch
Cc: David Cavins


When checking visibility for the current user, use the is_visible property in BP_Groups_Group rather than calculating it again. Similarly, use the user_has_access property when checking access to a group. (is_visible was introduced in 2.9.)

Attachments (3)

7608.01.diff (3.1 KB) - added by David Cavins 5 months ago.
Use BP_Groups_Group properties in some cases.
7608.revert.patch (481 bytes) - added by r-a-y 2 weeks ago.
7608.revert.02.patch (2.3 KB) - added by David Cavins 2 weeks ago.
Another revert option.

Download all attachments as: .zip

Change History (14)

@David Cavins
5 months ago

Use BP_Groups_Group properties in some cases.

#1 @Paul Gibbs
4 months ago

  • Keywords commit added

If ->is_visible handles both private and hidden groups correctly for this change, go for it.

#2 @Paul Gibbs
4 months ago

Yes it does.

#3 @djpaul
4 months ago

  • Owner set to djpaul
  • Resolution set to fixed
  • Status changed from new to closed

In 11761:

Groups: use class methods to check if user has visibility of a group.

Improves upon existing duplication of logic.

Fixes #7608

Props dcavins

#4 @r-a-y
2 weeks ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because the changes to bp_group_is_visible() allows unauthenticated users to view a private group's homepage:

@dcavins - Although it makes sense to use the 'is_visible' property in the bp_group_is_visible() function, we should probably revert due to backward compatibility issues. I don't have any bright ideas to fix this without introducing changes to our templates, which I don't think we want to do.

Last edited 2 weeks ago by r-a-y (previous) (diff)

2 weeks ago

#5 @David Cavins
2 weeks ago

Thanks for your comments. I see what you mean-- the function bp_group_is_visible() wasn't actually checking visibility, it was checking to see if the user had access. Here's a list of places in BP where we use bp_group_is_visible() to enable access to something:

  • groups_action_group_feed(): to display the feed or not (items posted to private groups are sensitive to the current user anyway, now)
  • bp-legacy/buddypress/groups/single/cover-image-header.php and bp-legacy/buddypress/groups/single/group-header.php: Whether to display Admin and Moderator lists
  • bp-legacy/buddypress/groups/single/home.php: Whether to load the bp_groups_front_template_part()
  • bp-nouveau/buddypress/groups/single/parts/header-item-actions.php: Whether to display Admin and Moderator lists
  • bp_nouveau_group_template_part(): Whether to display group no-access message.

Bummer. I suppose we should revert it, but can we, instead of putting the old logic back directly, have it wrap a call to bp_current_user_can( 'groups_access_group' ), and also add a comment about how the name got out of sync with the actual property it's checking. Also, before nouveau is released into the wild, can we just replace those checks with calls to the access_group check?

I'll attach a patch that includes the above.

Thanks again for catching this misunderstanding on my part.

#6 @r-a-y
2 weeks ago

I like revert.02.patch. Didn't even know of the new bp_groups_user_can_filter() function!

I think the changes to the Nouveau templates are good, but I believe the 'group_id' parameter needs to be passed in the bp_current_user_can() function?

Other than that, let's roll with your patch. Thanks @dcavins!

@David Cavins
2 weeks ago

Another revert option.

#7 @David Cavins
2 weeks ago

Hi @r-a-y-

Thanks again for your feedback. The group_id is optional in those bp_current_user_can() calls because bp_get_current_group_id() is reliable in those two instances (and bp_groups_user_can_filter() uses it to get the current group if no group_id is passed). I'm leaving it off to keep it simpler (in those template uses), but we can add it to be complete if that makes more sense.

@boonebgorges, as the keeper of the history of the groups component, can you check my "historical note" in the attached patch to make sure I'm actually correct in my understanding?


Last edited 2 weeks ago by David Cavins (previous) (diff)

#8 @r-a-y
2 weeks ago

I'm leaving it off to keep it simpler (in those template uses), but we can add it to be complete if that makes more sense.

I missed that! You're right, no need to add in the 'group_id' in this case.

#9 @Boone Gorges
2 weeks ago

I believe that the "historical note" is correct.

#10 @David Cavins
2 weeks ago

In 11896:

Revert bp_group_is_visible() to check access.

In r11761, bp_group_is_visible() was changed to rely on the BP_Groups_Group->is_visible property. However, bp_group_is_visible() has been used to check access, not visibility, making the new logic, even though it seems sensible, wrong.

This changeset also replaces bp_group_is_visible() in the BP Nouveau template parts, to start retiring this no-longer-correctly-named function.

Props r-a-y.

See #7608.

#11 @Paul Gibbs
9 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.