Deleted activity items remain favourited

[ewebber]

When an activity item that has been favourited is deleted, the favourite count is not removed from a users favourite count - as that item no longer exists it cannot be unfavourited.

[boonebgorges]

Yeah, this sounds about right.

Unfortunately it'll be next to impossible to solve. We currently store a user's activity favorites in usermeta as an array. There's no way to query (eg) how many times an activity item has been favorited, or by whom. So there's no way to loop through those users and delete the item from the activity favorites list.

Probably this will have to wait until such time as the favorites system is rewritten into something a bit more elegant. (Either that, or we'll have to start keeping track in activity_meta of the users who have favorited an item.)

[johnjamesjacoby]

Activity meta will get pretty crowded this way, and there isn't an index on the meta value so querying per user unfortunately wouldn't scale well.

Ideally favorites would be rewritten as its own component, more robust like what Paul is working on in BP Labs.

[mpa4hu]

BUMP? can I do this here?

Since I think this is quite important.

[r-a-y]

BUMP? can I do this here?

We can revisit this as soon as #5644 is addressed.

[imath]

Replying to r-a-y:

We can revisit this as soon as #5644 is addressed.

You're right, i think if it makes it, it will solve this issue.

[imath]

3794.patch is "extracting" the part of the code of 5644.activity_table.03.patch that is moving favorites into the activity table and fixing the favorite count issue. It does not change anything to the favorite functionality.

In 3794.patch i've also added some caching that would need to be tested.

Just in case 5644 needs more time... We would be able to fix this in 2.1

[DJPaul]

I think this needs more review time and unit tests; it'd be a rush to fit it into 2.1. Let's aim for 2.2.

[imath]

3794.unittest.patch contains the unit test to check if the favorite count is updated once a favorited activity has been deleted.

3794.02.patch is a refresh to latest trunk.

[boonebgorges]

3794.02.patch is great, and does cover the original issue described by the OP, but we really need broad unit test coverage of activity favoriting before doing this kind of refactoring. Please review existing favoriting-related functions (like bp_activity_add_user_favorite()) to ensure that there is fairly complete coverage - these are the most important parts to have unit tests for, as they're the ones that will cause backward compatibility. Would also be nice to see tests for the new functions you're introducing: bp_activity_add_favorite() etc and ideally the migration function too.

Thanks for your work on this!

[imath]

thanks for your feedback :)

I've added 9 tests to the one that is checking if the favorite count gets updated once a favorited activity is deleted.
Tests includes new bp_activity_add_favorite() and the migrate function (see 3794.unittest.02.patch)

It made me change a bit the patch (see 3794.03.patch):

• The activity fav_count update should happen after the 'activity_favorite' activity is created in bp_activity_add_favorite(), as this function is checking if the parent activity exists.
• in bp_activity_add_favorite(), if the parent activity is hide_sitewide then the 'activity_favorite' activity will also be hide_sitewide. This is an extra security, as anyway the visibility of the parent activity is used when listing favorites

[DJPaul]

I would like some details on some of the proposed architectural changes.

