Skip to:
Content

BuddyPress.org

Opened 12 years ago

Closed 12 years ago

#4482 closed enhancement (fixed)

Better member type support in bp_group_has_members()

Reported by: boonebgorges's profile boonebgorges Owned by: boonebgorges's profile boonebgorges
Milestone: 1.8 Priority: high
Severity: major Version:
Component: Groups Keywords: has-patch commit
Cc: trisha@…

Description

As part of the Group Admin screens (#4414), some enhancements will need to be added to bp_group_has_members(). Specifically, it must be possible to filter group members based on their group role (admin, mod, member, banned). Currently, bp_group_has_members() supports 'exclude_admins_mods' and 'exclude_banned' arguments; the function should be refactored so that it internally translates these arguments to a proper list of queried member types.

Attachments (2)

4482.patch (26.0 KB) - added by boonebgorges 12 years ago.
4482.2.patch (25.4 KB) - added by johnjamesjacoby 12 years ago.
Avoid using $bp global, additional code cleaning from 4482.patch

Download all attachments as: .zip

Change History (21)

#1 @johnjamesjacoby
12 years ago

  • Keywords needs-patch added
  • Milestone changed from 1.7 to 1.8
  • Type changed from defect (bug) to enhancement

#2 @boonebgorges
12 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major
  • Status changed from new to assigned

My goal is to implement this using BP_User_Query, which will have the additional benefit of removing global table joins.

#3 @trishasalas
12 years ago

I have some sql worked out to grab group members and filter based on the parameters you mentioned. I will try to get it into patch format but am not familiar with svn, trac or patching :) Will ping.

#4 @trishasalas
12 years ago

  • Cc trisha@… added

#5 @boonebgorges
12 years ago

See #4977, which is a symptom of the shortcomings this ticket is meant to fix.

#6 @trishasalas
12 years ago

I got sidetracked with client work but have made some progress with the 'how' of it.

My biggest hurdle is wading through the BuddyPress code itself since it is somewhat unfamiliar to me. I have a file I put together but it is just a conglomeration of all the parts I think might work. I still need to go through them and test and narrow it down after adding my sql to it.

I'm not sure if it is even 'post-worthy' at this point but I don't mind posting if only to get some direction. I'm trying to keep in mind that everyone is busy and do my learning on my time :)

I'm pretty sure I want to go with Boone's suggestion of query for the member list and then pass it to the
'include' param of BP_User_Query; but I might use one of BP_User_Query's filters...will need to test.

#7 @r-a-y
12 years ago

I took a brief look at this yesterday when I posted #5000.

It seems to me that we're duplicating a lot of code for the bp_group_has_members() loop. I was thinking we could probably reuse the bp_has_members() loop and pass only the member IDs we need to display using the include parameter.

The bp_group_has_members() loop would support a new parameter of role and we can query for these member IDs and then pass them to bp_has_members().

The negatives with this approach is this will require an additional query to grab the member IDs, but it reuses code that we don't have to duplicate.

Last edited 12 years ago by r-a-y (previous) (diff)

#8 @trishasalas
12 years ago

I am working on this tonight using the include param of the BP_User_Query mainly because I have it halfway started and that is the direction my brain is travelling at the moment. That said, I can see the value in bp_has_members().

This is very much a learning experience for me and I feel like I will learn more than I will contribute with this patch. @r-a-y I am open to any and all suggestions!!

I will post what I have, finished or not, before the devchat tomorrow.

#9 @boonebgorges
12 years ago

Thanks, r-a-y. This is similar to the idea floated above about using BP_User_Query. The thing about using bp_has_members() is that it acts sorta like a black box from the point of view of the Groups component. So, while it's easy enough to pass an 'include' parameter, there are a couple of limitations:

1) It'll make the include param of bp_has_members() unavailable for other, concurrent uses (like showing a subset of users in a group). We could probably do some parsing to work around this, but it's a bit clunky.
2) bp_has_members() has support for *limiting the results by user id* but not for other kinds of filtering/sorting that might be relevant for group queries (and other queries down the road - like friends). Eg, if we want to sort by the date the user joined the group, it won't be possible using this method.
3) I'm not sure that the template global $members_template created by bp_has_members() is structured the same as that created by bp_group_has_members(), so we may break backward compatibility.

