Load More Button loads duplicates

[jtymann]

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 );

[DJPaul]

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

[johnjamesjacoby]

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

[boonebgorges]

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.

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

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

[imath]

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 ?

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.

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.

2. 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.

[imath]

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.

[dcavins]

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.

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

[imath]

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.

[imath]

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

[imath]

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