FIX: Streamline topic bump functionality and notification settings behaviour (#34945)

Followup 2a65bf4522

With the above change, we inadvertently made it so that
when you edited the topic title or category, we bumped
the topic.

This fix makes it so we do not bump topics when only title, category,
or tags are changed, to restore the previous behaviour.

In addition, we are making it so the following settings no longer
influence topic bump behaviour, keeping only their intended notification
behaviour around. The site setting description never mentioned anything
about topic bumps:

* disable_category_edit_notifications
* disable_tag_edit_notifications

app/controllers/topics_controller.rb

@@ -442,15 +442,12 @@ class TopicsController < ApplicationController
     success = true
 
     if changes.length > 0
-      bypass_bump = should_bypass_bump?(changes)
-
       first_post = topic.ordered_posts.first
       success =
         PostRevisor.new(first_post, topic).revise!(
           current_user,
           changes,
           validate_post: false,
-          bypass_bump: bypass_bump,
           keep_existing_draft: params[:keep_existing_draft].to_s == "true",
         )
 
@@ -1257,11 +1254,6 @@ class TopicsController < ApplicationController
     Promotion.new(current_user).review if current_user.present?
   end
 
-  def should_bypass_bump?(changes)
-    (changes[:category_id].present? && SiteSetting.disable_category_edit_notifications) ||
-      (changes[:tags].present? && SiteSetting.disable_tags_edit_notifications)
-  end
-
   def slugs_do_not_match
     if SiteSetting.slug_generation_method != "encoded"
       params[:slug] && @topic_view.topic.slug != params[:slug]

lib/post_revisor.rb

@@ -587,7 +587,7 @@ class PostRevisor
   end
 
   def create_revision
-    modifications = post_changes.merge(@topic_changes.diff)
+    modifications = post_changes.merge(topic_diff)
 
     modifications["raw"][0] = cached_original_raw || modifications["raw"][0] if modifications["raw"]
 
@@ -608,7 +608,7 @@ class PostRevisor
   def update_revision
     return unless revision = PostRevision.find_by(post_id: @post.id, number: @post.version)
     revision.user_id = @post.last_editor_id
-    modifications = post_changes.merge(@topic_changes.diff)
+    modifications = post_changes.merge(topic_diff)
 
     modifications.each_key do |field|
       if revision.modifications.has_key?(field)
@@ -641,7 +641,7 @@ class PostRevisor
   end
 
   def topic_diff
-    @topic_changes.diff
+    @topic_changes.diff.with_indifferent_access
   end
 
   def perform_edit
@@ -662,14 +662,19 @@ class PostRevisor
   end
 
   def bypass_bump?
-    !@post_successfully_saved || post_changes.any? || @topic_changes.errored? ||
+    return true if @opts[:bypass_bump] == true
-      @opts[:bypass_bump] == true || @post.whisper? || only_hidden_tags_changed?
+    return true if @post.whisper? || !@post_successfully_saved || post_changes.any?
+    return true if @topic_changes.errored?
+    return true if topic_title_changed? || topic_category_changed? || topic_tags_changed?
+    return true if only_hidden_tags_changed?
+
+    false
   end
 
   def only_hidden_tags_changed?
     return false if (hidden_tag_names = DiscourseTagging.hidden_tag_names).blank?
 
-    modifications = post_changes.merge(@topic_changes.diff)
+    modifications = post_changes.merge(topic_diff)
     if modifications.keys.size == 1 && (tags_diff = modifications["tags"]).present?
       a, b = tags_diff[0] || [], tags_diff[1] || []
       changed_tags = ((a + b) - (a & b)).map(&:presence).compact
@@ -732,7 +737,7 @@ class PostRevisor
 
   def publish_changes
     options =
-      if !@topic_changes.diff.empty? && !@topic_changes.errored?
+      if !topic_diff.empty? && !@topic_changes.errored?
         { reload_topic: true }
       else
         {}
@@ -764,6 +769,16 @@ class PostRevisor
       !@topic_changes.errored?
   end
 
+  def topic_category_changed?
+    topic_changed? && @fields.has_key?(:category_id) && topic_diff.has_key?(:category_id) &&
+      !@topic_changes.errored?
+  end
+
+  def topic_tags_changed?
+    topic_changed? && @fields.has_key?(:tags) && topic_diff.has_key?(:tags) &&
+      !@topic_changes.errored?
+  end
+
   def reviewable_content_changed?
     raw_changed? || topic_title_changed?
   end

spec/lib/post_revisor_spec.rb

@@ -569,6 +569,26 @@ describe PostRevisor do
           )
         }.not_to change { post.topic.bumped_at }
       end
