Make notifications a separate component

[johnjamesjacoby]

Notifications in BuddyPress are powerful, but in their infancy, and in need of some love.

A few possible enhancements:

• Ability to enable/disable at the component level.
• Add a UI for read/unread notifications.
• A control panel for moderators to see all notifications, to clear them, delete them, etc...
• Allow users to toggle notifications back to unread.
• Improve the BP_Core_Notification class methods so they are more flexible like the activity and group components.

[dimensionmedia]

This would be incredibly useful and could think of some nice ways to extend this for future projects. Would this use a new database table or existing? Would love to see users mute or suspend notifications for a time (maybe while they are on vacation), but guessing this can be a plugin as well.

[r-a-y]

I know this is slated for 2.0, but I wanted to get some groundwork done to improve the BP_Core_Notification class methods.

class.01.patch does the following:

• Adds missing static method - delete() - as referenced in bp_core_delete_notification(). (Can't believe no one has ever reported this!)
• Adds static method - get(). We don't currently have a flexible get method like the activity and groups components, so this covers that. For a use-case on flexibility, see bp_activity_at_message_notification() in the patch. I also deprecated a bunch of get_*() static methods in favor of this.
• Adds '_before_' and '_after_' hooks in the save() method.
• Adds static method - get_where_sql(). The WHERE sql is shared between both get() and delete() methods, so I created a helper method for this.
• Removed references to the $bp global and replaced with the buddypress() singeleton.

Feedback welcome.

[boonebgorges]

Patch looks really good, r-a-y. I'm a big fan of moving the miscellaneous methods into central delete() and get() methods. Also, the fact that you fixed a couple of these basic but major bugs is crazy to me - I guess that means that few people are hacking on Notifications at the moment. (Heck, I've hardly ever hacked on it.)

Couple small comments:

• Since we're essentially gutting a couple of user facing functions and replacing them with the new all-purpose get and delete methods, we should have unit tests for the procedural wrappers. Then we can rest assured that the refactor didn't break anything.
• get_where_sql() looks good to me, but let's mark it protected static. It's probably a bad idea to encourage developers to use BP's utility methods for building partial SQL statements, as it binds our hands if we ever want to make internal syntax modifications.

But yeah, these look like very solid improvements for 1.9.

[r-a-y]

Thanks for the feedback, Boone. Good catch on marking get_where_sql() as protected.

class.01b.patch adds unit tests and fixes some minor things in get_where_sql() method when passing the secondary_item_id and is_new parameters. (Thank you unit tests!)

For the unit tests, I'm testing the generated SQL statement made by the get_where_sql() method against the current statements generated in 1.8.

Let me know what you think.

[boonebgorges]

Awesome, r-a-y!

I'd suggest that, since get_where_sql() is protected and thus shouldn't really be used externally, we don't need to test it directly; in the same way that we don't want others to use it because the implementation might change, we'll also be forced to rewrite those tests should we wish to reimplement the SQL. I humbly submit that the tests be for the public API functions/methods instead.

Rest of the patch looks really great.

[r-a-y]

I humbly submit that the tests be for the public API functions/methods instead.

class.01c.patch uses the public API functions in the unit tests. I wanted to test the WHERE sql statement only so that required using the new filter hooks, which work great!

[boonebgorges]

I guess I was more thinking of test for, for example, bp_core_get_notifications_for_user(), which would show us that the refactored function (which uses the new get() under the hood) is returning the same values as the function currently does. That said, I don't think it's a showstopper, as the changes seem pretty straightforward.

[henrywright]

If notifications is to become a component, does that mean a dedicated notifications screen will be introduced? members/username/notifications

I find the current notifications drop-down doesn't work very well when there are 5, 6, 7 etc notifications to display due to a lack of physical space. So for each member, I've introduced a dedicated page to display their (un-clicked) notifications:

