github-mirror/discourse
github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-03-10 01:10:10 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Liked notification consolidation has to account for user like frequency setting.

Browse Source
This commit is contained in:
Guo Xiang Tan 2019-01-17 14:31:07 +08:00
parent 384135845b
commit aa2cc4ab31
5 changed files with 247 additions and 152 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
app/models/notification.rb
View File

@ -13,9 +13,9 @@ class Notification < ActiveRecord::Base
scope :visible , lambda { joins('LEFT JOIN topics ON notifications.topic_id = topics.id')
.where('topics.id IS NULL OR topics.deleted_at IS NULL') }
scope :get_liked_by, ->(user) {
where("data::json ->> 'original_username' = ?", user.username_lower)
.where(notification_type: Notification.types[:liked])
scope :filter_by_display_username_and_type, ->(username, notification_type) {
where("data::json ->> 'display_username' = ?", username)
.where(notification_type: notification_type)
.order(created_at: :desc)
}

83
app/services/post_action_notifier.rb
View File

@ -76,53 +76,6 @@ class PostActionNotifier
post = post_action.post
return if post_action.user.blank?
if SiteSetting.likes_notification_consolidation_threshold.zero?
return create_liked_notification(alerter, post, post_action)
end
user_notifications = post.user.notifications
consolidation_window =
SiteSetting.likes_notification_consolidation_window_mins.minutes.ago
liked_by_user_notifications =
user_notifications
.get_liked_by(post_action.user)
.where("created_at > ?", consolidation_window)
user_liked_consolidated_notification =
user_notifications
.where(
"
created_at > ? AND
notification_type = ? AND
data::json ->> 'display_username' = ?
",
consolidation_window,
Notification.types[:liked_consolidated],
post_action.user.username_lower
)
.first
if user_liked_consolidated_notification
update_consolidated_liked_notification_count!(
user_liked_consolidated_notification
)
elsif (
liked_by_user_notifications.count >=
SiteSetting.likes_notification_consolidation_threshold
)
create_consolidated_liked_notification!(
liked_by_user_notifications,
post,
post_action
)
else
create_liked_notification(alerter, post, post_action)
end
end
def self.create_liked_notification(alerter, post, post_action)
alerter.create_notification(
post.user,
Notification.types[:liked],
@ -132,42 +85,6 @@ class PostActionNotifier
user_id: post_action.user_id
)
end
private_class_method :create_liked_notification
def self.update_consolidated_liked_notification_count!(notification)
data = JSON.parse(notification.data)
data["count"] += 1
notification.update!(
data: data.to_json,
read: false
)
end
private_class_method :update_consolidated_liked_notification_count!
def self.create_consolidated_liked_notification!(notifications,
post,
post_action)
Notification.transaction do
timestamp = notifications.last.created_at
Notification.create!(
notification_type: Notification.types[:liked_consolidated],
user_id: post.user_id,
data: {
username: post_action.user.username,
display_username: post_action.user.username,
count: notifications.count + 1
}.to_json,
updated_at: timestamp,
created_at: timestamp
)
notifications.delete_all
end
end
private_class_method :create_consolidated_liked_notification!
def self.after_create_post_revision(post_revision)
return if @disabled

115
app/services/post_alerter.rb
View File

@ -282,7 +282,8 @@ class PostAlerter
return if user.blank?
return if user.id < 0
return if type == Notification.types[:liked] && user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never]
is_liked = type == Notification.types[:liked]
return if is_liked && user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never]
# Make sure the user can see the post
return unless Guardian.new(user).can_see?(post)
@ -316,23 +317,38 @@ class PostAlerter
# Don't notify the same user about the same notification on the same post
existing_notification = user.notifications
.order("notifications.id DESC")
.find_by(topic_id: post.topic_id,
post_number: post.post_number,
notification_type: type)
.find_by(
topic_id: post.topic_id,
post_number: post.post_number,
notification_type: type
)
return if existing_notification && !should_notify_previous?(user, existing_notification, opts)
notification_data = {}
if existing_notification &&
if is_liked
if existing_notification &&
existing_notification.created_at > 1.day.ago &&
user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:always]
(
user.user_option.like_notification_frequency ==
UserOption.like_notification_frequency_type[:always]
)
data = existing_notification.data_hash
notification_data["username2"] = data["display_username"]
notification_data["count"] = (data["count"] || 1).to_i + 1
# don't use destroy so we don't trigger a notification count refresh
Notification.where(id: existing_notification.id).destroy_all
data = existing_notification.data_hash
notification_data["username2"] = data["display_username"]
notification_data["count"] = (data["count"] || 1).to_i + 1
# don't use destroy so we don't trigger a notification count refresh
Notification.where(id: existing_notification.id).destroy_all
elsif !SiteSetting.likes_notification_consolidation_threshold.zero?
notification = consolidate_liked_notifications(
user,
post,
opts[:display_username]
)
return notification if notification
end
end
collapsed = false
@ -601,4 +617,81 @@ class PostAlerter
Rails.logger.warn("PostAlerter.#{caller_locations(1, 1)[0].label} was called outside of sidekiq") unless Sidekiq.server?
end
private
def consolidate_liked_notifications(user, post, username)
user_notifications = user.notifications
consolidation_window =
SiteSetting.likes_notification_consolidation_window_mins.minutes.ago
liked_by_user_notifications =
user_notifications
.filter_by_display_username_and_type(
username, Notification.types[:liked]
)
.where(
"created_at > ? AND data::json ->> 'username2' IS NULL",
consolidation_window
)
user_liked_consolidated_notification =
user_notifications
.filter_by_display_username_and_type(
username, Notification.types[:liked_consolidated]
)
.where("created_at > ?", consolidation_window)
.first
if user_liked_consolidated_notification
return update_consolidated_liked_notification_count!(
user_liked_consolidated_notification
)
elsif (
liked_by_user_notifications.count >=
SiteSetting.likes_notification_consolidation_threshold
)
return create_consolidated_liked_notification!(
liked_by_user_notifications,
post,
username
)
end
end
def update_consolidated_liked_notification_count!(notification)
data = notification.data_hash
data["count"] += 1
notification.update!(
data: data.to_json,
read: false
)
notification
end
def create_consolidated_liked_notification!(notifications, post, username)
notification = nil
Notification.transaction do
timestamp = notifications.last.created_at
notification = Notification.create!(
notification_type: Notification.types[:liked_consolidated],
user_id: post.user_id,
data: {
username: username,
display_username: username,
count: notifications.count + 1
}.to_json,
updated_at: timestamp,
created_at: timestamp
)
notifications.each(&:destroy!)
end
notification
end
end

