Marking read notifications as unread in bulk not updating object cache (counts)

[niftythree]

Hello,

Firstly, thank you to everyone who works (and has worked) on BuddyPress. Everyone is doing a fantastic job. Thank you for the mention in your blog post, too! 🙂

This ticket relates to #8637; we've only just noticed this, sorry.

When marking any read notifications as unread in bulk, the badge icon does not update to accurately reflect the change to the number of unread notifications. From what we can tell, this only concerns bulk updating notifications and marking them as unread.

As a side note, if an unread notification is entered into or marked as read (using the Read link next to it), the object cache is cleared and the count is accurately reflected. This includes if it was inaccurate, prior to this action. The rest of the functionality seems to be working fine, including bulk actions in the unread section.

Running 10.1.0 with Legacy pack.

Thanks. 🙂

[oztaser]

Hi @niftythree,

Thanks for the report. The problem is here we don't pass current is_new value when notification is marked as read/unread and so we can't find the notification to be marked for cache invalidation because default value of is_new is true.

Example query when we are trying to mark notification as unread.

SELECT * FROM wp_bp_notifications n   WHERE id IN (32716634) AND component_name IN
('xprofile','messages','activity','blogs','members') AND is_new = 1  

I've prepared the patch but not sure it's good enough.

[imath]

Thanks for your feedback @niftythree and thanks for your patch @oztaser 👍.

I’ll test it asap and I guess you have the right cause. Let’s add some unit tests. To be sure we fixed the issue @niftythree we’ll kindly ask you to test the patch this time 😅. Sorry for this miss.

[imath]

Hi @oztaser

I've added two patches, the first one (​8642.tests.patch) only contains unit tests. If we only apply this one, the test_bp_notifications_clear_all_for_user_cache_before_update_when_marked_unread() one is failing (marking a list of notification ids as unread).

If we apply your patch, it's fixing the issue, but I believe it's best to use the $update_args that is passed into bp_notifications_clear_all_for_user_cache_before_update() to set the $is_new parameter of BP_Notifications_Notification::get(). See ​8642.alt.patch.

What do you think?

[niftythree]

Hello,

No worries. We've tested @oztaser's and @imath's patches on our cloud server's test website running Redis. Both patches work, and the counts update accordingly 🙂.

For your information, we've tested them by taking the changes in each patch and applying it to the specified files (this is our only available method at the moment, especially with Redis/object caching). We hope this is okay.

Thanks again 🙂.

[oztaser]

@imath, I think both patches are good enough to merge but 8642.alt.patch looks like more safer so I'm okay to use 8642.alt.patch. Thanks for the patch and unit tests.

@niftythree, thanks for your tests.

[imath]

Thanks for your feedbacks @oztaser & @niftythree Let's fix this 👍

[imath]

In 13247:

Clear the user notification cache when bulk marking as unread items

Props oztaser, niftythree

See #8642 (trunk)

[imath]

In 13248:

Clear the user notification cache when bulk marking as unread items

Props oztaser, niftythree

Fixes #8642 (branch 10.0)