#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)
Change History (13)
#1
@
11 years ago
- Milestone changed from Awaiting Review to 1.9.3
- Severity changed from normal to critical
#2
@
11 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:
- '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?
#4
@
11 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 7831:
#6
@
11 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
@
11 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
@
11 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
@
11 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)
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.