I think that we should definitely aim to consolidate here, but my gut feeling is that we should do it using BP_User_Query. Ideally, what I'd like is for BP_User_Query to be flexible enough that it could be extended using actual OOP techniques for specific components. So, from the appropriate procedural wrapper, you'd invoke a new BP_Group_Member_Query, which might look like something like

class BP_User_Query {
    // ...
    public function get_include_ids() {
        return wp_parse_id_list( $this->query_vars['include'] );
    }
}

// and then the extending class
class BP_Group_Member_Query extends BP_User_Query {
    public function get_include_ids() {
        // Passed to 'include'
        $include = wp_parse_id_list( $this->query_vars['include'] );

        // Figure out what user type is currently being requested
        if ( isset( $this->query_vars['group_id'] ) && isset( $this->query_vars['group_role'] ) ) {
            $group_members = $this->get_group_members_by_role( $this->query_vars['group_id'], $this->query_vars['group_role'] );
        }

        // Put them together
        return array_intersect( $include, $group_members );
    }

    protected function get_group_members_by_role( $group_id, $group_role ) {
        // Do some SQL to get the users
    }
}

BP_User_Query would then use get_include_ids() when building the master SQL query.

This model seems to be very flexible, and one that we could implement little by little (by abstracting different clauses of the main query into methods as needed). The downside is that it's not terribly WPish - we could do something similar using the bp_pre_user_query hook, though it's far less elegant.

So, that's sorta the shape of what I'm imagining, having mulled it over for a few weeks. It adds a bit more overhead than what r-a-y is suggesting, but it keeps things more organized, and it provides an excellent template for future enhancements (such as BP_Friend_Query, and other relevant kinds of user queries). I'm anxious to see how this meshes with some of the work that trishasalas has been doing.

#10 @trishasalas
12 years ago

@boonegorges you're throwing me off with that oop stuff ;) (although I have honestly wondered about using that within WordPress so glad to see it) When I said I know SQL I mean I know SQL, I am very comfortable in a database, highly frustrated that I can't easily translate that to what needs to happen here. ...I am working with the functions in question and understanding so far, I want to slog through it as it's the best way to learn even if what I end up with doesn't work. I'll cry uncle tomorrow :)

Here is my SQL...there are a couple of joins for my purposes mostly to see the data:

SELECT
    `bp_groups_members`.`id`
    , `bp_groups_members`.`user_id`
    , `bp_groups_members`.`is_admin`
    , `bp_groups_members`.`is_mod`
    , `bp_groups_members`.`is_banned`
    , `users`.`user_nicename`
    , `users`.`user_email`
    , `bp_groups`.`name`
FROM
    `bp_groups_members`
    INNER JOIN `users` 
        ON (`bp_groups_members`.`user_id` = `users`.`ID`)
    INNER JOIN `bp_groups` 
        ON (`bp_groups_members`.`group_id` = `bp_groups`.`id`);

I'm seeing that the joins aren't necessary with the way the BP_User_Query function is written. I'm also seeing that using PHP with the sql is way cool.

Feeling like such a dufus...but maybe making myself vulnerable will help others do the same. Appreciating more and more the open source community.

#11 @trishasalas
12 years ago

I may be way off base so I'm going to go ahead and post what I have so as not to waste time. I'll look at it again in the morning and I'll be here for devchat. boonegorges, if OOP is the direction you are going with BuddyPress php this is encouraging. I am starting to take classes to fill in the gaps with what I know and OOP has made the most sense but doesn't always seem to fit with WordPress. Let me know if this is an accurate assessment on my part. I have just signed up for a class in Plugin Development with Tom McFarlin so I'll likely do a bit of both.

Here is what I have so far...clueless as to how to submit a patch...unless this is that ;)

<?php


