Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5779 closed defect (bug) (fixed)

Ajax & searching in Activities, focusing on Activity Heartbeat feature

Reported by: imath Owned by: imath
Milestone: 2.1 Priority: normal
Severity: normal Version: 2.0
Component: Activity Keywords: good-first-bug has-patch commit
Cc:

Description

In a comment to #5505, SGr33n reported a problem with the activity Heartbeat feature (load newest) & searching activities. I confirm this problem : if you add ?s=random it will load all newest activities without taking in account the search.

First, SGr33n, can you confirm that when you say: 'search page', we are talking about site.url/activity/?s=random. Or is it a plugin that is creating a specific search feature ?

Second, i'm hesitating about search. As the "Load More" feature doesn't support search neither, my first idea was to neutralize "load newest" if we have a ?s=random query string: see the ticketnumber.heartbeat-nosearch.patch. Using this patch, we get rid of the bug.

Then i thought it's a bit too bad, and i've built ticketnumber.heartbeat-search.patch which will load newest activities if they match the search_terms.

Finally, i thought it's a bit more too bad to support search in activity heartbeat and not in "load more", so i've build a 3rd patch : ticketnumber.loadmore-search.patch.

So i think we have 2 options :

  • neutralize activity heartbeat if a search is performed > ticketnumber.heartbeat-nosearch.patch, or
  • support search in activity heartbeat and load more features > ticketnumber.heartbeat-search.patch + ticketnumber.loadmore-search.patch

What do you think ?

Attachments (4)

5779.heartbeat-nosearch.patch (535 bytes) - added by imath 5 years ago.
5779.heartbeat-search.patch (1.2 KB) - added by imath 5 years ago.
5779.loadmore-search.patch (880 bytes) - added by imath 5 years ago.
5779.02.patch (2.1 KB) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 in reply to: ↑ description @SGr33n
5 years ago

First, SGr33n, can you confirm that when you say: 'search page', we are talking about site.url/activity/?s=random. Or is it a plugin that is creating a specific search feature ?

Confirmed

Second, i'm hesitating about search. As the "Load More" feature doesn't support search neither, my first idea was to neutralize "load newest" if we have a ?s=random query string: see the ticketnumber.heartbeat-nosearch.patch. Using this patch, we get rid of the bug.

Seems a good option, also in a semantic view, since that if you are searching somenthing, means that this something was already published... maybe this ticket will be updated when BuddyPress will support hashtags.

What do you think ?

In my opinion, you can disable it on the search page :)

Last edited 5 years ago by SGr33n (previous) (diff)

#2 @boonebgorges
5 years ago

Second option seems better to me (support search for heartbeat and for Load More). Implementation seems straightforward, and it seems like it fixes an existing bug (that search terms are ignored on Load More)

#3 @DJPaul
5 years ago

  • Owner set to DJPaul
  • Status changed from new to assigned

I agree with Boone

#4 @DJPaul
5 years ago

  • Owner DJPaul deleted

#5 @DJPaul
5 years ago

  • Keywords needs-patch added; has-patch reporter-feedback 2nd-opinion removed

In 5779.heartbeat-search.patch, I don't think esc_html is the right function to use to sanitise the user input.

#6 @DJPaul
5 years ago

  • Keywords good-first-bug added

Ticket update:

  • 5779.heartbeat-search.patch​ needs updating (replace esc_html with an appropriate sanitisation function).
  • 5779.loadmore-search.patch​ is ready for commit.

@imath
5 years ago

#7 @imath
5 years ago

  • Keywords has-patch added; needs-patch removed

I suggest 5779.02.patch which is merging 5779.heartbeat-search.patch​ and 5779.loadmore-search.patch​.
It uses addslashes instead of esc_html. Tested searching for "l'apostrophe" for instance and it seems to be ok on the two features.

At the end in BP_Activity_Activity::get(), search_terms are escaped using bp_esc_like()

#8 @DJPaul
5 years ago

  • Keywords commit added

Not tested, but I think this looks OK. Get it in soon, please. :)

#9 @imath
5 years ago

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

In 8817:

Add search support in Ajax "Load More" and "Load Newest" activity actions

If a search is performed in activities using the query string "?s=search_terms", make sure the search terms are sent in the ajax arguments of the "Load More" and the "Load Newest" actions.

Fixes #5779

Note: See TracTickets for help on using tickets.