Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5384 closed defect (bug) (fixed)

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

Reported by: SlothLoveChunk Owned by: boonebgorges
Milestone: 2.0 Priority: normal
Severity: critical Version: 1.9.1
Component: Toolbar & Notifications Keywords: has-patch 2nd-opinion
Cc:

Description

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.

Attachments (1)

5384.patch (9.5 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (13)

#1 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 1.9.3
  • Severity changed from normal to critical

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.

#2 @boonebgorges
8 years ago

  • Keywords has-patch 2nd-opinion added

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

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:

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?

@boonebgorges
8 years ago

#3 @johnjamesjacoby
8 years ago

Patch looks good.

#4 @boonebgorges
8 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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

#5 @boonebgorges
8 years ago

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

#6 @SlothLoveChunk
8 years ago

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?

#7 @boonebgorges
8 years ago

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

#8 @johnjamesjacoby
8 years ago

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.

#9 @henry.wright
8 years ago

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)

#10 @boonebgorges
8 years ago

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 :)

#11 @henry.wright
8 years ago

I'll raise a new ticket :)

#12 @boonebgorges
8 years ago

  • Milestone changed from 1.9.3 to 2.0

Moving to 2.0 because 1.9.3 never happened.

Note: See TracTickets for help on using tickets.