Skip to:
Content

BuddyPress.org

Opened 3 years ago

Last modified 2 years ago

#7534 new enhancement

Remove read notifications from the DB table after a specified time period

Reported by: r-a-y Owned by:
Milestone: Under Consideration Priority: normal
Severity: normal Version:
Component: Performance Keywords: dev-feedback has-patch
Cc:

Description

I've thought about this for awhile now.

Once you've read a notification, chances are you will never go back to to the "Notifications > Read" tab to view it again.

So with that in mind, does it make sense to keep read notifications indefinitely in the database? What if you have a rather, large community that has a ton of notifications? (See https://buddypress.org/2017/05/largest-turkish-recipe-site-spiced-up-with-buddypress/)

To be conservative, we could purge read notifications from the site after a specified time period. I don't know what a good value would be (3 months, 6 months?), but thought it would be good bring this up for discussion.

Attachments (1)

7534.01.patch (6.9 KB) - added by r-a-y 3 years ago.
Changed deletion limit to 1000; add cron job on root blog only

Download all attachments as: .zip

Change History (10)

#1 @hnla
3 years ago

Makes a lot of sense, notifications are somewhat ephemeral, but the period would need to err on the longer side 6 months, then purge.

#2 @johnjamesjacoby
3 years ago

A few things:

  • Looks like Facebook goes back about 20 days
  • Twitter notifications technically are just mentions, and they go back seemingly forever
  • LinkedIn went back about 3 months

We could:

  • Have a global maximum for the entire community
  • Allow users to set their own preference (maybe some users don't want to keep them ever?)
  • Give users a "Delete read notifications instantly" type setting

We'd probably need to use a cron job to clean these up, which we've avoided needing to use so far, because of known issues with the WPCron API on multisite.

Other thoughts:

  • The query to remove notifications for all users based on their last activity is a scary one. We'd need to know the last_activity date for all users to purge the notifications table of old read ones across the board.
  • I don't think deleting all read notifications that are over 30 days old makes sense, because that clean-up routine would need to run every day across all users
  • If we are going to start running periodic clean-up routines, at the risk of making this a bigger deal, there are other things that could benefit (stale friend requests, groups, etc...)
Last edited 3 years ago by johnjamesjacoby (previous) (diff)

#3 @r-a-y
3 years ago

The query to remove notifications for all users based on their last activity is a scary one. We'd need to know the last_activity date for all users to purge the notifications table of old read ones across the board.

I was thinking of doing a date query against the notification table's date_notified column and check is_new = 0.

But:

Allow users to set their own preference (maybe some users don't want to keep them ever?)

.

Give users a "Delete read notifications instantly" type setting

A per-user setting would make this more complicated.

because that clean-up routine would need to run every day across all users

Perhaps run every week or every month?

If we are going to start running periodic clean-up routines, at the risk of making this a bigger deal, there are other things that could benefit (stale friend requests, groups, etc...)

Friend requests and groups are more pertinent data. If a group is empty with no activity, then sure.

Last edited 3 years ago by r-a-y (previous) (diff)

#4 @DJPaul
3 years ago

  • Milestone changed from Under Consideration to 3.0

We gotta do something here.

#5 @r-a-y
3 years ago

  • Keywords has-patch added

01.patch is an attempt to remove read notifications older than 30 days.

Patch does the following:

  • Fixes notification date queries(!). We were referencing the wrong date column in the BP_Notifications_Notification::get_date_query_sql() method.
  • Introduces a cronjob to delete stale notifications every week - bp_notifications_cron(). Can probably add a filter to change the interval if desired.
  • Introduces a utility function to delete stale notifications - bp_notifications_delete_stale(). This function deletes stale notifications older than 30 days, 1000 items at a time by default. If there are more stale notifications to remove, the function runs recursively until all stale notifications are removed.
  • Adds a new method to delete multiple notifications by ID - BP_Notifications_Notification::delete_by_ids(). I couldn't use the existing delete() method as it can only delete one notification at a time, which can be a hassle when deleting a ton of notifications at once.

Let me know what you think.

Last edited 3 years ago by r-a-y (previous) (diff)

@r-a-y
3 years ago

Changed deletion limit to 1000; add cron job on root blog only

#6 @DJPaul
3 years ago

Fixes notification date queries(!)

Can you commit the fix for this, as its own thing?

(EDIT: this was done in r11840)

Last edited 2 years ago by DJPaul (previous) (diff)

#7 @johnjamesjacoby
3 years ago

1000 seems high for most traditional WordPress hosting company servers. It’s possible trying to delete 1000 records will lock the database table for a few seconds.

If this is a stab in the dark, we should probably be more conservative: 250, but more frequently?

I’m also reluctant to push this change to core, like at all. Facebook/Twitter/LinkedIn and others don’t purge my old notifications/mentions/etc… The point of storing them is to keep them around, and removing them with a cron means it will happen somewhat randomly to end users without their acknowledgement.

#8 @r-a-y
3 years ago

We can drop the deletion limit to 250 or lower and can increase the date limit to something older like three months if that helps.

WordPress purges trashed posts with cron (see wp_scheduled_delete(), wp_delete_auto_drafts()). I understand that read notifications are different and are not exactly "trashed", but once you've marked a notification as read, the likelihood of ever going back to an older, read notification is extremely rare.

If we want to give some acknowledgment, we can also add a message on the "Notifications > Read" screen that read notifications older than X days will be removed.

If we don't want to run this with WP cron, then maybe this can be added as a tool under "Tools > BuddyPress" in the admin area or as a wp-cli command.

#9 @DJPaul
2 years ago

  • Milestone changed from 3.0 to Under Consideration
Note: See TracTickets for help on using tickets.