blog directory search returns incorrect result count

[jreeve]

The results count on the blog search results page sometimes returns results that are inconsistent with the actual number of returned results. See some screenshots of this behavior in action at my bug report here: ​https://github.com/mlaa/cbox-mla/issues/104. This is because, in BP_Blogs_Blog::get() from bp-blogs-classes.php, the returned blog summaries, $paged_blogs, searches blog names, while the results count of the blogs, $total_blogs, searches both blog names and blog descriptions. Thus, if there are, say, three blogs whose names contain "foo," but six blogs whose descriptions (but not names) contain "foo," then a search for "foo" would return three results, but the pagination display at the bottom of the page would say "Viewing blog 1-9 (of 9 blogs)," which is untrue.

To fix this, there are two options: (1) tell the blog search to look for its search terms in both the names and the descriptions of blogs, or (2) tell the blog search counter not to look in blog descriptions. Although (1) makes the blog search a little cooler, I can see how this wouldn't be desirable, since there aren't usually blog descriptions that come back from search results, and it could be confusing to users why there are search results that don't seem to contain their search terms.

[jreeve]

attempt at a patch. Not tested.

[jreeve]

better file naming scheme for patch

[r-a-y]

Thanks for investigating, jreeve.

This is another case where our results query does not match our total count query. (See #5099 for another example.)

Since we're in 2.1-beta at the moment, we'll look at this in 2.2.

[jreeve]

I tested this patch with the Buddypress PHPUnit tests, and all went OK:

OK, but incomplete or skipped tests!
Tests: 795, Assertions: 1342, Skipped: 5.

But by needs-unit-tests I'm guessing that means a unit test designed for this feature? I imagine that would be something like this:

public function test_get_totals_with_search_terms() {
		if ( ! is_multisite() ) {
			return;
		}

		$old_user = get_current_user_id();

		$u = $this->factory->user->create();
		$this->set_current_user( $u );
		$b = $this->factory->blog->create( array(
			'title' => 'The Foo Bar Blog',
                        'description' => 'This blog mentions apples..', 
			'user_id' => $u,
		) );
		bp_blogs_record_existing_blogs();

		// make the blog public or it won't turn up in generic results
		update_blog_option( $b, 'blog_public', '1' );

		$blogs = BP_Blogs_Blog::get( 'active', false, false, 0, 'apples' );
		$blog_ids = wp_list_pluck( $blogs['blogs'], 'blog_id' );

		$this->assertEquals( count( $blogs['blogs'] ), $blogs['total'] );

		$this->set_current_user( $old_user );
	}

But I have very little experience writing tests, and so something's failing here that I don't understand, and I'm not quite sure how to debug it. But hopefully this is a step in the right direction!

[boonebgorges]

Thanks, jreeve! The unit test you've suggested is along the right lines.

The SQL statement being edited here is truly terrible, so I'm going to take this opportunity to rework it. It still needs lots and lots of TLC (joining against wp_users? ack) but this is better than nothing.

[boonebgorges]

In 9328:

Search consistently against blog title and description in BP_Blogs_Blog::get().

Previously, the description field was only being matched for the 'total' query,
with the result that pagination and other count-based results did not match the
blogs actually returned by the query.

This changeset reworks the syntax a bit, to avoid the duplicated logic when
using search terms, and to use proper JOIN syntax instead of commas.

Props jreeve.
Fixes #5858.