Notifications no longer updating counts with object caching since 10.0

[niftythree]

Hello,

Since the 10.0 update, the count for Notifications does not update when using object caching, such as Redis. It seems to be independent of the plugin used to connect to Redis. Version 9.2.0 did not have this issue and updated accordingly. We are running the Legacy pack.

For example, if a user has three unread notifications, they select all three notifications, and mark them as read through Bulk Actions, they will see the "You have no unread notifications." message, but the badge icon will still show three unread notifications. The same thing happens if a user goes into the notification. If a user individually marks each message as read through the "read" button, then the badge icon accurately reflects the change to the number of unread notifications.

Thanks. 🙂

[imath]

Ouch 😓 this is really annoying. Let’s try to fix this asap.

[oztaser]

Hi there,

I wish to see this before, I've prepared a patch. I am open to any suggestions.

[imath]

Thanks a lot for your patch @oztaser 💪. No worries, we’re only humans, and only the ones who do great things take some risks. I’ll review the patch asap.

[imath]

I'd like to have some unit tests about this. I'm going to work on it.

[imath]

In ​8637.patch I've added 2 unit tests, it's not easy to find them as I've moved cache functions that were in tests/phpunit/testcases/notifications/functions.php into a new file: tests/phpunit/testcases/notifications/cache.php You can look for @ticket BP8637 to find these.

@oztaser I think we do not need to introduce new hooks and functions for this case, we can use existing hooks and functions. I think it's safe to add an ids key to the $where_args array when bulk updating notifications and an id key to the $args array when bulk deleting.

What's your opinion about it?

[oztaser]

Hi @imath,

Thanks for the patch and unit tests.

It looks much simpler than my patch but I think we also need to add support for item_id field on bp_notification_before_update action. We are using item_id field when updating message notifications.

[imath]

Hi @oztaser, you're totally right, thanks a lot for your feedback. I've updated the patch (​8637.2.patch) and added two unit tests for these cases. I believe it's good to go. I'll run some tests on a fresh install to give it a last check.

[oztaser]

Hi again @imath,

I didn't test the latest patch but I think it's not going to work if I'm not missing something. We need to add a new condition to bp_notifications_clear_all_for_user_cache_before_update like update_id_list. Actually that's why I created new action hooks. I didn't want to use two more if statements :).

BTW the rest of the patch looks great to me. Thank you.

[imath]

I understand your concern @oztaser but we don't need another condition if we pass the variable one of the existing ones needs.

For the case you mention, I'm adding this part:
$where_args['user_id'] = $where_args['data']['user_id'];

which ends up into the first condition of bp_notifications_clear_all_for_user_cache_before_update().

But I'll run some new tests to be 100% sure 👌

[oztaser]

I got it but shouldn't we consider the situation any user has custom code and doesn't send any user_id? I believe user_id is not required but on the other hand maybe I am overthinking.

[imath]

It's required in bp_notifications_mark_notifications_by_item_ids( $user_id, $item_ids, $component_name..., but not in the class' method I agree.

.3.patch is a compromise 😁
I'd say for a minor release, let's fix what BuddyPress needs first.

If some plugins are not using a user id, they'll need to handle the cache for now.

[imath]

I’m going to commit the 3rd patch to fix the issue in 10.1.0 but we’ll carry on working on improving the fix during 11.0.0. I can imagine situations where there could be more than one user id for the corresponding items or situations where a component action would like to update all users notifications at once.

[imath]

In 13243:

Clear the notifications user cache when bulk updating/deleting items

This commit also groups PHP Unit tests about notifications cache functions into a specific file.

Props niftythree, oztaser.

See #8637 (trunk)

[imath]

In 13244:

Clear the notifications user cache when bulk updating/deleting items

This commit also groups PHP Unit tests about notifications cache functions into a specific file.

Props niftythree, oztaser.

Fixes #8637 (branch 10.0)