Skip to:
Content

Opened 8 months ago

Closed 3 months ago

#7608 closed enhancement (fixed)

Use BP_Groups_Group visibility and access properties.

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

Description

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 dcavins 8 months ago.
Use BP_Groups_Group properties in some cases.
7608.revert.patch (481 bytes) - added by r-a-y 3 months ago.
7608.revert.02.patch (2.3 KB) - added by dcavins 3 months ago.
Another revert option.

Download all attachments as: .zip

Change History (14)

@dcavins
8 months ago

Use BP_Groups_Group properties in some cases.

#1 @DJPaul
7 months ago

  • Keywords commit added

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

#2 @DJPaul
6 months ago

Yes it does.

#3 @djpaul
6 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
3 months 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:
https://buddypress.trac.wordpress.org/browser/trunk/src/bp-templates/bp-legacy/buddypress/groups/single/home.php?marks=78#L75

@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 3 months ago by r-a-y (previous) (diff)

@r-a-y
3 months ago

#5 @dcavins
3 months ago

@r-a-y
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
3 months 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!

@dcavins
3 months ago

Another revert option.

#7 @dcavins
3 months 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?

Thanks!

Last edited 3 months ago by dcavins (previous) (diff)

#8 @r-a-y
3 months 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 @boonebgorges
3 months ago

I believe that the "historical note" is correct.

#10 @dcavins
3 months 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 @DJPaul
3 months ago

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