Skip to:
Content

BuddyPress.org

Opened 7 months ago

Closed 6 weeks ago

#8292 closed defect (bug) (fixed)

Multiple member types users table issue

Reported by: etatus Owned by: imath
Milestone: 7.0.0 Priority: normal
Severity: major Version: 5.2.0
Component: Members Keywords: has-patch
Cc: etatus

Description

Hello, when a member has more than one member type, the users table in admin view only shows one of them. To fix it I made the following changes:

buddypress/bp-members/classes/class-bp-members-admin.php
Line 2369:

Change this:

// Get the member type.
$type = bp_get_member_type( $user_id );

// Output the
if ( $type_obj = bp_get_member_type_object( $type ) ) {
	$url = add_query_arg( array( 'bp-member-type' => urlencode( $type ) ) );
	$retval = '<a href="' . esc_url( $url ) . '">' . esc_html( $type_obj->labels['singular_name'] ) . '</a>';
}

return $retval;

To this:

// Get the member types.
$types = bp_get_member_type( $user_id, false ); // Set to false to get member types array insted only one type
foreach( $types as $type ) {
	// Output the
	if ( $type_obj = bp_get_member_type_object( $type ) ) {
		$url = add_query_arg( array( 'bp-member-type' => urlencode( $type ) ) );
		$retval .= '<a href="' . esc_url( $url ) . '">' . esc_html( $type_obj->labels['singular_name'] ) . '</a>, ';
	}
}  		
return rtrim( $retval, ', ' ); // Remove last comma

I hope this piece of code helps to fix the problem. I don't know if there are other implications in other parts of code related to this issue.

Attachments (5)

8292.patch (10.3 KB) - added by vapvarun 7 weeks ago.
Initial Patch
8292-2.patch (8.2 KB) - added by vapvarun 7 weeks ago.
removed additional codes
8292-3.patch (7.8 KB) - added by vapvarun 6 weeks ago.
updated function
8292.2.patch (7.8 KB) - added by imath 6 weeks ago.
8292.3.patch (8.6 KB) - added by imath 6 weeks ago.
Improves the multiple select display (use all available width)

Download all attachments as: .zip

Change History (18)

#1 @imath
6 months ago

  • Milestone changed from Awaiting Review to 7.0.0

Thanks for your feedback. I guess we also need to make it possible to select more than one member type into the WP-Admin/Extended Profile's metabox.

#2 @etatus
6 months ago

  • Cc etatus added

Yes, definitely it's a good idea to allow selecting more than one member type as multiple member types support is a core feature.

Last edited 6 months ago by etatus (previous) (diff)

This ticket was mentioned in Slack in #buddypress by vapvarun. View the logs.


2 months ago

@vapvarun
7 weeks ago

Initial Patch

@vapvarun
7 weeks ago

removed additional codes

#5 @imath
7 weeks ago

  • Keywords needs-refresh added

Hi @vapvarun

Thanks a lot for you great work on this. The video looks very promising 🤩. I must say I have a very strong concern about changing the 2nd argument type of the bp_set_member_type() function from a string to an array because it will break BuddyPress plugins using this function. It will also break the BP REST API and if you run the PHPUnit test suite, you will probably see a lot unit tests fail.

I believe we possibly have 2 options to deal with this :

  1. create a new function to set member types like bp_set_member_types()
  2. accept array as well as string for the bp_set_member_type() function and put the string into an array instead of returning false.

The 2nd option seems to be what was chosen for the Group Type feature. So I guess we should use this way to keep some consistency. Have a look at the bp_groups_set_group_type() function.

The 2 very important things we absolutely need to preserve about the bp_set_member_type() function are :

  • bp_set_member_type( $user_id, 'foo' ) must add the 'foo' member type to the user.
  • bp_set_member_type( $user_id, '' ) must remove all member types from a user as it's used by the bp_remove_member_type_on_user_delete() function to do so.

About the rest of your patch in src/bp-members/classes/class-bp-members-admin.php:

  1. Lines 1301 to 1306 should look like this:
 	1301	<option value="">
 	1302	    <?php // should have its own line
 	1303	    /* translators: no option picked in select box */
 	1304	    esc_attr_e( '----', 'buddypress' );
 	1305	    ?> // should be on a new line.
 	1306	</option>

