Opened 6 years ago
Last modified 5 years ago
#7534 new enhancement
Remove read notifications from the DB table after a specified time period
Reported by: |
|
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)
Change History (10)
#2
@
6 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...)
#3
@
6 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.
#5
@
6 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 existingdelete()
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.
#6
@
6 years ago
Fixes notification date queries(!)
Can you commit the fix for this, as its own thing?
(EDIT: this was done in r11840)
#7
@
6 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
@
6 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.
Makes a lot of sense, notifications are somewhat ephemeral, but the period would need to err on the longer side 6 months, then purge.