function setup_notifications() {
	bp_core_new_nav_item( 
		array( 'name' => __( 'Notifications', 'buddypress' ), 
			'slug'	=> 'notifications',  
			'position' => 40,
			'show_for_displayed_user' => false,
			'item_css_id' => 'notifications',
			'screen_function' => 'notifications_screen',
		) 
	);
}
add_action( 'bp_setup_nav', 'setup_notifications' );

function notifications_screen() {
	bp_core_load_template( 'members/single/notifications' );
}

A standalone page (having lots more space than the drop-down) opens up the possibility of having a 'rich notifications' log such as

[avatar] member X followed you
[avatar] member X replied to your comment

There would be little need to 'dismiss' or delete a notification as the page could grow endlessly allowing the member to check back over old notifications.

The log might grow big so notifications might need to be paged. So bp_core_new_subnav_item() might be needed.

Hopefully i'm not way off with my thoughts on this one. Just thought i'd throw an idea into the mixing pot.

[johnjamesjacoby]

bp-notifications component. backpat, no unit tests, mock template parts for bp-legacy. Needs output API.

[johnjamesjacoby]

Replying to henrywright:

If notifications is to become a component, does that mean a dedicated notifications screen will be introduced? members/username/notifications

Yes, exactly.

A standalone page (having lots more space than the drop-down) opens up the possibility of having a 'rich notifications' log such as

[avatar] member X followed you
[avatar] member X replied to your comment

There would be little need to 'dismiss' or delete a notification as the page could grow endlessly allowing the member to check back over old notifications.

Correct. Notifications currently are always deleted, rather than mark is_new as 0. My attached patch introduces logic for marking notifications as read/unread.

The log might grow big so notifications might need to be paged. So bp_core_new_subnav_item() might be needed.

Attached patch introduces an extra method for getting paginated notifications on two notifications screens: Unread and Read. This should help with both the logging of events, and providing a more robust interface to see individual notifications, rather than lumping them all together like we do now.

Hopefully i'm not way off with my thoughts on this one. Just thought i'd throw an idea into the mixing pot.

You're spot on with what's been in my imagination for a while now.

[johnjamesjacoby]

I'd like to suggest fitting this into 1.9, since we've extended putting out a beta, and this is mostly complete.

[henrywright]

johnjamesjacoby thanks for giving an overview of how the new component will function. I was originally drawn to this ticket due to @-mentions not working in blog posts and comments due to a side effect of the @-mention architecture as pointed out by Boone in ticket ​5230. The new component will certainly open up lots of possibilities. Excited to see this in 1.9!

[johnjamesjacoby]

To-do's:

Because there's no full-text of the notification itself, bp-notifications should be responsible for putting the item_id, secondary_item_id, component_name, component_action, and date_notiified into real words.

In the current implementation, notification_callback is used as a singlular/cumulative catch-all; bp_activity_format_notifications() is a good example. bp_core_get_notifications_for_user() will be used as a foundation to loop through active notifications, and build the full-text strings for each line item, without grouping all similar notifications into 1 to be acknowledged.

We'll also need to change the behavior from delete to mark read and make sure bp_core_get_notifications_for_user() only shows unread notifications. This won't be difficult, but will likely touch each component, since they are individually responsible for knowing when their notifications are no longer new.

[boonebgorges]

johnjamesjacoby - Thanks for the initial patch. You've done much of the hard work.

As I mentioned to you earlier, I think that you were too conservative in porting from the current implementation. This is particularly true in the database class, where you've kept almost exactly the same logic. This loses all the elegance, flexibility, robustness, testability, maintainability, etc of what r-a-y had been exploring with his earlier patches.

In 5148.02.patch, I implement most of what r-a-y originally suggested on top of 5148.02.patch. All update, insert, delete, and get queries are now being piped through single, all-purpose methods. Unit tests have been provided for most major database functionality. Since $wpdb provides update(), insert(), and delete(), but nothing for SELECT, I knew I'd need my own get() with my own SQL; since I was going to do it anyway, I added support so that all params for get() can be either single values or arrays of values for an IN clause. I think this technique will give us much more flexibility down the road, and will be far easier to maintain (in addition to being far more DRY).

