FIX: don't trigger notifications when changing category/tags of unlisted topics (#7323)

app/jobs/regular/notify_category_change.rb

@@ -5,7 +5,7 @@ module Jobs
     def execute(args)
       post = Post.find_by(id: args[:post_id])
 
-      if post&.topic
+      if post&.topic&.visible?
         post_alerter = PostAlerter.new
         post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]))
         post_alerter.notify_first_post_watchers(post, post_alerter.category_watchers(post.topic))

app/jobs/regular/notify_tag_change.rb

@@ -5,7 +5,7 @@ module Jobs
     def execute(args)
       post = Post.find_by(id: args[:post_id])
 
-      if post&.topic
+      if post&.topic&.visible?
         post_alerter = PostAlerter.new
         post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]))
         post_alerter.notify_first_post_watchers(post, post_alerter.tag_watchers(post.topic))

spec/models/topic_spec.rb

@@ -1275,6 +1275,7 @@ describe Topic do
 
       describe 'to a different category' do
         let(:new_category) { Fabricate(:category, user: user, name: '2nd category') }
+        let(:another_user) { Fabricate(:user) }
 
         it 'should work' do
           topic.change_category_to_id(new_category.id)
@@ -1285,7 +1286,8 @@ describe Topic do
         end
 
         describe 'user that is watching the new category' do
-          it 'should generate the notification for the topic' do
+
+          before do
             Jobs.run_immediately!
 
             topic.posts << Fabricate(:post)
@@ -1296,14 +1298,14 @@ describe Topic do
               new_category.id
             )
 
-            another_user = Fabricate(:user)
-
             CategoryUser.set_notification_level_for_category(
               another_user,
               CategoryUser::notification_levels[:watching_first_post],
               new_category.id
             )
+          end
 
+          it 'should generate the notification for the topic' do
             expect do
               topic.change_category_to_id(new_category.id)
             end.to change { Notification.count }.by(2)
@@ -1322,6 +1324,14 @@ describe Topic do
               notification_type: Notification.types[:watching_first_post]
             ).exists?).to eq(true)
           end
+
+          it "should not generate a notification for unlisted topic" do
+            topic.update_column(:visible, false)
+
+            expect do
+              topic.change_category_to_id(new_category.id)
+            end.to change { Notification.count }.by(0)
+          end
         end
 
         describe 'when new category is set to auto close by default' do

spec/services/post_alerter_spec.rb

@@ -945,10 +945,10 @@ describe PostAlerter do
       before do
         SiteSetting.tagging_enabled = true
         Jobs.run_immediately!
+        TagUser.change(user.id, watched_tag.id, TagUser.notification_levels[:watching_first_post])
       end
 
       it "triggers a notification" do
-        TagUser.change(user.id, watched_tag.id, TagUser.notification_levels[:watching_first_post])
         expect(user.notifications.where(notification_type: Notification.types[:watching_first_post]).count).to eq(0)
 
         PostRevisor.new(post).revise!(Fabricate(:user), tags: [other_tag.name, watched_tag.name])
@@ -957,6 +957,15 @@ describe PostAlerter do
         PostRevisor.new(post).revise!(Fabricate(:user), tags: [watched_tag.name, other_tag.name])
         expect(user.notifications.where(notification_type: Notification.types[:watching_first_post]).count).to eq(1)
       end
+
+      it "doesn't trigger a notification if topic is unlisted" do
+        post.topic.update_column(:visible, false)
+
+        expect(user.notifications.where(notification_type: Notification.types[:watching_first_post]).count).to eq(0)
+
+        PostRevisor.new(post).revise!(Fabricate(:user), tags: [other_tag.name, watched_tag.name])
+        expect(user.notifications.where(notification_type: Notification.types[:watching_first_post]).count).to eq(0)
+      end
     end
   end