DEV: Combine all header notification bubbles into one in the new user menu (#17718)

Extracted from https://github.com/discourse/discourse/pull/17379.

app/assets/javascripts/discourse/app/components/reviewable-item.js

@@ -156,6 +156,12 @@ export default Component.extend({
               performResult.reviewable_count
             );
           }
+          if (performResult.unseen_reviewable_count !== undefined) {
+            this.currentUser.set(
+              "unseen_reviewable_count",
+              performResult.unseen_reviewable_count
+            );
+          }
 
           if (this.attrs.remove) {
             this.attrs.remove(performResult.remove_reviewable_ids);

app/assets/javascripts/discourse/app/components/site-header.js

@@ -30,7 +30,9 @@ const SiteHeaderComponent = MountWidget.extend(
     @observes(
       "currentUser.unread_notifications",
       "currentUser.unread_high_priority_notifications",
-      "currentUser.reviewable_count",
+      "currentUser.all_unread_notifications_count",
+      "currentUser.reviewable_count", // TODO: remove this when redesigned_user_menu_enabled is removed
+      "currentUser.unseen_reviewable_count",
       "session.defaultColorSchemeIsDark",
       "session.darkModeAvailable"
     )

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

@@ -22,38 +22,49 @@ export default {
     const user = container.lookup("service:current-user");
     const bus = container.lookup("service:message-bus");
     const appEvents = container.lookup("service:app-events");
+    const siteSettings = container.lookup("service:site-settings");
 
     if (user) {
-      bus.subscribe("/reviewable_counts", (data) => {
+      const channel = user.enable_redesigned_user_menu
-        user.set("reviewable_count", data.reviewable_count);
+        ? `/reviewable_counts/${user.id}`
+        : "/reviewable_counts";
+      bus.subscribe(channel, (data) => {
+        if (data.reviewable_count >= 0) {
+          user.set("reviewable_count", data.reviewable_count);
+        }
+        if (user.redesigned_user_menu_enabled) {
+          user.set("unseen_reviewable_count", data.unseen_reviewable_count);
+        }
       });
 
       bus.subscribe(
         `/notification/${user.get("id")}`,
         (data) => {
           const store = container.lookup("service:store");
-          const oldUnread = user.get("unread_notifications");
+          const oldUnread = user.unread_notifications;
-          const oldHighPriority = user.get(
+          const oldHighPriority = user.unread_high_priority_notifications;
-            "unread_high_priority_notifications"
+          const oldAllUnread = user.all_unread_notifications_count;
-          );
 
           user.setProperties({
             unread_notifications: data.unread_notifications,
             unread_high_priority_notifications:
               data.unread_high_priority_notifications,
             read_first_notification: data.read_first_notification,
+            all_unread_notifications_count: data.all_unread_notifications_count,
           });
 
           if (
             oldUnread !== data.unread_notifications ||
-            oldHighPriority !== data.unread_high_priority_notifications
+            oldHighPriority !== data.unread_high_priority_notifications ||
+            oldAllUnread !== data.all_unread_notifications_count
           ) {
             appEvents.trigger("notifications:changed");
 
             if (
               site.mobileView &&
               (data.unread_notifications - oldUnread > 0 ||
-                data.unread_high_priority_notifications - oldHighPriority > 0)
+                data.unread_high_priority_notifications - oldHighPriority > 0 ||
+                data.all_unread_notifications_count - oldAllUnread > 0)
             ) {
               appEvents.trigger("header:update-topic", null, 5000);
             }
@@ -74,9 +85,9 @@ export default {
             );
 
             if (staleIndex === -1) {
-              // high priority and unread notifications are first
               let insertPosition = 0;
 
+              // high priority and unread notifications are first
               if (!lastNotification.high_priority || lastNotification.read) {
                 const nextPosition = oldNotifications.findIndex(
                   (n) => !n.high_priority || n.read
@@ -122,7 +133,6 @@ export default {
       });
 
       const site = container.lookup("site:main");
-      const siteSettings = container.lookup("service:site-settings");
       const router = container.lookup("router:main");
 
       bus.subscribe("/categories", (data) => {

app/assets/javascripts/discourse/app/routes/review-index.js

@@ -22,6 +22,12 @@ export default DiscourseRoute.extend({
     if (meta.reviewable_count !== undefined) {
       this.currentUser.set("reviewable_count", meta.reviewable_count);
     }
+    if (meta.unseen_reviewable_count !== undefined) {
+      this.currentUser.set(
+        "unseen_reviewable_count",
+        meta.unseen_reviewable_count
+      );
+    }
 
     controller.setProperties({
       reviewables: model,
@@ -59,7 +65,10 @@ export default DiscourseRoute.extend({
       }
     });
 
-    this.messageBus.subscribe("/reviewable_counts", (data) => {
+    const channel = this.currentUser.enable_redesigned_user_menu
+      ? `/reviewable_counts/${this.currentUser.id}`
+      : "/reviewable_counts";
+    this.messageBus.subscribe(channel, (data) => {
       if (data.updates) {
         this.controller.reviewables.forEach((reviewable) => {
           const updates = data.updates[reviewable.id];

app/assets/javascripts/discourse/app/widgets/hamburger-menu.js

@@ -355,7 +355,11 @@ export default createWidget("hamburger-menu", {
   },
 
   html(attrs, state) {
-    if (!state.loaded) {
+    if (
+      this.currentUser &&
+      !this.currentUser.redesigned_user_menu_enabled &&
+      !state.loaded
+    ) {
       this.refreshReviewableCount(state);
     }
 

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

@@ -77,78 +77,109 @@ createWidget("header-notifications", {
     if (user.isInDoNotDisturb()) {
       contents.push(h("div.do-not-disturb-background", iconNode("moon")));
     } else {
-      const unreadNotifications = user.get("unread_notifications");
+      if (this.currentUser.redesigned_user_menu_enabled) {
-      if (!!unreadNotifications) {
+        const unread = user.all_unread_notifications_count || 0;
-        contents.push(
+        const reviewables = user.unseen_reviewable_count || 0;
-          this.attach("link", {
+        const count = unread + reviewables;
-            action: attrs.action,
+        if (count > 0) {
-            className: "badge-notification unread-notifications",
+          if (this._shouldHighlightAvatar()) {
-            rawLabel: unreadNotifications,
+            this._addAvatarHighlight(contents);
-            omitSpan: true,
-            title: "notifications.tooltip.regular",
-            titleOptions: { count: unreadNotifications },
-          })
-        );
-      }
-
-      const unreadHighPriority = user.get("unread_high_priority_notifications");
-      if (!!unreadHighPriority) {
-        // highlight the avatar if the first ever PM is not read
-        if (
-          !user.get("read_first_notification") &&
-          !user.get("enforcedSecondFactor")
-        ) {
-          if (!attrs.active && attrs.ringBackdrop) {
-            contents.push(h("span.ring"));
-            contents.push(h("span.ring-backdrop-spotlight"));
-            contents.push(
-              h(
-                "span.ring-backdrop",
-                {},
-                h("h1.ring-first-notification", {}, [
-                  h(
-                    "span",
-                    { className: "first-notification" },
-                    I18n.t("user.first_notification")
-                  ),
-                  h("span", { className: "read-later" }, [
-                    this.attach("link", {
-                      action: "readLater",
-                      className: "read-later-link",
-                      label: "user.skip_new_user_tips.read_later",
-                    }),
-                  ]),
-                  h("span", {}, [
-                    I18n.t("user.skip_new_user_tips.not_first_time"),
-                    " ",
-                    this.attach("link", {
-                      action: "skipNewUserTips",
-                      className: "skip-new-user-tips",
-                      label: "user.skip_new_user_tips.skip_link",
-                      title: "user.skip_new_user_tips.description",
-                    }),
-                  ]),
-                ])
-              )
-            );
           }
+          contents.push(
+            this.attach("link", {
+              action: attrs.action,
+              className: "badge-notification unread-notifications",
+              rawLabel: count,
+              omitSpan: true,
+              title: "notifications.tooltip.regular",
+              titleOptions: { count },
+            })
+          );
+        }
+      } else {
+        const unreadNotifications = user.unread_notifications;
+        if (!!unreadNotifications) {
+          contents.push(
+            this.attach("link", {
+              action: attrs.action,
+              className: "badge-notification unread-notifications",
+              rawLabel: unreadNotifications,
+              omitSpan: true,
+              title: "notifications.tooltip.regular",
+              titleOptions: { count: unreadNotifications },
+            })
+          );
         }
 
-        // add the counter for the unread high priority
+        const unreadHighPriority = user.unread_high_priority_notifications;
-        contents.push(
+        if (!!unreadHighPriority) {
-          this.attach("link", {
+          if (this._shouldHighlightAvatar()) {
-            action: attrs.action,
+            this._addAvatarHighlight(contents);
-            className: "badge-notification unread-high-priority-notifications",
+          }
-            rawLabel: unreadHighPriority,
+
-            omitSpan: true,
+          // add the counter for the unread high priority
-            title: "notifications.tooltip.high_priority",
+          contents.push(
-            titleOptions: { count: unreadHighPriority },
+            this.attach("link", {
-          })
+              action: attrs.action,
-        );
+              className:
+                "badge-notification unread-high-priority-notifications",
+              rawLabel: unreadHighPriority,
+              omitSpan: true,
+              title: "notifications.tooltip.high_priority",
+              titleOptions: { count: unreadHighPriority },
+            })
+          );
+        }
       }
     }
     return contents;
   },
+
+  _shouldHighlightAvatar() {
+    const attrs = this.attrs;
+    const { user } = attrs;
+    return (
+      !user.read_first_notification &&
+      !user.enforcedSecondFactor &&
+      !attrs.active &&
+      attrs.ringBackdrop
+    );
+  },
+
+  _addAvatarHighlight(contents) {
+    contents.push(h("span.ring"));
+    contents.push(h("span.ring-backdrop-spotlight"));
+    contents.push(
+      h(
+        "span.ring-backdrop",
+        {},
+        h("h1.ring-first-notification", {}, [
+          h(
+            "span",
+            { className: "first-notification" },
+            I18n.t("user.first_notification")
+          ),
+          h("span", { className: "read-later" }, [
+            this.attach("link", {
+              action: "readLater",
+              className: "read-later-link",
+              label: "user.skip_new_user_tips.read_later",
+            }),
+          ]),
+          h("span", {}, [
+            I18n.t("user.skip_new_user_tips.not_first_time"),
+            " ",
+            this.attach("link", {
+              action: "skipNewUserTips",
+              className: "skip-new-user-tips",
+              label: "user.skip_new_user_tips.skip_link",
+              title: "user.skip_new_user_tips.description",
+            }),
+          ]),
+        ])
+      )
+    );
+  },
 });
 
 createWidget(
@@ -255,7 +286,10 @@ createWidget("header-icons", {
 
       contents() {
         let { currentUser } = this;
-        if (currentUser && currentUser.reviewable_count) {
+        if (
+          currentUser?.reviewable_count &&
+          !this.currentUser.redesigned_user_menu_enabled
+        ) {
           return h(
             "div.badge-notification.reviewables",
             {

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

@@ -1,6 +1,6 @@
 import { module, test } from "qunit";
 import { setupRenderingTest } from "discourse/tests/helpers/component-test";
-import { click, render, waitUntil } from "@ember/test-helpers";
+import { click, render, settled, waitUntil } from "@ember/test-helpers";
 import { count, exists, query } from "discourse/tests/helpers/qunit-helpers";
 import pretender, { response } from "discourse/tests/helpers/create-pretender";
 import { hbs } from "ember-cli-htmlbars";
@@ -50,8 +50,35 @@ module("Integration | Component | site-header", function (hooks) {
     await click("header.d-header");
   });
 
+  test("displaying unread and reviewable notifications count when user's notifications and reviewables count are updated", async function (assert) {
+    this.currentUser.set("all_unread_notifications_count", 1);
+    this.currentUser.set("redesigned_user_menu_enabled", true);
+
+    await render(hbs`<SiteHeader />`);
+    let unreadBadge = query(
+      ".header-dropdown-toggle.current-user .unread-notifications"
+    );
+    assert.strictEqual(unreadBadge.textContent, "1");
+
+    this.currentUser.set("all_unread_notifications_count", 5);
+    await settled();
+
+    unreadBadge = query(
+      ".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("user avatar is highlighted when the user receives the first notification", async function (assert) {
-    this.currentUser.set("all_unread_notifications", 1);
+    this.currentUser.set("all_unread_notifications_count", 1);
     this.currentUser.set("redesigned_user_menu_enabled", true);
     this.currentUser.set("read_first_notification", false);
     await render(hbs`<SiteHeader />`);
@@ -60,7 +87,7 @@ module("Integration | Component | site-header", function (hooks) {
 
   test("user avatar is not highlighted when the user receives notifications beyond the first one", async function (assert) {
     this.currentUser.set("redesigned_user_menu_enabled", true);
-    this.currentUser.set("all_unread_notifications", 1);
+    this.currentUser.set("all_unread_notifications_count", 1);
     this.currentUser.set("read_first_notification", true);
     await render(hbs`<SiteHeader />`);
     assert.ok(!exists(".ring-first-notification"));
@@ -75,6 +102,13 @@ module("Integration | Component | site-header", function (hooks) {
     assert.strictEqual(pendingReviewablesBadge.textContent, "1");
   });
 
+  test("hamburger menu icon doesn't show pending reviewables count when revamped user menu is enabled", async function (assert) {
+    this.currentUser.set("reviewable_count", 1);
+    this.currentUser.set("redesigned_user_menu_enabled", true);
+    await render(hbs`<SiteHeader />`);
+    assert.ok(!exists(".hamburger-dropdown .badge-notification"));
+  });
+
   test("clicking outside the revamped menu closes it", async function (assert) {
     this.currentUser.set("redesigned_user_menu_enabled", true);
     await render(hbs`<SiteHeader />`);

app/controllers/notifications_controller.rb

@@ -44,6 +44,22 @@ class NotificationsController < ApplicationController
         current_user.publish_notifications_state
       end
 
+      if !params.has_key?(:silent) && params[:bump_last_seen_reviewable] && !@readonly_mode
+        current_user_id = current_user.id
+        Scheduler::Defer.later "bump last seen reviewable for user" do
+          # we lookup current_user again in the background thread to avoid
+          # concurrency issues where the objects returned by the current_user
+          # and/or methods are changed by the time the deferred block is
+          # executed
+          user = User.find_by(id: current_user_id)
+          next if user.blank?
+          new_guardian = Guardian.new(user)
+          if new_guardian.can_see_review_queue?
+            user.bump_last_seen_reviewable!
+          end
+        end
+      end
+
       render_json_dump(
         notifications: serialize_data(notifications, NotificationSerializer),
         seen_notification_id: current_user.seen_notification_id

app/controllers/reviewables_controller.rb

@@ -58,7 +58,8 @@ class ReviewablesController < ApplicationController
       end,
       meta: filters.merge(
         total_rows_reviewables: total_rows, types: meta_types, reviewable_types: Reviewable.types,
-        reviewable_count: current_user.reviewable_count
+        reviewable_count: current_user.reviewable_count,
+        unseen_reviewable_count: current_user.unseen_reviewable_count
       )
     }
     if (offset + PER_PAGE) < total_rows

app/jobs/regular/notify_reviewable.rb

@@ -1,20 +1,13 @@
 # frozen_string_literal: true
 
 class Jobs::NotifyReviewable < ::Jobs::Base
-
+  # remove all the legacy stuff here when redesigned_user_menu_enabled is
+  # removed
   def execute(args)
     return unless reviewable = Reviewable.find_by(id: args[:reviewable_id])
 
     @contacted = Set.new
 
-    counts = Hash.new(0)
-
-    Reviewable.default_visible.pending.each do |r|
-      counts[:admins] += 1
-      counts[:moderators] += 1 if r.reviewable_by_moderator?
-      counts[r.reviewable_by_group_id] += 1 if r.reviewable_by_group_id
-    end
-
     all_updates = Hash.new { |h, k| h[k] = {} }
 
     if args[:updated_reviewable_ids].present?
@@ -30,41 +23,63 @@ class Jobs::NotifyReviewable < ::Jobs::Base
       end
     end
 
-    # admins
+    counts = Hash.new(0)
-    notify(
+
-      User.real.admins.pluck(:id),
+    Reviewable.default_visible.pending.each do |r|
+      counts[:admins] += 1
+      counts[:moderators] += 1 if r.reviewable_by_moderator?
+      counts[r.reviewable_by_group_id] += 1 if r.reviewable_by_group_id
+    end
+
+    redesigned_menu_enabled_user_ids = User.redesigned_user_menu_enabled_user_ids
+
+    new_menu_admins = User.real.admins.where(id: redesigned_menu_enabled_user_ids)
+    notify_users(new_menu_admins, all_updates[:admins])
+
+    legacy_menu_admins = User.real.admins.where("id NOT IN (?)", @contacted).pluck(:id)
+    notify_legacy(
+      legacy_menu_admins,
       count: counts[:admins],
       updates: all_updates[:admins],
     )
 
-    # moderators
     if reviewable.reviewable_by_moderator?
-      notify(
+      new_menu_mods = User
-        User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id),
+        .real
+        .moderators
+        .where("id IN (?)", redesigned_menu_enabled_user_ids - @contacted.to_a)
+      notify_users(new_menu_mods, all_updates[:moderators])
+
+      legacy_menu_mods = User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id)
+      notify_legacy(
+        legacy_menu_mods,
         count: counts[:moderators],
         updates: all_updates[:moderators],
       )
     end
 
-    # category moderators
     if SiteSetting.enable_category_group_moderation? && (group = reviewable.reviewable_by_group)
-      group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted).find_each do |user|
+      users = group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted)
+      users.find_each do |user|
         count = 0
         updates = {}
-
         user.group_users.each do |gu|
-          count += counts[gu.group_id] || 0
+          updates.merge!(all_updates[gu.group_id])
-          updates.merge!(all_updates[gu.group_id] || {})
+          count += counts[gu.group_id]
+        end
+        if redesigned_menu_enabled_user_ids.include?(user.id)
+          notify_user(user, updates)
+        else
+          notify_legacy([user.id], count: count, updates: updates)
         end
-
-        notify([user.id], count: count, updates: updates)
       end
+      @contacted += users.pluck(:id)
     end
   end
 
   protected
 
-  def notify(user_ids, count:, updates:)
+  def notify_legacy(user_ids, count:, updates:)
     return if user_ids.blank?
 
     data = { reviewable_count: count }
@@ -74,4 +89,18 @@ class Jobs::NotifyReviewable < ::Jobs::Base
     @contacted += user_ids
   end
 
+  def notify_users(users, updates)
+    users.find_each { |user| notify_user(user, updates) }
+    @contacted += users.pluck(:id)
+  end
+
+  def notify_user(user, updates)
+    data = {
+      reviewable_count: user.reviewable_count,
+      unseen_reviewable_count: user.unseen_reviewable_count
+    }
+    data[:updates] = updates if updates.present?
+
+    user.publish_reviewable_counts(data)
+  end
 end

app/models/reviewable.rb

@@ -541,6 +541,17 @@ class Reviewable < ActiveRecord::Base
     result
   end
 
+  def self.unseen_list_for(user, preload: true, limit: nil)
+    results = list_for(user, preload: preload, limit: limit)
+    if user.last_seen_reviewable_id
+      results = results.where(
+        "reviewables.id > ?",
+        user.last_seen_reviewable_id
+      )
+    end
+    results
+  end
+
   def self.recent_list_with_pending_first(user, limit: 30)
     min_score = Reviewable.min_score_for_priority
 

app/models/user.rb

@@ -487,6 +487,7 @@ class User < ActiveRecord::Base
 
   def reload
     @unread_notifications = nil
+    @all_unread_notifications_count = nil
     @unread_total_notifications = nil
     @unread_pms = nil
     @unread_bookmarks = nil
@@ -587,6 +588,29 @@ class User < ActiveRecord::Base
     end
   end
 
+  def all_unread_notifications_count
+    @all_unread_notifications_count ||= begin
+      sql = <<~SQL
+        SELECT COUNT(*) FROM (
+          SELECT 1 FROM
+          notifications n
+          LEFT JOIN topics t ON t.id = n.topic_id
+           WHERE t.deleted_at IS NULL AND
+            n.user_id = :user_id AND
+            n.id > :seen_notification_id AND
+            NOT read
+          LIMIT :limit
+        ) AS X
+      SQL
+
+      DB.query_single(sql,
+        user_id: id,
+        seen_notification_id: seen_notification_id,
+        limit: User.max_unread_notifications
+      )[0].to_i
+    end
+  end
+
   def total_unread_notifications
     @unread_total_notifications ||= notifications.where("read = false").count
   end
@@ -595,6 +619,10 @@ class User < ActiveRecord::Base
     Reviewable.list_for(self).count
   end
 
+  def unseen_reviewable_count
+    Reviewable.unseen_list_for(self).count
+  end
+
   def saw_notification_id(notification_id)
     if seen_notification_id.to_i < notification_id.to_i
       update_columns(seen_notification_id: notification_id.to_i)
@@ -604,6 +632,29 @@ class User < ActiveRecord::Base
     end
   end
 
+  def bump_last_seen_reviewable!
+    query = Reviewable.unseen_list_for(self, preload: false)
+
+    if last_seen_reviewable_id
+      query = query.where("id > ?", last_seen_reviewable_id)
+    end
+    max_reviewable_id = query.maximum(:id)
+
+    if max_reviewable_id
+      update!(last_seen_reviewable_id: max_reviewable_id)
+      publish_reviewable_counts(unseen_reviewable_count: self.unseen_reviewable_count)
+      MessageBus.publish(
+        "/reviewable_counts",
+        { unseen_reviewable_count: self.unseen_reviewable_count },
+        user_ids: [self.id]
+      )
+    end
+  end
+
+  def publish_reviewable_counts(data)
+    MessageBus.publish("/reviewable_counts/#{self.id}", data, user_ids: [self.id])
+  end
+
   TRACK_FIRST_NOTIFICATION_READ_DURATION = 1.week.to_i
 
   def read_first_notification?
@@ -663,6 +714,10 @@ class User < ActiveRecord::Base
       seen_notification_id: seen_notification_id,
     }
 
+    if self.redesigned_user_menu_enabled?
+      payload[:all_unread_notifications_count] = all_unread_notifications_count
+    end
+
     MessageBus.publish("/notification/#{id}", payload, user_ids: [id])
   end
 

app/serializers/current_user_serializer.rb

@@ -7,6 +7,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :unread_notifications,
              :unread_private_messages,
              :unread_high_priority_notifications,
+             :all_unread_notifications_count,
              :read_first_notification?,
              :admin?,
              :notification_channel_position,
@@ -43,6 +44,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :dismissed_banner_key,
              :is_anonymous,
              :reviewable_count,
+             :unseen_reviewable_count,
              :read_faq?,
              :automatically_unpin_topics,
              :mailing_list_mode,
@@ -338,4 +340,12 @@ class CurrentUserSerializer < BasicUserSerializer
   def likes_notifications_disabled
     object.user_option&.likes_notifications_disabled?
   end
+
+  def include_all_unread_notifications_count?
+    redesigned_user_menu_enabled
+  end
+
+  def include_unseen_reviewable_count?
+    redesigned_user_menu_enabled
+  end
 end

app/serializers/reviewable_perform_result_serializer.rb

@@ -10,7 +10,8 @@ class ReviewablePerformResultSerializer < ApplicationSerializer
     :created_post_topic_id,
     :remove_reviewable_ids,
     :version,
-    :reviewable_count
+    :reviewable_count,
+    :unseen_reviewable_count
   )
 
   def success
@@ -42,6 +43,14 @@ class ReviewablePerformResultSerializer < ApplicationSerializer
   end
 
   def reviewable_count
-    Reviewable.list_for(scope.user).count
+    scope.user.reviewable_count
+  end
+
+  def unseen_reviewable_count
+    scope.user.unseen_reviewable_count
+  end
+
+  def include_unseen_reviewable_count?
+    scope.user.redesigned_user_menu_enabled?
   end
 end

spec/jobs/notify_reviewable_spec.rb

@@ -1,102 +1,174 @@
 # frozen_string_literal: true
 
 RSpec.describe Jobs::NotifyReviewable do
-  describe '.execute' do
+  # remove all the legacy stuff here when redesigned_user_menu_enabled is
-    fab!(:admin) { Fabricate(:admin, moderator: true) }
+  # removed
-    fab!(:moderator) { Fabricate(:moderator) }
+  describe '#execute' do
+    fab!(:legacy_menu_admin) { Fabricate(:admin, moderator: true) }
+    fab!(:legacy_menu_mod) { Fabricate(:moderator) }
     fab!(:group_user) { Fabricate(:group_user) }
-    let(:user) { group_user.user }
+    fab!(:legacy_menu_user) { group_user.user }
-    let(:group) { group_user.group }
 
-    it "will notify users of new reviewable content" do
+    fab!(:group) { group_user.group }
+
+    fab!(:new_menu_admin) { Fabricate(:admin, moderator: true) }
+    fab!(:new_menu_mod) { Fabricate(:moderator) }
+    fab!(:new_menu_user) { Fabricate(:user, groups: [group]) }
+
+    before do
+      [new_menu_admin, new_menu_mod, new_menu_user].each(&:enable_redesigned_user_menu)
+    end
+
+    after do
+      [new_menu_admin, new_menu_mod, new_menu_user].each(&:disable_redesigned_user_menu)
+    end
+
+    it "will notify users of new reviewable content and respects backward compatibility for the legacy user menu" do
       SiteSetting.enable_category_group_moderation = true
 
-      GroupUser.create!(group_id: group.id, user_id: moderator.id)
+      GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id)
 
       # Content for admins only
-      r1 = Fabricate(:reviewable, reviewable_by_moderator: false)
+      admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false)
-      messages = MessageBus.track_publish("/reviewable_counts") do
+      new_menu_admin.update!(last_seen_reviewable_id: admin_reviewable.id)
-        described_class.new.execute(reviewable_id: r1.id)
+      messages = MessageBus.track_publish do
+        described_class.new.execute(reviewable_id: admin_reviewable.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      expect(messages.size).to eq(2)
-      expect(admin_msg.data[:reviewable_count]).to eq(1)
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) }
-      expect(messages.any? { |m| m.user_ids.include?(moderator.id) }).to eq(false)
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(1)
-      expect(messages.any? { |m| m.user_ids.include?(user.id) }).to eq(false)
+      expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts")
+      expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false)
+
+      new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] }
+      expect(new_menu_admin_msg.data[:reviewable_count]).to eq(1)
+      expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}")
+      expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(0)
+
+      expect(messages.any? { |m| m.user_ids.include?(legacy_menu_mod.id) }).to eq(false)
+      expect(messages.any? { |m| m.user_ids.include?(legacy_menu_user.id) }).to eq(false)
+      expect(messages.any? { |m| m.user_ids.include?(new_menu_mod.id) }).to eq(false)
+      expect(messages.any? { |m| m.user_ids.include?(new_menu_user.id) }).to eq(false)
 
       # Content for moderators
-      r2 = Fabricate(:reviewable, reviewable_by_moderator: true)
+      mod_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true)
-      messages = MessageBus.track_publish("/reviewable_counts") do
+      messages = MessageBus.track_publish do
-        described_class.new.execute(reviewable_id: r2.id)
+        described_class.new.execute(reviewable_id: mod_reviewable.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      expect(messages.size).to eq(4)
-      expect(admin_msg.data[:reviewable_count]).to eq(2)
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids == [legacy_menu_admin.id] }
-      mod_msg = messages.find { |m| m.user_ids.include?(moderator.id) }
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(2)
-      expect(mod_msg.data[:reviewable_count]).to eq(1)
+      expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts")
-      expect(mod_msg.user_ids).to_not include(admin.id)
+      expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false)
-      expect(messages.any? { |m| m.user_ids.include?(user.id) }).to eq(false)
+
+      new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] }
+      expect(new_menu_admin_msg.data[:reviewable_count]).to eq(2)
+      expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}")
+      expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(1)
+
+      legacy_menu_mod_msg = messages.find { |m| m.user_ids == [legacy_menu_mod.id] }
+      expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(1)
+      expect(legacy_menu_mod_msg.channel).to eq("/reviewable_counts")
+      expect(legacy_menu_mod_msg.data.key?(:unseen_reviewable_count)).to eq(false)
+
+      new_menu_mod_msg = messages.find { |m| m.user_ids == [new_menu_mod.id] }
+      expect(new_menu_mod_msg.data[:reviewable_count]).to eq(1)
+      expect(new_menu_mod_msg.channel).to eq("/reviewable_counts/#{new_menu_mod.id}")
+      expect(new_menu_mod_msg.data[:unseen_reviewable_count]).to eq(1)
+
+      expect(messages.any? { |m| m.user_ids.include?(legacy_menu_user.id) }).to eq(false)
+      expect(messages.any? { |m| m.user_ids.include?(new_menu_user.id) }).to eq(false)
+
+      new_menu_mod.update!(last_seen_reviewable_id: mod_reviewable.id)
 
       # Content for a group
-      r3 = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group)
+      group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group)
-      messages = MessageBus.track_publish("/reviewable_counts") do
+      messages = MessageBus.track_publish do
-        described_class.new.execute(reviewable_id: r3.id)
+        described_class.new.execute(reviewable_id: group_reviewable.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      expect(messages.size).to eq(6)
-      expect(admin_msg.data[:reviewable_count]).to eq(3)
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids == [legacy_menu_admin.id] }
-      mod_messages = messages.select { |m| m.user_ids.include?(moderator.id) }
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(3)
-      expect(mod_messages.size).to eq(1)
+      expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts")
-      expect(mod_messages[0].data[:reviewable_count]).to eq(2)
+      expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false)
-      group_msg = messages.find { |m| m.user_ids.include?(user.id) }
+
-      expect(group_msg.data[:reviewable_count]).to eq(1)
+      new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] }
+      expect(new_menu_admin_msg.data[:reviewable_count]).to eq(3)
+      expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}")
+      expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(2)
+
+      legacy_menu_mod_msg = messages.find { |m| m.user_ids == [legacy_menu_mod.id] }
+      expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(2)
+      expect(legacy_menu_mod_msg.channel).to eq("/reviewable_counts")
+      expect(legacy_menu_mod_msg.data.key?(:unseen_reviewable_count)).to eq(false)
+
+      new_menu_mod_msg = messages.find { |m| m.user_ids == [new_menu_mod.id] }
+      expect(new_menu_mod_msg.data[:reviewable_count]).to eq(2)
+      expect(new_menu_mod_msg.channel).to eq("/reviewable_counts/#{new_menu_mod.id}")
+      expect(new_menu_mod_msg.data[:unseen_reviewable_count]).to eq(1)
+
+      legacy_menu_user_msg = messages.find { |m| m.user_ids == [legacy_menu_user.id] }
+      expect(legacy_menu_user_msg.data[:reviewable_count]).to eq(1)
+      expect(legacy_menu_user_msg.channel).to eq("/reviewable_counts")
+      expect(legacy_menu_user_msg.data.key?(:unseen_reviewable_count)).to eq(false)
+
+      new_menu_user_msg = messages.find { |m| m.user_ids == [new_menu_user.id] }
+      expect(new_menu_user_msg.data[:reviewable_count]).to eq(1)
+      expect(new_menu_user_msg.channel).to eq("/reviewable_counts/#{new_menu_user.id}")
+      expect(new_menu_user_msg.data[:unseen_reviewable_count]).to eq(1)
     end
 
     it "won't notify a group when disabled" do
       SiteSetting.enable_category_group_moderation = false
 
-      GroupUser.create!(group_id: group.id, user_id: moderator.id)
+      GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id)
+      GroupUser.create!(group_id: group.id, user_id: new_menu_mod.id)
       r3 = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group)
       messages = MessageBus.track_publish("/reviewable_counts") do
         described_class.new.execute(reviewable_id: r3.id)
       end
-      group_msg = messages.find { |m| m.user_ids.include?(user.id) }
+      group_msg = messages.find { |m| m.user_ids.include?(legacy_menu_user.id) }
+      expect(group_msg).to be_blank
+      group_msg = messages.find { |m| m.user_ids.include?(new_menu_user.id) }
       expect(group_msg).to be_blank
     end
 
-    it "respects visibility" do
+    it "respects priority" do
       SiteSetting.enable_category_group_moderation = true
       Reviewable.set_priorities(medium: 2.0)
       SiteSetting.reviewable_default_visibility = 'medium'
 
-      GroupUser.create!(group_id: group.id, user_id: moderator.id)
+      GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id)
 
       # Content for admins only
       r1 = Fabricate(:reviewable, reviewable_by_moderator: false)
       messages = MessageBus.track_publish("/reviewable_counts") do
         described_class.new.execute(reviewable_id: r1.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) }
-      expect(admin_msg.data[:reviewable_count]).to eq(0)
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0)
 
       # Content for moderators
       r2 = Fabricate(:reviewable, reviewable_by_moderator: true)
       messages = MessageBus.track_publish("/reviewable_counts") do
         described_class.new.execute(reviewable_id: r2.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) }
-      expect(admin_msg.data[:reviewable_count]).to eq(0)
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0)
-      mod_msg = messages.find { |m| m.user_ids.include?(moderator.id) }
+      legacy_menu_mod_msg = messages.find { |m| m.user_ids.include?(legacy_menu_mod.id) }
-      expect(mod_msg.data[:reviewable_count]).to eq(0)
+      expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(0)
 
       # Content for a group
       r3 = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group)
       messages = MessageBus.track_publish("/reviewable_counts") do
         described_class.new.execute(reviewable_id: r3.id)
       end
-      admin_msg = messages.find { |m| m.user_ids.include?(admin.id) }
+      legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) }
-      expect(admin_msg.data[:reviewable_count]).to eq(0)
+      expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0)
-      mod_messages = messages.select { |m| m.user_ids.include?(moderator.id) }
+      mod_messages = messages.select { |m| m.user_ids.include?(legacy_menu_mod.id) }
       expect(mod_messages.size).to eq(1)
       expect(mod_messages[0].data[:reviewable_count]).to eq(0)
-      group_msg = messages.find { |m| m.user_ids.include?(user.id) }
+      group_msg = messages.find { |m| m.user_ids.include?(legacy_menu_user.id) }
       expect(group_msg.data[:reviewable_count]).to eq(0)
     end
   end

spec/models/reviewable_spec.rb

@@ -244,6 +244,63 @@ RSpec.describe Reviewable, type: :model do
     end
   end
 
+  describe ".unseen_list_for" do
+    fab!(:admin) { Fabricate(:admin) }
+    fab!(:moderator) { Fabricate(:moderator) }
+    fab!(:group) { Fabricate(:group) }
+    fab!(:user) { Fabricate(:user, groups: [group]) }
+    fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) }
+    fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) }
+    fab!(:group_reviewable) {
+      Fabricate(:reviewable, reviewable_by_group: group, reviewable_by_moderator: false)
+    }
+
+    context "for admins" do
+      it "returns a list of pending reviewables that haven't been seen by the user" do
+        list = Reviewable.unseen_list_for(admin, preload: false)
+        expect(list).to contain_exactly(admin_reviewable, mod_reviewable, group_reviewable)
+        admin_reviewable.update!(status: Reviewable.statuses[:approved])
+        list = Reviewable.unseen_list_for(admin, preload: false)
+        expect(list).to contain_exactly(mod_reviewable, group_reviewable)
+        admin.update!(last_seen_reviewable_id: group_reviewable.id)
+        expect(Reviewable.unseen_list_for(admin, preload: false).empty?).to eq(true)
+      end
+    end
+
+    context "for moderators" do
+      it "returns a list of pending reviewables that haven't been seen by the user" do
+        list = Reviewable.unseen_list_for(moderator, preload: false)
+        expect(list).to contain_exactly(mod_reviewable)
+
+        group_reviewable.update!(reviewable_by_moderator: true)
+
+        list = Reviewable.unseen_list_for(moderator, preload: false)
+        expect(list).to contain_exactly(mod_reviewable, group_reviewable)
+
+        moderator.update!(last_seen_reviewable_id: mod_reviewable.id)
+
+        list = Reviewable.unseen_list_for(moderator, preload: false)
+        expect(list).to contain_exactly(group_reviewable)
+      end
+    end
+
+    context "for group moderators" do
+      before do
+        SiteSetting.enable_category_group_moderation = true
+      end
+
+      it "returns a list of pending reviewables that haven't been seen by the user" do
+        list = Reviewable.unseen_list_for(user, preload: false)
+        expect(list).to contain_exactly(group_reviewable)
+
+        user.update!(last_seen_reviewable_id: group_reviewable.id)
+
+        list = Reviewable.unseen_list_for(user, preload: false)
+        expect(list).to be_empty
+      end
+    end
+  end
+
   describe ".recent_list_with_pending_first" do
     fab!(:pending_reviewable1) do
       Fabricate(

spec/models/user_spec.rb

@@ -2057,6 +2057,24 @@ RSpec.describe User do
 
       expect(message).to eq(nil)
     end
+
+    context "with redesigned_user_menu_enabled on" do
+      it "adds all_unread_notifications_count to the payload" do
+        user.update!(admin: true)
+        user.enable_redesigned_user_menu
+        Fabricate(:notification, user: user)
+        Fabricate(:notification, notification_type: 15, high_priority: true, read: false, user: user)
+        messages = MessageBus.track_publish("/notification/#{user.id}") do
+          user.publish_notifications_state
+        end
+        expect(messages.size).to eq(1)
+
+        message = messages.first
+        expect(message.data[:all_unread_notifications_count]).to eq(2)
+      ensure
+        user.disable_redesigned_user_menu
+      end
+    end
   end
 
   describe "silenced?" do
@@ -2780,4 +2798,112 @@ RSpec.describe User do
       expect(user.whisperer?).to eq(false)
     end
   end
+
+  describe "#all_unread_notifications_count" do
+    it "returns count of unseen and unread high priority and normal priority notifications" do
+      Fabricate(:notification, user: user, high_priority: true, read: false)
+      n2 = Fabricate(:notification, user: user, high_priority: false, read: false)
+      expect(user.all_unread_notifications_count).to eq(2)
+
+      n2.update!(read: true)
+      user.reload
+
+      expect(user.all_unread_notifications_count).to eq(1)
+
+      user.update!(seen_notification_id: n2.id)
+      user.reload
+
+      expect(user.all_unread_notifications_count).to eq(0)
+
+      n3 = Fabricate(:notification, user: user)
+      user.reload
+
+      expect(user.all_unread_notifications_count).to eq(1)
+
+      n3.topic.trash!(Fabricate(:admin))
+      user.reload
+
+      expect(user.all_unread_notifications_count).to eq(0)
+    end
+  end
+
+  describe "#unseen_reviewable_count" do
+    fab!(:admin_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false) }
+    fab!(:mod_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: true) }
+    fab!(:group_reviewable) { Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group) }
+
+    it "doesn't include reviewables that can't be seen by the user" do
+      SiteSetting.enable_category_group_moderation = true
+      expect(user.unseen_reviewable_count).to eq(0)
+      user.groups << group
+      user.save!
+      expect(user.unseen_reviewable_count).to eq(1)
+      user.update!(moderator: true)
+      expect(user.unseen_reviewable_count).to eq(2)
+      user.update!(admin: true)
+      expect(user.unseen_reviewable_count).to eq(3)
+    end
+
+    it "returns count of unseen reviewables" do
+      user.update!(admin: true)
+      expect(user.unseen_reviewable_count).to eq(3)
+      user.update!(last_seen_reviewable_id: mod_reviewable.id)
+      expect(user.unseen_reviewable_count).to eq(1)
+      user.update!(last_seen_reviewable_id: group_reviewable.id)
+      expect(user.unseen_reviewable_count).to eq(0)
+    end
+  end
+
+  describe "#bump_last_seen_reviewable!" do
+    it "doesn't error if there are no reviewables" do
+      Reviewable.destroy_all
+      user.bump_last_seen_reviewable!
+      expect(user.last_seen_reviewable_id).to eq(nil)
+    end
+
+    it "picks the reviewable of the largest id" do
+      user.update!(admin: true)
+      Fabricate(
+        :reviewable,
+        created_at: 3.minutes.ago,
+        updated_at: 3.minutes.ago,
+        score: 100
+      )
+      reviewable2 = Fabricate(
+        :reviewable,
+        created_at: 30.minutes.ago,
+        updated_at: 30.minutes.ago,
+        score: 10
+      )
+      user.bump_last_seen_reviewable!
+      expect(user.last_seen_reviewable_id).to eq(reviewable2.id)
+    end
+
+    it "stays at the maximum reviewable if there are no new reviewables" do
+      user.update!(admin: true)
+      reviewable = Fabricate(:reviewable)
+      user.bump_last_seen_reviewable!
+      expect(user.last_seen_reviewable_id).to eq(reviewable.id)
+      user.bump_last_seen_reviewable!
+      expect(user.last_seen_reviewable_id).to eq(reviewable.id)
+    end
+
+    it "respects reviewables security" do
+      admin = Fabricate(:admin)
+      moderator = Fabricate(:moderator)
+      group = Fabricate(:group)
+      user.update!(groups: [group])
+      SiteSetting.enable_category_group_moderation = true
+
+      group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false, reviewable_by_group: group)
+      mod_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true)
+      admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false)
+
+      [admin, moderator, user].each(&:bump_last_seen_reviewable!)
+
+      expect(admin.last_seen_reviewable_id).to eq(admin_reviewable.id)
+      expect(moderator.last_seen_reviewable_id).to eq(mod_reviewable.id)
+      expect(user.last_seen_reviewable_id).to eq(group_reviewable.id)
+    end
+  end
 end

spec/requests/notifications_controller_spec.rb

@@ -87,6 +87,60 @@ RSpec.describe NotificationsController do
           Discourse.clear_redis_readonly!
         end
 
+        it "should not bump last seen reviewable in readonly mode" do
+          user.update!(admin: true)
+          Fabricate(:reviewable)
+          Discourse.received_redis_readonly!
+          expect {
+            get "/notifications.json", params: { recent: true }
+            expect(response.status).to eq(200)
+          }.not_to change { user.reload.last_seen_reviewable_id }
+        ensure
+          Discourse.clear_redis_readonly!
+        end
+
+        it "should not bump last seen reviewable if the user can't seen reviewables" do
+          Fabricate(:reviewable)
+          expect {
+            get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
+            expect(response.status).to eq(200)
+          }.not_to change { user.reload.last_seen_reviewable_id }
+        end
+
+        it "should not bump last seen reviewable if the silent param is present" do
+          user.update!(admin: true)
+          Fabricate(:reviewable)
+          expect {
+            get "/notifications.json", params: {
+              recent: true,
+              silent: true,
+              bump_last_seen_reviewable: true
+            }
+            expect(response.status).to eq(200)
+          }.not_to change { user.reload.last_seen_reviewable_id }
+        end
+
+        it "should not bump last seen reviewable if the bump_last_seen_reviewable param is not present" do
+          user.update!(admin: true)
+          Fabricate(:reviewable)
+          expect {
+            get "/notifications.json", params: { recent: true, silent: true }
+            expect(response.status).to eq(200)
+          }.not_to change { user.reload.last_seen_reviewable_id }
+        end
+
+        it "bumps last_seen_reviewable_id" do
+          user.update!(admin: true)
+          expect(user.last_seen_reviewable_id).to eq(nil)
+          reviewable = Fabricate(:reviewable)
+          get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
+          expect(user.reload.last_seen_reviewable_id).to eq(reviewable.id)
+
+          reviewable2 = Fabricate(:reviewable)
+          get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
+          expect(user.reload.last_seen_reviewable_id).to eq(reviewable2.id)
+        end
+
         it "get notifications with all filters" do
           notification = Fabricate(:notification, user: user)
           notification2 = Fabricate(:notification, user: user)

spec/requests/reviewables_controller_spec.rb

@@ -76,6 +76,7 @@ RSpec.describe ReviewablesController do
         expect(json['users'].any? { |u| u['id'] == reviewable.target_created_by_id }).to eq(true)
 
         expect(json['meta']['reviewable_count']).to eq(1)
+        expect(json['meta']['unseen_reviewable_count']).to eq(1)
         expect(json['meta']['status']).to eq("pending")
       end