Skip to:
Content

BuddyPress.org

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#8642 closed defect (bug) (fixed)

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

Reported by: niftythree's profile niftythree Owned by: imath's profile imath
Milestone: 10.2.0 Priority: normal
Severity: normal Version: 10.1.0
Component: Toolbar & Notifications Keywords: has-patch has-unit-tests commit
Cc:

Description

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. πŸ™‚

Attachments (3)

8642.diff​ (1.6 KB) - added by oztaser 2 years ago.
8642.tests.patch​ (3.2 KB) - added by imath 2 years ago.
8642.alt.patch​ (823 bytes) - added by imath 2 years ago.

Download all attachments as: .zip

Change History (12)

#1 @oztaser
2 years ago

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.

@oztaser
2 years ago

#2 @imath
2 years ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 10.2.0

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
2 years ago

@imath
2 years ago

#3 @imath
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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?

#4 @niftythree
2 years ago

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 πŸ™‚.

#5 @oztaser
2 years ago

@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.

#6 @imath
2 years ago

  • Keywords commit added

Thanks for your feedbacks @oztaser & @niftythree Let's fix this πŸ‘

#7 @imath
2 years ago

In 13247:

Clear the user notification cache when bulk marking as unread items

Props oztaser, niftythree

See #8642 (trunk)

#8 @imath
2 years ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 13248:

Clear the user notification cache when bulk marking as unread items

Props oztaser, niftythree

Fixes #8642 (branch 10.0)

This ticket was mentioned in ​Slack in #buddypress by imath. ​View the logs.


2 years ago

Note: See TracTickets for help on using tickets.