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

FEATURE: High priority bookmark reminder notifications (#9290) 

Introduce the concept of "high priority notifications" which include PM and bookmark reminder notifications. Now bookmark reminder notifications act in the same way as PM notifications (float to top of recent list, show in the green bubble) and most instances of unread_private_messages in the UI have been replaced with unread_high_priority_notifications.

The user email digest is changed to just have a section about unread high priority notifications, the unread PM section has been removed.

A high_priority boolean column has been added to the Notification table and relevant indices added to account for it.

unread_private_messages has been kept on the User model purely for backwards compat, but now just returns unread_high_priority_notifications count so this may cause some inconsistencies in the UI.
This commit is contained in:
Martin Brennan 2020-04-01 09:09:20 +10:00 committed by GitHub
parent b2a0d34bb7
commit b79ea986ac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 263 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/assets/javascripts/discourse/components/site-header.js
View File

@ -24,7 +24,7 @@ const SiteHeaderComponent = MountWidget.extend(Docking, PanEvents, {
@observes(
"currentUser.unread_notifications",
"currentUser.unread_private_messages",
"currentUser.unread_high_priority_notifications",
"currentUser.reviewable_count"
)
notificationsChanged() {

2
app/assets/javascripts/discourse/initializers/badging.js
View File

@ -10,7 +10,7 @@ export default {
if (!user) return; // must be logged in
this.notifications =
user.unread_notifications + user.unread_private_messages;
user.unread_notifications + user.unread_high_priority_notifications;
container
.lookup("service:app-events")

11
app/assets/javascripts/discourse/initializers/subscribe-user-notifications.js
View File

@ -32,24 +32,27 @@ export default {
data => {
const store = container.lookup("service:store");
const oldUnread = user.get("unread_notifications");
const oldPM = user.get("unread_private_messages");
const oldHighPriority = user.get(
"unread_high_priority_notifications"
);
user.setProperties({
unread_notifications: data.unread_notifications,
unread_private_messages: data.unread_private_messages,
unread_high_priority_notifications:
data.unread_high_priority_notifications,
read_first_notification: data.read_first_notification
});
if (
oldUnread !== data.unread_notifications ||
oldPM !== data.unread_private_messages
oldHighPriority !== data.unread_high_priority_notifications
) {
appEvents.trigger("notifications:changed");
if (
site.mobileView &&
(data.unread_notifications - oldUnread > 0 ||
data.unread_private_messages - oldPM > 0)
data.unread_high_priority_notifications - oldHighPriority > 0)
) {
appEvents.trigger("header:update-topic", null, 5000);
}

2
app/assets/javascripts/discourse/initializers/title-notifications.js
View File

@ -18,7 +18,7 @@ export default {
if (!user) return; // must be logged in
const notifications =
user.unread_notifications + user.unread_private_messages;
user.unread_notifications + user.unread_high_priority_notifications;
Discourse.updateNotificationCount(notifications);
}

14
app/assets/javascripts/discourse/widgets/header.js
View File

@ -67,8 +67,9 @@ createWidget("header-notifications", {
);
}
const unreadPMs = user.get("unread_private_messages");
if (!!unreadPMs) {
const unreadHighPriority = user.get("unread_high_priority_notifications");
if (!!unreadHighPriority) {
// highlight the avatar if the first ever PM is not read
if (
!user.get("read_first_notification") &&
!user.get("enforcedSecondFactor")
@ -90,14 +91,15 @@ createWidget("header-notifications", {
}
}
// add the counter for the unread high priority
contents.push(
this.attach("link", {
action: attrs.action,
className: "badge-notification unread-private-messages",
rawLabel: unreadPMs,
className: "badge-notification unread-high-priority-notifications",
rawLabel: unreadHighPriority,
omitSpan: true,
title: "notifications.tooltip.message",
titleOptions: { count: unreadPMs }
title: "notifications.tooltip.high_priority",
titleOptions: { count: unreadHighPriority }
})
);
}

2
app/assets/stylesheets/common/base/discourse.scss
View File

@ -422,7 +422,7 @@ table {
align-items: center;
}
.unread-private-messages {
.unread-high-priority-notifications {
color: $secondary;
background: $success;

2
app/assets/stylesheets/common/base/header.scss
View File

@ -202,7 +202,7 @@
right: -3px;
background-color: dark-light-choose($tertiary-medium, $tertiary);
}
.unread-private-messages,
.unread-high-priority-notifications,
.ring {
left: auto;
right: 25px;

4
app/mailers/user_notifications.rb
View File

@ -216,8 +216,8 @@ class UserNotifications < ActionMailer::Base
value = user.unread_notifications
@counts << { label_key: 'user_notifications.digest.unread_notifications', value: value, href: "#{Discourse.base_url}/my/notifications" } if value > 0
value = user.unread_private_messages
@counts << { label_key: 'user_notifications.digest.unread_messages', value: value, href: "#{Discourse.base_url}/my/messages" } if value > 0
value = user.unread_high_priority_notifications
@counts << { label_key: 'user_notifications.digest.unread_high_priority', value: value, href: "#{Discourse.base_url}/my/notifications" } if value > 0
if @counts.size < 3
value = user.unread_notifications_of_type(Notification.types[:liked])

38
app/models/notification.rb
View File

@ -44,6 +44,10 @@ class Notification < ActiveRecord::Base
send_email unless NotificationConsolidator.new(self).consolidate!
end
before_create do
self.high_priority = Notification.high_priority_types.include?(self.notification_type)
end
def self.purge_old!
return if SiteSetting.max_notifications_per_user == 0
@ -64,10 +68,10 @@ class Notification < ActiveRecord::Base
end
def self.ensure_consistency!
DB.exec(<<~SQL, Notification.types[:private_message])
DB.exec(<<~SQL)
DELETE
FROM notifications n
WHERE notification_type = ?
WHERE high_priority
AND NOT EXISTS (
SELECT 1
FROM posts p
@ -108,6 +112,17 @@ class Notification < ActiveRecord::Base
)
end
def self.high_priority_types
@high_priority_types ||= [
types[:private_message],
types[:bookmark_reminder]
]
end
def self.normal_priority_types
@normal_priority_types ||= types.reject { |_k, v| high_priority_types.include?(v) }.values
end
def self.mark_posts_read(user, topic_id, post_numbers)
Notification
.where(
@ -210,14 +225,14 @@ class Notification < ActiveRecord::Base
if notifications.present?
ids = DB.query_single(<<~SQL, count.to_i)
ids = DB.query_single(<<~SQL, limit: count.to_i)
SELECT n.id FROM notifications n
WHERE
n.notification_type = 6 AND
n.high_priority = TRUE AND
n.user_id = #{user.id.to_i} AND
NOT read
ORDER BY n.id ASC
LIMIT ?
LIMIT :limit
SQL
if ids.length > 0
@ -230,9 +245,9 @@ class Notification < ActiveRecord::Base
end
notifications.uniq(&:id).sort do |x, y|
if x.unread_pm? && !y.unread_pm?
if x.unread_high_priority? && !y.unread_high_priority?
-1
elsif y.unread_pm? && !x.unread_pm?
elsif y.unread_high_priority? && !x.unread_high_priority?
1
else
y.created_at <=> x.created_at
@ -244,8 +259,8 @@ class Notification < ActiveRecord::Base
end
def unread_pm?
Notification.types[:private_message] == self.notification_type && !read
def unread_high_priority?
self.high_priority? && !read
end
def post_id
@ -280,14 +295,15 @@ end
# topic_id :integer
# post_number :integer
# post_action_id :integer
# high_priority :boolean default(FALSE), not null
#
# Indexes
#
# idx_notifications_speedup_unread_count (user_id,notification_type) WHERE (NOT read)
# index_notifications_on_post_action_id (post_action_id)
# index_notifications_on_read_or_n_type (user_id,id DESC,read,topic_id) UNIQUE WHERE (read OR (notification_type <> 6))
# index_notifications_on_topic_id_and_post_number (topic_id,post_number)
# index_notifications_on_user_id_and_created_at (user_id,created_at)
# index_notifications_on_user_id_and_id (user_id,id) UNIQUE WHERE ((notification_type = 6) AND (NOT read))
# index_notifications_on_user_id_and_topic_id_and_post_number (user_id,topic_id,post_number)
# index_notifications_read_or_not_high_priority (user_id,id DESC,read,topic_id) WHERE (read OR (high_priority = false))
# index_notifications_unique_unread_high_priority (user_id,id) UNIQUE WHERE ((NOT read) AND (high_priority = true))
#

45
app/models/user.rb
View File

@ -470,6 +470,8 @@ class User < ActiveRecord::Base
@unread_notifications = nil
@unread_total_notifications = nil
@unread_pms = nil
@unread_bookmarks = nil
@unread_high_prios = nil
@user_fields_cache = nil
@ignored_user_ids = nil
@muted_user_ids = nil
@ -491,17 +493,41 @@ class User < ActiveRecord::Base
FROM notifications n
LEFT JOIN topics t ON t.id = n.topic_id
WHERE t.deleted_at IS NULL
AND n.notification_type = :type
AND n.notification_type = :notification_type
AND n.user_id = :user_id
AND NOT read
SQL
# to avoid coalesce we do to_i
DB.query_single(sql, user_id: id, type: notification_type)[0].to_i
DB.query_single(sql, user_id: id, notification_type: notification_type)[0].to_i
end
def unread_notifications_of_priority(high_priority:)
# perf critical, much more efficient than AR
sql = <<~SQL
SELECT COUNT(*)
FROM notifications n
LEFT JOIN topics t ON t.id = n.topic_id
WHERE t.deleted_at IS NULL
AND n.high_priority = :high_priority
AND n.user_id = :user_id
AND NOT read
SQL
# to avoid coalesce we do to_i
DB.query_single(sql, user_id: id, high_priority: high_priority)[0].to_i
end
###
# DEPRECATED: This is only maintained for backwards compat until v2.5. There
# may be inconsistencies with counts in the UI because of this, because unread
# high priority includes PMs AND bookmark reminders.
def unread_private_messages
@unread_pms ||= unread_notifications_of_type(Notification.types[:private_message])
@unread_pms ||= unread_high_priority_notifications
end
def unread_high_priority_notifications
@unread_high_prios ||= unread_notifications_of_priority(high_priority: true)
end
# PERF: This safeguard is in place to avoid situations where
@ -526,7 +552,7 @@ class User < ActiveRecord::Base
notifications n
LEFT JOIN topics t ON t.id = n.topic_id
WHERE t.deleted_at IS NULL AND
n.notification_type <> :pm AND
n.high_priority = FALSE AND
n.user_id = :user_id AND
n.id > :seen_notification_id AND
NOT read
@ -537,7 +563,6 @@ class User < ActiveRecord::Base
DB.query_single(sql,
user_id: id,
seen_notification_id: seen_notification_id,
pm: Notification.types[:private_message],
limit: User.max_unread_notifications
)[0].to_i
end
@ -579,7 +604,7 @@ class User < ActiveRecord::Base
LEFT JOIN topics t ON n.topic_id = t.id
WHERE
t.deleted_at IS NULL AND
n.notification_type = :type AND
n.high_priority AND
n.user_id = :user_id AND
NOT read
ORDER BY n.id DESC
@ -591,23 +616,21 @@ class User < ActiveRecord::Base
LEFT JOIN topics t ON n.topic_id = t.id
WHERE
t.deleted_at IS NULL AND
(n.notification_type <> :type OR read) AND
(n.high_priority = FALSE OR read) AND
n.user_id = :user_id
ORDER BY n.id DESC
LIMIT 20
) AS y
SQL
recent = DB.query(sql,
user_id: id,
type: Notification.types[:private_message]
).map! do |r|
recent = DB.query(sql, user_id: id).map! do |r|
[r.id, r.read]
end
payload = {
unread_notifications: unread_notifications,
unread_private_messages: unread_private_messages,
unread_high_priority_notifications: unread_high_priority_notifications,
read_first_notification: read_first_notification?,
last_notification: json,
recent: recent,

1
app/serializers/current_user_serializer.rb
View File

@ -5,6 +5,7 @@ class CurrentUserSerializer < BasicUserSerializer
attributes :name,
:unread_notifications,
:unread_private_messages,
:unread_high_priority_notifications,
:read_first_notification?,
:admin?,
:notification_channel_position,

3
config/locales/client.en.yml
View File

@ -1812,6 +1812,9 @@ en:
message:
one: "%{count} unread message"
other: "{{count}} unread messages"
high_priority:
one: "%{count} unread high priority notification"
other: "%{count} unread high priority notificationis"
title: "notifications of @name mentions, replies to your posts and topics, messages, etc"
none: "Unable to load notifications at this time."
empty: "No notifications found."

2
config/locales/server.en.yml
View File

@ -3521,8 +3521,8 @@ en:
why: "A brief summary of %{site_link} since your last visit on %{last_seen_at}"
since_last_visit: "Since your last visit"
new_topics: "New Topics"
unread_messages: "Unread Messages"
unread_notifications: "Unread Notifications"
unread_high_priority: "Unread High Priority Notifications"
liked_received: "Likes Received"
new_users: "New Users"
popular_topics: "Popular Topics"

42
db/migrate/20200329222246_add_high_priority_column_to_notifications.rb Normal file
View File

@ -0,0 +1,42 @@
# frozen_string_literal: true
class AddHighPriorityColumnToNotifications < ActiveRecord::Migration[6.0]
disable_ddl_transaction!
def up
if !column_exists?(:notifications, :high_priority)
add_column :notifications, :high_priority, :boolean, default: nil
end
# type 6 = private message, 24 = bookmark reminder
# priority 0 = low, 1 = normal, 2 = high
if column_exists?(:notifications, :high_priority)
execute <<~SQL
UPDATE notifications SET high_priority = TRUE WHERE notification_type IN (6, 24);
SQL
execute <<~SQL
UPDATE notifications SET high_priority = FALSE WHERE notification_type NOT IN (6, 24);
SQL
execute <<~SQL
ALTER TABLE notifications ALTER COLUMN high_priority SET DEFAULT FALSE;
SQL
execute <<~SQL
ALTER TABLE notifications ALTER COLUMN high_priority SET NOT NULL;
SQL
end
execute <<~SQL
CREATE INDEX CONCURRENTLY IF NOT EXISTS index_notifications_read_or_not_high_priority ON notifications(user_id, id DESC, read, topic_id) WHERE (read OR (high_priority = FALSE));
SQL
execute <<~SQL
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS index_notifications_unique_unread_high_priority ON notifications(user_id, id) WHERE NOT read AND high_priority = TRUE;
SQL
end
def down
DB.exec("ALTER TABLE notifications DROP COLUMN IF EXISTS high_priority")
end
end

12
db/post_migrate/20200330233427_drop_old_unread_pm_notification_indices.rb Normal file
View File

@ -0,0 +1,12 @@
# frozen_string_literal: true
class DropOldUnreadPmNotificationIndices < ActiveRecord::Migration[6.0]
def up
DB.exec("DROP INDEX IF EXISTS index_notifications_on_user_id_and_id")
DB.exec("DROP INDEX IF EXISTS index_notifications_on_read_or_n_type")
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

19
spec/fabricators/notification_fabricator.rb
View File

@ -3,6 +3,7 @@
Fabricator(:notification) do
transient :post
notification_type Notification.types[:mentioned]
high_priority false
user
topic { |attrs| attrs[:post]&.topic || Fabricate(:topic, user: attrs[:user]) }
post_number { |attrs| attrs[:post]&.post_number }
@ -17,6 +18,7 @@ end
Fabricator(:private_message_notification, from: :notification) do
notification_type Notification.types[:private_message]
high_priority true
data do |attrs|
post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user])
{
@ -30,6 +32,23 @@ Fabricator(:private_message_notification, from: :notification) do
end
end
Fabricator(:bookmark_reminder_notification, from: :notification) do
notification_type Notification.types[:bookmark_reminder]
high_priority true
data do |attrs|
post = attrs[:post] || Fabricate(:post, topic: attrs[:topic], user: attrs[:user])
{
topic_title: attrs[:topic].title,
original_post_id: post.id,
original_post_type: post.post_type,
original_username: post.user.username,
revision_number: nil,
display_username: post.user.username,
bookmark_name: "Check out Mr Freeze's opinion here"
}.to_json
end
end
Fabricator(:replied_notification, from: :notification) do
notification_type Notification.types[:replied]
data do |attrs|

53
spec/models/notification_spec.rb
View File

@ -117,6 +117,24 @@ describe Notification do
it "increases unread_private_messages" do
expect { Fabricate(:private_message_notification, user: user); user.reload }.to change(user, :unread_private_messages)
end
it "increases unread_high_priority_notifications" do
expect { Fabricate(:private_message_notification, user: user); user.reload }.to change(user, :unread_high_priority_notifications)
end
end
context 'a bookmark reminder message' do
it "doesn't increase unread_notifications" do
expect { Fabricate(:bookmark_reminder_notification, user: user); user.reload }.not_to change(user, :unread_notifications)
end
it 'increases total_unread_notifications' do
expect { Fabricate(:notification, user: user); user.reload }.to change(user, :total_unread_notifications)
end
it "increases unread_high_priority_notifications" do
expect { Fabricate(:bookmark_reminder_notification, user: user); user.reload }.to change(user, :unread_high_priority_notifications)
end
end
end
@ -219,6 +237,13 @@ describe Notification do
data: '{}',
notification_type: Notification.types[:private_message])
Notification.create!(read: false,
user_id: user.id,
topic_id: t.id,
post_number: 1,
data: '{}',
notification_type: Notification.types[:bookmark_reminder])
other = Notification.create!(read: false,
user_id: user.id,
topic_id: t.id,
@ -230,8 +255,11 @@ describe Notification do
user.reload
expect(user.unread_notifications).to eq(0)
expect(user.total_unread_notifications).to eq(2)
expect(user.unread_private_messages).to eq(1)
expect(user.total_unread_notifications).to eq(3)
# NOTE: because of deprecation this will be equal to unread_high_priority_notifications,
# to be remonved in 2.5
expect(user.unread_private_messages).to eq(2)
expect(user.unread_high_priority_notifications).to eq(2)
end
end
@ -248,7 +276,7 @@ describe Notification do
end
end
describe 'ensure consistency' do
describe '#ensure_consistency!' do
it 'deletes notifications if post is missing or deleted' do
NotificationEmailer.disable
@ -260,6 +288,8 @@ describe Notification do
notification_type: Notification.types[:private_message])
Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
notification_type: Notification.types[:private_message])
Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
notification_type: Notification.types[:bookmark_reminder])
Notification.create!(read: false, user_id: p2.user_id, topic_id: p2.topic_id, post_number: p2.post_number, data: '[]',
notification_type: Notification.types[:liked])
@ -321,6 +351,10 @@ describe Notification do
fab(Notification.types[:private_message], false)
end
def unread_bookmark_reminder
fab(Notification.types[:bookmark_reminder], false)
end
def pm
fab(Notification.types[:private_message], true)
end
@ -340,16 +374,17 @@ describe Notification do
expect(Notification.visible.count).to eq(0)
end
it 'orders stuff correctly' do
it 'orders stuff by creation descending, bumping unread high priority (pms, bookmark reminders) to top' do
# note we expect the final order to read bottom-up for this list of variables,
# with unread pm + bookmark reminder at the top of that list
a = unread_pm
regular
regular
b = unread_bookmark_reminder
c = pm
d = regular
# bumps unread pms to front of list
notifications = Notification.recent_report(user, 3)
expect(notifications.map { |n| n.id }).to eq([a.id, d.id, c.id])
notifications = Notification.recent_report(user, 4)
expect(notifications.map { |n| n.id }).to eq([b.id, a.id, d.id, c.id])
end

62
spec/models/user_spec.rb
View File

@ -1779,7 +1779,7 @@ describe User do
end
describe '#publish_notifications_state' do
it 'should publish the right message' do
it 'should publish the right message sorted by ID desc' do
notification = Fabricate(:notification, user: user)
notification2 = Fabricate(:notification, user: user, read: true)
@ -1788,8 +1788,41 @@ describe User do
end.first
expect(message.data[:recent]).to eq([
[notification2.id, true], [notification.id, false]
])
[notification2.id, true], [notification.id, false]
])
end
it 'floats the unread high priority notifications to the top' do
notification = Fabricate(:notification, user: user)
notification2 = Fabricate(:notification, user: user, read: true)
notification3 = Fabricate(:notification, user: user, notification_type: Notification.types[:private_message])
notification4 = Fabricate(:notification, user: user, notification_type: Notification.types[:bookmark_reminder])
message = MessageBus.track_publish("/notification/#{user.id}") do
user.publish_notifications_state
end.first
expect(message.data[:recent]).to eq([
[notification4.id, false], [notification3.id, false],
[notification2.id, true], [notification.id, false]
])
end
it "has the correct counts" do
notification = Fabricate(:notification, user: user)
notification2 = Fabricate(:notification, user: user, read: true)
notification3 = Fabricate(:notification, user: user, notification_type: Notification.types[:private_message])
notification4 = Fabricate(:notification, user: user, notification_type: Notification.types[:bookmark_reminder])
message = MessageBus.track_publish("/notification/#{user.id}") do
user.publish_notifications_state
end.first
expect(message.data[:unread_notifications]).to eq(1)
# NOTE: because of deprecation this will be equal to unread_high_priority_notifications,
# to be remonved in 2.5
expect(message.data[:unread_private_messages]).to eq(2)
expect(message.data[:unread_high_priority_notifications]).to eq(2)
end
end
@ -1821,6 +1854,7 @@ describe User do
end
describe "#unread_notifications" do
fab!(:user) { Fabricate(:user) }
before do
User.max_unread_notifications = 3
end
@ -1830,14 +1864,32 @@ describe User do
end
it "limits to MAX_UNREAD_NOTIFICATIONS" do
user = Fabricate(:user)
4.times do
Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
end
expect(user.unread_notifications).to eq(3)
end
it "does not include high priority notifications" do
Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
Notification.create!(user_id: user.id, notification_type: Notification.types[:private_message], read: false, data: '{}')
Notification.create!(user_id: user.id, notification_type: Notification.types[:bookmark_reminder], read: false, data: '{}')
expect(user.unread_notifications).to eq(1)
end
end
describe "#unread_high_priority_notifications" do
fab!(:user) { Fabricate(:user) }
it "only returns an unread count of PM and bookmark reminder notifications" do
Notification.create!(user_id: user.id, notification_type: 1, read: false, data: '{}')
Notification.create!(user_id: user.id, notification_type: Notification.types[:private_message], read: false, data: '{}')
Notification.create!(user_id: user.id, notification_type: Notification.types[:bookmark_reminder], read: false, data: '{}')
expect(user.unread_high_priority_notifications).to eq(2)
end
end
describe "#unstage!" do

1
test/javascripts/fixtures/session-fixtures.js
View File

@ -8,6 +8,7 @@ export default {
name: "Robin Ward",
unread_notifications: 0,
unread_private_messages: 0,
unread_high_priority_notifications: 0,
admin: true,
notification_channel_position: null,
site_flagged_posts_count: 1,