All @-mention notifications cleared when anonymous user visits another user's activity feed

[SlothLoveChunk]

I'm running the latest version of WP and BP, and for the last few weeks users have been reporting that @-mention notifications have been disappearing. Fortunately, they were not deleted, rather, they were being marked as read (/notifications/read/).

To debug I logged SAVEQUERIES and found that this query in the log:

UPDATE `wp_bp_notifications` SET `is_new` = 0 WHERE `component_name` = 'activity' AND `component_action` = 'new_at_mention'

As you can see, there is no 'user_id' in the where clause, which is causing all @-mentions to be marked as read. Backtracing, it looks like this is being invoked by:

function bp_activity_remove_screen_notifications() {
	if ( bp_is_active( 'notifications' ) ) {
		bp_notifications_mark_notifications_by_type( bp_loggedin_user_id(), buddypress()->activity->id, 'new_at_mention' );
	}
}
add_action( 'bp_activity_screen_my_activity',               'bp_activity_remove_screen_notifications' );
add_action( 'bp_activity_screen_single_activity_permalink', 'bp_activity_remove_screen_notifications' );
add_action( 'bp_activity_screen_mentions',                  'bp_activity_remove_screen_notifications' );

Which in turn calls:

function bp_notifications_mark_notifications_by_type( $user_id, $component_name, $component_action, $is_new = false ) {
	return BP_Notifications_Notification::update(
		array(
			'is_new' => $is_new
		),
		array(
			'user_id'          => $user_id,
			'component_name'   => $component_name,
			'component_action' => $component_action
		)
	);
}

Since activity pages are publicly accessible -- at least on my site -- when a non-logged in user visit any other user's activity page the offending query sans user_id is ran.

I have fixed the issue by first checking to make sure $user_id exists:

function bp_notifications_mark_notifications_by_type( $user_id, $component_name, $component_action, $is_new = false ) {
    if ( !empty( $user_id ) ) {  // Only remove is_new if there is a user_id
	return BP_Notifications_Notification::update(
		array(
			'is_new' => $is_new
		),
		array(
			'user_id'          => $user_id,
			'component_name'   => $component_name,
			'component_action' => $component_action
		)
	);
    }
}

I realize this is a hack and not ideal, but by posting here I am hoping someone might be able to recreate and offer a more elegant solution.

[boonebgorges]

Oy. Thanks for the report and the trace. We should be checking user ID one step further up the chain, where we detect the page visit.

[boonebgorges]

So, this is kind of a mess. Left over from the old notifications system, we clear at-mention notifications on the following events:

• 'bp_activity_screen_my_activity' (​http://example.com/members/boone/activity/)
• 'bp_activity_screen_single_activity_permalink' (​http://example.com/activity/p/12345/)
• 'bp_activity_screen_mentions' (​http://example.com/members/boone/activity/mentions/)

We don't take into account the current user in any of these cases. This seems far, far too broad to me. Here's the activity I'd expect:

• When user boone visits ​http://example.com/members/boone/activity/mentions/, all of his activity mention notifications are marked read
• When user boone visits ​http://example.com/activity/p/12345/, at-mention notifications belonging to boone and related to activity #12345 should be marked read
• When visiting ​http://example.com/members/boone/activity/, nothing happens.

See 5384.patch. Can I ask someone to chime in and let me know whether they share my intuition about how this ought to work?

[johnjamesjacoby]

Patch looks good.

[boonebgorges]

In 7831:

Improve logic for marking at-mention notifications as read

In previous iterations, at-mention notifications were marked as read in the
following cases:

• when a user visited his own activity page
• when a user visited his own mention page
• when a user visited a single-activity permalink page

When the new notifications component was introduced in BP 1.9, the old
notification functions were swapped out one-for-one with the new correlates.
However, this introduced a bug where a logged-out user visiting one of the
pages listed above would mark at-mention items as read for *all* users (due to
the user_id field being null, and the user_id WHERE clause being excluded from
the UPDATE query).

We take this bug as an opportunity to narrow the circumstances in which
at-mention notifications are marked as read. The new system:

• When a user visits his own mention page, mark all his at-mention notifications as read
• When a user visits the permalink page of a single activity item in which he is mentioned, mark that specific at-mention notification as read

Fixes #5384

Props SlothLoveChunk for an initial patch

[boonebgorges]

In 7832:

Improve logic for marking at-mention notifications as read

In previous iterations, at-mention notifications were marked as read in the
following cases:

• when a user visited his own activity page
• when a user visited his own mention page
• when a user visited a single-activity permalink page

When the new notifications component was introduced in BP 1.9, the old
notification functions were swapped out one-for-one with the new correlates.
However, this introduced a bug where a logged-out user visiting one of the
pages listed above would mark at-mention items as read for *all* users (due to
the user_id field being null, and the user_id WHERE clause being excluded from
the UPDATE query).

We take this bug as an opportunity to narrow the circumstances in which
at-mention notifications are marked as read. The new system:

• When a user visits his own mention page, mark all his at-mention notifications as read
• When a user visits the permalink page of a single activity item in which he is mentioned, mark that specific at-mention notification as read

Fixes #5384

Props SlothLoveChunk for an initial patch

[SlothLoveChunk]

Patch works great. Thanks!

My only feedback regarding the logic would be related to WP comments. Shouldn't an @-mention also be marked as read when the user visits the page / post where they were mentioned in said comment?

[boonebgorges]

Shouldn't an @-mention also be marked as read when the user visits the page / post where they were mentioned in said comment?

That's not a bad idea. However, it's beyond what the functionality was originally intended to do, so it's probably best to open a new ticket for it. (In fact, it might be nice to have an enhancement ticket where we try to catalog *all* the places where you might go to view an @-mention - stuff like forum posts, blog posts, etc.)

[johnjamesjacoby]

catalog *all* the places where you might go to view an @-mention

I thought about this quite a bit when refactoring Notifications; that there's really no recipe or map for when notifications are pushed, and how they are marked read. Because of the action oriented way WordPress works, it's both flexible and a bit unruly, so providing some semblance of a standard and map would be really helpful.

[henry.wright]

Shouldn't an @-mention also be marked as read when the user visits the page / post where they were mentioned in said comment?

@boonebgorges and @johnjamesjacoby

Would something like this work?

function mark_read_single_at_mention_notification( $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( 'bp_before_post', 'mark_read_single_at_mention_notification' );

One caveat is this requires a do action hook bp_before_post in single.php

I suppose in theory the same can be done for private messages (i.e. when the thread is visited)

[boonebgorges]

Yes, something like that should be done for blog posts and for private messages and for any other relevant things, but it really should go in a separate enhancement ticket :)

[henry.wright]

I'll raise a new ticket :)

[boonebgorges]

Moving to 2.0 because 1.9.3 never happened.