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(