Skip to:
Content

BuddyPress.org

Opened 2 years ago

Closed 2 years ago

#8637 closed defect (bug) (fixed)

Notifications no longer updating counts with object caching since 10.0

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

Description

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

Attachments (4)

8637.diff​ (4.3 KB) - added by oztaser 2 years ago.
8637.patch​ (15.5 KB) - added by imath 2 years ago.
8637.2.patch​ (19.7 KB) - added by imath 2 years ago.
8637.3.patch​ (19.8 KB) - added by imath 2 years ago.

Download all attachments as: .zip

Change History (18)

#1 @imath
2 years ago

  • Component changed from Core to Toolbar & Notifications
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 10.1.0
  • Priority changed from normal to high

Ouch πŸ˜“ this is really annoying. Let’s try to fix this asap.

#2 @oztaser
2 years ago

Hi there,

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

@oztaser
2 years ago

#3 @imath
2 years ago

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.

#4 @imath
2 years ago

  • Keywords needs-unit-tests added

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

@imath
2 years ago

#5 @imath
2 years ago

  • Keywords has-patch has-unit-tests 2nd-opinion added; needs-patch needs-unit-tests removed

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?

#6 @oztaser
2 years ago

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.

Last edited 2 years ago by oztaser (previous) (diff)

@imath
2 years ago

#7 @imath
2 years ago

  • Keywords needs-testing added

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.

#8 @oztaser
2 years ago

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.

#9 @imath
2 years ago

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 πŸ‘Œ

#10 @oztaser
2 years ago

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

#11 @imath
2 years ago

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.

#12 @imath
2 years ago

  • Keywords commit added; 2nd-opinion needs-testing removed

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.

#13 @imath
2 years ago

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)

#14 @imath
2 years ago

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

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)

Note: See TracTickets for help on using tickets.