In bp_activity_get_user_favorites, I see you've decided to use wp_cache to cache the total count of a user's favourites. Why do we want to change that away from storing the total count in user meta? User meta seems the appropriate place for this (just like how we already store an activity's total favourite count in its favorite_count activity meta).

If I wanted to find out the user IDs (for example) of everyone who has favourited a specific activity item, how would I do that? To clarify, I am not necessarily saying we should build new API functions to do this, I am just wondering how it would be possible to do this.

[imath]

Replying to DJPaul:

Thanks Paul for your feedback.

Why do we want to change that away from storing the total count in user meta? User meta seems the appropriate place for this (just like how we already store an activity's total favourite count in its favorite_count activity meta).

Well so far user metas are used to store the activity item ids that has been favorited (not the count). So when an activity item is deleted, its ID is still in the usermeta and the count( usermeta ) is wrong.

I have no objection to store a count in user meta, it's just the count is already there by requesting activities having the type 'activity_favorite' for the user id.

If I wanted to find out the user IDs (for example) of everyone who has favourited a specific activity item, how would I do that? To clarify, I am not necessarily saying we should build new API functions to do this, I am just wondering how it would be possible to do this.

Well i think you can do something like this :

$activity_id = 34;

$user_activities = bp_activity_get( array(
	'filter' => array(
		'action'  => 'activity_favorite',
		'item_id' => $activity_id,
	),
	'show_hidden'  => true,
	'per_page'     => false,
) );

$user_ids = wp_list_pluck( $user_activities['activities'], 'user_id' );

[DJPaul]

Also, going back to this comment on another ticket ​https://buddypress.trac.wordpress.org/ticket/5644#comment:2 what made you decide to use the activity table for this rather than, for example, creating a new table? I'm not sure if I've missed a discussion about this somewhere.

I am not necessarily saying we should definitely create a new table, I just wanted to talk about it.

[DJPaul]

Replying to imath:

Replying to DJPaul:

Why do we want to change that away from storing the total count in user meta? User meta seems the appropriate place for this (just like how we already store an activity's total favourite count in its favorite_count activity meta).

I have no objection to store a count in user meta, it's just the count is already there by requesting activities having the type 'activity_favorite' for the user id.

For simple use cases like displaying a number of how many times an activity has been favourited, a meta item will be much quicker (I'm 99% sure this is why bbPress has functions like bbp_get_topic_reply_count which use post meta).

[imath]

Replying to DJPaul:

Replying to imath:

Replying to DJPaul:

Why do we want to change that away from storing the total count in user meta? User meta seems the appropriate place for this (just like how we already store an activity's total favourite count in its favorite_count activity meta).

For simple use cases like displaying a number of how many times an activity has been favourited, a meta item will be much quicker (I'm 99% sure this is why bbPress has functions like bbp_get_topic_reply_count which use post meta).

Actually the activity meta is still there, i haven't changed this behavior. So you can use it to know how many times an activity was favorited.

I have no objection to store a count in user meta, it's just the count is already there by requesting activities having the type 'activity_favorite' for the user id.

I've changed my mind, i have an objection :) I think using a user meta to store the count will complexity the thing. If i have a user meta set to 3, and any random activity is deleted, then we would need to do another query to look for each user who favorited the activity and update the count for each user, else this ticket is not solved.

About using another table, i've changed my mind after other comments and the fact that at that time, it could be interested to have activities showing like 'imath favorited this activity'. If you apply the patch and go into the Activity administration screen, you'll see that the admin is able to see who favorited what by selecting the 'activity_favorite' type in the drop down.

Just to come back on :

If I wanted to find out the user IDs (for example) of everyone who has favourited a specific activity item, how would I do that? To clarify, I am not necessarily saying we should build new API functions to do this, I am just wondering how it would be possible to do this.

Today it's very complex to do this. Because you need to get all usermeta, check if the activity ID is in the array of activities of each user meta.. With that patch, i think it's easier ;)

[DJPaul]

If i have a user meta set to 3, and any random activity is deleted, then we would need to do another query to look for each user who favorited the activity and update the count for each user

I don't think this is a blocker problem. We couldn't do this before because each user's list of favourites topics was stored in the bp_favorite_activities usermeta (a serialized array); the proposed solution lets us do this now.

---

I am going to thinking more about the merits of using a separate DB table (apart from possible performance improvements that you and Boone discovered). It's not the first time the idea of a many-to-many relationships table has come up; see Ray's first suggestion in #5665. (pretty sure there might be another ticket where a new table is a proposed solution for some other problem, but trac ticket search is crap).

[imath]

Replying to DJPaul:

If i have a user meta set to 3, and any random activity is deleted, then we would need to do another query to look for each user who favorited the activity and update the count for each user

I don't think this is a blocker problem. We couldn't do this before because each user's list of favourites topics was stored in the bp_favorite_activities usermeta (a serialized array); the proposed solution lets us do this now.

I trust you :)
To be completely sure, maybe we could compare what's best in terms of query performance between calculating the number of favorites querying the user's 'activity_favorite' type VS using a usermeta for the count and making sure once an activity is deleted, get all user_ids for the 'activity_favorite' type having the activity item_id that was deleted an update each found user's meta. (< i'm not sure i'm clear, sorry for my english)

[johnjamesjacoby]

As long as we all agree that ditching the serialized arrays is a primary goal, and to make querying for both users-to-likes and likes-to-users easy and performant, I'm open to whatever approaches we feel are the most flexible and viable.

I could easily see having a many-to-many pointer table with a type entry. There's value in considering what WordPress core's future taxonomy table improvements might look like, to see what we can learn and/or experiment and pave the way for.

[boonebgorges]

To be completely sure, maybe we could compare what's best in terms of query performance between calculating the number of favorites querying the user's 'activity_favorite' type VS using a usermeta for the count and making sure once an activity is deleted,

Yes, I think we need some actual benchmarks here. Take the two cases:

1. We store the count in usermeta. The activity item is deleted. We now have to do this:

foreach ( $users_who_favorited as $user_id ) {
    reset_favorite_count_for_user( $user_id );
}

If the item has been favorited by 100 or 1000 people, this is an extremely expensive operation. (Though, granted, it only happens rarely.)

2. We get favorite counts dynamically from the activity table (or wherever it's stored) when the page is rendered. We no longer have to do the occasional very-resource-intensive task described in 1, but now we have added overhead to every page: instead of automatically fetching all relevant usermeta when the page is loaded and having the cached count as part of that (one query), we have at least one query for each user (twenty queries).

I would like to see real numbers for these cases on sites with user counts in the tens of thousands and activity tables in the hundreds of thousands or millions.

Side note - I think we can do a bit better with 2: a function that prefetches counts for all users in the stream with a single DB query. Something like: "SELECT item_id, user_id FROM wp_bp_activity WHERE type = 'activity_favorite' AND user_id IN (1,6,10,30)", then parse it out and add it to the cache. I'm fairly certain this will be faster than what imath's currently doing, but again, it would be helpful to benchmark a bit.

As for the question of storage, we seem to be talking about three options:

1. Standalone table (wp_bp_favorites or something like that)
2. Activity table (as in imath's patch)
3. Generic relationship table

Like r-a-y and jjj, I like option 3 from the point of flexibility and elegance. But I am also concerned about duplicating effort, both with what WP may one day create and what we already have. We already have a couple of relationship tables: wp_bp_groups_members, wp_bp_friends, wp_bp_user_blogs, and - most importantly - wp_bp_activity. Our activity table is already quite generic (it has a type column! and item_id and secondary_id!). We should only go to something even more generic if we have evidence that wp_bp_activity is, say, getting too bloated - but I don't think we have that.

[DJPaul]

I have to say that I don't care too much about duplicating DB structure with what WP core *may* do in the future. We need something "today", not in 6 months' time. :)

[DJPaul]

To update this, we want to get a many-to-many relationships table in BP, and then we'll use that to impleemnt this change. Some more notes on ​https://bpdevel.wordpress.com/2014/11/12/at-wcsf-some-attendees-of/

[Mamaduka]

DJPaul does this mean, that we need to move milestone to "Future Releases" for this ticket?

[DJPaul]

@Mamaduka: yep :)

[danbp]

Hi devs, FYI see ​https://buddypress.org/support/topic/my-favorites/#post-239915

i tested on a single install with BP 2.2.3.1 and got also this error, but only for site admin (user_id 1). Other users where not affected.
Modifying manually the meta_value of bp_favorite_activities unfrozed the situation and all went correctly after that for admin user.

[markob17]

Hi,

I'm just an active fan and regular end-user of Buddypress and I'm wondering why this hasn't been fixed yet even though version 2.4 has just been released. Any ideas when a fix will take place? I' not a coder or anything so I have 0 clue how to resolve this on my own if at all possible.

Every so often I delete very old activity posts to keep the size of the database down and any user who favorited one of these old posts will still see this reflected on the favorite count even though it has been removed. This happens for admins and non admins it seems.

Thanks!

[imath]

Hi @markob17

I understand your eagerness to see this ticket solved. We are still looking for the best way to handle this issue.

.04.patch is a new try. This time i'm using a specific relationship table. It keeps on using a user meta for the favorites count.

The relationship table avoids to exclude activity types from the stream, but if we don't want to create this table, the BP_Object_Relationships class should also work with the activity table using item_id/secondary_item_id instead of item_id/object_id

Let's talk about it during 2.5 :)

[imath]

Note to self: check bp_is_active( 'activity' ) inside the ugrade routine.

[imath]

The upgrade routine requires #6841 to be fixed first. But i'd really like to have this fixed for 2.6.

[imath]

So this is my ultimate attempt to fix this ticket.

To summarize, (since 2.1 dev-cycle) first i've suggested to use new activities. Although i really think it's the best way of dealing with it, i move to another way because of a possible problem with it about the user's favorite count.

So i've built 3794.04.patch, nobody looked at it, and because of #6841 it couldn't make it.

This bug is there since 1.2 i think, we're working on 2.7. Even if we are not finding the ideal solution we should at least make something so that users can have a consistent favorites count!

That's what is suggesting 3794.05.patch. Instead of storing favorites into a serialized array, it's saving a new meta for each favorited activity making it easier to delete the meta when the favorited activity is deleted.

[boonebgorges]

Thanks for continuing to work on this, @imath.

If I understand correctly, you're adding a new line in *usermeta* for each favorited activity. That means that if you have 10,000 users with 10 favorites each, you'll be adding 100,000 rows to usermeta. Usermeta is a table that's already subject to bloating and poor performance due to plugins that put lots of data in there. This problem is compounded at scale, when the users tables are global and shared between many networks.

At some point in the future, BP will have an item-to-item relationship API, and at that point, Favorites will be an obvious use case. So I think what we're trying to do here is come up with something that fixes the bug for now, without introducing too many scaling problems or requiring a huge amount of additional work.

The nice thing about the proposed usermeta solution is that operations like add_user_meta() are fast. But it has the potential to cause more global performance problems. Could we get away with doing something a bit less elegant, which would be a bit slower at the time of adding/deleting favorites and/or activity items, but would not cause global slowness? Something like:

// adding favorite
$favs = bp_get_user_meta( $user_id, 'bp_favorite_activities', true ); 
$favs[] = $activity_id;
bp_update_user_meta( $user_id, 'bp_favorite_activities', $favs );

$fav_users = bp_activity_get_meta( $activity_id, 'favorited_users', true );
$fav_users[] = $user_id;
bp_activity_update_meta( $activity_id, 'favorited_users', $fav_users );

// deleting activity item
$fav_users = bp_activity_get_meta( $activity_id, 'favorited_users', true );
foreach ( $fav_users as $fav_user ) {
    bp_recount_activity_favorites_for_user( $fav_user );
}

This may cause issues if a specific activity item is favorited by many users. In that case, we may have to have a system for resetting in batches. But it feels like a more modest architectural change than the huge amounts of green and red in 3794.05.patch​, especially since it's designed to be a temporary solution. (I know that much of the green/red in 3794.05.patch​ is due to moving stuff to a new file - it's pretty hard to tell what's new and what's old when it's all part of a single patch.)

@imath Is this a bad suggestion? Is there a way we can decide what strategy to take without you having to sit down and write a mountain of code? :)

[imath]

@boonebgorges sure. I understand green or red mountains can be annoying on trac, i won't build them anymore!

About the strategy, we've been discussing about it since 2.1 with no results. We're working on 2.7.
Here's some reminders:

• 21 months ago use the activity table: ​3794.03.patch
• 7 months ago: use a BP Object Relationship Table: ​3794.04.patch

There's always a good reason not to fix this issue.

• 03.patch favorite count update was problematic
• 04.patch was suggesting the 'item-to-item relationship API' you mentioned and i'm pretty sure nobody looked at it, and i assumed migrating favorites needed #6841 to be solved first.
• 05.patch wpdb->usermeta is subject to "bloating and poor performance due to plugins that put lots of data in there"

So i think your strategy is wrong because as you said 5 years ago on this ticket:

Unfortunately it'll be next to impossible to solve. We currently store a user's activity favorites in usermeta as an array. There's no way to query (eg) how many times an activity item has been favorited, or by whom. So there's no way to loop through those users and delete the item from the activity favorites list

And what you're doing is adding another serialized array into the activity meta. So it will be problematic one of these days. And somehow you're suggesting something that will have the same problem than 03.patch when updating the users favorites.

I see 2 alternatives:

• have one line per user into the activity metas which is actually moving the performance issues you mentioned about $wpdb->usermeta into the activity meta. But maybe there are less plugins using it.. Then we could use similar functions to the two green mountains i've added in 05.patch (bp_get_meta_by_key_value() and bp_delete_meta_by_key_value() btw it's amazing it's not possible to have object/meta ids given a meta_key and a meta_value in WordPress!!)
• Leave the things the way they are but move the code into bp-activity-favorites.php so at least we can filter bp_is_active( 'activity', 'favorites' ) and avoid loading some code that is failing anyway...

[mercime]

Sorry, moving this out of the 2.7 milestone.

[shonty]

Looking for a solution to this. I have hidden it for now.