Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5106 closed defect (bug) (fixed)

Better sorting for new bp_group_has_members() query

Reported by: aaclayton Owned by: boonebgorges
Milestone: 1.8.1 Priority: normal
Severity: normal Version: 1.8
Component: Groups Keywords: has-patch needs-testing commit
Cc:

Description

Since 1.8, the new bp_group_has_members() query is working great, but I perceive a limitation of the new approach. Previously, bp_group_has_members() sorted group members based on their join date (‘last_modified’ in the bp_groups_members table). After the update, the query is sorting based on ‘user_id’ instead.

It would be really great to include sorting arguments in bp_groups_has_members(). I think that this function could accept “type” and “sort” arguments, where “type” could take values of either ‘user_id’ or ‘last_modified’, and “sort” would be either ‘ASC’ or ‘DESC’.

From an organizational perspective, a group member’s join date is (typically) a more informative sorting variable, and depending on the use case, either ascending OR descending could make more sense. Giving BuddyPress users the ability to tweak their loop sorting in the function would be great.

Hope this is feedback that could be implemented. Thank you!

Attachments (1)

5106.patch (3.5 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (8)

#1 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 1.8.1
  • Type changed from enhancement to defect (bug)

Thanks for the report, aaclayton. I've confirmed the issue.

The changed behavior for bp_group_has_members() is a bug. It was intended that BP_Group_Member_Query would sort by last_modified. See http://buddypress.trac.wordpress.org/browser/tags/1.8/bp-groups/bp-groups-classes.php#L1100. And when these values were passed back to BP_User_Query, it was intended that the query would preserve this sorting. I'll look into why this isn't happening, but the fact that it's not is a regression, so I'm moving the ticket to 1.8.1.

As for the further request for better query params - I agree (see the inline todo!). Once I've fixed the regression in this ticket, I'll move this request to a separate enhancement ticket.

#2 @boonebgorges
8 years ago

  • Keywords has-patch needs-testing added

There's a fundamental problem with the way BP_User_Query's fallback sort order works, or at least the way BP_Group_Member_Query relies on it. In MySQL, when you fail to provide an ORDER BY clause, the primary column is assumed, which in this case is the user id.

Because we want to maintain the sort order based on the order of the values passed to IN, we have to use a ORDER BY FIELD() clause. See 5106.patch. This is not elegant, and probably won't perform well when a group has many thousands of members, but this is a larger architectural problem that doesn't really need to be addressed in this ticket.

Posting here for review by another dev.

@boonebgorges
8 years ago

#3 @DJPaul
8 years ago

  • Keywords commit added

Haven't tested it just yet, but the patch looks good (ORDER BY FIELD is how WP_Query does its post__in ordering).

#4 @boonebgorges
8 years ago

Thanks, DJPaul - never even thought to look at how WP_Query does it, but the fact that they do it the same way is a good indicator that it's good enough for us too.

#5 @boonebgorges
8 years ago

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

In 7294:

Tell BP_Group_Member_Query queries to order by FIELD()

BP_Group_Member_Query works by first querying for a list of group members
that match the query params, sorted by date_modified. Then, it feeds those ids
to the BP_User_Query parent class. In order to maintain the proper
date_modified sort, we have to override BP_User_Query's ORDER BY param, and
replace it with ORDER BY FIELD(), using the user ids from the first query.

This fixes a regression in BP 1.8, where the default sort order for
bp_group_has_members() was inadvertantly changed to user_id DESC, rather than
the legacy date_modified that existed before the introduction of
BP_Group_Member_Query.

Fixes #5106

#6 @boonebgorges
8 years ago

Opened #5109 for the enhancement request.

#7 @boonebgorges
8 years ago

In 7296:

Tell BP_Group_Member_Query queries to order by FIELD()

BP_Group_Member_Query works by first querying for a list of group members
that match the query params, sorted by date_modified. Then, it feeds those ids
to the BP_User_Query parent class. In order to maintain the proper
date_modified sort, we have to override BP_User_Query's ORDER BY param, and
replace it with ORDER BY FIELD(), using the user ids from the first query.

This fixes a regression in BP 1.8, where the default sort order for
bp_group_has_members() was inadvertantly changed to user_id DESC, rather than
the legacy date_modified that existed before the introduction of
BP_Group_Member_Query.

Fixes #5106

Note: See TracTickets for help on using tickets.