I've kept most of the original database methods suggested by 5148.patch, but they are all internally using the primary CRUD methods of the class. This includes everything under the header "Convienience Methods" in bp-notifications-classes.php. IMHO, these could all be removed, with their logic put instead into procedural functions with similar names. That'd keep the database class lean-n-mean, and promote a proper separation of duties.

Other miscellaneous cleanup in 5148.02.patch:

• Added/improved docblocks
• Better coding standards compliance (commas after final array values, stuff like that)
• Fixed some of the half-implemented logic for pagination (you had 'max' but hadn't fully implemented 'page' and 'per_page')

I haven't really touched any of the front-end stuff, as I'm going a little nutso staring at this all day :) Pagination obviously needs to be cleaned up in the display, in addition to the concatenation of strings like you mention above.

Another todo: activate the Notifications (and Settings) components for upgrading systems.

[johnjamesjacoby]

In 7521:

First pass at bp-notifications component. Includes:

• Backwards compatibility for old core functions.
• Screens, functions, classes, and actions for Notifications.
• Improved class methods for getting, updating, and deleting notifications.
• Template parts in bp-legacy for theme compatibility.
• A few basic unit tests.

@todo's:

• Improve template output with dedicated functions, markup, classes, et all.
• More unit tests.
• Pagination links.
• Auto-activate on update, so existing installations do not lose previously core functionality.

See #5148. Props johnjamesjacoby, boonebgorges, r-a-y.

[johnjamesjacoby]

In 7523:

Add pagination to notifications output. Includes functions for outputting and returning the count labels. See #5148.

[johnjamesjacoby]

In 7524:

Remove stripslashes_deep() copy-pasta in bp-notifications-template.php. See #5148.

[johnjamesjacoby]

In 7525:

Clean up bp-notifications-classes.php. See #5148.

[johnjamesjacoby]

In 7526:

Introduce functions for outputting notification descriptions and action links, and update template parts to use them. See #5148.

[johnjamesjacoby]

In 7527:

In Messages component, add sender_id to secondary_item_id in notifications. Will be used to include the username in notifications. See #5148.

[johnjamesjacoby]

In 7528:

Introduce functions to help mark messages as read/unread. Will replace _delete_ usages in other components, to allow logging of notifications VS instantly deleting them. See #5148.

[johnjamesjacoby]

In 7529:

Introduce bp_core_mark_all_notifications_by_type() and replace bp_core_delete_all_notifications_by_type() usages. Ensures notifications are toggled as acknowledged rather than deleted. See #5148.

[johnjamesjacoby]

In 7530:

Introduce bp_core_mark_notifications_by_type() and bp_core_mark_notifications_by_item_id() and replace bp_core_delete_notifications_by_type() and bp_core_delete_notifications_by_item_id() usages.

Ensures notifications are toggled as acknowledged rather than deleted. See #5148.

[johnjamesjacoby]

In 7531:

Introduce bp_core_mark_notifications_from_user() for completeness, though it's currently unused. Also fix some phpdoc. See #5148.

[johnjamesjacoby]

In 7532:

Add $is_new to bp_notifications_add_notification(), allowing developers to add notifications to the log without requiring user intervention. See #5148.

[johnjamesjacoby]

In 7533:

Make sure notifications always have date. See #5148.

[johnjamesjacoby]

In 7534:

If notification date is empty, be smarter about date output. See #5148.

[johnjamesjacoby]

In 7535:

In BP_Notifications_Notification::get() add clause for is_new to allow not-new notifications to be queried. See #5148.

[johnjamesjacoby]

In 7537:

Use the correct function and update methods for marking user notifications as read. See #5148.

[johnjamesjacoby]

In 7538:

Clean up notifications template parts to better utilize notifications-loop.php. See #5148.

[johnjamesjacoby]

In 7539:

Add functions for permalinks, mark and delete links, and use in action links output. See #5148.

[johnjamesjacoby]

In 7540:

Clean up from r7539. See #5148.

[johnjamesjacoby]

In 7543:

Introduce bp-notifications-actions.php to handle single notification status toggling.

