From e90f9e5cc479bccf02a518aa3208a5f4b5909335 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 14 Feb 2020 16:41:42 +1100 Subject: [PATCH] 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 --- app/services/post_alerter.rb | 3 ++- spec/services/post_alerter_spec.rb | 28 ++++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 7a4580ad189..ea1b8105903 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -261,6 +261,7 @@ class PostAlerter 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 @@ class PostAlerter # 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 = {} diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 301f16fadd0..4a73313c7b1 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -165,9 +165,11 @@ describe PostAlerter do 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,7 +178,7 @@ describe PostAlerter do 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 { @@ -184,9 +186,27 @@ describe PostAlerter do }.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(