Skip to:
Content

BuddyPress.org

Opened 11 years ago

Closed 16 months ago

Last modified 16 months ago

#4535 closed defect (bug) (fixed)

Load More Button loads duplicates

Reported by: jtymann's profile jtymann Owned by: imath's profile imath
Milestone: 11.0.0 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch commit
Cc:

Description

In the default buddypress theme/Activity Core there is a bug with the activity stream. If you load the activity stream. Then open up a new window, go to the site, and perform a few actions that create activities. If you go back to the original window and click 'load more' you will receive duplicate posts. This is highly noticeable on a site with multiple concurrent users creating activity.

Below is how I fixed the issue in a child theme of mine:

The concept of my changes is pretty simple, in addition to passing which page the user is on the id of the first post on the users page is passed as well. Then the sql is altered to take this into account.

javascript changes:
I added the line "'pag_first_post' : $(".activity-item:first").attr("id").split("-")[1]" (and a comma) to the following code. This passes the post id along with the load more request

/* Load more updates at the end of the page */
		if ( target.parent().hasClass('load-more') ) {
			jq("#content li.load-more").addClass('loading');

			if ( null == jq.cookie('bp-activity-oldestpage') )
				jq.cookie('bp-activity-oldestpage', 1, {
					path: '/'
				} );

			var oldest_page = ( jq.cookie('bp-activity-oldestpage') * 1 ) + 1;

			jq.post( ajaxurl, {
				action: 'activity_get_older_updates',
				'cookie': encodeURIComponent(document.cookie),
				'page': oldest_page,
				'pag_first_post' : $(".activity-item:first").attr("id").split("-")[1]
			},
			function(response)
			{
				jq("#content li.load-more").removeClass('loading');
				jq.cookie( 'bp-activity-oldestpage', oldest_page, {
					path: '/'
				} );
				jq("#content ul.activity-list").append(response.contents);

				target.parent().hide();
			}, 'json' );

			return false;
		}

ajax.php changes:
I added support to the function bp_dtheme_ajax_querystring to now handle this new parameter.

I added the lines:

	if ( ! empty( $_POST['pag_first_post'] ) && '-1' != $_POST['pag_first_post'] )
		$qs[] = 'pag_first_post=' . $_POST['pag_first_post'];

After the code

	if ( ! empty( $_POST['page'] ) && '-1' != $_POST['page'] )
		$qs[] = 'page=' . $_POST['page'];

Finally I added the following code to my functions.php file:

//Fix to correct load more duplicates
function ofa_activity_get_load_more_fix($prepared_sql, $select_sql, $from_sql, $where_sql, $sort, $pag_sql=false) {
	global $bp, $wpdb;
	$args = wp_parse_args(bp_ajax_querystring( 'activity' ));
	if($args['pag_first_post'] && $post_seed = (int)$args['pag_first_post']){
		if($pag_sql){
			$where_sql .= " AND a.id <= $post_seed";
			return $wpdb->prepare( "{$select_sql} {$from_sql} {$where_sql} ORDER BY a.date_recorded {$sort} {$pag_sql}");
		}
	}
	return $prepared_sql;
}
add_filter( 'bp_activity_get_user_join_filter', 'ofa_activity_get_load_more_fix', 10, 6 );

function ofa_activity_get_total_load_more_fix($prepared_sql, $select_sql, $from_sql, $where_sql, $sort) {
	global $bp, $wpdb;
	$args = wp_parse_args(bp_ajax_querystring( 'activity' ));
	if($args['pag_first_post'] && $post_seed = (int)$args['pag_first_post']){
		if($pag_sql){
			$where_sql .= " AND a.id <= $post_seed";
			return $wpdb->prepare( "SELECT count(a.id) FROM {$bp->activity->table_name} a {$index_hint_sql} {$where_sql} ORDER BY a.date_recorded {$sort}" );
		}
	}
	return $prepared_sql;
}
add_filter( 'ofa_activity_get_total_load_more_fix', '', 10, 5 );

Attachments (1)

4535.patch (6.1 KB) - added by imath 21 months ago.

Download all attachments as: .zip

Change History (18)

#1 @DJPaul
11 years ago

  • Milestone changed from Awaiting Review to 1.7

Need to check this, but I think I've seen it before.

#2 @johnjamesjacoby
11 years ago

  • Keywords needs-patch added
  • Milestone changed from 1.7 to 1.8

Happens all the time. Not a regression, and been around for years. Without a real patch, punting to 1.8.

#3 @boonebgorges
11 years ago

  • Milestone changed from 1.8 to Future Release

This issue has been partially addressed in r7116 / #4897. When creating new activity items via AJAX, duplicates are not returned. However, the OP's original scenario - leaving the window open and coming back to it later while items have been created elsewhere - will still cause duplicates.

