Opened 11 years ago
Closed 11 years ago
#5289 closed defect (bug) (fixed)
Single notifications not deleted from the unread or read notifications lists when the item in question is deleted in 1.9 beta 2
Reported by: | henrywright | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 2.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Toolbar & Notifications | Keywords: | |
Cc: |
Description
Single notifications are not deleted from the notifications lists (read and unread lists) when the item is deleted.
For example:
- Send a member a private message or mention them in a blog post comment
- View the notifications page
members/username/notifications
of the messaged or mentioned member - Now delete the private message or the blog post comment
- View the notifications page again and note that the notification remains in the list.
Both 1.9-beta-1 and 1.9-beta-2 seem to be affected.
Attachments (3)
Change History (25)
#1
@
11 years ago
#2
@
11 years ago
Hi imath
After a quick look at the patch, my first thought is bp_activity_at_mention_delete_notification
is hooked to just bp_activity_delete
. This takes care of @-mentions in activity updates but notifications for @-mentions in blog posts and blog post comments are not taken care of.
Would the solution be as simple as hooking the bp_activity_at_mention_delete_notification
function to delete_post
?
#3
@
11 years ago
Thanks for your feedback @henrywright
I've picked the wrong hook :(
When deleting a post, at the end bp_activity_delete is run, that's why i think it's best to hook in this function. So 5289.02.diff should take care of all activities (blog post / comment, "regular" mentions...)
Althought reading the comment r-a-y posted here on #5273, i wonder if deleting notifications about the messages is a so great idea for 1.9.. It's working but it adds a query to get the message id :(
#5
in reply to:
↑ 4
@
11 years ago
Replying to boonebgorges:
Is this a regression from 1.8.x?
Hi Boone,
I don't think it's a regression. In 1.8.1, notifications are deleted once "read", but not if :
- a group invite is removed
- a member is demoted in a group
- a message is deleted
- an activity mentioning an user is deleted.
Now in 1.9, as notifications evolved to be a great component : i think the biggest trouble would be if once a notification is read, the status wasn't changing. I've tested earlier today : even if the item is no more existing, once the notification link is clicked, the status correctly change to read.
So in the read tab, we will possibly have notifications about items that no more exists, just like users receive emails about item that may not exist anymore…
So imho, it can wait for a future release.
I've built the patch because, unlike what was occurring in 1.8.1, 1.9 is deleting notifications in case a group invite is deleted.
So i think, only the trouble with friends_friendship_withdrawn
used instead of friends_friendship_whithdrawn
should be fixed to make sure the notifications are deleted, because that's in 1.9 and else it's not working...
#6
@
11 years ago
So in the read tab, we will possibly have notifications about items that no more exists, just like users receive emails about item that may not exist anymore…
Just a thought on the above - I'm not entirely sure where exactly each of the various single notifications link to but if any of them link to the deleted item then this would this result in the 'Read' list having lots of notifications with broken links.
#7
@
11 years ago
@henrywright
I think risk is very limited, almost entirely sure it will not be the case ;)
Activity
- at mentions leads to site.url/username/activity/mentions > not broken
Friends
- friendship_accepted leads to site.url/username/friends/my-friends > not broken
- friendship_request leads to site.url/username/friends/requests > not broken
Groups
- new_membership_request leads to site.url/groups/groupslug/admin/membership-requests > not broken
- membership_request_accepted, membership_request_rejected, member_promoted_to_mod or member_promoted_to_admin lead to site.url/username/groups if more than 1 or site.url/groups/groupslug/ > not broken
- group_invite leads to site.url/username/groups/invites > not broken. In case of an invite removed, 1.9 is deleting the corresponding notification
For groups, the only possible case of broken link is when the group is deleted, and in this case 1.9 deletes all notifications for the specific group by hooking groups_delete_group, so it should never happen
Private massages :
- new_message leads to site.url/username/messages/inbox > not broken
#8
@
11 years ago
@imath I was planning to wake up today then go through which links may be broken - you beat me to it! :) It seems group deletion is the only case where this could happen but it looks like 1.9 will take care of that scenario.
Good catch in 5290 - this eventuality never occurred to me.
#10
follow-up:
↓ 11
@
11 years ago
@imath - i've been thinking about 5289.02.diff for when this ticket is revisited in 2.0 - in particular how private message notifications are deleted.
The function you hooked to messages_thread_deleted_thread
is executed only if no recipients remain in the message thread. Therefore, private message notifications can remain in the unread or read list even after a member has deleted the message thread. Only after every recipient of the message has deleted the thread will the notification be removed.
#11
in reply to:
↑ 10
@
11 years ago
Replying to henrywright:
The function you hooked to
messages_thread_deleted_thread
is executed only if no recipients remain in the message thread. Therefore, private message notifications can remain in the unread or read list even after a member has deleted the message thread. Only after every recipient of the message has deleted the thread will the notification be removed.
True. Modifying the suggested patch to 5289.03.diff to delete the notifications for logged in user.
#12
@
11 years ago
In bp-messages-classes.php, would it be better to add the following directly before the check to see if any more recipients remain for this message:
$message_id = $wpdb->get_var( $wpdb->prepare( "SELECT id FROM {$bp->messages->table_name_messages} WHERE thread_id = %d", $thread_id ) ); do_action_ref_array( 'messages_thread_deleted_thread', array( $message_id ) );
Then change the bp_messages_message_delete_notification
function to:
function bp_messages_message_delete_notification( $message_id = 0 ) { if ( bp_is_active( 'notifications' ) && !empty( $message_id ) ) { bp_notifications_delete_notifications_by_item_id( bp_loggedin_user_id(), $message_id, buddypress()->messages->id, 'new_message' ); } } add_action( 'messages_thread_deleted_thread', 'bp_messages_message_delete_notification', 10, 1 );
#17
@
11 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 8099:
#18
@
11 years ago
Thanks, guys. I've marked the ticket resolved. If any other specific cases come up, feel free to reopen, or create a new ticket with details.
#19
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Boone - couple more cases:
If you delete the activity item for 'posting a status update (in a group)', and for 'member joined a group', then the corresponding notification is left with blank content (linked to a non existent activity item).
#20
@
11 years ago
- Keywords reporter-feedback added
If you delete the activity item for 'posting a status update (in a group)', and for 'member joined a group', then the corresponding notification is left with blank content (linked to a non existent activity item).
By default, no notifications are created when an activity update is made in a group nor when a new member joins a group.
rogercoathup - I'm guessing you're using some custom code or a plugin to generate these notifications. Your code will need to be updated to use the new mark as read functionality in the notifications component.
Hi,
As emails sent cannot be deleted, there will always be a risk that the user arrives on a page with no content. So first i thought, maybe the notifications component should follow this behavior and act like a log of past interactions.
Then, i've noticed groups invite notifications were deleted in case an invite has been removed. When a group or a user is removed all notifications are also deleted. In case a friendship is deleted, the action was here but not fired.
I guess, the behavior for demoted users in groups, activity (@mention) and message thread completely deleted (that is to say all members deleted the message see #5254) should as a result follow the same behavior than previous cases.
Now the item_id for messages component in bp_notifications is not the thread_id but the message id, so i add to edit the BP_Messages_Thread::delete function to get that message id and add a new action to hook the function to delete the notifications.
I also edited the hook for friendship case, as it was not fired due to a missing letter :
friends_friendship_withdrawn
instead offriends_friendship_withdrawn
So i suggest the 5289.diff attached to this ticket.