FEATURE: Separate notification indicators for new PMs and reviewables (#19201)

This PR adds separate notification indicators for PMs and reviewables that have arrived since the last time the user opened the notifications menu.

The PM indicator is the strongest one of all three indicators followed by the reviewable indicator and then finally the blue indicator. This means that if there's a new PM and a new reviewable, then the PM indicator will be shown.

Meta topic: https://meta.discourse.org/t/no-green-or-red-notification-bubbles/242783?u=osama.

Internal topic: t/82995.

app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js

@@ -53,6 +53,8 @@ export default {
             read_first_notification: data.read_first_notification,
             all_unread_notifications_count: data.all_unread_notifications_count,
             grouped_unread_notifications: data.grouped_unread_notifications,
+            new_personal_messages_notifications_count:
+              data.new_personal_messages_notifications_count,
           });
 
           if (

app/assets/javascripts/discourse/app/widgets/header.js

@@ -82,25 +82,48 @@ createWidget("header-notifications", {
       contents.push(h("div.do-not-disturb-background", iconNode("moon")));
     } else {
       if (this.currentUser.redesigned_user_menu_enabled) {
-        const unread = user.all_unread_notifications_count || 0;
+        let ringClass = null;
-        const reviewables = user.unseen_reviewable_count || 0;
+        if (user.new_personal_messages_notifications_count) {
-        const count = unread + reviewables;
+          ringClass = "personal-messages";
-        if (count > 0) {
+          contents.push(
-          if (this._shouldHighlightAvatar()) {
+            this.attach("link", {
-            contents.push(h("span.ring"));
+              action: attrs.action,
-          }
+              className: "badge-notification with-icon new-pms",
-
+              icon: "envelope",
+              omitSpan: true,
+              title: "notifications.tooltip.new_message_notification",
+              titleOptions: {
+                count: user.new_personal_messages_notifications_count,
+              },
+            })
+          );
+        } else if (user.unseen_reviewable_count) {
+          contents.push(
+            this.attach("link", {
+              action: attrs.action,
+              className: "badge-notification with-icon new-reviewables",
+              icon: "flag",
+              omitSpan: true,
+              title: "notifications.tooltip.new_reviewable",
+              titleOptions: { count: user.unseen_reviewable_count },
+            })
+          );
+        } else if (user.all_unread_notifications_count) {
+          ringClass = "regular-notifications";
           contents.push(
             this.attach("link", {
               action: attrs.action,
               className: "badge-notification unread-notifications",
-              rawLabel: count,
+              rawLabel: user.all_unread_notifications_count,
               omitSpan: true,
               title: "notifications.tooltip.regular",
-              titleOptions: { count },
+              titleOptions: { count: user.all_unread_notifications_count },
             })
           );
         }
+        if (ringClass && this._shouldHighlightAvatar()) {
+          contents.push(h(`span.ring.revamped.${ringClass}`));
+        }
       } else {
         const unreadNotifications = user.unread_notifications;
         if (!!unreadNotifications) {

app/assets/javascripts/discourse/tests/integration/components/site-header-test.js

@@ -9,6 +9,7 @@ import {
 } from "@ember/test-helpers";
 import { exists, query } from "discourse/tests/helpers/qunit-helpers";
 import { hbs } from "ember-cli-htmlbars";
+import I18n from "I18n";
 
 module("Integration | Component | site-header", function (hooks) {
   setupRenderingTest(hooks);
@@ -18,7 +19,7 @@ module("Integration | Component | site-header", function (hooks) {
     this.currentUser.set("read_first_notification", false);
   });
 
-  test("displaying unread and reviewable notifications count when user's notifications and reviewables count are updated", async function (assert) {
+  test("unread notifications count rerenders when user's notifications count is updated", async function (assert) {
     this.currentUser.set("all_unread_notifications_count", 1);
     this.currentUser.set("redesigned_user_menu_enabled", true);
 
@@ -35,14 +36,6 @@ module("Integration | Component | site-header", function (hooks) {
       ".header-dropdown-toggle.current-user .unread-notifications"
     );
     assert.strictEqual(unreadBadge.textContent, "5");
-
-    this.currentUser.set("unseen_reviewable_count", 3);
-    await settled();
-
-    unreadBadge = query(
-      ".header-dropdown-toggle.current-user .unread-notifications"
-    );
-    assert.strictEqual(unreadBadge.textContent, "8");
   });
 
   test("hamburger menu icon shows pending reviewables count", async function (assert) {
@@ -133,4 +126,121 @@ module("Integration | Component | site-header", function (hooks) {
       "the up arrow key moves the focus in the opposite direction"
     );
   });
+
+  test("new personal messages bubble is prioritized over unseen reviewables and regular notifications bubbles", async function (assert) {
+    this.currentUser.set("redesigned_user_menu_enabled", true);
+    this.currentUser.set("all_unread_notifications_count", 5);
+    this.currentUser.set("new_personal_messages_notifications_count", 2);
+    this.currentUser.set("unseen_reviewable_count", 3);
+
+    await render(hbs`<SiteHeader />`);
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.unread-notifications"
+      ),
+      "regular notifications bubble isn't displayed when there are new personal messages notifications"
+    );
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-reviewables"
+      ),
+      "reviewables bubble isn't displayed when there are new personal messages notifications"
+    );
+
+    const pmsBubble = query(
+      ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-pms"
+    );
+    assert.strictEqual(
+      pmsBubble.textContent.trim(),
+      "",
+      "personal messages bubble has no count"
+    );
+    assert.ok(
+      pmsBubble.querySelector(".d-icon-envelope"),
+      "personal messages bubble has envelope icon"
+    );
+    assert.strictEqual(
+      pmsBubble.title,
+      I18n.t("notifications.tooltip.new_message_notification", { count: 2 }),
+      "personal messages bubble bubble has a title"
+    );
+  });
+
+  test("unseen reviewables bubble is prioritized over regular notifications", async function (assert) {
+    this.currentUser.set("redesigned_user_menu_enabled", true);
+    this.currentUser.set("all_unread_notifications_count", 5);
+    this.currentUser.set("new_personal_messages_notifications_count", 0);
+    this.currentUser.set("unseen_reviewable_count", 3);
+    await render(hbs`<SiteHeader />`);
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.unread-notifications"
+      ),
+      "regular notifications bubble isn't displayed when there are unseen reviewables notifications"
+    );
+
+    const reviewablesBubble = query(
+      ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-reviewables"
+    );
+    assert.strictEqual(
+      reviewablesBubble.textContent.trim(),
+      "",
+      "reviewables bubble has no count"
+    );
+    assert.ok(
+      reviewablesBubble.querySelector(".d-icon-flag"),
+      "reviewables bubble has flag icon"
+    );
+    assert.strictEqual(
+      reviewablesBubble.title,
+      I18n.t("notifications.tooltip.new_reviewable", { count: 3 }),
+      "reviewables bubble has a title"
+    );
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-pms"
+      ),
+      "personal messages bubble isn't displayed"
+    );
+  });
+
+  test("regular notifications bubble is shown if there are neither new personal messages nor unseen reviewables", async function (assert) {
+    this.currentUser.set("redesigned_user_menu_enabled", true);
+    this.currentUser.set("all_unread_notifications_count", 5);
+    this.currentUser.set("new_personal_messages_notifications_count", 0);
+    this.currentUser.set("unseen_reviewable_count", 0);
+    await render(hbs`<SiteHeader />`);
+
+    const regularNotificationsBubble = query(
+      ".header-dropdown-toggle.current-user .badge-notification.unread-notifications"
+    );
+    assert.strictEqual(
+      regularNotificationsBubble.textContent,
+      "5",
+      "regular notifications bubble has a count"
+    );
+    assert.strictEqual(
+      regularNotificationsBubble.title,
+      I18n.t("notifications.tooltip.regular", { count: 5 }),
+      "regular notifications bubble has a title"
+    );
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-reviewables"
+      ),
+      "reviewables bubble isn't displayed"
+    );
+
+    assert.notOk(
+      exists(
+        ".header-dropdown-toggle.current-user .badge-notification.with-icon.new-pms"
+      ),
+      "personal messages bubble isn't displayed"
+    );
+  });
 });