10
spec/models/notification_spec.rb
View File

@ -250,7 +250,7 @@ describe Notification do
end
end
describe '.get_liked_by' do
describe '.filter_by_display_username_and_type' do
let(:post) { Fabricate(:post) }
let(:user) { Fabricate(:user) }
@ -259,7 +259,9 @@ describe Notification do
end
it 'should return the right notifications' do
expect(Notification.get_liked_by(user)).to eq([])
expect(Notification.filter_by_display_username_and_type(
user.username_lower, Notification.types[:liked]
)).to eq([])
expect do
PostAlerter.post_created(Fabricate(:basic_reply,
@ -270,7 +272,9 @@ describe Notification do
PostAction.act(user, post, PostActionType.types[:like])
end.to change { Notification.count }.by(2)
expect(Notification.get_liked_by(user)).to contain_exactly(
expect(Notification.filter_by_display_username_and_type(
user.username_lower, Notification.types[:liked]
)).to contain_exactly(
Notification.find_by(notification_type: Notification.types[:liked])
)
end

185
spec/models/post_action_spec.rb
View File

@ -381,6 +381,7 @@ describe PostAction do
describe 'likes consolidation' do
let(:liker) { Fabricate(:user) }
let(:liker2) { Fabricate(:user) }
let(:likee) { Fabricate(:user) }
it "can be disabled" do
@ -405,80 +406,146 @@ describe PostAction do
end.to_not change { likee.reload.notifications.count }
end
it 'should consolidate likes notification when the threshold is reached' do
SiteSetting.likes_notification_consolidation_threshold = 2
describe 'frequency first_time_and_daily' do
before do
likee.user_option.update!(
like_notification_frequency:
UserOption.like_notification_frequency_type[:first_time_and_daily]
)
end
freeze_time
it 'should consolidate likes notification when the threshold is reached' do
SiteSetting.likes_notification_consolidation_threshold = 2
expect do
3.times do
expect do
3.times do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
expect(notification.notification_type).to eq(
Notification.types[:liked_consolidated]
)
data = JSON.parse(notification.data)
expect(data["username"]).to eq(liker.username)
expect(data["display_username"]).to eq(liker.username)
expect(data["count"]).to eq(3)
notification.update!(read: true)
expect do
2.times do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end
end.to_not change { likee.reload.notifications.count }
data = JSON.parse(notification.reload.data)
expect(notification.read).to eq(false)
expect(data["count"]).to eq(5)
# Like from a different user shouldn't be consolidated
expect do
PostAction.act(
Fabricate(:user),
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
expect(notification.notification_type).to eq(
Notification.types[:liked]
)
freeze_time((
SiteSetting.likes_notification_consolidation_window_mins.minutes +
1
).since)
expect do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end
end.to change { likee.reload.notifications.count }.by(1)
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
notification = likee.notifications.last
expect(notification.notification_type).to eq(
Notification.types[:liked_consolidated]
)
expect(notification.notification_type).to eq(Notification.types[:liked])
end
end
data = JSON.parse(notification.data)
describe 'frequency always' do
before do
likee.user_option.update!(
like_notification_frequency:
UserOption.like_notification_frequency_type[:always]
)
end
expect(data["username"]).to eq(liker.username)
expect(data["display_username"]).to eq(liker.username)
expect(data["count"]).to eq(3)
it 'should consolidate liked notifications when threshold is reached' do
SiteSetting.likes_notification_consolidation_threshold = 2
notification.update!(read: true)
post = Fabricate(:post, user: likee)
expect do
2.times do
expect do
[liker2, liker].each do |user|
PostAction.act(user, post, PostActionType.types[:like])
end
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
data_hash = notification.data_hash
expect(data_hash["original_username"]).to eq(liker.username)
expect(data_hash["username2"]).to eq(liker2.username)
expect(data_hash["count"].to_i).to eq(2)
expect do
2.times do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end
end.to change { likee.reload.notifications.count }.by(2)
expect(likee.notifications.pluck(:notification_type).uniq)
.to contain_exactly(Notification.types[:liked])
expect do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end
end.to_not change { likee.reload.notifications.count }
end.to change { likee.reload.notifications.count }.by(-1)
data = JSON.parse(notification.reload.data)
notification = likee.notifications.last
expect(notification.read).to eq(false)
expect(data["count"]).to eq(5)
# Like from a different user shouldn't be consolidated
expect do
PostAction.act(
Fabricate(:user),
Fabricate(:post, user: likee),
PostActionType.types[:like]
expect(notification.notification_type).to eq(
Notification.types[:liked_consolidated]
)
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
expect(notification.notification_type).to eq(
Notification.types[:liked]
)
freeze_time(
SiteSetting.likes_notification_consolidation_window_mins.minutes.since
)
expect do
PostAction.act(
liker,
Fabricate(:post, user: likee),
PostActionType.types[:like]
)
end.to change { likee.reload.notifications.count }.by(1)
notification = likee.notifications.last
expect(notification.notification_type).to eq(Notification.types[:liked])
expect(notification.data_hash["count"].to_i).to eq(3)
expect(notification.data_hash["username"]).to eq(liker.username)
end
end
end
@ -491,6 +558,20 @@ describe PostAction do
end.to_not change { Notification.count }
end
it 'should not generate a notification if liker has the topic muted' do
post = Fabricate(:post, user: eviltrout)
TopicUser.create!(
topic: post.topic,
user: eviltrout,
notification_level: TopicUser.notification_levels[:muted]
)
expect do
PostAction.act(codinghorror, post, PostActionType.types[:like])
end.to_not change { Notification.count }
end
it "should generate a notification if liker is an admin irregardles of \
muting" do