Skip to:
Content

Opened 5 years ago

Closed 2 months ago

Last modified 2 months ago

#921 closed enhancement (fixed)

bp_group_has_members() - "order" parameter request

Reported by: r-a-y Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: normal Version:
Component: Groups Keywords: has-patch needs-testing
Cc: math.viet@…

Description

Love the BP loops!

I would like to have a few additional parameters for the bp_group_has_members() loop.

Specifically, "orderby" and "order" arguments to sort by ascending, alphabetical order for example.

"order" could have the following options - 'alpha', 'joindate' (current default)
"orderby" could have the following - 'ASC', 'DESC'

The default list order is by whoever joined the group first.

Attachments (7)

921.diff (20.0 KB) - added by imath 7 months ago.
groupmembers-loop.php (3.9 KB) - added by imath 7 months ago.
921.02.diff (18.4 KB) - added by imath 2 months ago.
921.03.diff (13.8 KB) - added by boonebgorges 2 months ago.
921.04.diff (16.3 KB) - added by imath 2 months ago.
921.05.query.diff (8.8 KB) - added by boonebgorges 2 months ago.
921.05.template.diff (12.2 KB) - added by boonebgorges 2 months ago.

Download all attachments as: .zip

Change History (29)

comment:1 apeatling5 years ago

  • Milestone changed from 1.1 to 1.2

No time for this in 1.1, but will add for 1.2

comment:2 DJPaul4 years ago

  • Keywords reporter-feedback added
  • Milestone changed from 1.2 to 1.3

Still valid bearing in mind the numerous BP loop changes?

comment:3 johnjamesjacoby4 years ago

  • Component set to Core

Update on this:

Request is still valid, but the function names have changed.

Now this will apply to bp_has_members, and BP_Core_Members_Template.

comment:4 DJPaul3 years ago

  • Keywords needs-patch added
  • Priority changed from minor to normal

comment:5 boonebgorges3 years ago

  • Milestone changed from 1.3 to Future Release

Punting due to time constraints. If you can write a patch, we can consider rolling it back.

comment:6 aaclayton16 months ago

  • Component changed from Core to Groups
  • Severity set to normal

Seems like this ticket has been dead for a while, but I'd like to weigh in requesting that some arguments be added to bp_group_has_members();

Similar to the 'type' argument accepted by bp_has_members(), perhaps the group members loop could accept "newest, active, online, alphabetical, random"

Likewise, 'sort' => 'ASC' or 'type' => 'oldest' would be great for many groups. Showing the newest members (the current default) at the top of a roster might be counterproductive to some groups organizational structure.

Last edited 16 months ago by aaclayton (previous) (diff)

comment:7 imath7 months ago

  • Cc math.viet@… added
  • Keywords has-patch needs-testing 2nd-opinion added; reporter-feedback needs-patch removed

Hi,

I find the idea very interesting, it can be very convenient to sort group members, so i've explored it and i'm suggesting the attached patch : 921.diff.

I'm sorry i had to create a new template file i've called groupmembers-loop.php in bp-template/bp-legacy/buddypress/groups/single/. I've also attached this template to this ticket if you want to test the patch.

The patch creates a new select box in the group members area and in the group admin (front) area. The different options are :

  • Newest (same as current default way of sorting group members 'last_modified')
  • Oldest (Newest with a ASC order instead of DESC one)
  • Alphabetical (using if xprofile enabled profile fields, else $wpdb->users display_name to sort by name ASC)
  • Active (to sort members by their latest update in the current group if activity component is active), this way it can also be a reply to #3430
  • Friends (to only show the friends of the loggedin user)

About the last option, i think it can be interesting before joining a group for a regular user to easily see if some of his friends are already in the group to eventually join them. And in the admin front part of the group, a group admin might want to find his friends to eventually promote them as mods or admins.

imath7 months ago

imath7 months ago

comment:8 boonebgorges7 months ago

  • Keywords early-2.0 added

imath - thanks for the patch. I would definitely like to get this in, but I do think that we'll want to think more about how some of it works.

The way you've reworked the handlers in buddypress-functions.php seems less than ideal to me (we can do better, I think, than having special cases just for 'groupmembers'). In particular, if we're going to require component info in bp_legacy_theme_object_template_loader(), we should be either (a) sending it along with the JS, or (b) getting it from some centralized place where loop templates are registered.

