From 7194b31443c08cf3e50227f27aa59a1bf6b4343b Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Thu, 13 Aug 2020 17:22:34 +1000 Subject: [PATCH] FEATURE: don't notify about changed tags for a private message (#10408) * FEATURE: don't notify about changed tags for a private message Only staff members observing specific tag should receive a notification * FIX: remove other category which is not used * FIX: improved specs to ensure that revise was succesful --- app/jobs/regular/notify_tag_change.rb | 2 +- app/services/post_alerter.rb | 25 +++++++++++++++------ spec/services/post_alerter_spec.rb | 32 +++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/app/jobs/regular/notify_tag_change.rb b/app/jobs/regular/notify_tag_change.rb index dff6769de97..16cab8de7b5 100644 --- a/app/jobs/regular/notify_tag_change.rb +++ b/app/jobs/regular/notify_tag_change.rb @@ -7,7 +7,7 @@ module Jobs if post&.topic&.visible? post_alerter = PostAlerter.new - post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_category_watchers: false) + post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]), include_topic_watchers: !post.topic.private_message?, include_category_watchers: false) post_alerter.notify_first_post_watchers(post, post_alerter.tag_watchers(post.topic)) end end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index a1647dc6f59..b7461128817 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -613,21 +613,28 @@ class PostAlerter end end - def notify_post_users(post, notified, include_category_watchers: true, include_tag_watchers: true, new_record: false) + def notify_post_users(post, notified, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false) return unless post.topic warn_if_not_sidekiq condition = +<<~SQL id IN ( + SELECT id FROM users WHERE false + /*topic*/ + /*category*/ + /*tags*/ + ) + SQL + if include_topic_watchers + condition.sub! "/*topic*/", <<~SQL + UNION SELECT user_id FROM topic_users WHERE notification_level = :watching AND topic_id = :topic_id - /*category*/ - /*tags*/ - ) - SQL + SQL + end if include_category_watchers condition.sub! "/*category*/", <<~SQL @@ -639,7 +646,7 @@ class PostAlerter AND tu.topic_id = :topic_id WHERE cu.notification_level = :watching AND cu.category_id = :category_id - AND tu.user_id IS NULL + AND (tu.user_id IS NULL OR tu.notification_level = :watching) SQL end @@ -655,7 +662,7 @@ class PostAlerter AND tu.topic_id = :topic_id WHERE tag_users.notification_level = :watching AND tag_users.tag_id IN (:tag_ids) - AND tu.user_id IS NULL + AND (tu.user_id IS NULL OR tu.notification_level = :watching) SQL end @@ -666,6 +673,10 @@ class PostAlerter tag_ids: tag_ids ) + if post.topic.private_message? + notify = notify.where(staged: false).staff + end + exclude_user_ids = notified.map(&:id) notify = notify.where("id NOT IN (?)", exclude_user_ids) if exclude_user_ids.present? diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 0fc64e1416c..9a6c6556084 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1098,6 +1098,38 @@ describe PostAlerter do expect(user.notifications.where(notification_type: Notification.types[:watching_first_post]).count).to eq(0) end end + + context "private message" do + fab!(:post) { Fabricate(:private_message_post) } + fab!(:other_tag) { Fabricate(:tag) } + fab!(:other_tag2) { Fabricate(:tag) } + fab!(:other_tag3) { Fabricate(:tag) } + fab!(:user) { Fabricate(:user) } + fab!(:staged) { Fabricate(:staged) } + fab!(:admin) { Fabricate(:admin) } + + before do + SiteSetting.tagging_enabled = true + SiteSetting.allow_staff_to_tag_pms = true + Jobs.run_immediately! + TopicUser.change(user.id, post.topic.id, notification_level: TopicUser.notification_levels[:watching]) + TopicUser.change(staged.id, post.topic.id, notification_level: TopicUser.notification_levels[:watching]) + TopicUser.change(admin.id, post.topic.id, notification_level: TopicUser.notification_levels[:watching]) + TagUser.change(staged.id, other_tag.id, TagUser.notification_levels[:watching]) + TagUser.change(admin.id, other_tag3.id, TagUser.notification_levels[:watching]) + post.topic.allowed_users << user + post.topic.allowed_users << staged + end + + it "only notifes staff watching added tag" do + expect(PostRevisor.new(post).revise!(Fabricate(:admin), tags: [other_tag.name])).to be true + expect(Notification.where(user_id: staged.id).count).to eq(0) + expect(PostRevisor.new(post).revise!(Fabricate(:admin), tags: [other_tag2.name])).to be true + expect(Notification.where(user_id: admin.id).count).to eq(0) + expect(PostRevisor.new(post).revise!(Fabricate(:admin), tags: [other_tag3.name])).to be true + expect(Notification.where(user_id: admin.id).count).to eq(1) + end + end end describe '#extract_linked_users' do