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

[boonebgorges]

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.

[boonebgorges]

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]).

[johnjamesjacoby]

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

[r-a-y]

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.

[DJPaul]

Patch looks good. We can get this in and the discussion above doesn't block it, right? @r-a-y?

[djpaul]

In 11800:

Notifications, Messages: clear new message notification when the corresponding message is marked read.

Fixes #7569

Props boonebgorges