Single notifications are deleted instead of being marked as read for plugins

[henrywright]

Single follow notifications don't seem to appear in the 'Read' notifications list after the user has clicked on the 1 more user is now following you text.

Steps to reproduce

1. Visit the Unread notifications page members/username/notifications/unread after a member starts following you
2. Click on 1 more user is now following you which will take you to your list of followers
3. Now go to members/username/notifications/read

You will notice the notification is absent from this list.

I'm not sure if BuddyPress Followers is supported here on Trac but the issue is related to the going live of the new notifications component so I thought I'd report it.

[r-a-y]

Thanks for testing, henrywright.

We have to make sure that plugins using the older notification functions work properly as we transition from the old default of deleting notifications to the new default of marking them as read before 1.9-final hits.

[r-a-y]

This is a big backpat issue because internally we have switched out all previous delete notification functions to the newly-created bp_core_mark_notifications_by_X() functions (see r7529).

If we leave this as-is, 3rd-party plugins will need to do the same thing in order for notifications to default to mark as read instead of delete.

I know this will suck, but perhaps we should revert r7529 and alter most of the bp_core_delete_notifications_by_X() functions to mark notifications as read instead of deleting them.

[r-a-y]

01.patch is a quick patch that does a few things:

• Revert r7529
• Removes all bp_core_mark_notifications_by_X() wrapper functions.
• Moves all delete notification firing for all components to bp-notification-actions.php.
• In bp-notification-actions.php, use the new notification functions introduced in bp-notification-functions.php instead of the older notification functions.
• For the older notification functions, I've changed the behavior to mark as read instead of outright deleting them. I've also marked these functions as deprecated with phpDoc.

Let me know what you think of the approach.

[johnjamesjacoby]

I'm okay with third party plugins continuing to behave how they have in the past. I'd rather we keep existing functionalities intact than have functions named _delete_ that don't delete. I vote we keep the behavior as it is in trunk, and educate third party plugin authors that they'll want to update their plugins with the mark VS delete functionality.

[boonebgorges]

I'm generally the backwards compatibility stickler, but in this case I'm tempted to agree with johnjamesjacoby. Using _delete_ functions to refer to marking read is going to cause headaches down the line. And the following considerations make me think that this is going to be a very minor case of breaking backpat:

• Very few plugins are registering their own notifications. My computer with a checkout of plugins.svn.wordpress.org is in a box somewhere in my unpacked apartment :) but I'm pretty confident we're talking a very small number
• Notifications are a secondary impression of the original content. So, if in some cases they are deleted, we're not really deleting anything of primary importance. Keep in mind that *all* notifications are being deleted pre-1.9 - this issue will only become visible because the display behavior is being changed/augmented by the new Notifications panel. But the point remains that sites can't really be relying on persistent notifications as it is, because currently notifications are *not* persistent.

So, on balance, I think it's best to move forward with a system that is more internally coherent, and let plugin authors catch up. If others agree, this is worth putting a brief post up onto bpdevel before beta2.

[r-a-y]

I'd rather we keep existing functionalities intact than have functions named _delete_ that don't delete.

I'm fine with this. But we do need to educate plugin developers so a post on bpdevel will be needed.

However, I'm not so fond of the introduction of wrapper functions (bp_core_mark_notifications_by_X()) in order to use them throughout our own components and not utilizing the new bp-notifications functions (bp_notifications_mark_notifications_by_X()) that were specifically written.

I understand this was done because the notifications component can be turned off, but both sets of functions were introduced in 1.9 and both do the same thing. This also confuses those looking at the codebase.

This is probably worth discussing in another ticket, but my patch is a step in decoupling notifications from all BP components, which, in my opinion, is what we should be doing.

My ideas for an iterative patch include further separation:

• Moving all bp_core_add_notification() calls to bp-notifications and switching them out for bp_notifications_add_notification()
• Organizing the temporary code in bp-notifications-actions.php to bp-notifications-activity.php, bp-notifications-groups.php, etc.
• Deprecating most of the code in bp-members-notifications.php.

[johnjamesjacoby]

I understand this was done because the notifications component can be turned off, but both sets of functions were introduced in 1.9 and both do the same thing. This also confuses those looking at the codebase.

I agree that it's not ideal, but I think it's necessary to maintain the backwards compatibility of allowing the component to be disabled without third party components white-screening from functions-not-existing problems.

I also agree that the core components should include the same bp_is_active( 'notifications' ) checks that we use for other components, and rather than litter them through-out the entire codebase, I think we can use an action-based API that hooks notifications code into actions that only fire when Notifications is active.

Something like:

• bp_add_notifications
• bp_delete_notifications
• bp_mark_notifications

These actions would be hooked to mid-level execution point (maybe bp_template_redirect ?) so they can occur within a single pageload, effectively bypassing the count updating problem noted in a different ticket.

We're all in agreement that there are improvements we can make to this code -- I'm not convinced there is anything major looming over us to the point of delaying 1.9 though.

[johnjamesjacoby]

Also worth noting that the BP_Component->notification_callback variable only exists because Notifications were coupled with the Core component. Now that it's external, we can start to rethink how Notifications can stand on its own, and use that variable as a fallback.

[johnjamesjacoby]

Going to bump this to 2.0 so we can keep this conversation going, and to clear out the 1.9 milestone for now.

[henrywright]

I've decided to do this:

remove_action( 'bp_activity_screen_mentions', 'bp_activity_remove_screen_notifications' );

// do the same as above for the rest of the notification types

function mark_all_notifications_read() {
    bp_core_mark_notifications_by_type( bp_loggedin_user_id(), 'activity', 'new_at_mention' );
    // do the same for the rest of the notification types
}
add_action( 'bp_after_notifications_loop', 'mark_all_notifications_read' );

Then put do_action( 'bp_after_notifications_loop' ); in unread.php.

All single notifications would be cleared merely by visiting /members/my-username/notifications/

To my knowledge, this approach wouldn't require plugin authors to update their code.

Hoping my ideas are helpful but you can probably see flaws in my thoughts that I can't so any feedback would be great.

[r-a-y]

I've moved my thoughts into another ticket - #5266 - which should be appropriate changes for 1.9.

I think we can use an action-based API that hooks notifications code into actions that only fire when Notifications is active.

Also worth noting that the BP_Component->notification_callback variable only exists because Notifications were coupled with the Core component. Now that it's external, we can start to rethink how Notifications can stand on its own, and use that variable as a fallback.

These are good starting points that we can hopefully iterate on in the future.

---

We've kind of deviated from the initial issue in this ticket. Sorry henrywright!

I think we've agreed that this is a wontfix so plugins will need to update their code.

About your code snippet, you could probably improve mark_all_notifications_read() by using bp_notifications_mark_notifications_by_type( bp_loggedin_user_id(), false, false ) instead of having to gather all the different notification types and clearing them that way.

Adding some do_actions in the notification templates sound like a good idea.

[henrywright]

Sure thing r-a-y. Scrap my idea regardless as pagination won't work. Each time a user clicks on to page 2 of the notifications list they'll see nothing - because of course all unread notifications are cleared when bp_after_notifications_loop was fired.

[r-a-y]

I kind of hijacked henrywright's original ticket here! Apologies!

To get back to his original issue, plugins should move towards the new "mark as read" functionality.

To get back to my concerns, they were addressed in #5266.

Going to mark this one as a duplicate.