Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Osama Sayegh 2022-09-12 21:19:25 +03:00 committed by GitHub
parent 321aa4b4b4
commit 1fa21ed415
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 256 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
app/controllers/notifications_controller.rb
View File

@ -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

30
app/models/notification.rb
View File

@ -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

16
app/models/user.rb
View File

@ -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)

119
spec/models/notification_spec.rb
View File

@ -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)

18
spec/models/user_spec.rb
View File

@ -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

65
spec/requests/notifications_controller_spec.rb
View File

@ -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(