Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

#5357 closed defect (bug) (fixed)

404 when clicking "Request Membership" within single group page

Reported by: mgibbs189's profile mgibbs189 Owned by: boonebgorges's profile 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 10 years ago.
5357.option1.diff (6.6 KB) - added by imath 10 years ago.
5357.option2.diff (5.9 KB) - added by imath 10 years ago.
5357.option2.02.diff (6.2 KB) - added by imath 10 years ago.
5357.03.patch (1.3 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (15)

#1 @mgibbs189
10 years ago

  • Keywords has-patch added

#2 @imath
10 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 10 years ago by imath (previous) (diff)

#3 @imath
10 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
10 years ago

@imath
10 years ago

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


10 years ago

#5 follow-up: @DJPaul
10 years ago

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

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

Replying to DJPaul:

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

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.

Version 0, edited 10 years ago by imath (next)

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


10 years ago

#8 follow-up: @boonebgorges
10 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
10 years ago

Replying to boonebgorges:

Objections?

5357.03.patch looks good to me :)

#10 @boonebgorges
10 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.