FIX: unread notification counts

app/assets/javascripts/discourse/app/components/user-menu/bookmarks-list.js

@@ -51,35 +51,37 @@ export default class UserMenuBookmarksList extends UserMenuNotificationsList {
   }
 
   async fetchItems() {
-    const data = await ajax(
-      `/u/${this.currentUser.username}/user-menu-bookmarks`
-    );
     const content = [];
 
+    const { currentUser, siteSettings, site } = this;
+    const { username } = currentUser;
+
+    const data = await ajax(`/u/${username}/user-menu-bookmarks`);
+
     const notifications = data.notifications.map((n) => Notification.create(n));
     await Notification.applyTransformations(notifications);
     notifications.forEach((notification) => {
       content.push(
         new UserMenuNotificationItem({
           notification,
-          currentUser: this.currentUser,
+          currentUser,
-          siteSettings: this.siteSettings,
+          siteSettings,
-          site: this.site,
+          site,
         })
       );
     });
 
     const bookmarks = data.bookmarks.map((b) => Bookmark.create(b));
     await Bookmark.applyTransformations(bookmarks);
+    bookmarks.forEach((bookmark) => {
       content.push(
-      ...bookmarks.map((bookmark) => {
+        new UserMenuBookmarkItem({
-        return new UserMenuBookmarkItem({
           bookmark,
-          siteSettings: this.siteSettings,
+          siteSettings,
-          site: this.site,
+          site,
-        });
         })
       );
+    });
 
     return content;
   }

app/controllers/application_controller.rb

@@ -390,8 +390,7 @@ class ApplicationController < ActionController::Base
       end
 
       if notifications.present?
-        notification_ids = notifications.split(",").map(&:to_i)
+        Notification.read!(current_user, ids: notifications.split(",").map(&:to_i))
-        Notification.read(current_user, notification_ids)
         current_user.reload
         current_user.publish_notifications_state
         cookie_args = {}

app/controllers/notifications_controller.rb

@@ -9,10 +9,8 @@ class NotificationsController < ApplicationController
 
   def index
     user =
-      if params[:username] && !params[:recent]
+      if params[:username].present? && params[:recent].blank?
-        user_record = User.find_by(username: params[:username].to_s)
+        User.find_by_username(params[:username].to_s) || (raise Discourse::NotFound)
-        raise Discourse::NotFound if !user_record
-        user_record
       else
         current_user
       end
@@ -29,37 +27,31 @@ class NotificationsController < ApplicationController
     if params[:recent].present?
       limit = fetch_limit_from_params(default: 15, max: INDEX_LIMIT)
 
-      include_reviewables = false
-
       notifications =
         Notification.prioritized_list(current_user, count: limit, types: notification_types)
-      # notification_types is blank for the "all notifications" user menu tab
-      include_reviewables = notification_types.blank? && guardian.can_see_review_queue?
-
-      if notifications.present? && !(params.has_key?(:silent) || @readonly_mode)
-        if current_user.bump_last_seen_notification!
-          current_user.reload
-          current_user.publish_notifications_state
-        end
-      end
-
-      if !params.has_key?(:silent) && params[:bump_last_seen_reviewable] && !@readonly_mode &&
-           include_reviewables
-        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 user object returned by the
-          # current_user controller method is changed by the time the deferred
-          # block is executed
-          User.find_by(id: current_user_id)&.bump_last_seen_reviewable!
-        end
-      end
 
       notifications =
         Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
+
       notifications =
         Notification.populate_acting_user(notifications) if SiteSetting.show_user_menu_avatars
 
+      include_reviewables = notification_types.blank? && guardian.can_see_review_queue?
+      bump_notification = notifications.present?
+      bump_reviewable = include_reviewables && params[:bump_last_seen_reviewable]
+
+      if !params.has_key?(:silent) && !@readonly_mode
+        if bump_notification || bump_reviewable
+          current_user_id = current_user.id
+          Scheduler::Defer.later "bump last seen notification/reviewable for user" do
+            if user = User.find_by(id: current_user_id)
+              user.bump_last_seen_notification! if bump_notification
+              user.bump_last_seen_reviewable! if bump_reviewable
+            end
+          end
+        end
+      end
+
       json = {
         notifications: serialize_data(notifications, NotificationSerializer),
         seen_notification_id: current_user.seen_notification_id,
@@ -77,19 +69,20 @@ class NotificationsController < ApplicationController
       limit = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
       offset = params[:offset].to_i
 
-      notifications =
+      notifications = user.notifications.visible.includes(:topic).order(created_at: :desc)
-        Notification.where(user_id: user.id).visible.includes(:topic).order(created_at: :desc)
+      notifications = notifications.read if params[:filter] == "read"
-
+      notifications = notifications.unread if params[:filter] == "unread"
-      notifications = notifications.where(read: true) if params[:filter] == "read"
-
-      notifications = notifications.where(read: false) if params[:filter] == "unread"
 
       total_rows = notifications.dup.count
+
       notifications = notifications.offset(offset).limit(limit)
+
       notifications =
         Notification.filter_inaccessible_topic_notifications(current_user.guardian, notifications)
+
       notifications =
         Notification.populate_acting_user(notifications) if SiteSetting.show_user_menu_avatars
+
       render_json_dump(
         notifications: serialize_data(notifications, NotificationSerializer),
         total_rows_notifications: total_rows,
@@ -106,27 +99,20 @@ class NotificationsController < ApplicationController
   end
 
   def mark_read
-    if params[:id]
+    if id = params[:id]
-      Notification.read(current_user, [params[:id].to_i])
+      Notification.read!(current_user, id:)
     else
       if types = params[:dismiss_types]&.split(",").presence
-        invalid = []
         types.map! do |type|
-          type_id = Notification.types[type.to_sym]
+          Notification.types[type.to_sym] ||
-          invalid << type if !type_id
+            (raise Discourse::InvalidParameters.new("invalid notification type: #{type}"))
-          type_id
-        end
-        if invalid.size > 0
-          raise Discourse::InvalidParameters.new("invalid notification types: #{invalid.inspect}")
         end
       end
 
-      Notification.read_types(current_user, types)
+      Notification.read!(current_user, types:)
+    end
+
     current_user.bump_last_seen_notification!
-    end
-
-    current_user.reload
-    current_user.publish_notifications_state
 
     render json: success_json
   end

app/controllers/users_controller.rb

@@ -1879,55 +1879,40 @@ class UsersController < ApplicationController
   end
 
   USER_MENU_LIST_LIMIT = 20
+
   def user_menu_bookmarks
     if !current_user.username_equals_to?(params[:username])
       raise Discourse::InvalidAccess.new("username doesn't match current_user's username")
     end
 
-    reminder_notifications =
+    reminders =
-      BookmarkQuery.new(user: current_user).unread_notifications(limit: USER_MENU_LIST_LIMIT)
+      current_user
-    if reminder_notifications.size < USER_MENU_LIST_LIMIT
+        .notifications
-      exclude_bookmark_ids =
+        .for_user_menu(USER_MENU_LIST_LIMIT)
-        reminder_notifications.filter_map { |notification| notification.data_hash[:bookmark_id] }
+        .unread
+        .where(notification_type: Notification.types[:bookmark_reminder])
 
-      bookmark_list =
+    reminders =
-        UserBookmarkList.new(
+      Notification.filter_inaccessible_topic_notifications(current_user.guardian, reminders)
-          user: current_user,
-          guardian: guardian,
-          per_page: USER_MENU_LIST_LIMIT - reminder_notifications.size,
-        )
 
-      bookmark_list.load do |query|
+    reminders = Notification.populate_acting_user(reminders) if SiteSetting.show_user_menu_avatars
-        if exclude_bookmark_ids.present?
-          query.where("bookmarks.id NOT IN (?)", exclude_bookmark_ids)
-        end
-      end
-    end
 
-    if reminder_notifications.present?
+    bookmark_list = []
-      if SiteSetting.show_user_menu_avatars
-        Notification.populate_acting_user(reminder_notifications)
-      end
-      serialized_notifications =
-        ActiveModel::ArraySerializer.new(
-          reminder_notifications,
-          each_serializer: NotificationSerializer,
-          scope: guardian,
-        )
-    end
 
-    if bookmark_list
+    if reminders.count < USER_MENU_LIST_LIMIT
+      reminded_bookmark_ids = reminders.filter_map { _1.data_hash[:bookmark_id] }
+      per_page = USER_MENU_LIST_LIMIT - reminders.count
+      bookmark_list = UserBookmarkList.new(user: current_user, guardian:, per_page:)
       bookmark_list.bookmark_serializer_opts = { link_to_first_unread_post: true }
-      serialized_bookmarks =
+      bookmark_list.load do |query|
-        serialize_data(bookmark_list, UserBookmarkListSerializer, scope: guardian, root: false)[
+        query.where.not(id: reminded_bookmark_ids) if reminded_bookmark_ids.present?
-          :bookmarks
+      end
-        ]
     end
 
-    render json: {
+    notifications = serialize_data(reminders, NotificationSerializer)
-             notifications: serialized_notifications || [],
+    bookmarks = serialize_data(bookmark_list, UserBookmarkListSerializer, root: false)[:bookmarks]
-             bookmarks: serialized_bookmarks || [],
+
-           }
+    render json: { notifications:, bookmarks: }
   end
 
   def user_menu_messages
@@ -1941,8 +1926,9 @@ class UsersController < ApplicationController
     end
 
     unread_notifications =
-      Notification
+      current_user
-        .for_user_menu(current_user.id, limit: USER_MENU_LIST_LIMIT)
+        .notifications
+        .for_user_menu(USER_MENU_LIST_LIMIT)
         .unread
         .where(
           notification_type: [
@@ -1962,18 +1948,14 @@ class UsersController < ApplicationController
           .list_private_messages_direct_and_groups(
             current_user,
             groups_messages_notification_level: :watching,
-          ) do |query|
+          ) { |query| query.where.not(id: exclude_topic_ids) if exclude_topic_ids.present? }
-            if exclude_topic_ids.present?
-              query.where("topics.id NOT IN (?)", exclude_topic_ids)
-            else
-              query
-            end
-          end
 
       read_notifications =
-        Notification
+        current_user
-          .for_user_menu(current_user.id, limit: limit)
+          .notifications
-          .where(read: true, notification_type: Notification.types[:group_message_summary])
+          .for_user_menu(limit)
+          .read
+          .where(notification_type: Notification.types[:group_message_summary])
           .to_a
     end
 

app/mailers/user_notifications.rb

@@ -309,7 +309,11 @@ class UserNotifications < ActionMailer::Base
       end
 
       if @counts.size < 3
-        value = user.unread_notifications_of_type(Notification.types[:liked], since: @since)
+        value =
+          user.unread_notifications_count(
+            notification_type: Notification.types[:liked],
+            since: @since,
+          )
         if value > 0
           @counts << {
             id: "likes_received",

app/models/notification.rb

@@ -14,49 +14,26 @@ class Notification < ActiveRecord::Base
   validates_presence_of :data
   validates_presence_of :notification_type
 
-  scope :unread, lambda { where(read: false) }
+  scope :read, -> { where(read: true) }
-  scope :recent,
+  scope :unread, -> { where(read: false) }
-        lambda { |n = nil|
-          n ||= 10
-          order("notifications.created_at desc").limit(n)
-        }
   scope :visible,
-        lambda {
+        -> do
           joins("LEFT JOIN topics ON notifications.topic_id = topics.id").where(
-            "topics.id IS NULL OR topics.deleted_at IS NULL",
+            "notifications.topic_id IS NULL OR (topics.id IS NOT NULL AND topics.deleted_at IS NULL)",
           )
-        }
-  scope :unread_type, ->(user, type, limit = 30) { unread_types(user, [type], limit) }
-  scope :unread_types,
-        ->(user, types, limit = 30) do
-          where(user_id: user.id, read: false, notification_type: types)
-            .visible
-            .includes(:topic)
-            .limit(limit)
         end
   scope :prioritized,
         ->(deprioritized_types = []) do
-          scope = order("notifications.high_priority AND NOT notifications.read DESC")
+          low_pri =
+            "AND notifications.notification_type NOT IN (#{deprioritized_types.join(",")})" if deprioritized_types.present?
 
-          if deprioritized_types.present?
+          order <<~SQL
-            scope =
+            NOT notifications.read AND notifications.high_priority DESC,
-              scope.order(
+            NOT notifications.read #{low_pri || ""} DESC,
-                DB.sql_fragment(
+            notifications.created_at DESC
-                  "NOT notifications.read AND notifications.notification_type NOT IN (?) DESC",
+          SQL
-                  deprioritized_types,
-                ),
-              )
-          else
-            scope = scope.order("NOT notifications.read DESC")
-          end
-
-          scope.order("notifications.created_at DESC")
-        end
-
-  scope :for_user_menu,
-        ->(user_id, limit: 30) do
-          where(user_id: user_id).visible.prioritized.includes(:topic).limit(limit)
         end
+  scope :for_user_menu, ->(limit) { visible.prioritized.includes(:topic).limit(limit) }
 
   attr_accessor :skip_send_email
 
@@ -68,8 +45,7 @@ class Notification < ActiveRecord::Base
   before_create do
     # if we have manually set the notification to high_priority on create then
     # make sure that is respected
-    self.high_priority =
+    self.high_priority ||= Notification.high_priority_types.include?(self.notification_type)
-      self.high_priority || Notification.high_priority_types.include?(self.notification_type)
   end
 
   def self.consolidate_or_create!(notification_params)
@@ -172,71 +148,31 @@ class Notification < ActiveRecord::Base
   end
 
   def self.normal_priority_types
-    @normal_priority_types ||= types.reject { |_k, v| high_priority_types.include?(v) }.values
+    @normal_priority_types ||= types.values - high_priority_types
   end
 
-  def self.mark_posts_read(user, topic_id, post_numbers)
+  def self.read!(user, id: nil, ids: nil, types: nil, topic_id: nil, post_numbers: nil)
-    Notification.where(
+    query = user.notifications.unread
-      user_id: user.id,
+    query = query.where(id: id) if id.present?
-      topic_id: topic_id,
+    query = query.where(id: ids) if ids.present?
-      post_number: post_numbers,
+    query = query.where(notification_type: types) if types.present?
-      read: false,
+
-    ).update_all(read: true)
+    if topic_id.present? && post_numbers.present?
+      query = query.where(topic_id: topic_id, post_number: post_numbers)
     end
 
-  def self.read(user, notification_ids)
-    Notification.where(id: notification_ids, user_id: user.id, read: false).update_all(read: true)
-  end
-
-  def self.read_types(user, types = nil)
-    query = Notification.where(user_id: user.id, read: false)
-    query = query.where(notification_type: types) if types
     query.update_all(read: true)
   end
 
-  def self.interesting_after(min_date)
+  # Clean up any notifications the user can no longer see.
-    result =
+  # For example, if a topic was previously public then turns private.
-      where("created_at > ?", min_date)
-        .includes(:topic)
-        .visible
-        .unread
-        .limit(20)
-        .order(
-          "CASE WHEN notification_type = #{Notification.types[:replied]} THEN 1
-                           WHEN notification_type = #{Notification.types[:mentioned]} THEN 2
-                           ELSE 3
-                      END, created_at DESC",
-        )
-        .to_a
-
-    # Remove any duplicates by type and topic
-    if result.present?
-      seen = {}
-      to_remove = Set.new
-
-      result.each do |r|
-        seen[r.notification_type] ||= Set.new
-        if seen[r.notification_type].include?(r.topic_id)
-          to_remove << r.id
-        else
-          seen[r.notification_type] << r.topic_id
-        end
-      end
-      result.reject! { |r| to_remove.include?(r.id) }
-    end
-
-    result
-  end
-
-  # Clean up any notifications the user can no longer see. For example, if a topic was previously
-  # public then turns private.
   def self.remove_for(user_id, topic_id)
     Notification.where(user_id: user_id, topic_id: topic_id).delete_all
   end
 
   def self.filter_inaccessible_topic_notifications(guardian, notifications)
-    topic_ids = notifications.map { |n| n.topic_id }.compact.uniq
+    topic_ids = notifications.map(&:topic_id).compact.uniq
-    accessible_topic_ids = guardian.can_see_topic_ids(topic_ids: topic_ids)
+    accessible_topic_ids = guardian.can_see_topic_ids(topic_ids:)
     notifications.select { |n| n.topic_id.blank? || accessible_topic_ids.include?(n.topic_id) }
   end
 
@@ -266,114 +202,43 @@ class Notification < ActiveRecord::Base
   # `Notification.prioritized_list` to deprioritize like typed notifications. Also See
   # `db/migrate/20240306063428_add_indexes_to_notifications.rb`.
   def self.like_types
-    [
+    @@like_types ||= [
       Notification.types[:liked],
       Notification.types[:liked_consolidated],
       Notification.types[:reaction],
     ]
   end
 
-  def self.prioritized_list(user, count: 30, types: [])
+  def self.never = UserOption.like_notification_frequency_type[:never]
-    return [] if !user&.user_option
 
-    notifications =
+  def self.prioritized_list(user, count: 30, types: [])
-      user
+    return Notification.none unless user&.user_option
-        .notifications
+
-        .includes(:topic)
+    notifications = user.notifications.includes(:topic).visible.limit(count)
-        .visible
-        .prioritized(types.present? ? [] : like_types)
-        .limit(count)
 
     if types.present?
-      notifications = notifications.where(notification_type: types)
+      notifications = notifications.prioritized.where(notification_type: types)
-    elsif user.user_option.like_notification_frequency ==
+    else
-          UserOption.like_notification_frequency_type[:never]
+      notifications = notifications.prioritized(like_types)
-      like_types.each do |notification_type|
+      if user.user_option.like_notification_frequency == never
-        notifications = notifications.where("notification_type <> ?", notification_type)
+        notifications = notifications.where.not(notification_type: like_types)
       end
     end
-    notifications.to_a
-  end
-
-  def self.recent_report(user, count = nil, types = [])
-    return unless user && user.user_option
-
-    count ||= 10
-    notifications = user.notifications.visible.recent(count).includes(:topic)
-
-    notifications = notifications.where(notification_type: types) if types.present?
-    if user.user_option.like_notification_frequency ==
-         UserOption.like_notification_frequency_type[:never]
-      [
-        Notification.types[:liked],
-        Notification.types[:liked_consolidated],
-      ].each do |notification_type|
-        notifications = notifications.where("notification_type <> ?", notification_type)
-      end
-    end
-
-    notifications = notifications.to_a
-
-    if notifications.present?
-      builder = DB.build(<<~SQL)
-         SELECT n.id FROM notifications n
-         /*where*/
-        ORDER BY n.id ASC
-        /*limit*/
-      SQL
-
-      builder.where(<<~SQL, user_id: user.id)
-        n.high_priority = TRUE AND
-        n.user_id = :user_id AND
-        NOT read
-      SQL
-      builder.where("notification_type IN (:types)", types: types) if types.present?
-      builder.limit(count.to_i)
-
-      ids = builder.query_single
-
-      if ids.length > 0
-        notifications +=
-          user
-            .notifications
-            .order("notifications.created_at DESC")
-            .where(id: ids)
-            .joins(:topic)
-            .limit(count)
-      end
 
     notifications
-        .uniq(&:id)
-        .sort do |x, y|
-          if x.unread_high_priority? && !y.unread_high_priority?
-            -1
-          elsif y.unread_high_priority? && !x.unread_high_priority?
-            1
-          else
-            y.created_at <=> x.created_at
-          end
-        end
-        .take(count)
-    else
-      []
-    end
   end
 
+  USERNAME_FIELDS ||= %i[username display_username mentioned_by_username invited_by_username]
+
   def self.populate_acting_user(notifications)
     usernames =
-      notifications.map do |notification|
+      notifications.filter_map do |n|
-        notification.acting_username =
+        n.acting_username = n.data_hash.values_at(*USERNAME_FIELDS).compact.first&.downcase
-          (
-            notification.data_hash[:username] || notification.data_hash[:display_username] ||
-              notification.data_hash[:mentioned_by_username] ||
-              notification.data_hash[:invited_by_username]
-          )&.downcase
       end
 
     users = User.where(username_lower: usernames.uniq).index_by(&:username_lower)
-    notifications.each do |notification|
+
-      notification.acting_user = users[notification.acting_username]
+    notifications.each { |n| n.acting_user = users[n.acting_username] }
-    end
 
     notifications
   end

app/models/post_timing.rb

@@ -197,10 +197,11 @@ SQL
       existing = Set.new(DB.query_single(sql))
 
       sql = <<~SQL
-      SELECT 1 FROM topics
+        SELECT 1 
-      WHERE deleted_at IS NULL AND
+        FROM topics
-        archetype = 'regular' AND
+        WHERE deleted_at IS NULL 
-        id = :topic_id
+        AND archetype = 'regular'
+        AND id = :topic_id
       SQL
 
       is_regular = DB.exec(sql, topic_id: topic_id) == 1
@@ -220,7 +221,8 @@ SQL
 
     total_changed = 0
     if timings.length > 0
-      total_changed = Notification.mark_posts_read(current_user, topic_id, timings.map { |t| t[0] })
+      post_numbers = timings.map(&:first)
+      total_changed = Notification.read!(current_user, topic_id:, post_numbers:)
     end
 
     topic_time = max_time_per_post if topic_time > max_time_per_post

app/models/user.rb

@@ -620,12 +620,9 @@ class User < ActiveRecord::Base
   end
 
   def reload
-    @unread_notifications = nil
+    @unread_notifications_count = {}
-    @all_unread_notifications_count = nil
-    @unread_total_notifications = nil
     @unread_pms = nil
     @unread_bookmarks = nil
-    @unread_high_prios = nil
     @ignored_user_ids = nil
     @muted_user_ids = nil
     @belonging_to_group_ids = nil
@@ -640,81 +637,6 @@ class User < ActiveRecord::Base
     @muted_user_ids ||= muted_users.pluck(:id)
   end
 
-  def unread_notifications_of_type(notification_type, since: nil)
-    # perf critical, much more efficient than AR
-    sql = <<~SQL
-        SELECT COUNT(*)
-          FROM notifications n
-     LEFT JOIN topics t ON t.id = n.topic_id
-         WHERE t.deleted_at IS NULL
-           AND n.notification_type = :notification_type
-           AND n.user_id = :user_id
-           AND NOT read
-           #{since ? "AND n.created_at > :since" : ""}
-    SQL
-
-    # to avoid coalesce we do to_i
-    DB.query_single(sql, user_id: id, notification_type: notification_type, since: since)[0].to_i
-  end
-
-  def unread_notifications_of_priority(high_priority:)
-    # perf critical, much more efficient than AR
-    sql = <<~SQL
-        SELECT COUNT(*)
-          FROM notifications n
-     LEFT JOIN topics t ON t.id = n.topic_id
-         WHERE t.deleted_at IS NULL
-           AND n.high_priority = :high_priority
-           AND n.user_id = :user_id
-           AND NOT read
-    SQL
-
-    # to avoid coalesce we do to_i
-    DB.query_single(sql, user_id: id, high_priority: high_priority)[0].to_i
-  end
-
-  MAX_UNREAD_BACKLOG = 400
-  def grouped_unread_notifications
-    results = DB.query(<<~SQL, user_id: self.id, limit: MAX_UNREAD_BACKLOG)
-      SELECT X.notification_type AS type, COUNT(*) FROM (
-        SELECT n.notification_type
-        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 NOT n.read
-        LIMIT :limit
-      ) AS X
-      GROUP BY X.notification_type
-    SQL
-    results.map! { |row| [row.type, row.count] }
-    results.to_h
-  end
-
-  def unread_high_priority_notifications
-    @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
   MAX_UNREAD_NOTIFICATIONS = 99
 
   def self.max_unread_notifications
@@ -725,64 +647,76 @@ class User < ActiveRecord::Base
     @max_unread_notifications = val
   end
 
-  def unread_notifications
+  def grouped_unread_notifications
-    @unread_notifications ||=
+    DB.query_array(<<~SQL, user_id: id, limit: MAX_UNREAD_NOTIFICATIONS).to_h
-      begin
+      SELECT X.notification_type, COUNT(*)
-        # perf critical, much more efficient than AR
+      FROM (
-        sql = <<~SQL
+        SELECT n.notification_type
-        SELECT COUNT(*) FROM (
+        FROM notifications n
-          SELECT 1 FROM
-          notifications n
         LEFT JOIN topics t ON t.id = n.topic_id
-           WHERE t.deleted_at IS NULL AND
+        WHERE (n.topic_id IS NULL OR (t.id IS NOT NULL AND t.deleted_at IS NULL))
-            n.high_priority = FALSE AND
+        AND n.user_id = :user_id
-            n.user_id = :user_id AND
+        AND NOT n.read
-            n.id > :seen_notification_id AND
+        LIMIT :limit
-            NOT read
+      ) AS X
+      GROUP BY X.notification_type
+    SQL
+  end
+
+  def unread_notifications_count(
+    user_id: id,
+    notification_type: nil,
+    high_priority: nil,
+    since: nil,
+    seen_notification_id: nil,
+    limit: nil
+  )
+    limit = [limit, User.max_unread_notifications].compact.min
+    args = { user_id:, notification_type:, high_priority:, since:, seen_notification_id:, limit: }
+    key = args.values.compact.join("-")
+
+    @unread_notifications_count ||= {}
+    @unread_notifications_count[key] ||= begin
+      DB.query_single(<<~SQL, args)[0].to_i
+        SELECT COUNT(*)
+        FROM (
+          SELECT 1
+          FROM notifications n
+          LEFT JOIN topics t ON n.topic_id = t.id
+          WHERE (n.topic_id IS NULL OR (t.id IS NOT NULL AND t.deleted_at IS NULL))
+            AND NOT n.read
+            AND n.user_id = :user_id
+            #{notification_type.blank? ? "" : "AND n.notification_type = :notification_type"}
+            #{high_priority.nil? ? "" : "AND n.high_priority = :high_priority"}
+            #{since.nil? ? "" : "AND n.created_at > :since"}
+            #{seen_notification_id.nil? ? "" : "AND n.id > :seen_notification_id"}
             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 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
+    unread_notifications_count
+  end
+
+  def unread_high_priority_notifications
+    unread_notifications_count(high_priority: true)
+  end
+
+  def unread_notifications
+    unread_notifications_count(seen_notification_id:, high_priority: false)
+  end
+
+  def all_unread_notifications_count
+    unread_notifications_count(seen_notification_id:)
+  end
+
+  def new_personal_messages_notifications_count
+    unread_notifications_count(
+      seen_notification_id:,
+      notification_type: Notification.types[:private_message],
+    )
   end
 
   def reviewable_count
@@ -794,6 +728,7 @@ class User < ActiveRecord::Base
     query = query.where("notifications.id > ?", seen_notification_id) if seen_notification_id
     if max_notification_id = query.maximum(:id)
       update!(seen_notification_id: max_notification_id)
+      publish_notifications_state
       true
     else
       false
@@ -802,13 +737,13 @@ class User < ActiveRecord::Base
 
   def bump_last_seen_reviewable!
     query = Reviewable.unseen_list_for(self, preload: false)
-
     query = query.where("reviewables.id > ?", last_seen_reviewable_id) if last_seen_reviewable_id
-    max_reviewable_id = query.maximum(:id)
+    if max_reviewable_id = query.maximum(:id)
-
-    if max_reviewable_id
       update!(last_seen_reviewable_id: max_reviewable_id)
       publish_reviewable_counts
+      true
+    else
+      false
     end
   end
 
@@ -829,49 +764,50 @@ class User < ActiveRecord::Base
     return if !self.allow_live_notifications?
 
     # publish last notification json with the message so we can apply an update
-    notification = notifications.visible.order("notifications.created_at desc").first
+    notification = notifications.visible.order(created_at: :desc).first
-    json = NotificationSerializer.new(notification).as_json if notification
+    last_notification = NotificationSerializer.new(notification).as_json if notification
 
-    sql = (<<~SQL)
+    recent = DB.query_array(<<~SQL, user_id: id)
-       SELECT * FROM (
+      SELECT * 
-         SELECT n.id, n.read FROM notifications n
+      FROM (
+        SELECT n.id, n.read 
+        FROM notifications n
         LEFT JOIN topics t ON n.topic_id = t.id
-         WHERE
+        WHERE (n.topic_id IS NULL OR (t.id IS NOT NULL AND t.deleted_at IS NULL))
-          t.deleted_at IS NULL AND
+        AND n.user_id = :user_id
-          n.high_priority AND
+        AND n.high_priority
-          n.user_id = :user_id AND
+        AND NOT read
-          NOT read
         ORDER BY n.id DESC
         LIMIT 20
       ) AS x
+
       UNION ALL
-      SELECT * FROM (
+
-       SELECT n.id, n.read FROM notifications n
+      SELECT * 
+      FROM (
+        SELECT n.id, n.read 
+        FROM notifications n
         LEFT JOIN topics t ON n.topic_id = t.id
-       WHERE
+        WHERE (n.topic_id IS NULL OR (t.id IS NOT NULL AND t.deleted_at IS NULL))
-        t.deleted_at IS NULL AND
+        AND n.user_id = :user_id
-        (n.high_priority = FALSE OR read) AND
+        AND (NOT n.high_priority OR read)
-        n.user_id = :user_id
         ORDER BY n.id DESC
         LIMIT 20
       ) AS y
     SQL
 
-    recent = DB.query(sql, user_id: id).map! { |r| [r.id, r.read] }
-
     payload = {
-      unread_notifications: unread_notifications,
+      unread_notifications:,
-      unread_high_priority_notifications: unread_high_priority_notifications,
+      unread_high_priority_notifications:,
       read_first_notification: read_first_notification?,
-      last_notification: json,
+      last_notification:,
-      recent: recent,
+      recent:,
-      seen_notification_id: seen_notification_id,
+      seen_notification_id:,
+      all_unread_notifications_count:,
+      grouped_unread_notifications:,
+      new_personal_messages_notifications_count:,
     }
 
-    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
-
     MessageBus.publish("/notification/#{id}", payload, user_ids: [id])
   end
 

app/models/user_bookmark_list.rb

@@ -40,11 +40,10 @@ class UserBookmarkList
   def categories
     @categories ||=
       @bookmarks
-        .map do |bm|
+        .flat_map do |bm|
           category = bm.bookmarkable.try(:category) || bm.bookmarkable.try(:topic)&.category
           [category&.parent_category, category]
         end
-        .flatten
         .compact
         .uniq
   end

lib/bookmark_query.rb

@@ -40,9 +40,7 @@ class BookmarkQuery
     search_term_wildcard = @search_term.present? ? "%#{@search_term}%" : nil
 
     queries =
-      Bookmark
+      Bookmark.registered_bookmarkables.filter_map do |bookmarkable|
-        .registered_bookmarkables
-        .map do |bookmarkable|
         interim_results = bookmarkable.perform_list_query(@user, @guardian)
 
         # this could occur if there is some security reason that the user cannot
@@ -59,7 +57,6 @@ class BookmarkQuery
         # all mashed up into a massive ball in MiniProfiler :)
         "---- #{bookmarkable.model} bookmarkable ---\n\n #{interim_results.to_sql}"
       end
-        .compact
 
     # same for interim results being blank, the user might have been locked out
     # from all their various bookmarks, in which case they will see nothing and
@@ -68,12 +65,7 @@ class BookmarkQuery
 
     union_sql = queries.join("\n\nUNION\n\n")
     results = Bookmark.select("bookmarks.*").from("(\n\n#{union_sql}\n\n) as bookmarks")
-    results =
+    results = results.order("NOT pinned ASC, reminder_at ASC, updated_at DESC")
-      results.order(
-        "(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END),
-        bookmarks.reminder_at ASC,
-        bookmarks.updated_at DESC",
-      )
 
     @count = results.count
 
@@ -86,82 +78,7 @@ class BookmarkQuery
     results = results.limit(@per_page).to_a
 
     BookmarkQuery.preload(results, self)
+
     results
   end
-
-  def unread_notifications(limit: 20)
-    reminder_notifications =
-      Notification
-        .for_user_menu(@user.id, limit: [limit, 100].min)
-        .unread
-        .where(notification_type: Notification.types[:bookmark_reminder])
-
-    reminder_bookmark_ids = reminder_notifications.map { |n| n.data_hash[:bookmark_id] }.compact
-
-    # We preload associations like we do above for the list to avoid
-    # N1s in the can_see? guardian calls for each bookmark.
-    bookmarks = Bookmark.where(user: @user, id: reminder_bookmark_ids)
-    BookmarkQuery.preload(bookmarks, self)
-
-    # Any bookmarks that no longer exist, we need to find the associated
-    # records using bookmarkable details.
-    #
-    # First we want to group these by type into a hash to reduce queries:
-    #
-    # {
-    #   "Post": {
-    #     1234: <Post>,
-    #     566: <Post>,
-    #   },
-    #   "Topic": {
-    #     123: <Topic>,
-    #     99: <Topic>,
-    #   }
-    # }
-    #
-    # We may not need to do this most of the time. It depends mostly on
-    # a user's auto_delete_preference for bookmarks.
-    deleted_bookmark_ids = reminder_bookmark_ids - bookmarks.map(&:id)
-    deleted_bookmarkables =
-      reminder_notifications
-        .select do |notif|
-          deleted_bookmark_ids.include?(notif.data_hash[:bookmark_id]) &&
-            notif.data_hash[:bookmarkable_type].present?
-        end
-        .inject({}) do |hash, notif|
-          hash[notif.data_hash[:bookmarkable_type]] ||= {}
-          hash[notif.data_hash[:bookmarkable_type]][notif.data_hash[:bookmarkable_id]] = nil
-          hash
-        end
-
-    # Then, we can actually find the associated records for each type in the database.
-    deleted_bookmarkables.each do |type, bookmarkable|
-      records = Bookmark.registered_bookmarkable_from_type(type).model.where(id: bookmarkable.keys)
-      records.each { |record| deleted_bookmarkables[type][record.id] = record }
-    end
-
-    reminder_notifications.select do |notif|
-      bookmark = bookmarks.find { |bm| bm.id == notif.data_hash[:bookmark_id] }
-
-      # This is the happy path, it's easiest to look up using a bookmark
-      # that hasn't been deleted.
-      if bookmark.present?
-        bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmark.bookmarkable_type)
-        bookmarkable.can_see?(@guardian, bookmark)
-      else
-        # Otherwise, we have to use our cached records from the deleted
-        # bookmarks' related bookmarkable (e.g. Post, Topic) to determine
-        # secure access.
-        bookmarkable =
-          deleted_bookmarkables.dig(
-            notif.data_hash[:bookmarkable_type],
-            notif.data_hash[:bookmarkable_id],
-          )
-        bookmarkable.present? &&
-          Bookmark.registered_bookmarkable_from_type(
-            notif.data_hash[:bookmarkable_type],
-          ).can_see_bookmarkable?(@guardian, bookmarkable)
-      end
-    end
-  end
 end

plugins/discourse-narrative-bot/plugin.rb

@@ -112,8 +112,9 @@ after_initialize do
     first_post = topic.ordered_posts.first
 
     notification = Notification.where(topic_id: topic.id, post_number: first_post.post_number).first
+
     if notification.present?
-      Notification.read(self, notification.id)
+      Notification.read!(self, id: notification.id)
       self.reload
       self.publish_notifications_state
     end

spec/models/notification_spec.rb

@@ -329,7 +329,6 @@ RSpec.describe Notification do
         notification_type: Notification.types[:bookmark_reminder],
       )
 
-      other =
       Notification.create!(
         read: false,
         user_id: user.id,
@@ -348,7 +347,7 @@ RSpec.describe Notification do
     end
   end
 
-  describe "mark_posts_read" do
+  describe "read posts" do
     it "marks multiple posts as read if needed" do
       (1..3).map do |i|
         Notification.create!(
@@ -360,6 +359,7 @@ RSpec.describe Notification do
           notification_type: 1,
         )
       end
+
       Notification.create!(
         read: true,
         user_id: user.id,
@@ -369,8 +369,8 @@ RSpec.describe Notification do
         notification_type: 1,
       )
 
-      expect { Notification.mark_posts_read(user, 2, [1, 2, 3, 4]) }.to change {
+      expect { Notification.read!(user, topic_id: 2, post_numbers: [1, 2, 3, 4]) }.to change {
-        Notification.where(read: true).count
+        Notification.read.count
       }.by(3)
     end
   end
@@ -621,79 +621,6 @@ RSpec.describe Notification do
     end
   end
 
-  describe "#recent_report" do
-    let(:post) { Fabricate(:post) }
-
-    def fab(type, read)
-      @i ||= 0
-      @i += 1
-      Notification.create!(
-        read: read,
-        user_id: user.id,
-        topic_id: post.topic_id,
-        post_number: post.post_number,
-        data: "[]",
-        notification_type: type,
-        created_at: @i.days.from_now,
-      )
-    end
-
-    def unread_pm
-      fab(Notification.types[:private_message], false)
-    end
-
-    def unread_bookmark_reminder
-      fab(Notification.types[:bookmark_reminder], false)
-    end
-
-    def pm
-      fab(Notification.types[:private_message], true)
-    end
-
-    def regular
-      fab(Notification.types[:liked], true)
-    end
-
-    def liked_consolidated
-      fab(Notification.types[:liked_consolidated], true)
-    end
-
-    it "correctly finds visible notifications" do
-      pm
-      expect(Notification.visible.count).to eq(1)
-      post.topic.trash!
-      expect(Notification.visible.count).to eq(0)
-    end
-
-    it "orders stuff by creation descending, bumping unread high priority (pms, bookmark reminders) to top" do
-      # note we expect the final order to read bottom-up for this list of variables,
-      # with unread pm + bookmark reminder at the top of that list
-      a = unread_pm
-      regular
-      b = unread_bookmark_reminder
-      c = pm
-      d = regular
-
-      notifications = Notification.recent_report(user, 4)
-      expect(notifications.map { |n| n.id }).to eq([b.id, a.id, d.id, c.id])
-    end
-
-    describe "for a user that does not want to be notify on liked" do
-      before do
-        user.user_option.update!(
-          like_notification_frequency: UserOption.like_notification_frequency_type[:never],
-        )
-      end
-
-      it "should not return any form of liked notifications" do
-        notification = pm
-        regular
-        liked_consolidated
-
-        expect(Notification.recent_report(user)).to contain_exactly(notification)
-      end
-    end
-
   describe "#consolidate_membership_requests" do
     fab!(:group) { Fabricate(:group, name: "XXsssssddd") }
     fab!(:user)
@@ -750,7 +677,6 @@ RSpec.describe Notification do
       expect(notification.shelved_notification).to be_present
     end
   end
-  end
 
   describe "purge_old!" do
     fab!(:user)

spec/requests/notifications_controller_spec.rb

@@ -461,15 +461,17 @@ RSpec.describe NotificationsController do
       end
 
       it "updates the `read` status" do
-        expect(user.reload.unread_notifications).to eq(1)
+        user.reload
-        expect(user.reload.total_unread_notifications).to eq(1)
+        expect(user.unread_notifications).to eq(1)
+        expect(user.total_unread_notifications).to eq(1)
 
         put "/notifications/mark-read.json"
 
         expect(response.status).to eq(200)
+
         user.reload
-        expect(user.reload.unread_notifications).to eq(0)
+        expect(user.unread_notifications).to eq(0)
-        expect(user.reload.total_unread_notifications).to eq(0)
+        expect(user.total_unread_notifications).to eq(0)
       end
 
       describe "#create" do