From 1fa21ed415c6c8d7edc44cb77a2019f16f8fdd26 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Mon, 12 Sep 2022 21:19:25 +0300 Subject: [PATCH] DEV: Prioritize unread notifications in the experimental user menu (#18216) Right now the experimental user menu sorts notifications the same way that the old menu does: unread high-priority notifications are shown first in reverse-chronological order followed by everything else also in reverse-chronological order. However, since the experimental user menu has dedicated tabs for some notification types and each tab displays a badge with the count of unread notifications in the tab, we feel like it makes sense to change how notifications are sorted in the experimental user menu to this: 1. unread high-priority notifications 2. unread regular notifications 3. all read notifications (both high-priority and regular) 4. within each group, notifications are sorted in reverse-chronological order (i.e. newest is shown first). This new sorting logic applies to all tabs in the experimental user menu, however it doesn't change anything in the old menu. With this change, if a tab in the experimental user menu shows an unread notification badge for a really old notification, it will be surfaced to the top and prevents confusing scenarios where a user sees an unread notification badge on a tab, but the tab doesn't show the unread notification because it's too old to make it to the list. Internal topic: t72199. --- app/controllers/notifications_controller.rb | 21 ++-- app/models/notification.rb | 30 +++++ app/models/user.rb | 16 +++ spec/models/notification_spec.rb | 119 +++++++++++++++++- spec/models/user_spec.rb | 18 +++ .../requests/notifications_controller_spec.rb | 65 ++++++++++ 6 files changed, 256 insertions(+), 13 deletions(-) diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 0f8eb4126db..a33964575d7 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -30,18 +30,17 @@ class NotificationsController < ApplicationController limit = (params[:limit] || 15).to_i limit = 50 if limit > 50 - notifications = Notification.recent_report(current_user, limit, notification_types) - changed = false - - if notifications.present? && !(params.has_key?(:silent) || @readonly_mode) - # ordering can be off due to PMs - max_id = notifications.map(&:id).max - changed = current_user.saw_notification_id(max_id) + if SiteSetting.enable_experimental_sidebar_hamburger + notifications = Notification.prioritized_list(current_user, count: limit, types: notification_types) + else + notifications = Notification.recent_report(current_user, limit, notification_types) end - if changed - current_user.reload - current_user.publish_notifications_state + if notifications.present? && !(params.has_key?(:silent) || @readonly_mode) + if changed = 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 @@ -103,7 +102,7 @@ class NotificationsController < ApplicationController end Notification.read_types(current_user, types) - current_user.saw_notification_id(Notification.recent_report(current_user, 1).max.try(:id)) + current_user.bump_last_seen_notification! end current_user.reload diff --git a/app/models/notification.rb b/app/models/notification.rb index 70bf5547751..fc4e8ffe69a 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -18,6 +18,12 @@ class Notification < ActiveRecord::Base scope :unread_type, ->(user, type, limit = 20) do where(user_id: user.id, read: false, notification_type: type).visible.includes(:topic).limit(limit) end + scope :prioritized, ->(limit = nil) do + order("notifications.high_priority AND NOT notifications.read DESC") + .order("NOT notifications.read DESC") + .order("notifications.created_at DESC") + .limit(limit || 30) + end attr_accessor :skip_send_email @@ -214,6 +220,30 @@ class Notification < ActiveRecord::Base Post.find_by(topic_id: topic_id, post_number: post_number) end + def self.prioritized_list(user, count: 30, types: []) + return [] if !user&.user_option + + notifications = user.notifications + .includes(:topic) + .visible + .prioritized(count) + + if types.present? + notifications = notifications.where(notification_type: types) + elsif 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.to_a + end + + # TODO(osama): deprecate this method when redesigned_user_menu_enabled is removed def self.recent_report(user, count = nil, types = []) return unless user && user.user_option diff --git a/app/models/user.rb b/app/models/user.rb index 62815394345..ff8cb516266 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -642,6 +642,9 @@ class User < ActiveRecord::Base end def saw_notification_id(notification_id) + Discourse.deprecate(<<~TEXT, since: "2.9", drop_from: "3.0") + User#saw_notification_id is deprecated. Please use User#bump_last_seen_notification! instead. + TEXT if seen_notification_id.to_i < notification_id.to_i update_columns(seen_notification_id: notification_id.to_i) true @@ -650,6 +653,19 @@ class User < ActiveRecord::Base end end + def bump_last_seen_notification! + query = self.notifications.visible + if seen_notification_id + query = query.where("notifications.id > ?", seen_notification_id) + end + if max_notification_id = query.maximum(:id) + update!(seen_notification_id: max_notification_id) + true + else + false + end + end + def bump_last_seen_reviewable! query = Reviewable.unseen_list_for(self, preload: false) diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index d43bcf31be8..dde6dfd83a7 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -289,7 +289,7 @@ RSpec.describe Notification do data: '{}', notification_type: Notification.types[:mentioned]) - user.saw_notification_id(other.id) + user.bump_last_seen_notification! user.reload expect(user.unread_notifications).to eq(0) @@ -366,8 +366,123 @@ end # pulling this out cause I don't want an observer RSpec.describe Notification do + fab!(:user) { Fabricate(:user) } + + describe '.prioritized_list' do + def create(**opts) + opts[:user] = user if !opts[:user] + Fabricate(:notification, user: user, **opts) + end + + fab!(:unread_high_priority_1) { create(high_priority: true, read: false, created_at: 8.minutes.ago) } + fab!(:read_high_priority_1) { create(high_priority: true, read: true, created_at: 7.minutes.ago) } + fab!(:unread_regular_1) { create(high_priority: false, read: false, created_at: 6.minutes.ago) } + fab!(:read_regular_1) { create(high_priority: false, read: true, created_at: 5.minutes.ago) } + + fab!(:unread_high_priority_2) { create(high_priority: true, read: false, created_at: 1.minutes.ago) } + fab!(:read_high_priority_2) { create(high_priority: true, read: true, created_at: 2.minutes.ago) } + fab!(:unread_regular_2) { create(high_priority: false, read: false, created_at: 3.minutes.ago) } + fab!(:read_regular_2) { create(high_priority: false, read: true, created_at: 4.minutes.ago) } + + it "puts unread high_priority on top followed by unread normal notifications and then everything else in reverse chronological order" do + expect(Notification.prioritized_list(user).map(&:id)).to eq([ + unread_high_priority_2, + unread_high_priority_1, + unread_regular_2, + unread_regular_1, + read_high_priority_2, + read_regular_2, + read_regular_1, + read_high_priority_1, + ].map(&:id)) + end + + it "doesn't include notifications from other users" do + another_user_notification = create(high_priority: true, read: false, user: Fabricate(:user)) + expect(Notification.prioritized_list(user).map(&:id)).to contain_exactly(*[ + unread_high_priority_2, + unread_high_priority_1, + unread_regular_2, + unread_regular_1, + read_high_priority_2, + read_regular_2, + read_regular_1, + read_high_priority_1, + ].map(&:id)) + expect( + Notification.prioritized_list(another_user_notification.user).map(&:id) + ).to contain_exactly(another_user_notification.id) + end + + it "doesn't include notifications from deleted topics" do + unread_high_priority_1.topic.trash! + unread_regular_2.topic.trash! + read_regular_1.topic.trash! + expect(Notification.prioritized_list(user).map(&:id)).to contain_exactly(*[ + unread_high_priority_2, + unread_regular_1, + read_high_priority_2, + read_regular_2, + read_high_priority_1, + ].map(&:id)) + end + + it "doesn't include like notifications if the user doesn't want like notifications" do + user.user_option.update!( + like_notification_frequency: + UserOption.like_notification_frequency_type[:never] + ) + unread_regular_1.update!(notification_type: Notification.types[:liked]) + read_regular_2.update!(notification_type: Notification.types[:liked_consolidated]) + expect(Notification.prioritized_list(user).map(&:id)).to eq([ + unread_high_priority_2, + unread_high_priority_1, + unread_regular_2, + read_high_priority_2, + read_regular_1, + read_high_priority_1, + ].map(&:id)) + end + + it "respects the count param" do + expect(Notification.prioritized_list(user, count: 1).map(&:id)).to eq([ + unread_high_priority_2, + ].map(&:id)) + expect(Notification.prioritized_list(user, count: 3).map(&:id)).to eq([ + unread_high_priority_2, + unread_high_priority_1, + unread_regular_2, + ].map(&:id)) + end + + it "can filter the list by specific types" do + unread_regular_1.update!(notification_type: Notification.types[:liked]) + read_regular_2.update!(notification_type: Notification.types[:liked_consolidated]) + expect(Notification.prioritized_list( + user, + types: [Notification.types[:liked], Notification.types[:liked_consolidated]] + ).map(&:id)).to eq([unread_regular_1, read_regular_2].map(&:id)) + end + + it "includes like notifications when filtering by like types even if the user doesn't want like notifications" do + user.user_option.update!( + like_notification_frequency: + UserOption.like_notification_frequency_type[:never] + ) + unread_regular_1.update!(notification_type: Notification.types[:liked]) + read_regular_2.update!(notification_type: Notification.types[:liked_consolidated]) + expect(Notification.prioritized_list( + user, + types: [Notification.types[:liked], Notification.types[:liked_consolidated]] + ).map(&:id)).to eq([unread_regular_1, read_regular_2].map(&:id)) + expect(Notification.prioritized_list( + user, + types: [Notification.types[:liked]] + ).map(&:id)).to contain_exactly(unread_regular_1.id) + end + end + describe '#recent_report' do - fab!(:user) { Fabricate(:user) } let(:post) { Fabricate(:post) } def fab(type, read) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9cf035f0654..d2e01363cfb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2955,4 +2955,22 @@ RSpec.describe User do ) end end + + describe "#bump_last_seen_notification!" do + it "doesn't error if there are no notifications" do + Notification.destroy_all + expect(user.bump_last_seen_notification!).to eq(false) + expect(user.reload.seen_notification_id).to eq(0) + end + + it "updates seen_notification_id to the last notification that the user can see" do + last_notification = Fabricate(:notification, user: user) + deleted_notification = Fabricate(:notification, user: user) + deleted_notification.topic.trash! + someone_else_notification = Fabricate(:notification, user: Fabricate(:user)) + + expect(user.bump_last_seen_notification!).to eq(true) + expect(user.reload.seen_notification_id).to eq(last_notification.id) + end + end end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index 85f5f2ddb2e..2bade5ac1e4 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -165,6 +165,71 @@ RSpec.describe NotificationsController do expect(JSON.parse(response.body)['notifications'][0]['read']).to eq(false) end + context "with the enable_experimental_sidebar_hamburger setting" do + fab!(:unread_high_priority) do + Fabricate( + :notification, + user: user, + high_priority: true, + read: false, + created_at: 10.minutes.ago + ) + end + fab!(:read_high_priority) do + Fabricate( + :notification, + user: user, + high_priority: true, + read: true, + created_at: 8.minutes.ago + ) + end + fab!(:unread_regular) do + Fabricate( + :notification, + user: user, + high_priority: false, + read: false, + created_at: 6.minutes.ago + ) + end + fab!(:read_regular) do + Fabricate( + :notification, + user: user, + high_priority: false, + read: true, + created_at: 4.minutes.ago + ) + end + + it "gets notifications list with unread ones at the top when the setting is enabled" do + SiteSetting.enable_experimental_sidebar_hamburger = true + get "/notifications.json", params: { recent: true } + expect(response.status).to eq(200) + expect(response.parsed_body["notifications"].map { |n| n["id"] }).to eq([ + unread_high_priority.id, + notification.id, + unread_regular.id, + read_regular.id, + read_high_priority.id + ]) + end + + it "gets notifications list with unread high priority notifications at the top when the setting is disabled" do + SiteSetting.enable_experimental_sidebar_hamburger = false + get "/notifications.json", params: { recent: true } + expect(response.status).to eq(200) + expect(response.parsed_body["notifications"].map { |n| n["id"] }).to eq([ + unread_high_priority.id, + notification.id, + read_regular.id, + unread_regular.id, + read_high_priority.id + ]) + end + end + context "when filter_by_types param is present" do fab!(:liked1) do Fabricate(