diff --git a/app/jobs/regular/delete_inaccessible_notifications.rb b/app/jobs/regular/delete_inaccessible_notifications.rb new file mode 100644 index 00000000000..ca38dc4ec82 --- /dev/null +++ b/app/jobs/regular/delete_inaccessible_notifications.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Jobs + class DeleteInaccessibleNotifications < Jobs::Base + def execute(args) + raise Discourse::InvalidParameters.new(:topic_id) if args[:topic_id].blank? + + Notification.where(topic_id: args[:topic_id]).find_each do |notification| + next unless notification.user && notification.topic + + if !Guardian.new(notification.user).can_see?(notification.topic) + notification.destroy + end + end + end + end +end diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index bac69927b0f..81afb535662 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -27,6 +27,7 @@ class PostMover move_posts_to topic end add_allowed_users(participants) if participants.present? && @move_to_pm + enqueue_jobs(topic) topic end @@ -37,7 +38,7 @@ class PostMover raise Discourse::InvalidParameters unless post archetype = @move_to_pm ? Archetype.private_message : Archetype.default - Topic.transaction do + topic = Topic.transaction do new_topic = Topic.create!( user: post.user, title: title, @@ -50,6 +51,8 @@ class PostMover watch_new_topic new_topic end + enqueue_jobs(topic) + topic end private @@ -77,6 +80,7 @@ class PostMover def move_each_post max_post_number = destination_topic.max_post_number + 1 + @post_creator = nil @move_map = {} @reply_count = {} posts.each_with_index do |post, offset| @@ -110,7 +114,7 @@ class PostMover def create_first_post(post) old_post_attributes = post_attributes(post) - new_post = PostCreator.create( + @post_creator = PostCreator.new( post.user, raw: post.raw, topic_id: destination_topic.id, @@ -120,8 +124,10 @@ class PostMover raw_email: post.raw_email, skip_validations: true, created_at: post.created_at, - guardian: Guardian.new(user) + guardian: Guardian.new(user), + skip_jobs: true ) + new_post = @post_creator.create move_incoming_emails(post, new_post) move_email_logs(post, new_post) @@ -312,4 +318,13 @@ class PostMover post_number: post.post_number } end + + def enqueue_jobs(topic) + @post_creator.enqueue_jobs if @post_creator + + Jobs.enqueue( + :delete_inaccessible_notifications, + topic_id: topic.id + ) + end end diff --git a/app/models/topic_converter.rb b/app/models/topic_converter.rb index d0ce8dacf03..f59e96c4d01 100644 --- a/app/models/topic_converter.rb +++ b/app/models/topic_converter.rb @@ -31,6 +31,7 @@ class TopicConverter update_user_stats Jobs.enqueue(:topic_action_converter, topic_id: @topic.id) + Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id) watch_topic(topic) end @@ -50,6 +51,8 @@ class TopicConverter add_allowed_users Jobs.enqueue(:topic_action_converter, topic_id: @topic.id) + Jobs.enqueue(:delete_inaccessible_notifications, topic_id: @topic.id) + watch_topic(topic) end @topic diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 833705e9a56..ce6495356d8 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -308,6 +308,18 @@ describe PostMover do expect(n4.topic_id).to eq(topic.id) expect(n4.post_number).to eq(4) end + + it "deletes notifications for users not allowed to see the topic" do + another_admin = Fabricate(:admin) + staff_category = Fabricate(:private_category, group: Group[:staff]) + user_notification = Fabricate(:mentioned_notification, post: p3, user: another_user) + admin_notification = Fabricate(:mentioned_notification, post: p3, user: another_admin) + + topic.move_posts(user, [p3.id], title: "new testing topic name", category_id: staff_category.id) + + expect(Notification.exists?(user_notification.id)).to eq(false) + expect(Notification.exists?(admin_notification.id)).to eq(true) + end end context "to an existing topic" do @@ -401,6 +413,19 @@ describe PostMover do expect(n4.topic_id).to eq(topic.id) expect(n4.post_number).to eq(4) end + + it "deletes notifications for users not allowed to see the topic" do + another_admin = Fabricate(:admin) + staff_category = Fabricate(:private_category, group: Group[:staff]) + user_notification = Fabricate(:mentioned_notification, post: p3, user: another_user) + admin_notification = Fabricate(:mentioned_notification, post: p3, user: another_admin) + + destination_topic.update!(category_id: staff_category.id) + topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id) + + expect(Notification.exists?(user_notification.id)).to eq(false) + expect(Notification.exists?(admin_notification.id)).to eq(true) + end end context "to a message" do diff --git a/spec/models/topic_converter_spec.rb b/spec/models/topic_converter_spec.rb index 15fadd18be2..fb4b351d21f 100644 --- a/spec/models/topic_converter_spec.rb +++ b/spec/models/topic_converter_spec.rb @@ -90,6 +90,18 @@ describe TopicConverter do expect(other_user.user_actions.where(action_type: UserAction::REPLY).count).to eq(1) end end + + it "deletes notifications for users not allowed to see the topic" do + staff_category = Fabricate(:private_category, group: Group[:staff]) + user_notification = Fabricate(:mentioned_notification, post: first_post, user: Fabricate(:user)) + admin_notification = Fabricate(:mentioned_notification, post: first_post, user: Fabricate(:admin)) + + Jobs.run_immediately! + TopicConverter.new(first_post.topic, admin).convert_to_public_topic(staff_category.id) + + expect(Notification.exists?(user_notification.id)).to eq(false) + expect(Notification.exists?(admin_notification.id)).to eq(true) + end end end @@ -129,6 +141,17 @@ describe TopicConverter do expect(author.user_actions.where(action_type: UserAction::NEW_TOPIC).count).to eq(0) expect(author.user_actions.where(action_type: UserAction::NEW_PRIVATE_MESSAGE).count).to eq(1) end + + it "deletes notifications for users not allowed to see the message" do + user_notification = Fabricate(:mentioned_notification, post: post, user: Fabricate(:user)) + admin_notification = Fabricate(:mentioned_notification, post: post, user: Fabricate(:admin)) + + Jobs.run_immediately! + topic.convert_to_private_message(admin) + + expect(Notification.exists?(user_notification.id)).to eq(false) + expect(Notification.exists?(admin_notification.id)).to eq(true) + end end context 'topic has replies' do