• Include new actions file in loader.
• Tweak link output functions to make it moderately easier to debug the URL.
• Introduce bp_notifications_mark_notification() to help with marking single notifications.
• Fix bp_notifications_delete_notification() to work with new array argument pattern, introduced previously.
• It's alive!

See #5148.

[johnjamesjacoby]

In 7544:

In Notifications toolbar menu, ensure unread notification bubble cascades into sub-menu for consistent component behavior. See #5148.

[johnjamesjacoby]

In 7545:

No humility. Bump notifications menu and navigation position to below Profile, and ensure that custom notifications toolbar menu now links to new notifications page for logged in user. See #5148.

[johnjamesjacoby]

In 7546:

Ensure users with bp_moderate capability are allowed to navigate to other user's notifications. See #5148.

[johnjamesjacoby]

In 7547:

Ensure when users with bp_moderate capability navigate to other user's notifications, the correct notifications are shown. See #5148.

[johnjamesjacoby]

In 7548:

Break feedback markup into separate template part. Include verbiage for users with bp_moderate capability. See #5148.

[johnjamesjacoby]

In 7549:

Introduce BP_Notifications_Notification methods for handling LIMIT and ORDER BY MySQL to act as the foundation for customizing sorting and pagination arguments. Also flip default notification order from ASC to DESC for a top-down view. See #5148.

[johnjamesjacoby]

In 7550:

Default notifications from 10 to 25 for consistency across Notifications component. See #5148.

[johnjamesjacoby]

In 7551:

Add database upgrade routine for 1.9, to automatically activate the Notifications component. Ensures previous installations will continue to have notifications functionality. See #5148.

[johnjamesjacoby]

Everything is done, minus some JS for the sort-order drop-down selection. Anyone want some easy props?

[johnjamesjacoby]

In 7553:

PHP5'ize the BP_Notifications_Notification class, and add missing $order_by and $sort_order variables. See #5148.

[vhauri]

Looking into the JS drop-down functionality now.

[vhauri]

Ok, quick rundown on the behavior of the JS order select:

If 1 page of notifications or less, JS swaps order of existing items

If > 1 page of notifications, we have some options:

• JS resets order and loads first page of notifications based on new sort.
• JS resets order but keeps current page (i.e. page 4 of ASC sort flips and shows page 4 of DESC sort)
• JS resets order and loads inverse of page (i.e. loads page 7 of 8 instead of 2 of 8)

I'm leaning towards one of the first two options, but am open to hearing how this behavior works throughout BP and matching that.

[vhauri]

implements JS behavior for sorting notifications

[vhauri]

Ok, here's a first stab at the functionality outlined above, using the first option for paginated notifications (i.e. reverses sort and sends the user back to the first page of that sort order). Possible red flags:

• We're checking a $_GET var directly to set the selected value of the select input in /bp-templates/bp-legacy/buddypress/members/single/notifications.php. I'm doing this because it seems the BP_Notifications_Template object has not yet instantiated at that point with the corresponding sort_order property. If there's a better way to do this, the code should be updated to use that.

• JS is being enqueued in bp-notifications-template.php conditionally when the object finds notifications for the user. This is great performance-wise, but I'm not sure if it's the right place for the enqueue.

• The closing </tbody> tag must be fixed (included in this patch) for the JS to work--otherwise it will balk at the presence of multiple tbody tags in the table.

[johnjamesjacoby]

In 7557:

Kill Mr. Boddy. See #5148. Props vhauri.

[johnjamesjacoby]

In 7559:

Add sorting of notifications via form to submit sort_order value. See #5148. Hat tip vhauri.

[johnjamesjacoby]

The notifications component is feature complete for 1.9, so I'm marking this one as fixed.

Let's open enhancement tickets (such as ajaxifications for links/sorting/pagination) for 2.0.

[johnjamesjacoby]

In 7578:

Allow non-core components to hook into notifications via a filter. This allows components that are not registered as part of the main BuddyPress singleton to hook a callback into a filter instead. See #5148.