From f3fdc7a6e866bfb40b1fbb54412ed9eeedcf35ed Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Mon, 31 May 2021 09:27:13 +0300 Subject: [PATCH] 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. --- .../subscribe-user-notifications.js | 18 +-- .../tests/acceptance/notifications-test.js | 136 ++++++++++++++++++ app/serializers/notification_serializer.rb | 1 + 3 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/acceptance/notifications-test.js 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 2e93d643ea..52895fd0ad 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 0000000000..4ff691b932 --- /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 f4b7119b02..40ff319b76 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,