+
+      it "doesn't bump the topic when editing the topic title" do
+        expect {
+          post_revisor.revise!(
+            post.user,
+            { title: "This is an updated topic title" },
+            revised_at: post.updated_at + SiteSetting.editing_grace_period + 1.seconds,
+          )
+        }.not_to change { post.topic.bumped_at }
+      end
+
+      it "doesn't bump the topic when editing the topic category" do
+        expect {
+          post_revisor.revise!(
+            post.user,
+            { category_id: Fabricate(:category).id },
+            revised_at: post.updated_at + SiteSetting.editing_grace_period + 1.seconds,
+          )
+        }.not_to change { post.topic.bumped_at }
+      end
     end
 
     describe "edit reasons" do
@@ -1278,6 +1298,12 @@ describe PostRevisor do
             expect(post.topic.tags.size).to eq(0)
           end
 
+          it "doesn't bump the topic when editing tags" do
+            expect { post_revisor.revise!(post.user, { tags: %w[totally update] }) }.not_to change {
+              post.topic.bumped_at
+            }
+          end
+
           it "can't add staff-only tags" do
             create_staff_only_tags(["important"])
             result =
@@ -1417,7 +1443,7 @@ describe PostRevisor do
               }.to_not change { topic.reload.bumped_at }
             end
 
-            it "bumps topic if non staff-only tags are added" do
+            it "doesn't bump topic if non staff-only tags are added" do
               expect {
                 result =
                   post_revisor.revise!(
@@ -1426,7 +1452,7 @@ describe PostRevisor do
                     tags: topic.tags.map(&:name) + [Fabricate(:tag).name],
                   )
                 expect(result).to eq(true)
-              }.to change { topic.reload.bumped_at }
+              }.to_not change { topic.reload.bumped_at }
             end
 
             it "creates a hidden revision" do

spec/requests/topics_controller_spec.rb

@@ -1859,64 +1859,6 @@ RSpec.describe TopicsController do
           expect(response.status).to eq(200)
         end
 
-        context "when using SiteSetting.disable_category_edit_notifications" do
-          it "doesn't bump the topic if the setting is enabled" do
-            SiteSetting.disable_category_edit_notifications = true
-            last_bumped_at = topic.bumped_at
-            expect(last_bumped_at).not_to be_nil
-
-            expect do
-              put "/t/#{topic.slug}/#{topic.id}.json", params: { category_id: category.id }
-            end.to change { topic.reload.category_id }.to(category.id)
-
-            expect(response.status).to eq(200)
-            expect(topic.reload.bumped_at).to eq_time(last_bumped_at)
-          end
-
-          it "bumps the topic if the setting is disabled" do
-            SiteSetting.disable_category_edit_notifications = false
-            last_bumped_at = topic.bumped_at
-            expect(last_bumped_at).not_to be_nil
-
-            expect do
-              put "/t/#{topic.slug}/#{topic.id}.json", params: { category_id: category.id }
-            end.to change { topic.reload.category_id }.to(category.id)
-
-            expect(response.status).to eq(200)
-            expect(topic.reload.bumped_at).not_to eq_time(last_bumped_at)
-          end
-        end
-
-        context "when using SiteSetting.disable_tags_edit_notifications" do
-          fab!(:t1, :tag)
-          fab!(:t2, :tag)
-          let(:tags) { [t1, t2] }
-
-          it "doesn't bump the topic if the setting is enabled" do
-            SiteSetting.disable_tags_edit_notifications = true
-            last_bumped_at = topic.bumped_at
-            expect(last_bumped_at).not_to be_nil
-
-            put "/t/#{topic.slug}/#{topic.id}.json", params: { tags: tags.map(&:name) }
-
-            expect(topic.reload.tags).to match_array(tags)
-            expect(response.status).to eq(200)
-            expect(topic.reload.bumped_at).to eq_time(last_bumped_at)
-          end
-
-          it "bumps the topic if the setting is disabled" do
-            SiteSetting.disable_tags_edit_notifications = false
-            last_bumped_at = topic.bumped_at
-            expect(last_bumped_at).not_to be_nil
-
-            put "/t/#{topic.slug}/#{topic.id}.json", params: { tags: tags.map(&:name) }
-
-            expect(topic.reload.tags).to match_array(tags)
-            expect(response.status).to eq(200)
-            expect(topic.reload.bumped_at).not_to eq_time(last_bumped_at)
-          end
-        end
-
         describe "when first post is locked" do
           it "blocks user from editing even if they are in 'edit_all_topic_groups' and 'edit_all_post_groups'" do
             SiteSetting.edit_all_topic_groups = Group::AUTO_GROUPS[:trust_level_3]