Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 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:

  1. Send a member a private message or mention them in a blog post comment
  2. View the notifications page members/username/notifications of the messaged or mentioned member
  3. Now delete the private message or the blog post comment
  4. 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)

5289.diff (4.5 KB) - added by imath 6 years ago.
5289.02.diff (4.6 KB) - added by imath 6 years ago.
5289.03.diff (4.2 KB) - added by imath 6 years ago.

Download all attachments as: .zip

Change History (25)

#1 @imath
6 years ago

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 of friends_friendship_whithdrawn

So i suggest the 5289.diff attached to this ticket.

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

@imath
6 years ago

#2 @henrywright
6 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 @imath
6 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 :(

@imath
6 years ago

#4 follow-up: @boonebgorges
6 years ago

Is this a regression from 1.8.x?

#5 in reply to: ↑ 4 @imath
6 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 @henrywright
6 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 @imath
6 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 @henrywright
6 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.

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

#9 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to 2.0

#10 follow-up: @henrywright
6 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 @imath
6 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.

@imath
6 years ago

#12 @henrywright
6 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 );

#13 follow-up: @henrywright
6 years ago

Oops - you beat me to it. I was 59 seconds too late :)

#14 in reply to: ↑ 13 @imath
6 years ago

Replying to henrywright:

Oops - you beat me to it. I was 59 seconds too late :)

;)

#15 @boonebgorges
6 years ago

In 8095:

When deleting activity items, delete corresponding notifications

See #5289

Props imath

#16 @boonebgorges
6 years ago

In 8097:

When demoting a member in a group, delete corresponding promotion notifications

See #5289

Props imath

#17 @boonebgorges
6 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 8099:

When a message is deleted, delete corresponding notifications

Fixes #5289

Props imath, henrywright

#18 @boonebgorges
6 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 @rogercoathup
6 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 @r-a-y
6 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.

#21 @rogercoathup
6 years ago

@r-a-y -- yes, sorry my mistake. We had Brajesh's Group Activity Notification plugin installed on that site.

#22 @r-a-y
6 years ago

  • Keywords reporter-feedback removed
  • Resolution set to fixed
  • Status changed from reopened to closed

rogercoathup - Thanks for confirming. Going to close this one to clear the milestone.

Note: See TracTickets for help on using tickets.