Skip to content
Permalink
Browse files

FIX: when unread reply notification exists don't create new (#8921)

* FIX: when unread reply notification exists don't create new

From time to time, the user is creating a reply post and then they want to add additional details. They edit an existing post and for example, add a quote from a previous one.

In that situation, if the user to whom reply was directed to already have the unread notification, we should not create the new one.

That behaviour was mentioned here: https://meta.discourse.org/t/reply-then-edit-to-add-quote-notification-redundancy/138358

* FIX: dont create new notification if already exists
  • Loading branch information
lis2 committed Feb 14, 2020
1 parent 38dd184 commit e90f9e5cc479bccf02a518aa3208a5f4b5909335
Showing with 26 additions and 5 deletions.
  1. +2 −1 app/services/post_alerter.rb
  2. +24 −4 spec/services/post_alerter_spec.rb
@@ -261,6 +261,7 @@ def should_notify_like?(user, notification)
end

def should_notify_previous?(user, post, notification, opts)
return false unless notification
case notification.notification_type
when Notification.types[:edited] then should_notify_edit?(notification, post, opts)
when Notification.types[:liked] then should_notify_like?(user, notification)
@@ -330,7 +331,7 @@ def create_notification(user, type, post, opts = {})
# Don't notify the same user about the same type of notification on the same post
existing_notification_of_same_type = existing_notifications.find { |n| n.notification_type == type }

return if existing_notification_of_same_type && !should_notify_previous?(user, post, existing_notification_of_same_type, opts)
return if existing_notifications.present? && !should_notify_previous?(user, post, existing_notification_of_same_type, opts)

notification_data = {}

@@ -165,9 +165,11 @@ def create_post_with_alerts(args = {})
end

context 'quotes' do
let(:category) { Fabricate(:category) }
let(:topic) { Fabricate(:topic, category: category) }

it 'does not notify for muted users' do
post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]')
post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
MutedUser.create!(user_id: evil_trout.id, muted_user_id: post.user_id)

expect {
@@ -176,17 +178,35 @@ def create_post_with_alerts(args = {})
end

it 'does not notify for ignored users' do
post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]')
post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
IgnoredUser.create!(user_id: evil_trout.id, ignored_user_id: post.user_id)

expect {
PostAlerter.post_created(post)
}.to change(evil_trout.notifications, :count).by(0)
end

it 'does not collapse quote notifications' do
topic = Fabricate(:topic)
it 'does not notify for users with new reply notification' do
post = Fabricate(:post, raw: '[quote="EvilTrout, post:1"]whatup[/quote]', topic: topic)
notification = Notification.create!(topic: post.topic,
post_number: post.post_number,
read: false,
notification_type: Notification.types[:replied],
user: evil_trout,
data: { topic_title: "test topic" }.to_json
)

expect {
PostAlerter.post_created(post)
}.to change(evil_trout.notifications, :count).by(0)

notification.destroy
expect {
PostAlerter.post_created(post)
}.to change(evil_trout.notifications, :count).by(1)
end

it 'does not collapse quote notifications' do
expect {
2.times do
create_post_with_alerts(

1 comment on commit e90f9e5

@discoursereviewbot

This comment has been minimized.

Copy link

discoursereviewbot commented on e90f9e5 Feb 14, 2020

system posted:

This commit appears in #8921 which was approved by @eviltrout and @ZogStriP. It was merged by @SamSaffron.

Please sign in to comment.
You can’t perform that action at this time.