FIX: Maintain notification order by priority (#13186)

FIX: Maintain notification order by priority (#13186)

When the client received a new notification, it prioritized only PM notifications instead of maintaining the priority order. Later, the check for missing notification deleted all notifications that were in the wrong order because it could not match the IDs.

The correct order puts high_priority AND unread notifications first. Low priority or read notifications (including high priority, but read notifications) come after.

diff --git a/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js b/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js
index 2e93d64..52895fd 100644
--- a/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js
+++ b/app/assets/javascripts/discourse/app/initializers/subscribe-user-notifications.js
@@ -73,17 +73,19 @@ export default {
             );
 
             if (staleIndex === -1) {
-              // this gets a bit tricky, unread pms are bumped to front
+              // high priority and unread notifications are first
               let insertPosition = 0;
-              if (lastNotification.notification_type !== 6) {
-                insertPosition = oldNotifications.findIndex(
-                  (n) => n.notification_type !== 6 || n.read
+
+              if (!lastNotification.high_priority || lastNotification.read) {
+                const nextPosition = oldNotifications.findIndex(
+                  (n) => !n.high_priority || n.read
                 );
-                insertPosition =
-                  insertPosition === -1
-                    ? oldNotifications.length - 1
-                    : insertPosition;
+
+                if (nextPosition !== -1) {
+                  insertPosition = nextPosition;
+                }
               }
+
               oldNotifications.insertAt(
                 insertPosition,
                 EmberObject.create(lastNotification)
diff --git a/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js b/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js
new file mode 100644
index 0000000..4ff691b
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js
@@ -0,0 +1,136 @@
+import { visit } from "@ember/test-helpers";
+import {
+  acceptance,
+  publishToMessageBus,
+} from "discourse/tests/helpers/qunit-helpers";
+import { test } from "qunit";
+
+acceptance("User Notifications", function (needs) {
+  needs.user();
+
+  test("Update works correctly", async function (assert) {
+    await visit("/");
+    await click("li.current-user");
+
+    // set older notifications to read
+
+    publishToMessageBus("/notification/19", {
+      unread_notifications: 5,
+      unread_private_messages: 0,
+      unread_high_priority_notifications: 0,
+      read_first_notification: false,
+      last_notification: {},
+      recent: [
+        [123, false],
+        [456, false],
+        [789, true],
+        [1234, true],
+        [5678, true],
+      ],
+      seen_notification_id: null,
+    });
+
+    await visit("/"); // wait for re-render
+
+    assert.equal(find("#quick-access-notifications li").length, 5);
+
+    // high priority, unread notification - should be first
+
+    publishToMessageBus("/notification/19", {
+      unread_notifications: 6,
+      unread_private_messages: 0,
+      unread_high_priority_notifications: 1,
+      read_first_notification: false,
+      last_notification: {
+        notification: {
+          id: 42,
+          user_id: 1,
+          notification_type: 5,
+          high_priority: true,
+          read: false,
+          high_priority: true,
+          created_at: "2021-01-01 12:00:00 UTC",
+          post_number: 1,
+          topic_id: 42,
+          fancy_title: "First notification",
+          slug: "topic",
+          data: {
+            topic_title: "First notification",
+            original_post_id: 42,
+            original_post_type: 1,
+            original_username: "foo",
+            revision_number: null,
+            display_username: "foo",
+          },
+        },
+      },
+      recent: [
+        [42, false],
+        [123, false],
+        [456, false],
+        [789, true],
+        [1234, true],
+        [5678, true],
+      ],
+      seen_notification_id: null,
+    });
+
+    await visit("/"); // wait for re-render
+
+    assert.equal(find("#quick-access-notifications li").length, 6);
+    assert.equal(
+      find("#quick-access-notifications li span[data-topic-id]")[0].innerText,
+      "First notification"
+    );
+
+    // high priority, read notification - should be second
+
+    publishToMessageBus("/notification/19", {
+      unread_notifications: 7,
+      unread_private_messages: 0,
+      unread_high_priority_notifications: 1,
+      read_first_notification: false,
+      last_notification: {
+        notification: {
+          id: 43,
+          user_id: 1,
+          notification_type: 5,
+          high_priority: true,
+          read: true,
+          high_priority: false,
+          created_at: "2021-01-01 12:00:00 UTC",
+          post_number: 1,
+          topic_id: 42,
+          fancy_title: "Second notification",
+          slug: "topic",
+          data: {
+            topic_title: "Second notification",
+            original_post_id: 42,
+            original_post_type: 1,
+            original_username: "foo",
+            revision_number: null,
+            display_username: "foo",
+          },
+        },
+      },
+      recent: [
+        [42, false],
+        [43, true],
+        [123, false],
+        [456, false],
+        [789, true],
+        [1234, true],
+        [5678, true],
+      ],
+      seen_notification_id: null,
+    });
+
+    await visit("/"); // wait for re-render
+
+    assert.equal(find("#quick-access-notifications li").length, 7);
+    assert.equal(
+      find("#quick-access-notifications li span[data-topic-id]")[1].innerText,
+      "Second notification"
+    );
+  });
+});
diff --git a/app/serializers/notification_serializer.rb b/app/serializers/notification_serializer.rb
index f4b7119..40ff319 100644
--- a/app/serializers/notification_serializer.rb
+++ b/app/serializers/notification_serializer.rb
@@ -7,6 +7,7 @@ class NotificationSerializer < ApplicationSerializer
              :external_id,
              :notification_type,
              :read,
+             :high_priority,
              :created_at,
              :post_number,
              :topic_id,

GitHub sha: f3fdc7a6

This commit appears in #13186 which was approved by eviltrout and ZogStriP. It was merged by udan11.