FIX: Auto-tag improperly when category is changed (#215)

Author: rngus2344

lib/category_experts/post_handler.rb

@@ -66,7 +66,11 @@ def mark_topic_as_question
       topic.save!
     end
 
-    def correct_topic_custom_fields_after_removal(group_name:, new_post: false)
+    def correct_topic_custom_fields_after_removal(
+      group_name:,
+      new_post: false,
+      category: topic.category
+    )
       has_accepted_posts_from_same_group =
         group_name &&
           PostCustomField.where(
@@ -101,10 +105,10 @@ def correct_topic_custom_fields_after_removal(group_name:, new_post: false)
 
       DiscourseEvent.trigger(:category_experts_unapproved, post) if post && !new_post
 
-      remove_auto_tag if should_remove_auto_tag
+      remove_auto_tag(category: category) if should_remove_auto_tag
     end
 
-    def correct_topic_custom_fields_after_addition(new_post: false)
+    def correct_topic_custom_fields_after_addition(new_post: false, category: topic.category)
       topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES] = (
         topic.custom_fields[CategoryExperts::TOPIC_EXPERT_POST_GROUP_NAMES]&.split("|") || []
       ).push(users_expert_group.name).uniq.join("|")
@@ -119,13 +123,17 @@ def correct_topic_custom_fields_after_addition(new_post: false)
 
       DiscourseEvent.trigger(:category_experts_approved, post) unless new_post
 
-      add_auto_tag
+      add_auto_tag(category: category)
     end
 
     def handle_topic_category_change(old_category_id, new_category_id)
       old_category = Category.find_by(id: old_category_id)
       new_category = Category.find_by(id: new_category_id)
 
+      # Get auto-tags before processing posts
+      old_auto_tag = old_category&.custom_fields&.[](CategoryExperts::CATEGORY_EXPERT_AUTO_TAG)
+      new_auto_tag = new_category&.custom_fields&.[](CategoryExperts::CATEGORY_EXPERT_AUTO_TAG)
+
       # Get all posts in the topic that have expert status
       expert_posts =
         Post
@@ -158,7 +166,10 @@ def handle_topic_category_change(old_category_id, new_category_id)
           CategoryExperts::PostHandler.new(
             post: expert_post,
             topic: topic,
-          ).correct_topic_custom_fields_after_removal(group_name: old_group_name)
+          ).correct_topic_custom_fields_after_removal(
+            group_name: old_group_name,
+            category: old_category,
+          )
         else
           # Author is still an expert in the new category - update the group name
           new_expert_group = Group.find_by(id: author_expert_group_ids.first)
@@ -175,20 +186,20 @@ def handle_topic_category_change(old_category_id, new_category_id)
             CategoryExperts::PostHandler.new(
               post: expert_post,
               topic: topic,
-            ).correct_topic_custom_fields_after_removal(group_name: old_group_name)
+            ).correct_topic_custom_fields_after_removal(
+              group_name: old_group_name,
+              category: old_category,
+            )
 
             # Add new group to topic custom fields
             CategoryExperts::PostHandler.new(
               post: expert_post,
               topic: topic,
               user: post_author,
-            ).correct_topic_custom_fields_after_addition
+            ).correct_topic_custom_fields_after_addition(category: new_category)
           end
         end
       end
-
-      # Handle auto-tag changes
-      handle_auto_tag_change(old_category, new_category)
     end
 
     private
@@ -206,63 +217,40 @@ def users_expert_group
       @users_expert_group = group_id.nil? ? nil : Group.find_by(id: group_id)
     end
 
-    def add_auto_tag
+    def add_auto_tag(category:)
       return if !SiteSetting.tagging_enabled
-      return if auto_tag_for_category.blank?
+
+      auto_tag = auto_tag_for_category(category: category)
+      return if auto_tag.blank?
 
       existing_tag_names = topic.tags.map(&:name)
       # Return early if the topic already has the automatic tag
-      return if existing_tag_names.include?(auto_tag_for_category)
+      return if existing_tag_names.include?(auto_tag)
 
       PostRevisor.new(topic.ordered_posts.first).revise!(
         Discourse.system_user,
-        { tags: (existing_tag_names << auto_tag_for_category) },
+        { tags: (existing_tag_names << auto_tag) },
       )
     end
 
-    def remove_auto_tag
+    def remove_auto_tag(category:)
       return if !SiteSetting.tagging_enabled
-      return if auto_tag_for_category.blank?
+
+      auto_tag = auto_tag_for_category(category: category)
+      return if auto_tag.blank?
 
       existing_tag_names = topic.tags.map(&:name)
       # Return early if the topic doesn't have the automatic tag
-      return if !existing_tag_names.include?(auto_tag_for_category)
+      return if !existing_tag_names.include?(auto_tag)
 
       PostRevisor.new(topic.ordered_posts.first).revise!(
         Discourse.system_user,
-        { tags: ((existing_tag_names || []) - [auto_tag_for_category]) },
+        { tags: ((existing_tag_names || []) - [auto_tag]) },
       )
     end
 
