Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#5790 closed defect (bug) (fixed)

If leave a private group and click request membership button before refresh page get -1

Reported by: SinOB Owned by: DJPaul
Milestone: 2.4 Priority: normal
Severity: normal Version: 2.0
Component: Groups Keywords: has-patch
Cc: alexander.berthelsen@…

Description

I am using
Wordpress 3.9.1
Buddypress 2.0.1
With the theme TwentyThirteen
I have disabled all plugins and can still recreate the issue.
Wordpress is installed in root directory (not sub directory)
as a Single site install.
I don't know if this was an issue with previous versions of wp/bp.
I have not modified any core files.
No errors are showing up in my error logs.
Server is linux

Issue:
If someone leaves a private group from the group list page and straight away(no page reload) clicks on 'Request Membership' button for that same group the button disappears and is replaced by -1

Can be recreated as follows;
(Requires that you are a member of a private group.)

  1. Navigate to Groups page i.e. www.examplesite.com/groups/
  2. Click on the button 'Leave Group' of a private group you are already a member of.
  3. You will get a pop-up asking if you are sure. Click yes.
  4. The 'Leave Group' button will have changed to a 'Request Membership' button. All looks fine so far.
  5. Without reloading the page click on that new 'Request Membership' button for that same group.
  6. The button disappears and is replaced with a -1

It looks like an issue with the nonce. check_ajax_referer() is not accepting the nonce it is being sent, returning a -1 and the script appears to be dying at that point.

Attachments (2)

5790.diff (1.3 KB) - added by lakrisgubben 5 years ago.
5790_1.diff (2.5 KB) - added by lakrisgubben 5 years ago.

Download all attachments as: .zip

Change History (20)

#1 @DJPaul
6 years ago

Thanks for the report. We'll try to look at this for 2.1, but it might slip into the 2.2 release.

#2 @DJPaul
6 years ago

  • Milestone changed from Awaiting Review to 2.2

#3 @DJPaul
6 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from 2.2 to Future Release

Confirmed per ticket description

#4 @lakrisgubben
5 years ago

  • Cc alexander.berthelsen@… added
  • Keywords has-patch added; needs-patch removed

The problem seems to lie in the fact that the link returned from bp_legacy_theme_ajax_joinleave_group had the wrong nonce name, groups_send_membership_request instead of groups_request_membership. Attached patch fixes this.

@lakrisgubben
5 years ago

#5 @lakrisgubben
5 years ago

digging a bit deeper into this I realized that the copy sent back via ajax was also different than the copy rendered through php (it said Membership Requested instead of Request Sent). Latest patch fixes that plus adds the correct html-classes to the link so that it becomes "disabled". To get this to work I had to make one minor css change to remove some specificity on the .generic-button selector, I removed the div from div.generic-button...

@lakrisgubben
5 years ago

#6 @DJPaul
5 years ago

  • Milestone changed from Future Release to 2.4

#7 @boonebgorges
5 years ago

lakrisgubben - Are both of the patches necessary to fix the problem?

#8 @lakrisgubben
5 years ago

@boone, no only the last one is needed, sry for being unclear. :)

#9 @DJPaul
5 years ago

  • Milestone changed from 2.4 to Future Release

Issue is still valid, except patch doesn't work (it LOOKS like it should), and I dislike the idea of changing that selector, but I couldn't get far enough to figure out a better CSS change. Very annoying for this to miss yet another release. :(

#10 @DJPaul
5 years ago

  • Milestone changed from Future Release to 2.5

#11 @DJPaul
5 years ago

  • Keywords needs-patch added; good-first-bug has-patch removed

#12 @DJPaul
5 years ago

Wait! I had a pesky build folder.

#13 @DJPaul
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 2.5 to 2.4
  • Owner set to DJPaul
  • Status changed from new to assigned

@hnla @mercime

This patch works and I would like it committed, but it makes a change from #buddypress div.generic-button a to #buddypress .generic-button a (this hasn't changed since theme compat. was introduced).

Do you reckon this is safe to do? If not, any chance of a quick look to come up with an alternate?

Otherwise, we'll ship this patch without the CSS change for now.

#14 @hnla
5 years ago

All the pre-pending of the selector to the class means is a heavier weight to the selector set. Seldom is it necessary to add the selector to a token unless that class token might exist on various elements and you need to specify the one the ruleset is applying to in this instance.

Almost certainly we can and should remove these sorts of over specified selector sets, we have where possible in js and css files, a fair few do still exist though.

I would recommend we roll with this patch removing the 'div', I doubt whether we'll see any issues but I'll run the patch and try and see if anything looks odd.

#15 @hnla
5 years ago

I removed the div in my stylesheet and ran some very quick scans and couldn't see any obvious issues, this isn't to say there wasn't a reason the extra specificity was thought necessary or that something might go awry but I think it's very edge casey if it were to. I would run with patch and 'div' removed.

#16 @DJPaul
5 years ago

#yolo

#17 @djpaul
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 10237:

bp-legacy: fix nonce and message when requesting private group membership

The AJAX handler for the “request private group membership” button was
sending the wrong nonce in its response when you leave a private group.
The button refreshes its label to read “request membership” but as the
nonce was wrong, only “-1” was printed to the screen. Nothing actually
happened.

By coincidence, this is one of those legacy parts of BuddyPress where
markup is shared between a template file and inside an AJAX callback
function in the PHP. While resolving that duplication is beyond the
scope of this immediate change, the AJAX version of the template has
been updated to match the template file (the only change is the button
label).

Fixes #5790

Props lakrisgubben

#18 @DJPaul
5 years ago

Thanks for the quick feedback Hugo!

Note: See TracTickets for help on using tickets.