This might, in fact, be a good time to rethink how the AJAX works here more generally. What we have now is a nifty little system, but the hoops you jump through in your patch demonstrate that it's not very flexible. This could be done in conjuction with the new template pack slated for the next release.

Let's look at this for 2.0.

comment:9 imath2 months ago

Hi boonebgorges,

So i put a new eye on the patch and choose option (a) : sending the template from js. I focused on the "display" screen of the group members (group admin screen hasn't changed). I've added 2 new options to filter the group member :

  • 'Oldest' which is the same than 'Newest' (last_modified) but with a ASC order
  • 'Alphabetical' which was used in Group single WordPress Administration screen but was not really working (see the comment in BP_Group_Member_Query->set_orderby() method).

I've also added a new search box as we can benefit from BP_User_Query.

Finally i've added a filter the way you suggested in #5356 to eventually let people add a custom option in the group members select box to change the order of the member_ids.

This is a screen capture of the result of 921.02.diff patch :

http://farm4.staticflickr.com/3798/12479282763_1b95721dd2_o.png

imath2 months ago

comment:10 ircbot2 months ago

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

comment:11 boonebgorges2 months ago

I have some thoughts about how this ticket can be improved, but I'm first going to make a couple preliminary fixes (and then post an updated patch).

comment:12 boonebgorges2 months ago

In 7930:

Refactor bp_group_has_members() stack to use array-style parameters

See #3797, #921

comment:13 boonebgorges2 months ago

In 7931:

Ensure that 'page' value passed to BP_Groups_Group_Members_Template is respected

Previously, it was being ignored - the only value considered was
$_GETmlpage?, with a hardcoded fallback of 1.

See #921

Props imath

comment:14 boonebgorges2 months ago

In 7932:

Add support for 'search_terms' param in bp_group_has_members() stack

See #921

Props imath

boonebgorges2 months ago

comment:15 follow-up: boonebgorges2 months ago

  • Keywords 2nd-opinion removed

921.03.diff is imath's .02.diff, but updated for the current trunk. There were a couple of random bugfixes and improvements mixed up in the original patch, which I separated into different commits. Also, .02.diff had some changes to parameter order in various functions that would've broken backward compatibility, so I thought this would be a good time to convert to using the parameters-as-array format.

Specific thoughts on imath's approach:

  • Let's not pass parameters to BP_Group_Member_Query::get_group_member_ids(). Just get the value out of $this->query_vars.
  • I know we use the 'type' param (like type=last_modified) throughout BuddyPress, but I'm not a huge fan of it. It doesn't provide very much flexibility; 'order' and 'orderby' are much better, with 'type' being translated into 'order' and 'orderby' if necessary. Of course, now that I type this, I see that BP_User_Query doesn't support this syntax, so maybe it's an area for future enhancement.
  • Maybe instead of setting the $type in get_group_member_ids(), it'd be possible to do it in setup_hooks(). I don't totally remember the logic behind setting it in setup_hooks, but it looks like it might be reproducing (or reversing) some logic to do it later on as well. Play with it and see what you find.
  • The changes to BP_Group_Member_Query really need unit tests.
  • The special case for the Group Members page in bp_legacy_theme_ajax_querystring() is really ugly. Isn't there another way we can handle this? Why is 'object' not set properly by passing 'group_members' to bp_ajax_querystring() in groups/single/members.php? I'm not sure about the exclude_admins_mods item - maybe we could set this in the JS somewhere (like you're doing with template)? or via a hidden input in the template that's then slurped up by the JS? or maybe in the AJAX handler? bp_legacy_theme_ajax_querystring() is really meant to be a generic utility function, so I'd really like to keep special cases out of it if possible.

The rest of the approach looks good. The technique for determining member sort in BP_Group_Member_Query looks pretty elegant, and I like the change to bp_legacy_theme_ajax_querystring() and bp_filter_request() that let you pass a custom template. If we address some of the items I list above (especially unit tests! no messing with queries without them!) I think this'd be ready for 2.0.

imath2 months ago

comment:16 in reply to: ↑ 15 imath2 months ago

Replying to boonebgorges:

  • The special case for the Group Members page in bp_legacy_theme_ajax_querystring() is really ugly. Isn't there another way we can handle this? Why is 'object' not set properly

Thanks a lot boonebgorges for the commits and the "updated to the trunk" patch.

a) I've played with it and noticed that the filters were not working, so i've edited the arguments of groups_get_group_members() & BP_Groups_Group_Members_Template::__construct() to include a "type" one.
b) As i was checking theme backcompat i've found a specific case (group/single/home template updated but not groups/single/members) where the pagination were messed up with admin-ajax. So i suggest to use an absolute url for the pagination links, just in case ;)