-    def auto_tag_for_category
-      return @auto_tag_for_category if defined?(@auto_tag_for_category)
-
-      @auto_tag_for_category =
-        @topic.category.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG]
-    end
-
-    def handle_auto_tag_change(old_category, new_category)
-      return if !SiteSetting.tagging_enabled
-
-      old_auto_tag = old_category&.custom_fields&.[](CategoryExperts::CATEGORY_EXPERT_AUTO_TAG)
-      new_auto_tag = new_category&.custom_fields&.[](CategoryExperts::CATEGORY_EXPERT_AUTO_TAG)
-
-      existing_tag_names = topic.tags.map(&:name)
-
-      # Determine what tags to add/remove
-      tags_to_remove = old_auto_tag.present? ? [old_auto_tag] : []
-      tags_to_add = new_auto_tag.present? ? [new_auto_tag] : []
-
-      # Only revise if there's actually a change needed
-      if (tags_to_remove.any? && existing_tag_names.include?(old_auto_tag)) ||
-           (tags_to_add.any? && !existing_tag_names.include?(new_auto_tag))
-        new_tag_names = (existing_tag_names - tags_to_remove + tags_to_add).uniq
-
-        PostRevisor.new(topic.ordered_posts.first).revise!(
-          Discourse.system_user,
-          { tags: new_tag_names },
-        )
-      end
+    def auto_tag_for_category(category:)
+      category.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG]
     end
   end
 end

spec/plugin_spec.rb

@@ -10,7 +10,7 @@
   fab!(:group) { Fabricate(:group, users: [expert]) }
   fab!(:auto_tag, :tag)
 
-  fab!(:original_topic) { Fabricate(:topic, category: category, tags: [auto_tag]) }
+  fab!(:original_topic) { Fabricate(:topic, category: category) }
   fab!(:first_post) { Fabricate(:post, topic: original_topic, user: expert) }
   fab!(:second_post) { Fabricate(:post, topic: original_topic, user: expert) }
 
@@ -221,9 +221,12 @@
     describe "on 'post_edited' with category change" do
       fab!(:category_b, :category)
       fab!(:group_b, :group)
+      fab!(:auto_tag_b, :tag)
 
       before do
+        SiteSetting.tagging_enabled = true
         category_b.custom_fields[CategoryExperts::CATEGORY_EXPERT_GROUP_IDS] = "#{group_b.id}"
+        category_b.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG] = auto_tag_b.name
         category_b.save!
       end
 
@@ -257,6 +260,27 @@
           ).to be_blank
           expect(second_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to be_blank
         end
+
+        it "removes old auto-tag and do not add new one" do
+          expect(original_topic.tags.map(&:name)).to include(auto_tag.name)
+          expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
+
+          category_without_experts = Fabricate(:category)
+          category_without_experts.custom_fields[
+            CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
+          ] = auto_tag_b.name
+          category_without_experts.save!
+
+          PostRevisor.new(original_topic.first_post).revise!(
+            admin,
+            category_id: category_without_experts.id,
+          )
+
+          original_topic.reload
+
+          expect(original_topic.tags.map(&:name)).not_to include(auto_tag.name)
+          expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
+        end
       end
 
       context "when topic is moved from one category with experts to another with different experts" do
@@ -339,18 +363,9 @@
             expect(third_post.custom_fields[CategoryExperts::POST_APPROVED_GROUP_NAME]).to be_blank
           end
         end
-      end
-
-      context "when topic has auto-tag" do
-        fab!(:auto_tag_b, :tag)
 
-        before do
-          SiteSetting.tagging_enabled = true
-          category_b.custom_fields[CategoryExperts::CATEGORY_EXPERT_AUTO_TAG] = auto_tag_b.name
-          category_b.save!
-        end
-
-        it "removes old auto-tag and adds new auto-tag when category changes" do
+        it "updates auto-tag appropriately" do
+          group_b.add(expert)
           expect(original_topic.tags.map(&:name)).to include(auto_tag.name)
           expect(original_topic.tags.map(&:name)).not_to include(auto_tag_b.name)
 
@@ -362,6 +377,40 @@
           expect(original_topic.tags.map(&:name)).to include(auto_tag_b.name)
         end
       end
+
+      context "when topic is moved between two categories without experts" do
+        fab!(:category_without_experts_a, :category)
+        fab!(:category_without_experts_b, :category)
+
+        fab!(:topic_without_experts) { Fabricate(:topic, category: category_without_experts_a) }
+        fab!(:post_without_experts) { Fabricate(:post, topic: topic_without_experts) }
+
+        before do
+          category_without_experts_a.custom_fields[
+            CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
+          ] = auto_tag.name
+          category_without_experts_b.custom_fields[
+            CategoryExperts::CATEGORY_EXPERT_AUTO_TAG
+          ] = auto_tag_b.name
+          category_without_experts_a.save!
+          category_without_experts_b.save!
+        end
+
+        it "does not auto-tag" do
+          expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag.name)
+          expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag_b.name)
+
+          PostRevisor.new(topic_without_experts.first_post).revise!(
+            admin,
+            category_id: category_without_experts_b.id,
+          )
+
+          topic_without_experts.reload
+
+          expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag.name)
+          expect(topic_without_experts.tags.map(&:name)).not_to include(auto_tag_b.name)
+        end
+      end
     end
   end
 end