Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#5357 closed defect (bug) (fixed)

404 when clicking "Request Membership" within single group page

Reported by: mgibbs189 Owned by: boonebgorges
Milestone: 2.0 Priority: low
Severity: minor Version: 1.9.1
Component: Groups Keywords: has-patch 2nd-opinion
Cc:

Description

The "Request Membership" button works properly on the Groups landing page, but not on single group pages.

Reference: http://buddypress.org/support/topic/404-error-on-request-membership-for-private-group-in-bp-1-8-1/#post-174554

Attachments (5)

buddypress.js.patch (170 bytes) - added by mgibbs189 6 years ago.
5357.option1.diff (6.6 KB) - added by imath 6 years ago.
5357.option2.diff (5.9 KB) - added by imath 6 years ago.
5357.option2.02.diff (6.2 KB) - added by imath 6 years ago.
5357.03.patch (1.3 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (15)

#1 @mgibbs189
6 years ago

  • Keywords has-patch added

#2 @imath
6 years ago

  • Component changed from Core to Groups
  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 2.0
  • Priority changed from normal to low
  • Severity changed from normal to minor

Thanks for your feedback and patch @mgibbs189

As i've said here #5140 i've managed to reproduce the trouble.

1/ When on a single group, the group button (item's header) click is not managed with Ajax but displays the request-membership screen
2/ While regular users are reaching the site.url/groups/groupslug/request-membership/, a superadmin gets a 404.

Trouble starts in bp-groups/bp-groups-loader.php because the screen function is not available as there's this check ! bp_current_user_can( 'bp_moderate' ) around line 422

I'll check @mgibbs189's patch and i think we should also do something for part 2 of the bug to avoid the 404 in case the loggedin user is a superadmin.

As superadmins can see all groups content, they don't need to be part of the group, but i agree it should be fixed.

Last edited 6 years ago by imath (previous) (diff)

#3 @imath
6 years ago

  • Keywords 2nd-opinion added; needs-testing removed

So i've explored a bit more the issue. And i came to the conclusion that fixing it using javascript was not the best move.

The main reason is that the mechanism that loads the group template is also acting as a fallback if javascript is disabled. So the fix will not completely solve the trouble.

When on a single group, the "Request membership" button in group header acts like the subnav item having the same name. It loads the bp-template/bp-legacy/buddypress/groups/request-membership.php. With a regular user, there is no issue.

Issue only occurs when a site Administrator is clicking on the button while the group is private. As, for this particular type of user the subnav is not set in group loader, the screen function is not run and finally the function bp_core_load_template() doesn't load the group's home template resulting in a 404.

So i've built 2 patches :

  • Option 1 first adds a query arg into the button, when clicked a function is hooking bp_actions to process the membership requests.
  • Option 2 , that seemed the simple way at first.. is not so simple :) It consists in first removing ! bp_current_user_can( 'bp_moderate' ) check in the groups loader class to allow the request-membership subnav. Then, as each group is visible for a site administrator, it needs the bp-template/bp-legacy/buddypress/groups/home.php to be edited in order to include a new conditional statement to eventually load the request membership template. Then, having the request membership subnav is a bit annoying in this case so i've edited bp_get_options_nav() to remove it.

Then, as i've played with the group members subnav of the group edit screen, i've noticed that a group Administrator was able to "kick & ban" a site Administrator. If we leave it this way, then a banned site Administrator will have a request membership button although he's already member.. So i think, we shouldn't let a group admin ban a site Administrator.. I've included a way to do so in both patches.

I've chosed 2.0 milestone as it's not a regression from 1.9, it was already there in 1.8, and this is really a very particular case. On the other hand, the thread in the BuddyPress.org forum began 5 months ago, so it can be interesting, if you think one of the 2 patches is ok, to add this in 1.9.2.

What is your opinion ?

@imath
6 years ago

@imath
6 years ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.


6 years ago

#5 follow-up: @DJPaul
6 years ago

Does option2 have backpat for people who've customised those templates in their theme?

#6 in reply to: ↑ 5 @imath
6 years ago

Replying to DJPaul:

Does option2 have backpat for people who've customised those templates in their theme?

Thanks a lot DJPaul for your question. I've modified the patch for option 2 (see 5357.option2.02.diff) to reply to it.

In this diff, i'm no more modifying the 'groups/single/home' to instead hook to an action of the 'groups/single/plugins' template, this way it should maximize backpat.
About the modifications i've added in the 'groups/single/admin' template, i've left them as anyway, even if a template don't have them, i'm also checking in bp-groups/bp-groups-screens.php if the user to ban is not a site administrator.

Last edited 6 years ago by imath (previous) (diff)

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.


6 years ago

#8 follow-up: @boonebgorges
6 years ago

The don't-ban-administrators issue seems separate, and the patch seems to add a lot of complexity for an edge case. Let's think about it in a separate ticket.

My suggestion is to go with your original option 2. See 5357.03.patch. Yes, it does not provide support for themes that have customized groups/single/home.php. But this will be a small number of installations; and we won't be breaking anything for them worse than it's already broken; and in any case, people who make template-level changes should be prepared to update their templates for new fixes. So let's keep our routing logic centralized. Objections?

#9 in reply to: ↑ 8 @imath
6 years ago

Replying to boonebgorges:

Objections?

5357.03.patch looks good to me :)

#10 @boonebgorges
6 years ago

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

In 8159:

Allow site admins to visit the Request Membership page of a private group

Fixes #5357

Props imath

Note: See TracTickets for help on using tickets.