app/assets/stylesheets/common/base/discourse.scss

@@ -486,21 +486,6 @@ table {
   }
 }
 
-.ring {
-  $gradient-start: transparent;
-  $gradient-end: #090;
-  background: radial-gradient($gradient-start, $gradient-end);
-  top: -11px !important;
-  right: 23.5px !important;
-  border-radius: 100%;
-  width: 20px;
-  height: 20px;
-  transform-origin: center;
-  animation-iteration-count: infinite;
-  animation-duration: 3s;
-  animation-name: ping;
-}
-
 .fade {
   opacity: 0;
   transition: opacity 0.15s linear;

app/assets/stylesheets/common/base/header.scss

@@ -141,7 +141,7 @@
       border-top: 1px solid transparent;
       border-left: 1px solid transparent;
       border-right: 1px solid transparent;
-      .d-icon {
+      > .d-icon {
         color: var(--primary-medium);
       }
     }
@@ -158,7 +158,7 @@
       border-top: 1px solid var(--primary-low);
       border-left: 1px solid var(--primary-low);
       border-right: 1px solid var(--primary-low);
-      .d-icon {
+      > .d-icon {
         color: var(--primary-medium);
       }
       &:after {
@@ -185,14 +185,36 @@
     display: inline-block;
     color: var(--header_primary-low-mid);
   }
+
   .notifications {
     position: relative;
   }
   .ring {
     position: absolute;
-    top: -9px;
+    top: -11px;
+    right: 23.5px;
     z-index: z("base");
     margin-left: 0;
+    background: radial-gradient(transparent, var(--success));
+    border-radius: 100%;
+    width: 20px;
+    height: 20px;
+    transform-origin: center;
+    animation-iteration-count: infinite;
+    animation-duration: 3s;
+    animation-name: ping;
+
+    &.revamped {
+      right: -4px;
+      top: -6px;
+
+      &.regular-notifications {
+        background: radial-gradient(
+          transparent,
+          var(--tertiary-med-or-tertiary)
+        );
+      }
+    }
   }
   .header-dropdown-toggle {
     position: relative;
@@ -203,10 +225,24 @@
     left: 0;
     top: -4px;
     min-width: 0.6em;
-  }
-  .unread-notifications {
     left: auto;
     right: -3px;
+
+    &.with-icon {
+      &.new-pms {
+        background-color: var(--success);
+      }
+      &.new-reviewables {
+        background-color: var(--danger);
+      }
+      .d-icon {
+        color: var(--secondary);
+        font-size: var(--font-down-1);
+        width: 1em;
+      }
+    }
+  }
+  .unread-notifications {
     background-color: var(--tertiary-med-or-tertiary);
   }
   .unread-high-priority-notifications,

app/models/user.rb

@@ -597,6 +597,23 @@ class User < ActiveRecord::Base
     @unread_high_prios ||= unread_notifications_of_priority(high_priority: true)
   end
 
+  def new_personal_messages_notifications_count
+    args = {
+      user_id: self.id,
+      seen_notification_id: self.seen_notification_id,
+      private_message: Notification.types[:private_message]
+    }
+
+    DB.query_single(<<~SQL, args).first
+      SELECT COUNT(*)
+      FROM notifications
+      WHERE user_id = :user_id
+      AND id > :seen_notification_id
+      AND NOT read
+      AND notification_type = :private_message
+    SQL
+  end
+
   # PERF: This safeguard is in place to avoid situations where
   # a user with enormous amounts of unread data can issue extremely
   # expensive queries
@@ -775,6 +792,7 @@ class User < ActiveRecord::Base
     if self.redesigned_user_menu_enabled?
       payload[:all_unread_notifications_count] = all_unread_notifications_count
       payload[:grouped_unread_notifications] = grouped_unread_notifications
+      payload[:new_personal_messages_notifications_count] = new_personal_messages_notifications_count
     end
 
     MessageBus.publish("/notification/#{id}", payload, user_ids: [id])

app/serializers/current_user_serializer.rb

@@ -46,6 +46,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :is_anonymous,
              :reviewable_count,
              :unseen_reviewable_count,
+             :new_personal_messages_notifications_count,
              :read_faq?,
              :automatically_unpin_topics,
              :mailing_list_mode,
@@ -374,6 +375,10 @@ class CurrentUserSerializer < BasicUserSerializer
     redesigned_user_menu_enabled
   end
 
+  def include_new_personal_messages_notifications_count?
+    redesigned_user_menu_enabled
+  end
+
   def redesigned_user_page_nav_enabled
     if SiteSetting.enable_new_user_profile_nav_groups.present?
       object.in_any_groups?(SiteSetting.enable_new_user_profile_nav_groups_map)

config/locales/client.en.yml

@@ -2383,6 +2383,12 @@ en:
         high_priority:
           one: "%{count} unread high priority notification"
           other: "%{count} unread high priority notifications"
+        new_message_notification:
+          one: "%{count} new message notification"
+          other: "%{count} new message notifications"
+        new_reviewable:
+          one: "%{count} new reviewable"
+          other: "%{count} new reviewables"
       title: "notifications of @name mentions, replies to your posts and topics, messages, etc"
       none: "Unable to load notifications at this time."
       empty: "No notifications found."

spec/models/user_spec.rb

@@ -2176,6 +2176,7 @@ RSpec.describe User do
         user.update!(admin: true)
         Fabricate(:notification, user: user, notification_type: 1)
         Fabricate(:notification, notification_type: 15, high_priority: true, read: false, user: user)
+        Fabricate(:notification, user: user, notification_type: Notification.types[:private_message], read: false)
 
         messages = MessageBus.track_publish("/notification/#{user.id}") do
           user.publish_notifications_state
@@ -2185,8 +2186,9 @@ RSpec.describe User do
 
         message = messages.first
 
-        expect(message.data[:all_unread_notifications_count]).to eq(2)
+        expect(message.data[:all_unread_notifications_count]).to eq(3)
-        expect(message.data[:grouped_unread_notifications]).to eq({ 1 => 1, 15 => 1 })
+        expect(message.data[:grouped_unread_notifications]).to eq({ 1 => 1, 15 => 1, Notification.types[:private_message] => 1 })
+        expect(message.data[:new_personal_messages_notifications_count]).to eq(1)
       end
     end
   end
@@ -3116,6 +3118,53 @@ RSpec.describe User do
         expect(admin.secure_category_ids).not_to include(private_category.id)
       end
     end
+  end
 
+  describe '#new_personal_messages_notifications_count' do
+    it "returns count of new and unread private_message notifications of the user" do
+      another_user = Fabricate(:user)
+
+      Fabricate(:notification, user: user, read: false)
+
+      last_seen_id = Fabricate(
+        :notification,
+        user: user,
+        read: false,
+        notification_type: Notification.types[:private_message]
+      ).id
+
+      expect(user.new_personal_messages_notifications_count).to eq(1)
+
+      Fabricate(
+        :notification,
+        user: user,
+        read: false,
+        notification_type: Notification.types[:private_message]
+      )
+
+      Fabricate(
+        :notification,
+        user: another_user,
+        read: false,
+        notification_type: Notification.types[:private_message]
+      )
+
+      Fabricate(
+        :notification,
+        user: user,
+        read: true,
+        notification_type: Notification.types[:private_message]
+      )
+
+      Fabricate(
+        :notification,
+        user: user,
+        read: false,
+        notification_type: Notification.types[:replied]
+      )
+
+      user.update!(seen_notification_id: last_seen_id)
+      expect(user.new_personal_messages_notifications_count).to eq(1)
+    end
   end
 end

spec/serializers/current_user_serializer_spec.rb

@@ -390,5 +390,19 @@ RSpec.describe CurrentUserSerializer do
     end
   end
 
+  describe "#new_personal_messages_notifications_count" do
+    fab!(:notification) { Fabricate(:notification, user: user, read: false, notification_type: Notification.types[:private_message]) }
+
+    it "isn't included when enable_experimental_sidebar_hamburger is disabled" do
+      SiteSetting.enable_experimental_sidebar_hamburger = false
+      expect(serializer.as_json[:new_personal_messages_notifications_count]).to be_nil
+    end
+
+    it "is included when enable_experimental_sidebar_hamburger is enabled" do
+      SiteSetting.enable_experimental_sidebar_hamburger = true
+      expect(serializer.as_json[:new_personal_messages_notifications_count]).to eq(1)
+    end
+  end
+
   include_examples "#display_sidebar_tags", described_class
 end