From b79ea986ac44c1ced0b332722c1bb16b4239b565 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Wed, 1 Apr 2020 09:09:20 +1000
Subject: [PATCH] FEATURE: High priority bookmark reminder notifications
 (#9290)

Introduce the concept of "high priority notifications" which include PM and bookmark reminder notifications. Now bookmark reminder notifications act in the same way as PM notifications (float to top of recent list, show in the green bubble) and most instances of unread_private_messages in the UI have been replaced with unread_high_priority_notifications.

The user email digest is changed to just have a section about unread high priority notifications, the unread PM section has been removed.

A high_priority boolean column has been added to the Notification table and relevant indices added to account for it.

unread_private_messages has been kept on the User model purely for backwards compat, but now just returns unread_high_priority_notifications count so this may cause some inconsistencies in the UI.
---
 .../discourse/components/site-header.js       |  2 +-
 .../discourse/initializers/badging.js         |  2 +-
 .../subscribe-user-notifications.js           | 11 ++--
 .../initializers/title-notifications.js       |  2 +-
 .../javascripts/discourse/widgets/header.js   | 14 +++--
 .../stylesheets/common/base/discourse.scss    |  2 +-
 .../stylesheets/common/base/header.scss       |  2 +-
 app/mailers/user_notifications.rb             |  4 +-
 app/models/notification.rb                    | 38 ++++++++----
 app/models/user.rb                            | 45 ++++++++++----
 app/serializers/current_user_serializer.rb    |  1 +
 config/locales/client.en.yml                  |  3 +
 config/locales/server.en.yml                  |  2 +-
 ...d_high_priority_column_to_notifications.rb | 42 +++++++++++++
 ...drop_old_unread_pm_notification_indices.rb | 12 ++++
 spec/fabricators/notification_fabricator.rb   | 19 ++++++
 spec/models/notification_spec.rb              | 53 +++++++++++++---
 spec/models/user_spec.rb                      | 62 +++++++++++++++++--
 test/javascripts/fixtures/session-fixtures.js |  1 +
 19 files changed, 263 insertions(+), 54 deletions(-)
 create mode 100644 db/migrate/20200329222246_add_high_priority_column_to_notifications.rb
 create mode 100644 db/post_migrate/20200330233427_drop_old_unread_pm_notification_indices.rb

diff --git a/app/assets/javascripts/discourse/components/site-header.js b/app/assets/javascripts/discourse/components/site-header.js
index b8b6ed90e3a..03bc7b3608c 100644
--- a/app/assets/javascripts/discourse/components/site-header.js
+++ b/app/assets/javascripts/discourse/components/site-header.js
@@ -24,7 +24,7 @@ const SiteHeaderComponent = MountWidget.extend(Docking, PanEvents, {
 
   @observes(
     "currentUser.unread_notifications",
-    "currentUser.unread_private_messages",
+    "currentUser.unread_high_priority_notifications",
     "currentUser.reviewable_count"
   )
   notificationsChanged() {
diff --git a/app/assets/javascripts/discourse/initializers/badging.js b/app/assets/javascripts/discourse/initializers/badging.js
index f406604dfa7..8bcefa7915c 100644
--- a/app/assets/javascripts/discourse/initializers/badging.js
+++ b/app/assets/javascripts/discourse/initializers/badging.js
@@ -10,7 +10,7 @@ export default {
     if (!user) return; // must be logged in
 
     this.notifications =
-      user.unread_notifications + user.unread_private_messages;
+      user.unread_notifications + user.unread_high_priority_notifications;
 
     container
       .lookup("service:app-events")
diff --git a/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js b/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js
index 14c2bc5f95b..1482cf9d180 100644
--- a/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js
+++ b/app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js
@@ -32,24 +32,27 @@ export default {
         data => {
           const store = container.lookup("service:store");
           const oldUnread = user.get("unread_notifications");
-          const oldPM = user.get("unread_private_messages");
+          const oldHighPriority = user.get(
+            "unread_high_priority_notifications"
+          );
 
           user.setProperties({
             unread_notifications: data.unread_notifications,
-            unread_private_messages: data.unread_private_messages,
+            unread_high_priority_notifications:
+              data.unread_high_priority_notifications,
             read_first_notification: data.read_first_notification
           });
 
           if (
             oldUnread !== data.unread_notifications ||
-            oldPM !== data.unread_private_messages
+            oldHighPriority !== data.unread_high_priority_notifications
           ) {
             appEvents.trigger("notifications:changed");
 
             if (
               site.mobileView &&
               (data.unread_notifications - oldUnread > 0 ||
-                data.unread_private_messages - oldPM > 0)
+                data.unread_high_priority_notifications - oldHighPriority > 0)
             ) {
               appEvents.trigger("header:update-topic", null, 5000);
             }
diff --git a/app/assets/javascripts/discourse/initializers/title-notifications.js b/app/assets/javascripts/discourse/initializers/title-notifications.js
index 9801e1f688f..cfc0c3ed6d8 100644
--- a/app/assets/javascripts/discourse/initializers/title-notifications.js
+++ b/app/assets/javascripts/discourse/initializers/title-notifications.js
@@ -18,7 +18,7 @@ export default {
     if (!user) return; // must be logged in
 
     const notifications =
-      user.unread_notifications + user.unread_private_messages;
+      user.unread_notifications + user.unread_high_priority_notifications;
 
     Discourse.updateNotificationCount(notifications);
   }
diff --git a/app/assets/javascripts/discourse/widgets/header.js b/app/assets/javascripts/discourse/widgets/header.js
index e08bab87412..4910913a4eb 100644
--- a/app/assets/javascripts/discourse/widgets/header.js
+++ b/app/assets/javascripts/discourse/widgets/header.js
@@ -67,8 +67,9 @@ createWidget("header-notifications", {
       );
     }
 
-    const unreadPMs = user.get("unread_private_messages");
-    if (!!unreadPMs) {
+    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")
@@ -90,14 +91,15 @@ createWidget("header-notifications", {
         }
       }
 
+      // add the counter for the unread high priority
       contents.push(
         this.attach("link", {
           action: attrs.action,
-          className: "badge-notification unread-private-messages",
-          rawLabel: unreadPMs,
+          className: "badge-notification unread-high-priority-notifications",
+          rawLabel: unreadHighPriority,
           omitSpan: true,
-          title: "notifications.tooltip.message",
-          titleOptions: { count: unreadPMs }
+          title: "notifications.tooltip.high_priority",
+          titleOptions: { count: unreadHighPriority }
         })
       );
     }
diff --git a/app/assets/stylesheets/common/base/discourse.scss b/app/assets/stylesheets/common/base/discourse.scss
index 023154c7dab..831aba61b0b 100644
--- a/app/assets/stylesheets/common/base/discourse.scss
+++ b/app/assets/stylesheets/common/base/discourse.scss
@@ -422,7 +422,7 @@ table {
   align-items: center;
 }
 
-.unread-private-messages {
+.unread-high-priority-notifications {
   color: $secondary;
   background: $success;
 
diff --git a/app/assets/stylesheets/common/base/header.scss b/app/assets/stylesheets/common/base/header.scss
index ba3ccb1d2f1..42e57ae88f7 100644
--- a/app/assets/stylesheets/common/base/header.scss
+++ b/app/assets/stylesheets/common/base/header.scss
@@ -202,7 +202,7 @@
     right: -3px;
     background-color: dark-light-choose($tertiary-medium, $tertiary);
   }
-  .unread-private-messages,
+  .unread-high-priority-notifications,
   .ring {
     left: auto;
     right: 25px;
diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb
index d91ee570ec1..7b35e9b3e87 100644
--- a/app/mailers/user_notifications.rb
+++ b/app/mailers/user_notifications.rb
@@ -216,8 +216,8 @@ class UserNotifications < ActionMailer::Base
       value = user.unread_notifications
       @counts << { label_key: 'user_notifications.digest.unread_notifications', value: value, href: "#{Discourse.base_url}/my/notifications" } if value > 0
 
-      value = user.unread_private_messages
-      @counts << { label_key: 'user_notifications.digest.unread_messages', value: value, href: "#{Discourse.base_url}/my/messages" } if value > 0
+      value = user.unread_high_priority_notifications
+      @counts << { label_key: 'user_notifications.digest.unread_high_priority', value: value, href: "#{Discourse.base_url}/my/notifications" } if value > 0
 
       if @counts.size < 3
         value = user.unread_notifications_of_type(Notification.types[:liked])
diff --git a/app/models/notification.rb b/app/models/notification.rb
index a44086f6529..0407f17c96e 100644
--- a/app/models/notification.rb
+++ b/app/models/notification.rb
@@ -44,6 +44,10 @@ class Notification < ActiveRecord::Base
     send_email unless NotificationConsolidator.new(self).consolidate!
   end
 
+  before_create do
+    self.high_priority = Notification.high_priority_types.include?(self.notification_type)
+  end
+
   def self.purge_old!
     return if SiteSetting.max_notifications_per_user == 0
 
@@ -64,10 +68,10 @@ class Notification < ActiveRecord::Base
   end
 
   def self.ensure_consistency!
-    DB.exec(<<~SQL, Notification.types[:private_message])
+    DB.exec(<<~SQL)
       DELETE
         FROM notifications n
-       WHERE notification_type = ?
+       WHERE high_priority
          AND NOT EXISTS (
             SELECT 1
               FROM posts p
@@ -108,6 +112,17 @@ class Notification < ActiveRecord::Base
                        )
   end
 
+  def self.high_priority_types
+    @high_priority_types ||= [
+      types[:private_message],
+      types[:bookmark_reminder]
+    ]
+  end
+
+  def self.normal_priority_types
+    @normal_priority_types ||= types.reject { |_k, v| high_priority_types.include?(v) }.values
+  end
+
   def self.mark_posts_read(user, topic_id, post_numbers)
     Notification
       .where(
@@ -210,14 +225,14 @@ class Notification < ActiveRecord::Base
 
     if notifications.present?
 
-      ids = DB.query_single(<<~SQL, count.to_i)
+      ids = DB.query_single(<<~SQL, limit: count.to_i)
          SELECT n.id FROM notifications n
          WHERE
-           n.notification_type = 6 AND
+           n.high_priority = TRUE AND
            n.user_id = #{user.id.to_i} AND
            NOT read
         ORDER BY n.id ASC
-        LIMIT ?
+        LIMIT :limit
       SQL
 
       if ids.length > 0
@@ -230,9 +245,9 @@ class Notification < ActiveRecord::Base
       end
 
       notifications.uniq(&:id).sort do |x, y|
-        if x.unread_pm? && !y.unread_pm?
+        if x.unread_high_priority? && !y.unread_high_priority?
           -1
-        elsif y.unread_pm? && !x.unread_pm?
+        elsif y.unread_high_priority? && !x.unread_high_priority?
           1
         else
           y.created_at <=> x.created_at
@@ -244,8 +259,8 @@ class Notification < ActiveRecord::Base
 
   end
 
-  def unread_pm?
-    Notification.types[:private_message] == self.notification_type && !read
+  def unread_high_priority?
+    self.high_priority? && !read
   end
 
   def post_id
@@ -280,14 +295,15 @@ end
 #  topic_id          :integer
 #  post_number       :integer
 #  post_action_id    :integer
+#  high_priority     :boolean          default(FALSE), not null
 #
 # Indexes
 #
 #  idx_notifications_speedup_unread_count                       (user_id,notification_type) WHERE (NOT read)
 #  index_notifications_on_post_action_id                        (post_action_id)
-#  index_notifications_on_read_or_n_type                        (user_id,id DESC,read,topic_id) UNIQUE WHERE (read OR (notification_type <> 6))
 #  index_notifications_on_topic_id_and_post_number              (topic_id,post_number)
 #  index_notifications_on_user_id_and_created_at                (user_id,created_at)
-#  index_notifications_on_user_id_and_id                        (user_id,id) UNIQUE WHERE ((notification_type = 6) AND (NOT read))
 #  index_notifications_on_user_id_and_topic_id_and_post_number  (user_id,topic_id,post_number)
+#  index_notifications_read_or_not_high_priority                (user_id,id DESC,read,topic_id) WHERE (read OR (high_priority = false))
+#  index_notifications_unique_unread_high_priority              (user_id,id) UNIQUE WHERE ((NOT read) AND (high_priority = true))
 #
diff --git a/app/models/user.rb b/app/models/user.rb
index 3993d4ea79b..284f15c7883 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -470,6 +470,8 @@ class User < ActiveRecord::Base
     @unread_notifications = nil
     @unread_total_notifications = nil
     @unread_pms = nil
+    @unread_bookmarks = nil
+    @unread_high_prios = nil
     @user_fields_cache = nil
     @ignored_user_ids = nil
     @muted_user_ids = nil
@@ -491,17 +493,41 @@ class User < ActiveRecord::Base
           FROM notifications n
      LEFT JOIN topics t ON t.id = n.topic_id
          WHERE t.deleted_at IS NULL
-           AND n.notification_type = :type
+           AND n.notification_type = :notification_type
            AND n.user_id = :user_id
            AND NOT read
     SQL
 
     # to avoid coalesce we do to_i
-    DB.query_single(sql, user_id: id, type: notification_type)[0].to_i
+    DB.query_single(sql, user_id: id, notification_type: notification_type)[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
+
+  ###
+  # DEPRECATED: This is only maintained for backwards compat until v2.5. There
+  # may be inconsistencies with counts in the UI because of this, because unread
+  # high priority includes PMs AND bookmark reminders.
   def unread_private_messages
-    @unread_pms ||= unread_notifications_of_type(Notification.types[:private_message])
+    @unread_pms ||= unread_high_priority_notifications
+  end
+
+  def unread_high_priority_notifications
+    @unread_high_prios ||= unread_notifications_of_priority(high_priority: true)
   end
 
   # PERF: This safeguard is in place to avoid situations where
@@ -526,7 +552,7 @@ class User < ActiveRecord::Base
           notifications n
           LEFT JOIN topics t ON t.id = n.topic_id
            WHERE t.deleted_at IS NULL AND
-            n.notification_type <> :pm AND
+            n.high_priority = FALSE AND
             n.user_id = :user_id AND
             n.id > :seen_notification_id AND
             NOT read
@@ -537,7 +563,6 @@ class User < ActiveRecord::Base
       DB.query_single(sql,
         user_id: id,
         seen_notification_id: seen_notification_id,
-        pm: Notification.types[:private_message],
         limit: User.max_unread_notifications
     )[0].to_i
     end
@@ -579,7 +604,7 @@ class User < ActiveRecord::Base
          LEFT JOIN topics t ON n.topic_id = t.id
          WHERE
           t.deleted_at IS NULL AND
-          n.notification_type = :type AND
+          n.high_priority AND
           n.user_id = :user_id AND
           NOT read
         ORDER BY n.id DESC
@@ -591,23 +616,21 @@ class User < ActiveRecord::Base
        LEFT JOIN topics t ON n.topic_id = t.id
        WHERE
         t.deleted_at IS NULL AND
-        (n.notification_type <> :type OR read) AND
+        (n.high_priority = FALSE OR read) AND
         n.user_id = :user_id
        ORDER BY n.id DESC
        LIMIT 20
       ) AS y
     SQL
 
-    recent = DB.query(sql,
-      user_id: id,
-      type: Notification.types[:private_message]
-    ).map! do |r|
+    recent = DB.query(sql, user_id: id).map! do |r|
       [r.id, r.read]
     end
 
     payload = {
       unread_notifications: unread_notifications,
       unread_private_messages: unread_private_messages,
+      unread_high_priority_notifications: unread_high_priority_notifications,
       read_first_notification: read_first_notification?,
       last_notification: json,
       recent: recent,
diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb
index 9851baab7d7..5fa2db54e8c 100644
--- a/app/serializers/current_user_serializer.rb
+++ b/app/serializers/current_user_serializer.rb
@@ -5,6 +5,7 @@ class CurrentUserSerializer < BasicUserSerializer
   attributes :name,
              :unread_notifications,
              :unread_private_messages,
+             :unread_high_priority_notifications,
              :read_first_notification?,
              :admin?,
              :notification_channel_position,
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 98d4a5bf1b9..e9bbb296094 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -1812,6 +1812,9 @@ en:
         message:
           one: "%{count} unread message"
           other: "{{count}} unread messages"
+        high_priority:
+          one: "%{count} unread high priority notification"
+          other: "%{count} unread high priority notificationis"
       title: "notifications of @name mentions, replies to your posts and topics, messages, etc"
       none: "Unable to load notifications at this time."
       empty: "No notifications found."
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 8fa0a360870..e3b8271e5df 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -3521,8 +3521,8 @@ en:
       why: "A brief summary of %{site_link} since your last visit on %{last_seen_at}"
       since_last_visit: "Since your last visit"
       new_topics: "New Topics"
-      unread_messages: "Unread Messages"
       unread_notifications: "Unread Notifications"
+      unread_high_priority: "Unread High Priority Notifications"
       liked_received: "Likes Received"
       new_users: "New Users"
       popular_topics: "Popular Topics"
diff --git a/db/migrate/20200329222246_add_high_priority_column_to_notifications.rb b/db/migrate/20200329222246_add_high_priority_column_to_notifications.rb
new file mode 100644
index 00000000000..4bb5f39d1e4
--- /dev/null
+++ b/db/migrate/20200329222246_add_high_priority_column_to_notifications.rb
@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+class AddHighPriorityColumnToNotifications < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+  def up
+    if !column_exists?(:notifications, :high_priority)
+      add_column :notifications, :high_priority, :boolean, default: nil
+    end
+
+    # type 6 = private message, 24 = bookmark reminder
+    # priority 0 = low, 1 = normal, 2 = high
+    if column_exists?(:notifications, :high_priority)
+      execute <<~SQL
+        UPDATE notifications SET high_priority = TRUE WHERE notification_type IN (6, 24);
+      SQL
+
+      execute <<~SQL
+        UPDATE notifications SET high_priority = FALSE WHERE notification_type NOT IN (6, 24);
+      SQL
+
+      execute <<~SQL
+        ALTER TABLE notifications ALTER COLUMN high_priority SET DEFAULT FALSE;
+      SQL
+
+      execute <<~SQL
+        ALTER TABLE notifications ALTER COLUMN high_priority SET NOT NULL;
+      SQL
+    end
+
+    execute <<~SQL
+      CREATE INDEX CONCURRENTLY IF NOT EXISTS index_notifications_read_or_not_high_priority ON notifications(user_id, id DESC, read, topic_id) WHERE (read OR (high_priority = FALSE));
+    SQL
+
+    execute <<~SQL
+      CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS index_notifications_unique_unread_high_priority ON notifications(user_id, id) WHERE NOT read AND high_priority = TRUE;
+    SQL
+  end
+
+  def down
+    DB.exec("ALTER TABLE notifications DROP COLUMN IF EXISTS high_priority")
+  end
+end
diff --git a/db/post_migrate/20200330233427_drop_old_unread_pm_notification_indices.rb b/db/post_migrate/20200330233427_drop_old_unread_pm_notification_indices.rb
new file mode 100644
index 00000000000..33f27cffa24
--- /dev/null
+++ b/db/post_migrate/20200330233427_drop_old_unread_pm_notification_indices.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class DropOldUnreadPmNotificationIndices < ActiveRecord::Migration[6.0]
+  def up
+    DB.exec("DROP INDEX IF EXISTS index_notifications_on_user_id_and_id")
+    DB.exec("DROP INDEX IF EXISTS index_notifications_on_read_or_n_type")
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
diff --git a/spec/fabricators/notification_fabricator.rb b/spec/fabricators/notification_fabricator.rb
index 6c386c73369..6ee4c1bcfb9 100644
--- a/spec/fabricators/notification_fabricator.rb
+++ b/spec/fabricators/notification_fabricator.rb
@@ -3,6 +3,7 @@
 Fabricator(:notification) do
   transient :post
   notification_type Notification.types[:mentioned]
+  high_priority false
   user
   topic { |attrs| attrs[:post]&.topic || Fabricate(:topic, user: attrs[:user]) }
   post_number { |attrs| attrs[:post]&.post_number }
@@ -17,6 +18,7 @@ end
 
 Fabricator(:private_message_notification, from: :notification) do
   notification_type Notification.types[:private_message]
+  high_priority true
   data do |attrs|
     post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user])
     {
@@ -30,6 +32,23 @@ Fabricator(:private_message_notification, from: :notification) do
   end
 end
 
+Fabricator(:bookmark_reminder_notification, from: :notification) do
+  notification_type Notification.types[:bookmark_reminder]
+  high_priority true
+  data do |attrs|
+    post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user])
+    {
+      topic_title: attrs[:topic].title,
+      original_post_id: post.id,
+      original_post_type: post.post_type,
+      original_username: post.user.username,
+      revision_number: nil,
+      display_username: post.user.username,
+      bookmark_name: "Check out Mr Freeze's opinion here"
+    }.to_json
+  end
+end
+
 Fabricator(:replied_notification, from: :notification) do
   notification_type Notification.types[:replied]
   data do |attrs|
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index f8bf62a82fd..9f0a38a8363 100644
--- a/spec/models/notification_spec.rb
+++ b/spec/models/notification_spec.rb
@@ -117,6 +117,24 @@ describe Notification do
       it "increases unread_private_messages" do
         expect { Fabricate(:private_message_notification, user: user); user.reload }.to change(user, :unread_private_messages)
       end
+
+      it "increases unread_high_priority_notifications" do
+        expect { Fabricate(:private_message_notification, user: user); user.reload }.to change(user, :unread_high_priority_notifications)
+      end
+    end
+
+    context 'a bookmark reminder message' do
+      it "doesn't increase unread_notifications" do
+        expect { Fabricate(:bookmark_reminder_notification, user: user); user.reload }.not_to change(user, :unread_notifications)
+      end
+
+      it 'increases total_unread_notifications' do
+        expect { Fabricate(:notification, user: user); user.reload }.to change(user, :total_unread_notifications)
+      end
+
+      it "increases unread_high_priority_notifications" do
+        expect { Fabricate(:bookmark_reminder_notification, user: user); user.reload }.to change(user, :unread_high_priority_notifications)
+      end
     end
 
   end
@@ -219,6 +237,13 @@ describe Notification do
                            data: '{}',
                            notification_type: Notification.types[:private_message])
 
+      Notification.create!(read: false,
+                           user_id: user.id,
+                           topic_id: t.id,
+                           post_number: 1,
+                           data: '{}',
+                           notification_type: Notification.types[:bookmark_reminder])
+
       other = Notification.create!(read: false,
                                    user_id: user.id,
                                    topic_id: t.id,
@@ -230,8 +255,11 @@ describe Notification do
       user.reload
 
       expect(user.unread_notifications).to eq(0)
-      expect(user.total_unread_notifications).to eq(2)
-      expect(user.unread_private_messages).to eq(1)
+      expect(user.total_unread_notifications).to eq(3)
+      # NOTE: because of deprecation this will be equal to unread_high_priority_notifications,
+      #       to be remonved in 2.5
+      expect(user.unread_private_messages).to eq(2)
+      expect(user.unread_high_priority_notifications).to eq(2)
     end
   end
 
@@ -248,7 +276,7 @@ describe Notification do
     end
   end
 
-  describe 'ensure consistency' do
+  describe '#ensure_consistency!' do
     it 'deletes notifications if post is missing or deleted' do
 
       NotificationEmailer.disable
@@ -260,6 +288,8 @@ describe Notification do
                            notification_type: Notification.types[:private_message])
       Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
                            notification_type: Notification.types[:private_message])
+      Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
+                           notification_type: Notification.types[:bookmark_reminder])
 
       Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
                            notification_type: Notification.types[:liked])
@@ -321,6 +351,10 @@ describe Notification do
       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
@@ -340,16 +374,17 @@ describe Notification do
       expect(Notification.visible.count).to eq(0)
     end
 
-    it 'orders stuff correctly' do
+    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
+      regular
+      b = unread_bookmark_reminder
       c = pm
       d = regular
 
-      # bumps unread pms to front of list
-
-      notifications = Notification.recent_report(user, 3)
-      expect(notifications.map { |n| n.id }).to eq([a.id, d.id, c.id])
+      notifications = Notification.recent_report(user, 4)
+      expect(notifications.map { |n| n.id }).to eq([b.id, a.id, d.id, c.id])
 
     end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 995a532290a..bb27aa4af0a 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1779,7 +1779,7 @@ describe User do
   end
 
   describe '#publish_notifications_state' do
-    it 'should publish the right message' do
+    it 'should publish the right message sorted by ID desc' do
       notification = Fabricate(:notification, user: user)
       notification2 = Fabricate(:notification, user: user, read: true)
 
@@ -1788,8 +1788,41 @@ describe User do
       end.first
 
       expect(message.data[:recent]).to eq([
-                                            [notification2.id, true], [notification.id, false]
-                                          ])
+        [notification2.id, true], [notification.id, false]
+      ])
+    end
+
+    it 'floats the unread high priority notifications to the top' do
+      notification = Fabricate(:notification, user: user)
+      notification2 = Fabricate(:notification, user: user, read: true)
+      notification3 = Fabricate(:notification, user: user, notification_type: Notification.types[:private_message])
+      notification4 = Fabricate(:notification, user: user, notification_type: Notification.types[:bookmark_reminder])
+
+      message = MessageBus.track_publish("/notification/#{user.id}") do
+        user.publish_notifications_state
+      end.first
+
+      expect(message.data[:recent]).to eq([
+        [notification4.id, false], [notification3.id, false],
+        [notification2.id, true], [notification.id, false]
+      ])
+    end
+
+    it "has the correct counts" do
+      notification = Fabricate(:notification, user: user)
+      notification2 = Fabricate(:notification, user: user, read: true)
+      notification3 = Fabricate(:notification, user: user, notification_type: Notification.types[:private_message])
+      notification4 = Fabricate(:notification, user: user, notification_type: Notification.types[:bookmark_reminder])
+
+      message = MessageBus.track_publish("/notification/#{user.id}") do
+        user.publish_notifications_state
+      end.first
+
+      expect(message.data[:unread_notifications]).to eq(1)
+      # NOTE: because of deprecation this will be equal to unread_high_priority_notifications,
+      #       to be remonved in 2.5
+      expect(message.data[:unread_private_messages]).to eq(2)
+      expect(message.data[:unread_high_priority_notifications]).to eq(2)
     end
   end
 
@@ -1821,6 +1854,7 @@ describe User do
   end
 
   describe "#unread_notifications" do
+    fab!(:user) { Fabricate(:user) }
     before do
       User.max_unread_notifications = 3
     end
@@ -1830,14 +1864,32 @@ describe User do
     end
 
     it "limits to MAX_UNREAD_NOTIFICATIONS" do
-      user = Fabricate(:user)
-
       4.times do
         Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
       end
 
       expect(user.unread_notifications).to eq(3)
     end
+
+    it "does not include high priority notifications" do
+      Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
+      Notification.create!(user_id: user.id, notification_type: Notification.types[:private_message], read: false, data: '{}')
+      Notification.create!(user_id: user.id, notification_type: Notification.types[:bookmark_reminder], read: false, data: '{}')
+
+      expect(user.unread_notifications).to eq(1)
+    end
+  end
+
+  describe "#unread_high_priority_notifications" do
+    fab!(:user) { Fabricate(:user) }
+
+    it "only returns an unread count of PM and bookmark reminder notifications" do
+      Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
+      Notification.create!(user_id: user.id, notification_type: Notification.types[:private_message], read: false, data: '{}')
+      Notification.create!(user_id: user.id, notification_type: Notification.types[:bookmark_reminder], read: false, data: '{}')
+
+      expect(user.unread_high_priority_notifications).to eq(2)
+    end
   end
 
   describe "#unstage!" do
diff --git a/test/javascripts/fixtures/session-fixtures.js b/test/javascripts/fixtures/session-fixtures.js
index 090bf099366..fe26b20cd6c 100644
--- a/test/javascripts/fixtures/session-fixtures.js
+++ b/test/javascripts/fixtures/session-fixtures.js
@@ -8,6 +8,7 @@ export default {
       name: "Robin Ward",
       unread_notifications: 0,
       unread_private_messages: 0,
+      unread_high_priority_notifications: 0,
       admin: true,
       notification_channel_position: null,
       site_flagged_posts_count: 1,