Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#5477 closed enhancement (fixed)

Allow bp_has_groups to return all groups

Reported by: dcavins Owned by: dcavins
Milestone: 2.1 Priority: normal
Severity: minor Version:
Component: Groups Keywords: has-patch
Cc:

Description

For some non-presentation uses, it'd be handy to get every group using bp_has_groups() to loop through. This patch allows the user to specify 'per_page=-1' to ignore pagination parameters (like WP_Query's 'posts_per_page=-1' behavior) .

Attachments (3)

bp_has_all_groups.patch (561 bytes) - added by dcavins 6 years ago.
5477-2.patch (4.0 KB) - added by dcavins 5 years ago.
5477-3.patch (2.3 KB) - added by dcavins 5 years ago.

Download all attachments as: .zip

Change History (15)

#1 @boonebgorges
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 2.1

I don't have a problem with this specifically. Needs unit tests. Let's do it for 2.1.

In the meantime, you can get the same effect by passing false to 'per_page' and 'page'.

#2 @DJPaul
5 years ago

  • Keywords dev-feedback added

I'm not fond of this. -1 pagination doesn't scale, plus this looks like the wrong place to be preventing the LIMIT logic being added to the query. wontfix?

#3 @boonebgorges
5 years ago

-1 pagination doesn't scale

Since we're not using -1 pagination in BP core, I say that this is a consequence that we leave up to those who are implementing (plugin devs, etc). WP_Query allows posts_per_page=-1, with the same performance implications. Yet I've used posts_per_page=-1 countless times in building tools for clients, etc. So I think it's a reasonable API enhancement.

this looks like the wrong place to be preventing the LIMIT logic being added to the query.

It's not too dissimilar from what WP does (via the 'nopaging' bridge): https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-includes/query.php#L2260 https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-includes/query.php#L2866

I'd like to see a unit test, but otherwise this seems OK to me personally.

#4 @DJPaul
5 years ago

We can either commit this without a unit test, or punt the entire thing to 2.2. Let's decide soon.

#5 @dcavins
5 years ago

  • Milestone changed from 2.1 to 2.2
  • Owner set to dcavins
  • Status changed from new to assigned

I'll try to work out a unit test for 2.2.

@dcavins
5 years ago

#6 @dcavins
5 years ago

I've added a few test cases to cover setting page & per_page parameters to specific values, setting page & per_page parameters to false and setting per_page to -1 in BP_Groups_Group::get().

Please offer feedback about the unit tests, especially; I'd like to know that I'm testing the right situations. (For instance, these tests test BP_Groups_Group::get() behavior. Should I instead be testing groups_get_groups() or the bp_has_groups() loop?)

This is for 2.2, so there's no urgency on feedback. Thanks!

#7 follow-up: @boonebgorges
5 years ago

Testing BP_Groups_Group::get() is good - the lower down the stack the better.

The basic logic of the test looks good. The one thing I'd say is that the no-pagination tests don't really show much, since you'd get the same results with the default pagination args (per_page=20,page=1). It's annoying, but I think that to test this you need to create more than 20 groups. To save CPU overhead, do it in a single test and run multiple assertions on it. And no need to bother with the varying timestamps, since you're just checking counts (and thus don't care about the order). So:

$group_ids = array();
for ( $i = 1; $i <= 25; $i++ ) {
    $group_ids[] = $this->factory->group->create();
}

Then go ahead and run all of your three different pagination assertions in the same test, on the same data.

#8 in reply to: ↑ 7 @dcavins
5 years ago

Replying to boonebgorges:

Testing BP_Groups_Group::get() is good - the lower down the stack the better.

The basic logic of the test looks good. The one thing I'd say is that the no-pagination tests don't really show much, since you'd get the same results with the default pagination args (per_page=20,page=1). It's annoying, but I think that to test this you need to create more than 20 groups. To save CPU overhead, do it in a single test and run multiple assertions on it. And no need to bother with the varying timestamps, since you're just checking counts (and thus don't care about the order). So:

$group_ids = array();
for ( $i = 1; $i <= 25; $i++ ) {
    $group_ids[] = $this->factory->group->create();
}

Then go ahead and run all of your three different pagination assertions in the same test, on the same data.

Thanks for the feedback. I was thinking of 20 as the magic number, too, although it's really only the magic number in groups_get_groups() or the bp_has_groups(), since BP_Groups_Group::get() doesn't try to set a default. I'll update the tests and submit a new patch.

So it's OK to lump some tests together, if they basically test the same parameters? (I'm thinking more of code style than functionality.) Thanks again for the feedback.

@dcavins
5 years ago

#9 @boonebgorges
5 years ago

  • Keywords needs-unit-tests dev-feedback removed
  • Milestone changed from 2.2 to 2.1

BP_Groups_Group::get() doesn't try to set a default. I'll update the tests and submit a new patch.

Oy, you're right. Sorry about that.

So it's OK to lump some tests together, if they basically test the same parameters? (I'm thinking more of code style than functionality.)

Ideally, test case methods would be as small as possible. It makes debugging easier, because one failed assertion in a test marks the whole test as failed. However, in cases where lumping them together creates major performance improvements, I think it's fine to do so. Thanks again.

#10 @boonebgorges
5 years ago

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

In 8829:

Add support for disabling pagination in BP_Groups_Group::get() by passing per_page=-1

Also adds supporting unit tests.

Fixes #5477

Props dcavins

#11 @dcavins
5 years ago

Would it be useful for me to add some downstream tests of groups_get_groups() to the test case? I'm happy to do it if it's useful. I do think that testing groups_get_groups() would test pagination more completely. Besides, writing test cases is fun (especially now that I've got phpunit actually running, ha ha).

Oy, you're right. Sorry about that.

I, for one, am shocked-shocked, I tell you-that you don't know the 'blame' history for every line of code in BuddyPress. :)

#12 @boonebgorges
5 years ago

Besides, writing test cases is fun (especially now that I've got phpunit actually running, ha ha).

Sing it!! Feel free to write tests for any part of BuddyPress you see fit :)

Would it be useful for me to add some downstream tests of groups_get_groups() to the test case? I'm happy to do it if it's useful. I do think that testing groups_get_groups() would test pagination more completely.

The only reason to test wrapper functions like groups_get_groups() is if those wrappers do something to modify what goes into the wrapped function. groups_get_groups() doesn't really do anything but set some default values. So, it would be non-redundant to write tests that ensure that those default values are working correctly. (Eg, groups_get_groups() with no params should return 20 groups.) That said, as a rule, it's not really worth writing these kinds of tests when there are loads of lower-level functions that have no coverage at all - especially database methods. So, if you've got the hankering, you should go to those instead.

Note: See TracTickets for help on using tickets.