Same thing for lines 1308 to 1312, they should look like this:

 	1308	<?php // should have its own line
 	1309	if ( ! empty( $current_types ) && in_array( $type->name, $current_types ) ) { // add a space between 'if' and '(' and before '{'
 	1310	    $selected = 'selected';
 	1311	} else { // add a space before and after 'else'
 	1312	    $selected = '';
 	1313	}
 	1314	?> // should be on a new line.           

Let's strictly follow WordPress PHP code standards 😉 for the new code we're adding (we'll need to review existing code later)

  1. What's the goal of line 2340 : $counter = 0;
  2. It looks like there's an indentation problem from line 2351 to 2364.
  3. At line 2510 : if ( count( $types ) > 5 ) why slicing the array when there are more than 5 member types ?
  4. lines 2522 to 2523 should look like this:
    2522	$url           = add_query_arg( array( 'bp-member-type' => urlencode( $type ) ) );
    2523	$member_types .= '<a href="' . esc_url( $url ) . '">' . esc_html( $type_obj->labels['singular_name'] ) . '</a>';
    // consecutive equal signs must be aligned.
    

#6 @vapvarun
7 weeks ago

@imath will update it, it was just an initial raw code to show how to do it.

#7 @vapvarun
7 weeks ago

@imath if any member has more than 5 member type, we can display a couple or a max number of member type in all user listing column, it will help to keep the integrity of the user row, instead of display all assigned member types ( let's say 10 ) at a time.

#8 @imath
7 weeks ago

Sure no problem 👍

About the integrity of the user row, I believe we should simply list all member types just like WordPress does for terms into the WP Posts List table. I believe it can be confusing to filter the members list according to a type that is not shown into the list of the 5 ones.

You can eventually add a filter before the output for people wishing to do this slice 😉

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


6 weeks ago

@vapvarun
6 weeks ago

updated function

#10 @imath
6 weeks ago

  • Keywords needs-refresh removed

@vapvarun thanks a lot for the improvements you brought to your patch. To be consistent with how setting group types is managed, I believe we should allow to pass an array to the bp_set_member_type() function.

8292.2.patch is doing so. The patch is also making sure bulk adding/removing a member type functionality is more consistent with what we should expect: remove all member types when the remove bulk action was used (instead of one of the member type) and replace all member types with the selected member type for the add/set bulk action.

About this last point, I believe we should edit the bulk adding/removing a group type the same way in another ticket.

Unit tests are doing ok. Could you test the patch to confirm it behaves the right way?

@imath
6 weeks ago

@imath
6 weeks ago

Improves the multiple select display (use all available width)

#11 @vapvarun
6 weeks ago

Record screencast with patch
1- Able to add multiple Type
2- Able to remove all
3- Able to change multiple-member types and assign new
4- Assigned member types are visible at all users list.
https://screencast-o-matic.com/watch/cY60Q3sVw8

Also got a one-time error during bulk update, but was not able to replicate it again

Warning: count(): Parameter must be an array or an object that implements Countable in /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php on line 2396 Notice: Trying to access array offset on value of type bool in /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php on line 2396 Warning: count(): Parameter must be an array or an object that implements Countable in /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php on line 2396 Notice: Trying to access array offset on value of type bool in /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php on line 2396 Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php:2396) in /var/www/html/wp-includes/pluggable.php on line 1296 Warning: Cannot modify header information - headers already sent by (output started at /var/www/html/wp-content/plugins/buddypress/src/bp-members/classes/class-bp-members-admin.php:2396) in /var/www/html/wp-includes/pluggable.php on line 1299

https://screencast-o-matic.com/watch/cY60QDsVwq

#12 @imath
6 weeks ago

Thanks a lot for your tests and for sharing the warning about count() usage @vapvarun

I'm going to look at it asap and will commit soon.

#13 @imath
6 weeks ago

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

In 12765:

Administration: add support for multiple member types assignment

Community Administrators can now use the multiple select box of the User's Extended Profile Member Type metabox to assign one or more Member Types to the user.

Data for the Member Type column of the WP Users List Table are now displaying a comma separated list of Member Types for Users being assigned to more than one Member Type (which was the original ticket's need).

The bulk removing or assigning member types functionalities have been improved to behave as follows :

  • Bulk removing member types is removing all assigned member types.
  • Bulk assigning a member type is replacing all assigned member types with the selected member type.

Props etatus, vapvarun

Fixes #8292

Note: See TracTickets for help on using tickets.