#4535 closed defect (bug) (fixed)
Load More Button loads duplicates
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (18)
#2
@
12 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
@
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.
2 years 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
2 years ago
#5
Hi @marklawntalk are you sure it's about activity cards as your patch is editing the notifications' table offset limit query ?
#6
@
2 years 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:
2 years 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.
2 years 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.
- 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.
- 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
@
2 years 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.
2 years ago
#11
@
2 years 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.
2 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
2 years ago
This ticket was mentioned in PR #29 on buddypress/buddypress by @imath.
23 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
@
23 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.
Need to check this, but I think I've seen it before.