diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 40bafa40b7..640382b217 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -57,13 +57,11 @@ class PostAlerter end expand_group_mentions(mentioned_groups, post) do |group, users| - notify_non_pm_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group)) - notified += users + notified += notify_users(users - notified, :group_mentioned, post, mentioned_opts.merge(group: group)) end if mentioned_users - notify_non_pm_users(mentioned_users - notified, :mentioned, post, mentioned_opts) - notified += mentioned_users + notified += notify_users(mentioned_users - notified, :mentioned, post, mentioned_opts) end end @@ -71,34 +69,31 @@ class PostAlerter reply_to_user = post.reply_notification_target if new_record && reply_to_user && !notified.include?(reply_to_user) && notify_about_reply?(post) - notify_non_pm_users(reply_to_user, :replied, post) - notified += [reply_to_user] + notified += notify_non_pm_users(reply_to_user, :replied, post) end # quotes quoted_users = extract_quoted_users(post) - notify_non_pm_users(quoted_users - notified, :quoted, post) - notified += quoted_users + notified += notify_non_pm_users(quoted_users - notified, :quoted, post) # linked linked_users = extract_linked_users(post) - notify_non_pm_users(linked_users - notified, :linked, post) - notified += linked_users + notified += notify_non_pm_users(linked_users - notified, :linked, post) # private messages if new_record if post.topic.private_message? # users that aren't part of any mentioned groups - users = directly_targeted_users(post) + users = directly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| notification_level = TopicUser.get(post.topic, user).try(:notification_level) - if notified.include?(user) || notification_level == TopicUser.notification_levels[:watching] || user.staged? + if reply_to_user == user || notification_level == TopicUser.notification_levels[:watching] || user.staged? create_notification(user, Notification.types[:private_message], post) end end # users that are part of all mentionned groups - users = indirectly_targeted_users(post) + users = indirectly_targeted_users(post).reject { |u| notified.include?(u) } DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |user| # only create a notification when watching the group @@ -107,11 +102,7 @@ class PostAlerter if notification_level == TopicUser.notification_levels[:watching] create_notification(user, Notification.types[:private_message], post) elsif notification_level == TopicUser.notification_levels[:tracking] - if notified.include?(user) - create_notification(user, Notification.types[:private_message], post) - else - notify_group_summary(user, post) - end + notify_group_summary(user, post) end end elsif post.post_type == Post.types[:regular] @@ -499,15 +490,21 @@ class PostAlerter # Notify a bunch of users def notify_non_pm_users(users, type, post, opts = {}) + return [] if post.topic&.private_message? - return if post.topic.try(:private_message?) + notify_users(users, type, post, opts) + end + def notify_users(users, type, post, opts = {}) users = [users] unless users.is_a?(Array) + users = users.reject { |u| u.staged? } if post.topic&.private_message? DiscourseEvent.trigger(:before_create_notifications_for_users, users, post) users.each do |u| create_notification(u, Notification.types[type], post, opts) end + + users end def notify_post_users(post, notified) diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index c500ce2d1e..8682d72811 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1,5 +1,26 @@ require 'rails_helper' +RSpec::Matchers.define :add_notification do |user, notification_type| + match(notify_expectation_failures: true) do |actual| + notifications = user.notifications + before = notifications.count + + actual.call + + expect(notifications.count).to eq(before + 1), "expected 1 new notification, got #{notifications.count - before}" + + last_notification_type = notifications.last.notification_type + expect(last_notification_type).to eq(Notification.types[notification_type]), + "expected notification type to be '#{notification_type}', got '#{Notification.types.key(last_notification_type)}'" + end + + match_when_negated do |actual| + expect { actual.call }.to_not change { user.notifications.where(notification_type: Notification.types[notification_type]).count } + end + + supports_block_expectations +end + describe PostAlerter do let!(:evil_trout) { Fabricate(:evil_trout) } @@ -337,15 +358,122 @@ describe PostAlerter do expect(events).to include(event_name: :before_create_notifications_for_users, params: [[evil_trout], mention_post]) end - it "notification comes from editor is mention is added later" do - admin = Fabricate(:admin) - post = create_post_with_alerts(user: user, raw: 'No mention here.') - expect { - post.revise(admin, raw: "Mention @eviltrout in this edit.") - }.to change(evil_trout.notifications, :count) - n = evil_trout.notifications.last - expect(n.data_hash["original_username"]).to eq(admin.username) - end + it "notification comes from editor if mention is added later" do + admin = Fabricate(:admin) + post = create_post_with_alerts(user: user, raw: 'No mention here.') + expect { + post.revise(admin, raw: "Mention @eviltrout in this edit.") + }.to change(evil_trout.notifications, :count) + n = evil_trout.notifications.last + expect(n.data_hash["original_username"]).to eq(admin.username) + end + + let(:alice) { Fabricate(:user, username: 'alice') } + let(:bob) { Fabricate(:user, username: 'bob') } + + def create_post_with_alerts(args = {}) + post = Fabricate(:post, args) + PostAlerter.post_created(post) + end + + def set_topic_notification_level(user, topic, level_name) + TopicUser.change(user.id, topic.id, notification_level: TopicUser.notification_levels[level_name]) + end + + context "topic" do + let(:topic) { Fabricate(:topic, user: alice) } + let(:first_post) { Fabricate(:post, user: topic.user) } + + [:watching, :tracking, :regular].each do |notification_level| + context "when notification level is '#{notification_level}'" do + before do + set_topic_notification_level(alice, topic, notification_level) + end + + it "notifies about @username mention" do + args = { user: bob, topic: topic, raw: 'Hello @alice' } + expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned) + end + end + end + + context "when notification level is 'muted'" do + before do + set_topic_notification_level(alice, topic, :muted) + end + + it "does not notify about @username mention" do + args = { user: bob, topic: topic, raw: 'Hello @alice' } + expect { create_post_with_alerts(args) }.to_not add_notification(alice, :mentioned) + end + end + end + + shared_context "message" do + context "when mentioned user is part of conversation" do + [:watching, :tracking, :regular].each do |notification_level| + context "when notification level is '#{notification_level}'" do + before do + set_topic_notification_level(alice, pm_topic, notification_level) + end + + it "notifies about @username mention" do + args = { user: bob, topic: pm_topic, raw: 'Hello @alice' } + expect { create_post_with_alerts(args) }.to add_notification(alice, :mentioned) + end + end + end + + context "when notification level is 'muted'" do + before do + set_topic_notification_level(alice, pm_topic, :muted) + end + + it "does not notify about @username mention" do + args = { user: bob, topic: pm_topic, raw: 'Hello @alice' } + expect { create_post_with_alerts(args) }.to_not add_notification(alice, :mentioned) + end + end + end + + context "when mentioned user is not part of conversation" do + it "notifies about @username mention when mentioned user is allowed to see message" do + carol = Fabricate(:admin, username: 'carol') + args = { user: bob, topic: pm_topic, raw: 'Hello @carol' } + expect { create_post_with_alerts(args) }.to add_notification(carol, :mentioned) + end + + it "does not notify about @username mention when mentioned user is not allowed to see message" do + dave = Fabricate(:user, username: 'dave') + args = { user: bob, topic: pm_topic, raw: 'Hello @dave' } + expect { create_post_with_alerts(args) }.to_not add_notification(dave, :mentioned) + end + end + end + + context "personal message" do + let(:pm_topic) do + Fabricate(:private_message_topic, user: alice, topic_allowed_users: [ + Fabricate.build(:topic_allowed_user, user: alice), + Fabricate.build(:topic_allowed_user, user: bob) + ]) + end + let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) } + + include_context "message" + end + + context "group message" do + let(:group) { Fabricate(:group) } + let(:pm_topic) do + Fabricate(:private_message_topic, user: alice, topic_allowed_groups: [ + Fabricate.build(:topic_allowed_group, group: group) + ]) + end + let(:first_post) { Fabricate(:post, topic: pm_topic, user: pm_topic.user) } + + include_context "message" + end end describe ".create_notification" do