Mark at-mention notifications as read when the item (blog post, private message, etc) is visited

[henry.wright]

Currently, unread at-mention notifications are marked as read when the notified member visits their mentions page members/username/activity/mentions.

It was suggested in ​5384 that at-mention notifications be marked as read when the blog post that contains the at-mention is visited by the mentioned member.

The same idea could be applied to private messages (i.e. when the notified private message recipient visits the private message thread)

[boonebgorges]

Thanks, henry.wright.

To keep things organized, it would be nice to approach this systematically. We should assemble a list of notification types, and under each one we should list the cases where they should be cleared. Here's a start at a list:

• 'new_at_mention'
  • When viewing your own @-mention page, clear all
  • When viewing a specific activity item, clear associated with that item
  • If notification is in a blog post, when viewing that blog post
  • if notification is in a blog comment, when viewing that comment
  • If notification is in a forum post, when viewing that forum post

• 'friendship_request'
  • When viewing your Friend Requests page, clear all

• 'friendship_accepted'
  • When viewing your Friends page, clear all
  • When viewing the profile of the new friend, clear

• 'new_membership_request'
  • When viewing the Requests page, clear all for group

• 'membership_request_accepted'
  • When viewing the My Groups page, clear all
  • When viewing single group page, clear for that group

• 'membership_request_rejected'
  • When viewing single group page, clear for that group

• 'member_promoted_to_admin', 'member_promoted_to_mod'
  • When viewing single group page, clear for that group

• 'group_invite'
  • When viewing Group Invites page, clear all

• 'new_message'
  • When viewing Inbox, clear all
  • When viewing single message, clear for message

Does this look right?

[henry.wright]

Does this look right?

1. I've searched the code base (v 1.9.2) for all instances of bp_notifications_add_notification - you've got all component_action(s) covered!

2. Regarding when each of the various notifications should be cleared - what you've come up with looks spot on to me.

As far as I can tell - that is everything accounted for.

[henry.wright]

I've been trying to clear at-mention notifications when the at-mention is in a blog post or a blog post comment and the blog post is visited but have hit a stumbling block.

Failed solution:

function mark_read_single_at_mention_notification() {
    if ( is_single() ) {
        global $post;
        $post_id = $post->ID;
        $user_id = bp_loggedin_user_id();
    
        $notifications = BP_Notifications_Notification::get( array(
            'user_id' => $user_id,
            'is_new' => true,
        ) );

        if ( $notifications ) {
            foreach( $notifications as $notification ) {

                // get the activity details
                $activity = new BP_Activity_Activity( $notification->item_id );

                if ( isset( $activity ) ) {
                    $comment = get_comment( $activity->secondary_item_id );

                    if ( $comment ) { 
                        $id = $comment->comment_post_ID;

                        if ( $id == $post_id ) {
                            bp_notifications_mark_notifications_by_item_id( bp_loggedin_user_id(), $notification->item_id, 'activity', 'new_at_mention' );
                        }
                    }
                }
            }
        }
    }
}
add_action( 'init', 'mark_read_single_at_mention_notification' );

Reason for failure:

Activity items can be deleted. Therefore, it is useless doing $activity = new BP_Activity_Activity( $notification->item_id );. If the activity item is deleted I'm not sure there is a way to associate the notification with the comment's parent post ID.

Proposal:

Capture the comment's parent post ID when recording the notification.

This is what is currently captured:

bp_notifications_add_notification( array(
    'user_id'           => $receiver_user_id,
    'item_id'           => $activity->id,
    'secondary_item_id' => $activity->user_id,
    'component_name'    => buddypress()->activity->id,
    'component_action'  => 'new_at_mention',
    'date_notified'     => bp_core_current_time(),
    'is_new'            => 1,
) );

[boonebgorges]

henry.wright - Good catch. You're correct that there'll be no way to clear this notification if the associated activity id is created.

The underlying problem is that, from BP's point of view, @-mentions are inherently a property of *activity items*, not of source items like blog comments, forum posts, etc. If you don't have the activity component, putting an @-mention in a blog post will do nothing. It looks like we may have had this discussion before; see #4046.

In the function you post above, the notification will simply fail to be marked as read if the activity item has been deleted. From my point of view, this is not a catastrophic failure - there are other ways to mark these notifications as read (manually, or by visiting your @-mentions page). Since it'll be fairly rare for this situation to come up (the activity item having been deleted), I'm OK with letting the failure happen and going with something like you've suggested above. (Until such time as we come up with a better system for @-mentions ;) )

[henry.wright]

With reference to ticket #4046, I'm sure we've had that conversation more than once before actually :)

You got me thinking - When an at-mentions happens to be in a blog post, could we be cheeky and conditionally slip the blog post's ID into the item_id instead of the activity ID?

For example, at the point of notification creation (please excuse the pseudo code):

is this at-mention in an activity item {
   $item_id = $activity->id;
} else is this at-mention in a blog post {
   $item_id = $post->ID;
}

The $item_id holds the value we need either way.

bp_notifications_add_notification( array(
    'user_id'           => $receiver_user_id,
    'item_id'           => $item_id,
    'secondary_item_id' => $activity->user_id,
    'component_name'    => buddypress()->activity->id,
    'component_action'  => 'new_at_mention',
    'date_notified'     => bp_core_current_time(),
    'is_new'            => 1,
) );

To my knowledge, having a flexible item_id which can hold either the activity ID or the blog post ID wouldn't have implications elsewhere.

If I'm wrong and this will cause side-affects elsewhere, then I agree it's time to throw in the towel. As you say there are more ways to dismiss the notification and activity items aren't deleted often so although it's a failure, it isn't a huge failure :)

[boonebgorges]

could we be cheeky and conditionally slip the blog post's ID into the item_id instead of the activity ID?

We could. But this alone wouldn't be enough, because of multisite. So we'd have to swap out the item_id *and* the secondary_item_id, in the way that blog activity items are done. Then, by extension, we'd probably want to parallel the activity item_id schema for *all* notification types.

I guess I'm not fundamentally opposed to doing that, but we'd want to be consistent throughout. We'd also then need a review of all uses of the mark-notifications-read-by-item-id etc functions to make sure that we're clearing the right things, and in cases where changes are required, we'd have to do research into whether we'll break backward compatibility by changing the meaning of the columns.

[boonebgorges]

(I guess my gut feeling is that we should change nothing, but that's mostly because changing would be a lot of work :) )

[henry.wright]

Replying to boonebgorges:

(I guess my gut feeling is that we should change nothing, but that's mostly because changing would be a lot of work :) )

Agreed. Seems as though the costs (time and effort) heavily outweigh the benefits of this!

[DJPaul]

Not exactly sure what further investigation this ticket requires before we can assign it to a release milestone, so moving to Future Release for now.

[slaFFik]

Seems Henry and Boone come to an agreement of doing nothing, so lets close this ticket for now as maybelater.
As always, brave souls can reopen and provide further ideas or even patches!