Opened 8 years ago
Closed 7 years ago
#7569 closed defect (bug) (fixed)
Bulk message actions (Mark as Read) do not affect corresponding notifications
Reported by: |
|
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)
Change History (7)
#2
@
8 years 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
@
8 years 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 runbp_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 thatbp_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.
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]).