Notifications from inactive components are rendered on the Notifications panel

[boonebgorges]

In #5290, we excluded notifications from inactive components from the admin bar Notifications dropdown, but didn't apply the same fix to the Notifications panel.

[boonebgorges]

Suggested fix is in 5300.patch. tl;dr: introduce an 'active' param to the stack, default true; when 'active', don't fetch notifications from inactive components.

If I can get confirmation from another dev that this is a good way forward, I'll clean up the patch and write tests/docs.

[johnjamesjacoby]

Patch looks fine, but I don't know that I like the approach. Do we have a wrapper function for buddypress()->active_components ? One that includes a filter maybe? We should probably remove the changes from r7677 too?

[johnjamesjacoby]

It might make sense to treat this similar to WP_Query, and have component__in and component__not_in arguments, or even pass a component arg as an array of the components to include, similar to post_type.

[boonebgorges]

Do we have a wrapper function for buddypress()->active_components ?

Doesn't look like it, though it's not a bad idea. Probably not critical for the purposes of this ticket, since the introduction of such a wrapper will require sweeping through the codebase anyway.

We should probably remove the changes from r7677 too?

Yes, good call.

It might make sense to treat this similar to WP_Query, and have componentin and componentnot_in arguments, or even pass a component arg as an array of the components to include, similar to post_type.

The 'component_name' parameter already exists, and accepts arrays/comma-separated lists. So it works like post_type. In the case of active vs inactive components, though, I'm assuming that the vast majority of uses will not want to fetch notifications from inactive components. So I think it's worth having shorthand for it, and I think it's worth having this shorthand fairly far down in the stack, instead of requiring a list of active components to be passed each time notifications are fetched. If this strategy sounds OK, I say we go with the simpler 'active' param for now. If, for 2.0 or beyond, we want to make it more powerful, we can refactor the way that 'active' gets implemented in BP_Notifications_Notification::get() (ie, we could juggle it with component_name or something like that)

[johnjamesjacoby]

I'd rather not introduce a possibly temporary switch to handle something we can already do with a default parameter. I like the idea of having the default component_name value be active components only. That fixes the bug, maintains 1.9 arguments, and mimics the behavior of similar query methods in WordPress core.

I agree that active VS inactive is a fairly clear designation that should be made, but since we can handle it transparently, in a smarter way that doesn't make it incredibly easy to override back to the buggered state, I'd rather we do it in a way that let's us slow roll API additions later.

[johnjamesjacoby]

To be clear, I think we should handle this designation in the base class, and not need to pass active components for every function call. I'm proposing the default query state is to only fetch active components always, forcing developers to override the component_name if they desire different results.

[boonebgorges]

I'm not suggesting that 'active' be *temporary*, but that it be *shorthand* for 'component_name' = [active components]. (Maybe the way that 'active' is implemented deep down would be temporary, but that wouldn't be visible to end users or devs.)

In any case, I do share your inclination not to introduce new params where possible, but in this case, the syntax seemed funny to me. Passing a null value for 'component_name' should, at a glance, return *no* results at all, but we're proposing that it return results for active components. Does this seem misleading? I might be overthinking it.

[johnjamesjacoby]

It seems confusing to have two arguments that can perform similar functions. I think passing null and returning no results is semantically correct, but obviously silly. We can safely assume no one wants to purposely query for no components, and we have a responsibility to provide an error free and intuitive interface for users and developers, so let's not enable what we know to be confusing results.

It makes me wonder what happens if you query for post_type null.

[johnjamesjacoby]

array_keys()

[johnjamesjacoby]

In 7686:

Notifications:

Make buddypress()->active_components the default value for the component_name argument.

This is necessary because notifications are persistent in the database, and active components register callbacks for generating output based on the queried results. If notifications exist for components that are inactive, no callback will be registered, and the output will be empty. By only querying for active components, we can be sure no empty notifications will be displayed to the user.

Reverts r7677. See #5290. Fixes #5300. (trunk)

[johnjamesjacoby]

