Opened 9 years ago
Closed 9 years ago
#6681 closed enhancement (fixed)
Delete all of a user's notifications after they're deleted
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 2.5 | Priority: | normal |
Severity: | normal | Version: | 2.3.3 |
Component: | Toolbar & Notifications | Keywords: | has-patch has-unit-tests |
Cc: |
Description
Some notifications seem to remain in the database after a user is deleted. I don't see a reason to keep them? Even custom notifications added by plugins could get the chop:
function example_func( $user_id ) { $where = array( 'user_id' => $user_id, 'item_id' => false, 'secondary_item_id' => false, 'component_action' => false, 'component_name' => false ); BP_Notifications_Notification::delete( $where ); } add_action( 'delete_user', 'example_func' ); // More hooks needed here to cover the various ways users can be deleted.
Attachments (4)
Change History (13)
#1
@
9 years ago
- Keywords needs-patch good-first-bug added
- Milestone changed from Awaiting Review to Future Release
#2
@
9 years ago
Probably wouldn't want to do this on spamming a user just in case that action needs to be undone.
#3
@
9 years ago
- Keywords has-patch added; needs-patch removed
I've added a new function bp_core_delete_notifications_on_user_delete()
to bp-members-functions.php. Not sure if that's the right place?
#4
follow-up:
↓ 5
@
9 years ago
- Keywords needs-unit-tests added; good-first-bug removed
- Milestone changed from Future Release to 2.5
Thanks, henry.wright! The function looks good at a glance. It ought to live in bp-notifications, because currently, if notifications is disabled, the function will produce fatal errors. Unit tests would also be rad.
#5
in reply to:
↑ 4
@
9 years ago
Replying to boonebgorges:
...It ought to live in bp-notifications, because currently, if notifications is disabled, the function will produce fatal errors...
Right, see what you mean, but I think we might have a problem?
If the notifications component is disabled, and a user is then deleted, we'd still want the relevant items in the notifications table removed (even though notifications are now disabled, they may have been enabled at an earlier date).
#6
@
9 years ago
Do functions in bp-notifications fire if the notifications component is disabled?
No. But I think we shouldn't worry about that. Components can't be expected to clean up after themselves if they've been turned off. Moving all of this logic inside of bp-core is going to create all sorts of bp_is_active()
headaches that I'd like to avoid. In any case, it's not going to break anyone's site if this data is not cleared up. So let's keep it in bp-notifications.
#7
@
9 years ago
Components can't be expected to clean up after themselves if they've been turned off.
That sounds reasonable to me!
2nd patch to follow...
#8
@
9 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
Update original patch with correct PHPDoc version also added unit tests.
Sorry for direct queries BP_Notifications_Notification::get()
checks agains registered components, while BP_Notifications_Notification::delete()
doesn't care about that.
Yes. The rest of BP, we normally hook to
wpmu_delete_user
anddelete_user
for this, and sometimes evenbp_make_spam_user
if it makes sense to tidy-up after a user has been spammed.