Something like what's being suggested here would be a more general fix, but the implementation used here is designed for a plugin/theme. Filtering the activity query is fine for a plugin, but we would need to do something more systematic for BuddyPress. Something like an offset parameter for bp_has_activities() would do the trick.

This ticket was mentioned in PR #19 on buddypress/buddypress by marklawntalk.


21 months ago
#4

  • Keywords has-patch added; needs-patch removed

The current LIMIT offset calculator is wrong resulting in duplicate acitivty cards.
Possible related to https://buddypress.trac.wordpress.org/ticket/4535

Fix the correct formula to calculate the 'offset' value.

Trac ticket: https://buddypress.trac.wordpress.org/ticket/4535

imath commented on PR #19:


21 months ago
#5

Hi @marklawntalk are you sure it's about activity cards as your patch is editing the notifications' table offset limit query ?

@imath
21 months ago

#6 @imath
21 months ago

  • Milestone changed from Awaiting Contributions to 11.0.0

The patch on the PR doesn't seem to relate to activity. But it made me have a look at this ticket and build a working patch for BP Nouveau, which probably needs to be complemented to support Legacy. Who wants to give some of his/her times to have it fixed for 11.0.0 ?

marklawntalk commented on PR #19:


21 months ago
#7

Hi @imath , my bad, yes it's the notification module. The site I worked on uses the notifications module for both activity and notification modules hence the confusion. But I understand their difference.

imath commented on PR #19:


21 months ago
#8

So here's what in place.

{{{php
$page = absint( $argspage? );
$per_page = absint( $argsper_page? );
$offset = $per_page * ( $page - 1 );
$retval = $wpdb->prepare( "LIMIT %d, %d", $offset, $per_page );
}}}

Let's take some exemples.

  1. To get the first ten results, we need the 1st page of results which should be converted to LIMIT 0, 10 for MySql (as the offset of the initial row is 0 (not 1), see https://dev.mysql.com/doc/refman/8.0/en/select.html) :
  • $args['per_page'] is 10,
  • $args['page'] is 1.

{{{php
$page = 1;
$per_page = 10;
$offset = 10 * ( 1 - 1 ); = 0
$retval = $wpdb->prepare( "LIMIT %d, %d", 0, 10 );
LIMIT 0, 10
}}}
✅ Which is right imho.

  1. If we need to get the second page of results, it should be converted to : LIMIT 10, 20 for MySql
  • $args['per_page'] is 10,
  • $args['page'] is 2.

{{{php
$page = 2;
$per_page = 10;
$offset = 10 * ( 2 - 1 ); = 10
$retval = $wpdb->prepare( "LIMIT %d, %d", 10, 20 );
LIMIT 10, 20
}}}
✅ Which is right imho.

With your suggested patch, we'd have a wrong offset value, because we would ignore the first db entry each time :
{{{php
$page = 1;
$per_page = 10;
$offset = 10 * ( 1 - 1 ) + 1; = 1
$retval = $wpdb->prepare( "LIMIT %d, %d", 1, 10 );
LIMIT 1, 10
}}}
❌ Which is wrong imho.

So everything is fine with how the offset is generated. Thanks for your report, but I'm going to close it and leave the code the way it is.

#9 @imath
20 months ago

FYI: the PR was closed and was not relative to this ticket. But as I've said in #comment:6 it made me work on a patch to fix this issue: 4535.patch

Let's improve this patch to also take care of Legacy.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


20 months ago

#11 @dcavins
19 months ago

This is an interesting problem, and regarding pagination, your patch looks good to me. It's a clever idea to set the max ID when fetching the next page to avoid surprise shifts/omissions.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


19 months ago

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


19 months ago

This ticket was mentioned in PR #29 on buddypress/buddypress by @imath.


16 months ago
#14

Introduces a new offset_lower parameter to only load activities having an ID lower than the provided offset_lower value. Using this parameter when clicking on the Load More link avoids loading possible duplicates if some activity were published by another user although the one clicking on the Load More link haven't refreshed the page yet.

Trac ticket: https://buddypress.trac.wordpress.org/ticket/4535

#15 @imath
16 months ago

  • Keywords commit added

I've just added the BP Legacy code needed so that this template pack also enjoys this fix. If no objections are raised until tomorrow, I'll commit the PR.

#16 @imath
16 months ago

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

In 13344:

Improve the activity loop to be able to get items below a max. ID

The $filter parameter of this loop now includes a new $offset_lower
argument making it possible to only retrieve activities having an ID
lower than this argument provided value.

We are using this filter to avoid a possible activity duplicate when
clicking on the "Load More" link.

Closes https://github.com/buddypress/buddypress/pull/29
Fixes #4535

#17 @imath
16 months ago

In 13345:

Add an inline comment to include contributor credits to latest commit

The following line was missed in [13344] :

Props jtymann, johnjamesjacoby, boonebgorges, dcavins

See #4535

Note: See TracTickets for help on using tickets.