Skip to:
Content

BuddyPress.org

Opened 10 years ago

Last modified 8 years ago

#6179 accepted enhancement

notification count doesn't match on admin bar

Reported by: danbp's profile danbp Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version:
Component: Toolbar & Notifications Keywords: needs-unit-tests needs-testing needs-refresh
Cc: danco38@…, mikebolton.ca@…

Description

BP 2.2 RC2 theme 2013

All is in the title and the joined picture.

To reproduce: mention a user several times (more as once preferably :) )

To solve the issue, i modified the $count in bp_notifications_toolbar_menu function (bp-notifications-adminbar.php:20)

Change

$count = ! empty( $notifications ) ? count( $notifications ) : 0;

to

$count = bp_get_total_mention_count_for_user( bp_loggedin_user_id() );

Attachments (1)

notification_count.jpg (22.4 KB) - added by danbp 10 years ago.
view the issue

Download all attachments as: .zip

Change History (14)

@danbp
10 years ago

view the issue

#1 @danbp
10 years ago

  • Summary changed from notification cound doesn't match on admin bar to notification count doesn't match on admin bar

#2 @johnjamesjacoby
10 years ago

  • Keywords needs-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to 2.2
  • Owner set to johnjamesjacoby
  • Status changed from new to accepted

#3 @DJPaul
10 years ago

Do we know if this is a regression from 2.1?

#4 @imath
10 years ago

Actually, i'm not sure it's a bug. I think we're fetching the number of notifications grouped by type. So if a user has 2 mentions, it can be correct to have one notification of the mention type. Checking this before tonight's dev-chat.

#5 @imath
10 years ago

IMHO, there's no bug see this image :

https://farm8.staticflickr.com/7314/16255789067_9935306cdf_o.png

In the WP Admin Bar, we are grouping notifications by type and it seems pretty constistent to me to have a count set to 3 if i've received 5 notifications concerning 3 different types. 3 == number of lines that are appearing on hover.

If we look at the user's item menus, the notifications count is ok.

#6 @danbp
10 years ago

It's effectively not a bug , but IMHO a count that isn't very usefull, or confusing.

"type" is never mentionned actually to public. It's only counted, then dispatched over the menu. So i don't see the utility of a number beside howdy.
This is a under the hood information, and necessary to get it to work, but...

Instead a count of different things, it could be a simple icon "attention - you received something" and let unchanged the details of what, from, where, on the different usermenu items.

Sorry for this UX pov.

#7 @DJPaul
10 years ago

  • Milestone changed from 2.2 to Future Release
  • Type changed from defect (bug) to enhancement

This has bothered me for a long time.

#8 @johnjamesjacoby
10 years ago

I think you're both right that the code is doing what we've told it to, but it's not a great experience. I agree that it makes the most sense for the digit in the top-level menu to match the total number of new notifications across all components, but I'm not confident it ever behaved the way we've wanted it to.

Thanks Paul for the bump to Future Release.

#9 @DJPaul
9 years ago

  • Keywords needs-refresh added; has-patch removed

#10 @slaFFik
8 years ago

  • Milestone changed from Future Release to 2.8

Facebook etc shows notifications count, not notifications "types" (group) count.
So I would prefer to make the counter to display the total number of notifications, not types.

Any objections?

#11 @danbp
8 years ago

@slaFFik, it's OK for me.
Anyway, too much information kills information. ;-)

#12 @mikeboltonca
8 years ago

  • Cc mikebolton.ca@… added

I have discovered and partially* solved this issue over the past couple of days, and found this ticket after coming up with my solution.
Since then, I tried the proposed change above:

$count = bp_get_total_mention_count_for_user( bp_loggedin_user_id() );

and found that the notification counter would show '0' regardless of how many notifications there were.

In my solution, I changed that line to:

$count = ! empty( $notifications ) ? bp_notifications_get_unread_notification_count( bp_loggedin_user_id() ) : 0;

This works as desired. It shows a count of individual notifications, rather than a count of groups of notifications.

Something for your consideration.


*I say "partially" only because I can't get the modified function to work in bp-custom.php or functions.php; it works flawlessly if I hack the core file bp-notifications-adminbar.php (not an acceptable solution). This is a problem with my level of experience, not the solution.

Last edited 8 years ago by mikeboltonca (previous) (diff)

#13 @DJPaul
8 years ago

  • Milestone changed from 2.8 to Future Release
Note: See TracTickets for help on using tickets.