FIX: Delete notifications users can't see after moving posts

No need to let notifications stay around when users can't access
a topic after it was converted into a PM or posts were moved
into a restricted topic.

Also makes sure that moving to a new topic correctly uses the
guardian for the first post by enqueuing jobs outside of a
transaction.

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

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

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

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

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