class BP_User_Query {
     // ...
     public function get_include_ids() {
         return wp_parse_id_list( $this->query_vars['include'] );
     }
 }

 // and then the extending class

 class BP_Group_Member_Query extends BP_User_Query {

     public function get_include_ids() {

         // Passed to 'include'

         $include = wp_parse_id_list( $this->query_vars['include'] );

         // Figure out what user type is currently being requested

         if ( isset( $this->query_vars['group_id'] ) && isset(

 $this->query_vars['group_role'] ) ) {

             $group_members = $this->get_group_members_by_role(

 $this->query_vars['group_id'], $this->query_vars['group_role'] );

         }

         // Put them together
         return array_intersect( $include, $group_members );
     }

     protected function get_group_members_by_role( $group_id, $group_role )
 {
         //Get the group Admins

{		{
		$this->uid_name = 'user_id';
		$sql['select'] = "SELECT {$this->user_id} FROM {$wpdb->bp_groups}";
		$sql['where'][] = $wpdb->prepare( "group_id = %s", group_id( $group_id ) ); //I'm not sure how to pass the group_id variable here

				if  {
					$sql['where'][] = $wpdb->prepare( is_admin( 1 ) );
					$group_role = 'admin';
				} else if  {
					$sql['where'][] = $wpdb->prepare( is_admin( 0 ) );
					return;
				} 

		//Get the group Moderators


		$this->uid_name = 'user_id';
		$sql['select'] = "SELECT {$this->user_id} FROM {$wpdb->bp_groups}";
		$sql['where'][] = $wpdb->prepare( "group_id = %s", group_id( $group_id ) ); //I'm not sure how to pass the group_id variable here

				if  {
					$sql['where'][] = $wpdb->prepare( is_mod( 1 ) );
					$group_role = 'moderator';
				} else if  {
					$sql['where'][] = $wpdb->prepare( is_mod( 0 ) );
					return;
				} 	

		//Get Banned Members


		$this->uid_name = 'user_id';
		$sql['select'] = "SELECT {$this->user_id} FROM {$wpdb->bp_groups}";
		$sql['where'][] = $wpdb->prepare( "group_id = %s", group_id( $group_id ) ); //I'm not sure how to pass the group_id variable here

				if  {
					$sql['where'][] = $wpdb->prepare( is_banned( 1 ) );
					$group_role = 'banned';
				} else if  {
					$sql['where'][] = $wpdb->prepare( is_banned( 0 ) );
					return;
				} 		
	}
 		
     }
 }
 //I'm also assuming here that the $group_id variable is defined previously in the page as it is currently

#12 @boonebgorges
12 years ago

In 7091:

Don't use extract in bp_group_has_members. See #4482

#13 @boonebgorges
12 years ago

trishasalas - Thanks for the patch. This is moving in the right direction, but there are some limitations in your approach. Mainly, if we're going to be able to query by a *single* role, it'd also be nice to query by *multiple* roles (eg, give me all admins + mods). Also, you're reproducing a big failing of the current implementation, which is JOINing against WP's wp_users table. This causes problems on certain sorts of installations, and it's something I'd like to factor out of BP to whatever extent possible.

4482.patch is a first pass at the approach discussed above. As often happens, it ended up being a bigger job that I'd anticipated. Here's a brief summary:

  • Introduces BP_Group_Member_Query, which extends BP_User_Query. This required modding BP_User_Query in a couple of ways, mainly by way of abstracting certain bits so that they could be overridden by specific bits in the new class.
  • Deprecates BP_Groups_Member::get_all_for_group(), and replaces it in the bp_group_has_members() stack with BP_Group_Member_Query (with legacy support via filter, just like with bp_core_get_users() and BP_User_Query
  • Unit tests to make sure that the swap-out described above does not break backward compatibility.
  • Added a 'group_role' param to the bp_group_has_members() stack, which supports multiple roles being passed.
  • Adds unit tests for all of the role combinations.

Overall, I think the changes are pretty good. By eliminating the use of get_all_for_group() we are starkly decreasing the amount of SQL required (since we use BP_User_Query for limit/pagination and exclude arguments), while greatly increasing the functionality of bp_group_has_members() (since it'll now accept any BP_User_Query parameters. This is definitely moving in the right direction.

I have mixed feelings about the technique I've chosen to build BP_Group_Member_Query. On the one hand, it'll make our life easier down the road if we use real OOP techniques, like I've suggested with get_include_ids. On the other hand, it's not very WordPressy - what I've done is pretty close to being a regular WP filter. The problem with WP filters, though, is that they're hard to juggle: you have to have a place for hooking them in the first place (see my setup_hooks() method), and then you have to remove them later on, because there might be more than one instance of a BP_User_Query on a page. The OOP technique is much cleaner in this sense. At the same time, we're already using a regular WP filter for 'bp_user_query_populate_extras'. So what you see in my implementation is a mix of the two techniques: class inheritance with get_member_ids(), and WP filters with populate_group_member_extras(). On balance, when I think about the different techniques I could have used (and I have thought about a lot of them....), I think this is the best one.

Any feedback welcome. If people like this technique, it can be extended to create BP_Friend_Query very easily, giving us a uniform approach for user queries throughout BP.

@boonebgorges
12 years ago

#14 @trishasalas
12 years ago

My comments won't have too much to do with your method, looks as elegant as possible to me.

I knew the join was not a good way to handle the situation but what you are seeing with my code is:

1) My lack of familiarity with BuddyPress (if I grab the users from group members how do I know who they are from just their id? is that in BP_User_query? I thought it might be but wasn't sure.
2) My lack of php coding experience. In retrospect I probably should have said as much but I wanted to try.

The idealist in my loves the use of OOP techniques, I would like to see WordPress head that direction but understand and really trust the devs working on that code base.

Once the dust settles on the push for 1.8 I would value some direction as to how to get my coding skills where they need to be to be helpful in situations like this. I am doing as much as I can find as far as online classes, etc. The problem with php is that there is so much out there and not all of it is good.

All that being said. I like the idea of uniform queries throughout BuddyPress and I would love to do some documentation in the codex on that. I know it's in the code but I can definitely see the value of getting it out there.

#15 @boonebgorges
12 years ago

what you are seeing with my code is

No explanations needed :) Seeing your approach is definitely instructive. It also happens that this particular ticket is a hard one, because the goal (or *my* goal at least) was not so much to enhance an existing feature as to reconceptualize and rebuild the implementation.

Once the dust settles on the push for 1.8 I would value some direction as to how to get my coding skills where they need to be to be helpful in situations like this

We can chat off-Trac about it, but my most immediate feedback is to keep combing through Trac, and keep patching - the best way to learn, in my experience, is to write patches and get feedback.

I like the idea of uniform queries throughout BuddyPress and I would love to do some documentation in the codex on that.

This is a good idea. If you've got ideas for how it might be organized, you should feel free to jump in and start creating pages on the Codex - you should have Author access.

#16 @boonebgorges
12 years ago

  • Keywords has-patch commit added; needs-patch removed

Hi all. One last ping for feedback before I commit this. Note that the technique put forward in my patch doesn't have to represent the final word in this implementation - I'm happy to put it in and then iterate. But of course it'll be hard to iterate on something fatally flawed, so it's really the fatal flaws I want to hear about.

In any case, having a fix for this ticket in place is key to fixing a number of existing bugs in the Groups Admin interface (see eg #4977), so I would like to get it done and fix those bugs before the end of our dev period in a few days.

johnjamesjacoby - I know you said you might have something to say about it, so please do so now, or forever hold your piece :)

@johnjamesjacoby
12 years ago

Avoid using $bp global, additional code cleaning from 4482.patch

#17 @johnjamesjacoby
12 years ago

Approach looks great. The new methods make sense, and the new class is structured nicely. Hard coding the group roles might be a problem if anyone is trying to be creative, but we'll figure that out.

Looks ready to go to me!

#18 @boonebgorges
12 years ago

Excellent - Thanks for the feeback.

#19 @boonebgorges
12 years ago

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

In 7141:

Introduces BP_Group_Member_Query and refactors bp_group_has_members() to use it

BP_Group_Member_Query extends BP_User_Query, which has a number of notable
benefits:

  • Group member queries no longer JOIN against global user tables
  • Less code duplication, since general logic like 'exclude' is handled by BP_User_Query
  • Future access to the additional parameters of BP_User_Query, such as 'type'

Using the new BP_Group_Member_Query, this changeset also changes the way that
group member queries filter by group roles (member, mod, admin). The new
group_role parameter in the bp_group_has_members() stack accepts an array of
group roles. The legacy argument 'exclude_admins_mods' is still accepted, and
translates to 'group_role' => array( 'member' ) when true. These group_role
enhancements will allow for future enhancements in the Groups Admin section of
the Dashboard, and other places where it might be useful to query for the
members of a group matching a specific role. See #4977.

Fixes #4482

Props trishasalas for early patches and feedback. Props johnjamesjacoby for
review.

Note: See TracTickets for help on using tickets.