Now the "ugly" check in bp_legacy_theme_ajax_querystring() :)

"once upon a time" of its birth :
In buddypress.js, the object is first defined by a parsing on the value of the id attribute of .item-list-tabs ul li.selected. in group members case, this value is "members" as the id is #members-groups-li. I was a bit afraid to modify this logic for this specific case, and chose to mimic the way friends case was handled by forcing the object to be set as "members" in case it is defined to "group_members" in bp_filter_request(). That's the reason, i use the ugly check.

In 921.04.diff, i've included a) & b) and suggest to move the check at the top of bp_group_has_members() function.

comment:17 ircbot2 months ago

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

boonebgorges2 months ago

comment:18 follow-up: boonebgorges2 months ago

  • Keywords early-2.0 removed
  • Milestone changed from Future Release to 2.0

Cool, I think we're getting close!

This patch is getting a bit unwieldy, and really it does two different things (and so should be committed separately anyway), so I've split it into two patches:

921.05.query.diff

This contains the modifications to BP_Group_Member_Query that allow for $type sorting, as well as the mods to the bp_has_members() function stack that allow you to take advantage of $type. I've made a couple of modifications to 04.patch:

  • Changed 'last_modified' and 'first_modified' to 'last_joined' and 'first_joined'. This makes much more sense from the point of view of themers/developers.
  • Changed some of your logic in set_orderby(), so that it's possible to use *any* of the $type values supported by BP_User_Query (even though you're only using 'alphabetical' in your patch).
  • Added unit tests for the new $type parameter.
  • Reworked some documentation
  • Removed one instance of setting $this->query_vars['type']. This is already done in setup_hooks().

921.05.template.diff

This is the patch that implements the new features in query.diff within the context of single groups. I think the only thing I changed here was 'last_joined' and 'first_joined'. The rest of it looks fine to me.

Give it a look and see what you think.

comment:19 in reply to: ↑ 18 imath2 months ago

Replying to boonebgorges:

I've just tested and it works great!!

  • Changed 'last_modified' and 'first_modified' to 'last_joined' and 'first_joined'. This makes much more sense from the point of view of themers/developers.

I agree.

  • Changed some of your logic in set_orderby(), so that it's possible to use *any* of the $type values supported by BP_User_Query (even though you're only using 'alphabetical' in your patch).

Yes, it's a great idea. I've limited to alphabetical in my patches because i thought maybe at wrong ("peut-être à tort" ) that available orders in BP_User_Query was a bit too global (active in the whole community instead of in the group for instance). But online and popular can be interesting.

Give it a look and see what you think.

I think it's really great. This feature is really nice and i think it gives a new opportunity to theme/plugin developers to extend with new types of order using the action 'bp_groups_members_order_options' to add a customized order in the dropbox and the filter 'bp_group_member_query_group_member_ids' to build the corresponding order.
Bravo for the amazing work :)

comment:20 boonebgorges2 months ago

In 7948:

Introduce support for a 'type' sorting parameter in BP_Group_Member_Query

The 'type' parameter supports any of the arguments ('newest', 'active', etc)
supported by the parent class BP_User_Query. In addition, it's possible to
sort by type 'last_joined' and 'first_joined'.

This changeset also introduces the 'type' parameter through the
bp_group_has_members() function stack.

See #921

Props imath, boonebgorges

comment:21 boonebgorges2 months ago

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

In 7949:

Enable member sorting and pagination on group Members pages

Fixes #921

Props imath, boonebgorges

comment:22 aaclayton2 months ago

Hey guys, thanks so much for the awesome work on this, I'm really looking forward to incorporating some group member sorting into my theme. Thumbs up!

Note: See TracTickets for help on using tickets.