Skip to:
Content

Opened 4 months ago

Last modified 4 months ago

#7569 new defect (bug)

Bulk message actions (Mark as Read) do not affect corresponding notifications

Reported by: boonebgorges Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Messages Keywords: has-patch 2nd-opinion
Cc:

Description

If you have one or more new messages, manually marking those messages as read - whether via bulk actions or "Read" links in bp-legacy or some other mechanism - should cause the corresponding "new_message" notifications to be marked as read. Currently, this is not the case: the only way a new_message notification is marked as read is if you visit the thread directly.

Attachments (1)

7569.diff (1.2 KB) - added by boonebgorges 4 months ago.

Download all attachments as: .zip

Change History (5)

@boonebgorges
4 months ago

#1 @boonebgorges
4 months ago

  • Keywords has-patch 2nd-opinion added

7569.diff fixes this by hooking directly to the messages_thread_mark_as_read action.

I think, though I haven't tested, that this is a better and more general way of handling message notifications than the technique used in bp_messages_screen_conversation_mark_notifications(), and so perhaps could replace it. But for this, I'd like a second opinion from @johnjamesjacoby (who originally wrote this [7621]) or @r-a-y (who touched it last [8202]).

#2 @johnjamesjacoby
4 months ago

I agree this patch seems more inline with what we would expect to happen, at the time it would naturally happen at.

I imagine it was originally deferred to the inbox view of the recipient to avoid a bunch of pre-processing should that user never actually log in, but it's also kinda silly to keep those notifications around forever too.

+1

#3 @r-a-y
4 months ago

Good catch. I guess no one ever really uses the bulk "mark as read" option much! Commit at will.


I think, though I haven't tested, that this is a better and more general way of handling message notifications than the technique used in bp_messages_screen_conversation_mark_notifications()

Not necessarily.

  • bp_messages_screen_conversation_mark_notifications() grabs the logged-in user's unread messages and cross-references it with the messages in the current message thread before marking the message as read. The con here is grabbing the logged-in user's unread messages, which could be a big lookup. The positive is this will only run bp_notifications_mark_notifications_by_item_id() as necessary, which is important as this runs when one visits a private message thread all the time.
  • The patch takes the approach of grabbing all the messages in the current message thread. Then, it loops through that and attempts to mark each message as read for the logged-in user. This is somewhat okay because this runs on the 'messages_thread_mark_as_read' hook. Howver, this means that bp_notifications_mark_notifications_by_item_id() will run a DB query for the number of messages in the thread. For large message threads, this will not be so great.

I guess what would be better is to introduce a bulk read / unread method for the Notifications component so we can just mark a bunch of messages in bulk with one DB query. Currently, we have to do a loop through a message thread and then mark each mesasge one at a time.

Edit: BP_Notification_Notification::mark_all_for_user() seems like it can handle bulk operations in one DB query. We might want to switch to that instead.

Last edited 4 months ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.