Skip to:
Content

Opened 4 years ago

Last modified 4 weeks ago

#3794 new defect (bug)

Deleted activity items remain favourited

Reported by: ewebber Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 1.5.1
Component: Component - Activity Keywords:
Cc: georgemamadashvili@…

Description

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.

Attachments (5)

3794.patch (17.5 KB) - added by imath 13 months ago.
3794.unittest.patch (807 bytes) - added by imath 9 months ago.
3794.02.patch (18.1 KB) - added by imath 9 months ago.
3794.unittest.02.patch (6.9 KB) - added by imath 9 months ago.
3794.03.patch (18.3 KB) - added by imath 9 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 @boonebgorges4 years ago

  • Component changed from Core to Activity
  • Milestone changed from Awaiting Review to Future Release

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

comment:2 @johnjamesjacoby3 years ago

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.

comment:3 @mpa4hu16 months ago

BUMP? can I do this here?

Since I think this is quite important.

comment:4 follow-up: @r-a-y13 months ago

BUMP? can I do this here?

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

comment:5 in reply to: ↑ 4 @imath13 months ago

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.

comment:6 @imath13 months ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 2.1

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

@imath13 months ago

comment:7 @ircbot11 months ago

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.

comment:8 @DJPaul11 months ago

  • Keywords early added
  • Milestone changed from 2.1 to 2.2

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.

comment:9 @boonebgorges9 months ago

  • Keywords needs-unit-tests added; early removed

@imath9 months ago

@imath9 months ago

comment:10 @imath9 months ago

  • Keywords needs-unit-tests removed

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.

comment:11 follow-up: @boonebgorges9 months ago

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!

comment:12 in reply to: ↑ 11 @imath9 months ago

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

@imath9 months ago

@imath9 months ago

comment:13 follow-up: @DJPaul9 months ago

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.

comment:14 in reply to: ↑ 13 ; follow-up: @imath9 months ago

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' );
Last edited 9 months ago by imath (previous) (diff)

comment:15 @DJPaul9 months ago

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.

comment:16 in reply to: ↑ 14 ; follow-up: @DJPaul9 months ago

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

comment:17 in reply to: ↑ 16 @imath9 months ago

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

comment:18 follow-up: @DJPaul9 months ago

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

comment:19 in reply to: ↑ 18 @imath9 months ago

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)

comment:20 @johnjamesjacoby9 months ago

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.

comment:21 @boonebgorges9 months ago

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

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

comment:22 @DJPaul9 months ago

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. :)

comment:23 @ircbot9 months ago

This ticket was mentioned in IRC in #buddypress-dev by paulgibbs. View the logs.

comment:24 @DJPaul7 months ago

  • Keywords has-patch removed

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/

comment:25 @Mamaduka7 months ago

  • Cc georgemamadashvili@… added

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

comment:26 @DJPaul6 months ago

  • Milestone changed from 2.2 to Future Release

@Mamaduka: yep :)

comment:27 @slackbot4 months ago

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

comment:28 @danbp4 weeks ago

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.

Note: See TracTickets for help on using tickets.