Skip to:
Content

Opened 6 years ago

Closed 5 years ago

Last modified 3 years ago

#4393 closed defect (bug) (worksforme)

Deleting favorited activity does not update user favorite counts

Reported by: kumo99 Owned by: rachelbaker
Milestone: 1.7 Priority: normal
Severity: normal Version: 1.6
Component: Activity Keywords: has-patch dev-feedback
Cc: rachel@…

Description

Hello,

When you add a new activity and make it a favorite, the "Favorites" tab show "1" next to it. Now if you visit the "Favorites" tab you have two options either to Unfavorite the activity or Delete it. If you select Unfavorite the count number next to the tab becomes "0", but if you Delete the activity the number would not change. In this case you have no favorites but still it says you have one.

I think the Delete button should not be in the "Favorites" tab, a user needs only to unfavorite the activity not delete it.

Attachments (4)

scrn-501b4feb.png (13.4 KB) - added by kumo99 6 years ago.
it says I have one favorite but the notice says not
bp-activity-functions.php (54.5 KB) - added by rachelbaker 6 years ago.
Initial code to create array in activity meta for activity favorites
bp-activity-functions.php.patch (765 bytes) - added by rachelbaker 6 years ago.
Correcting previous upload, patch file only
4393.01.patch (4.0 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (23)

@kumo99
6 years ago

it says I have one favorite but the notice says not

#1 @rachelbaker
6 years ago

  • Cc rachel@… added

Confirmed, in 1.6 trunk.

Last edited 6 years ago by rachelbaker (previous) (diff)

#2 @r-a-y
6 years ago

  • Keywords dev-feedback added

The way the favorites system was originally designed makes it hard to search for whomever favorited the activity in order to remove it from a user's favorites.

One proposed solution involves checking a user's favorited activity items on user login to see if these activity items still exist and update the user's 'bp_favorite_activities' meta entry to reflect this.

If the core devs like this idea, I can work up a patch.

#3 @boonebgorges
6 years ago

rachelbaker and I spent some time today working on a patch that takes a slightly different tack. In addition to storing an array of favorited activities with each user, the proposal is to store an array of user ids who have favorited a given activity item (in the activitymeta table). That way, when an item is deleted, we can loop through and remove the favorite for each of these members.

We thought we'd go this direction, instead of doing something like what you've suggested, because it checking those activity items on login would be both too much (there's a lot of unnecessary overhead in many cases) and not enough (since a user may remain logged in for a long time, and it still leaves the count incorrect for long periods). That said, we could combine the two a bit: each time a favorite is added or deleted, we could pull up a list of the user's favorites, and make sure that they all exist. This would help with backward compatibility, as well as making sure that the mirrored data stays in sync. What do you think, r-a-y?

#4 @r-a-y
6 years ago

  • Component changed from Core to Activity

I like the idea of using an additional activity meta entry. So a definite "yes" to that.

In my proposal, I overvalued how much the favorite system is used, which is why my rationale was from the perspective of efficiency vs. real-time calculation.

I would be okay with moving the favorite activity exists check if we can minimize the overhead on the lookup.

Maybe with a direct DB query like:

SELECT id FROM {$bp->activity->table_name} WHERE id IN ( $comma_delimited_favorite_ids );"

instead of using bp_activity_get() due to its joining on the usermeta and xprofile tables?

#5 follow-up: @boonebgorges
6 years ago

Maybe with a direct DB query

Yes, this sounds reasonable. (We can abstract it into a static method of the db class, but same idea.)

Let's wait a couple days to see if rachelbaker posts a patch (nudge nudge). Doesn't have to be perfect, but it would give us a nice starting point. Thanks, all!

#6 @boonebgorges
6 years ago

  • Keywords needs-patch added; dev-feedback removed
  • Milestone changed from Awaiting Review to 1.7
  • Summary changed from Delete button in Favorites page should not be their to Deleting favorited activity does not update user favorite counts

I'm assuming we can come up with a fix for 1.7, so I'm tentatively putting it in this milestone.

#7 in reply to: ↑ 5 @rachelbaker
6 years ago

boonebgorges,

My fault for not saying this before leaving the Dev Day on Sunday, but I am on vacation in Napa until Thursday. I will post the code/approach I have on Thursday when I am back. Hopefully, that will still give us plenty of time to review and decide if a different approach should be taken.

Replying to boonebgorges:

Maybe with a direct DB query

Yes, this sounds reasonable. (We can abstract it into a static method of the db class, but same idea.)

Let's wait a couple days to see if rachelbaker posts a patch (nudge nudge). Doesn't have to be perfect, but it would give us a nice starting point. Thanks, all!

#8 @boonebgorges
6 years ago

rachelbaker - No rush, and no pressure! I just didn't want to reduplicate efforts.

@rachelbaker
6 years ago

Initial code to create array in activity meta for activity favorites

@rachelbaker
6 years ago

Correcting previous upload, patch file only

#9 @rachelbaker
6 years ago

Added patch file with just the beginnings of the user activity favoriting reworking that I had from Sunday. This just adds the current user id to an array in activity meta to keep track of all the users that favorited a specific activity id.

Still to do:

  • decide if this is still the route we want to take
  • add method (or something) that will update the different activity meta arrays of users that have favorited an activity with the current data in the user meta table
  • Correct the favorite counts

Hope that all makes sense.

#10 @boonebgorges
6 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Thanks, rachelbaker!

I fleshed out the patch a bit, to handle the deleting, as well as user favorite removal.

I didn't add in the activity_exists check. We could probably abstract it into a separate function, and call it both on favorite creation and favorite removal. It adds a bit of overhead, but will be good for data integrity. bp_activity_get() takes an 'in' parameter, so we should be able to do it with just a single query - and if we want an even leaner DB method (just to grab the ids, instead of a join or whatever crummy thing the activity query does :) ) we can just add it, as r-a-y suggests.

I also rearranged some of the strange error logic of the remove_favorite function, which didn't seem to do anything useful.

#11 @rachelbaker
6 years ago

Thanks, boonebgorges.

I agree that the activity_exists check should be moved into a separate method. I do agree with r-a-y that running the integrity check on every favorite action is a bit much. What about triggering it on user login as r-a-y suggests?

#12 @rachelbaker
6 years ago

  • Owner set to rachelbaker
  • Status changed from new to accepted

Just an update here:
Going to take a stab at the activity_exists check sometime in the next few days.

#13 @johnjamesjacoby
6 years ago

  • Milestone changed from 1.7 to 1.8

Any updates here? Moving to 1.8 until we get a refresh.

#14 @rachelbaker
5 years ago

  • Resolution set to worksforme
  • Status changed from accepted to closed

JJJ,

I am no longer seeing this issue occur in BuddyPress 1.7 trunk

#15 follow-up: @johnjamesjacoby
5 years ago

  • Milestone changed from 1.8 to 1.7

Excellent. Putting into 1.7 milestone.

#16 in reply to: ↑ 15 @danbrellis
4 years ago

Replying to johnjamesjacoby:

Excellent. Putting into 1.7 milestone.

Sorry for my ignorance, but I've noticed that when I delete an activity item that was favorited, the user's favorite count still remains the same. I'm using BP version 2.1.1 and can't find any of the code from the above patch in my bp-activity/bp-activity-functions.php (ie reference to activity_favorited_users meta for activity items, etc). If I am missing something (this was removed for some reason or addressed in a plugin) please let me know.

Thanks!

#17 follow-up: @boonebgorges
4 years ago

danbrellis - This ticket probably shouldn't have been marked worksforme, but instead as a duplicate of #3794. Work on that ticket is ongoing, and the core team does have plans to address it in the next version or two.

#18 in reply to: ↑ 17 @danbrellis
4 years ago

Replying to boonebgorges:

danbrellis - This ticket probably shouldn't have been marked worksforme, but instead as a duplicate of #3794. Work on that ticket is ongoing, and the core team does have plans to address it in the next version or two.

thanks for the super quick response boonebgorges (one of the reasons buddypress is so awesome). I'll look forward to a fix. In the meantime, I might just hack the core and keep an eye out for the patch! thx again

#19 @johnjamesjacoby
3 years ago

  • Version changed from 1.6-rc to 1.6
Note: See TracTickets for help on using tickets.