Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#5858 closed defect (bug) (fixed)

blog directory search returns incorrect result count

Reported by: jreeve Owned by: boonebgorges
Milestone: 2.2 Priority: normal
Severity: normal Version: 1.2
Component: Blogs Keywords:
Cc: jon.reeve@…

Description

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.

Attachments (2)

patch.txt (3.2 KB) - added by jreeve 6 years ago.
attempt at a patch. Not tested.
5858.patch (3.2 KB) - added by jreeve 6 years ago.
better file naming scheme for patch

Download all attachments as: .zip

Change History (8)

@jreeve
6 years ago

attempt at a patch. Not tested.

#1 @jreeve
6 years ago

  • Cc jon.reeve@… added

@jreeve
6 years ago

better file naming scheme for patch

#2 @r-a-y
6 years ago

  • Component changed from Core to Blogs
  • Milestone changed from Awaiting Review to 2.2
  • Version changed from 2.0 to 1.2

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.

#3 @DJPaul
6 years ago

  • Keywords needs-refresh needs-unit-tests added
  • Milestone changed from 2.2 to Future Release

#4 @jreeve
6 years ago

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!

#5 @boonebgorges
6 years ago

  • Keywords needs-refresh needs-unit-tests removed
  • Milestone changed from Future Release to 2.2

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.

#6 @boonebgorges
6 years ago

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

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.

Note: See TracTickets for help on using tickets.