In 7687:

Notifications:

Make buddypress()->active_components the default value for the component_name argument.

This is necessary because notifications are persistent in the database, and active components register callbacks for generating output based on the queried results. If notifications exist for components that are inactive, no callback will be registered, and the output will be empty. By only querying for active components, we can be sure no empty notifications will be displayed to the user.

Reverts r7677. See #5290. Fixes #5300. (1.9 branch)

[imath]

Hi @jjj, @boone,

First, i'm really sorry and disappointed by myself about #5290, i was so obsessed by the admin bar that i completely missed the notifications panel.

@jjj applying your patch solves the select query, but i think i understand why @boone was modifying the get_where_sql() function. Because, there's also a count query and using r7677, the count is taking in account notifications of inactive components.

so i suggest the diff attached to also restrict the count to active components

[imath]

adding a screen cap to illustrate the count trouble

[boonebgorges]

No more messing around with this until we have unit tests :-D

[boonebgorges]

In 7694:

Unit tests for BP_Notifications_Notification::get_total_count()

As it relates to passing a null value for component_name.

See #5300

[boonebgorges]

I've though about this a bit. My take is that the purpose of get_where_sql() is to translate arguments into sql statements. Our decision to interpret a null 'component_name' as "active components" is really something that should happen at a bit of a higher level, to make tracing easier for developers.

For that reason, I'm going to commit a fix that leaves get_where_sql() the way it is, and instead moves the active-components stuff into BP_Notifications_Notification::get_total_count(). This results in a very small amount of code duplication (between the latter method and get()), but it makes much more sense semantically, because this kind of thing is exactly what wp_parse_args() is meant for.

[boonebgorges]

In 7695:

Adjust notification total_count for null component_value.

This ensures that total counts are correct when BP is assuming we only want
notifications from active components.

Props imath for an initial patch

Fixes #5300

[boonebgorges]

In 7696:

Unit tests for BP_Notifications_Notification::get_total_count()

As it relates to passing a null value for component_name.

See #5300

[boonebgorges]

In 7697:

Adjust notification total_count for null component_value.

This ensures that total counts are correct when BP is assuming we only want
notifications from active components.

Props imath for an initial patch

Fixes #5300

[johnjamesjacoby]

In 7707:

Revert r7695. This is now handled in a lower level Notifications method. See #5300.

[johnjamesjacoby]

In 7709:

Revert r7707, and add inline documentation to BP_Notifications_Notification:get_total_count(). See #5300.

[johnjamesjacoby]

There are still some problems with this, namely third party components that do not register themselves in the active components array.

A few other approaches:

• Add a filter to a function that returns the active components, so actions can be explicitly added to it.
• Use the 'notification_callback for each BP_Component` extension to build an array of components that are currently active and also using the notifications API.
• Some other way that isn't immediately apparent.

I think the second option currently makes the most sense, until we can revisit how code hooks into Notifications in a future version. Will update this ticket with progress.

[johnjamesjacoby]

See: ​http://bbpress.trac.wordpress.org/ticket/2495 as an example.

[johnjamesjacoby]

In 7711:

First pass at extracting registered Notifications callbacks into a helper function. More to do here. See #5300.

[johnjamesjacoby]

In 7712:

First pass at extracting registered Notifications callbacks into a helper function. More to do here. See #5300. (1.9 branch)

[johnjamesjacoby]

In 7713:

In bp_notifications_get_registered_components(), loop through active components looking for notification_callback and only include components that actually touch Notifications. See #5300. (1.9 branch)

[johnjamesjacoby]

In 7714:

Use correct filter name in bp_notifications_get_registered_components(). See #5300. (1.9 branch)

[johnjamesjacoby]

In 7715:

In bp_notifications_get_registered_components(), loop through active components looking for notification_callback and only include components that actually touch Notifications. See #5300. (trunk)

[johnjamesjacoby]

This is fixed, enough for 1.9.1 anyways. I'd like to revisit a better API for hooking into Notifications in a future iteration.