From 0719531bd37530341616745d39b3d155c9468358 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 15 Dec 2021 09:07:39 -0300 Subject: [PATCH] FIX: Notify group members when someone quotes or replies to their post. (#15295) When a member set a group PM notification level to Normal, we didn't notify group members if someone quoted or replied to one of their posts. --- app/services/post_alerter.rb | 31 +++++++++++---- spec/services/post_alerter_spec.rb | 63 ++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index aa7db9f4464..e2dcbcb9d70 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -157,7 +157,7 @@ class PostAlerter # private messages if new_record if post.topic.private_message? - notify_pm_users(post, reply_to_user, notified) + notify_pm_users(post, reply_to_user, quoted_users, notified) elsif notify_about_reply?(post) notify_post_users(post, notified, new_record: new_record) end @@ -582,9 +582,9 @@ class PostAlerter # TODO: Move to post-analyzer? # Returns a list of users who were quoted in the post def extract_quoted_users(post) - post.raw.scan(/\[quote=\"([^,]+),.+\"\]/).uniq.map do |m| - User.find_by("username_lower = :username AND id != :id", username: m.first.strip.downcase, id: post.user_id) - end.compact + usernames = post.raw.scan(/\[quote=\"([^,]+),.+\"\]/).uniq.map { |q| q.first.strip.downcase } + + User.where.not(id: post.user_id).where(username_lower: usernames) end def extract_linked_users(post) @@ -622,7 +622,7 @@ class PostAlerter users end - def notify_pm_users(post, reply_to_user, notified) + def notify_pm_users(post, reply_to_user, quoted_users, notified) return unless post.topic warn_if_not_sidekiq @@ -653,10 +653,17 @@ class PostAlerter users.each do |user| case TopicUser.get(post.topic, user)&.notification_level when TopicUser.notification_levels[:watching] - # only create a notification when watching the group - create_notification(user, Notification.types[:private_message], post, skip_send_email_to: emails_to_skip_send) + create_pm_notification(user, post, emails_to_skip_send) when TopicUser.notification_levels[:tracking] - notify_group_summary(user, post) + if is_replying?(user, reply_to_user, quoted_users) + create_pm_notification(user, post, emails_to_skip_send) + else + notify_group_summary(user, post) + end + when TopicUser.notification_levels[:regular] + if is_replying?(user, reply_to_user, quoted_users) + create_pm_notification(user, post, emails_to_skip_send) + end end end end @@ -842,4 +849,12 @@ class PostAlerter User.where(id: user_ids_batch).includes(:do_not_disturb_timings).each { |user| yield(user) } end end + + def create_pm_notification(user, post, emails_to_skip_send) + create_notification(user, Notification.types[:private_message], post, skip_send_email_to: emails_to_skip_send) + end + + def is_replying?(user, reply_to_user, quoted_users) + reply_to_user == user || quoted_users.include?(user) + end end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 335ae626a79..1723c8fa0fc 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -125,6 +125,69 @@ describe PostAlerter do expect(message_data.dig(:last_notification, :notification, :data, :inbox_count)).to eq(starting_count + 1) expect(message_data[:unread_notifications]).to eq(1) end + + it 'sends a PM notification when replying to a member tracking the topic' do + group.add(user1) + + post = Fabricate(:post, topic: pm, user: user1) + TopicUser.change(user1.id, pm.id, notification_level: TopicUser.notification_levels[:tracking]) + + expect { + create_post_with_alerts( + raw: 'this is a reply to your post...', topic: pm, user: user2, + reply_to_post_number: post.post_number + ) + }.to change( + user1.notifications.where(notification_type: Notification.types[:private_message]), + :count + ).by(1) + end + + it 'notifies a group member if someone replies to their post' do + group.add(user1) + + post = Fabricate(:post, topic: pm, user: user1) + TopicUser.change(user1.id, pm.id, notification_level: TopicUser.notification_levels[:regular]) + + expect { + create_post_with_alerts( + raw: 'this is a reply to your post...', topic: pm, user: user2, + reply_to_post_number: post.post_number + ) + }.to change(user1.notifications, :count).by(1) + end + + it 'nofies a group member if someone quotes their post' do + group.add(user1) + + post = Fabricate(:post, topic: pm, user: user1) + TopicUser.change(user1.id, pm.id, notification_level: TopicUser.notification_levels[:regular]) + quote_raw = <<~STRING + [quote="#{user1.username}, post:1, topic:#{pm.id}"]#{post.raw}[/quote] + STRING + + expect { + create_post_with_alerts( + raw: quote_raw, topic: pm, user: user2, + ) + }.to change(user1.notifications, :count).by(1) + end + + it "Doesn't notify non-admin users when their post is quoted inside a whisper" do + admin = Fabricate(:admin) + group.add(admin) + + TopicUser.change(user2.id, pm.id, notification_level: TopicUser.notification_levels[:regular]) + quote_raw = <<~STRING + [quote="#{user2.username}, post:1, topic:#{pm.id}"]#{op.raw}[/quote] + STRING + + expect { + create_post_with_alerts( + raw: quote_raw, topic: pm, user: admin, post_type: Post.types[:whisper] + ) + }.to change(user2.notifications, :count).by(0) + end end end