FIX: Fail if none of our tags could be updated

FIX: Fail if none of our tags could be updated

For example, if a category has a tag restriction and the API tries to attempt to update it but cannot.

See: Unallowed tag in conversation returns 200 - bug - Discourse Meta

diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index 9f824a2..39e7cb1 100644
--- a/lib/discourse_tagging.rb
+++ b/lib/discourse_tagging.rb
@@ -83,6 +83,7 @@ module DiscourseTagging
           return false
         end
 
+        return false if tags.size == 0
         topic.tags = tags
       else
         # validate minimum required tags for a category
diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb
index 8ab8b61..3dfa176 100644
--- a/spec/components/discourse_tagging_spec.rb
+++ b/spec/components/discourse_tagging_spec.rb
@@ -116,14 +116,29 @@ describe DiscourseTagging do
       other_category = Fabricate(:category, allowed_tags: [other_tag.name])
       topic = Fabricate(:topic, category: category)
 
-      DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag.name, other_tag.name, 'hello'])
+      result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag.name, other_tag.name, 'hello'])
+      expect(result).to eq(true)
       expect(topic.tags.pluck(:name)).to contain_exactly(tag.name)
 
       category.update!(allow_global_tags: true)
-      DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag.name, other_tag.name, 'hello'])
+      result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [tag.name, other_tag.name, 'hello'])
+      expect(result).to eq(true)
       expect(topic.tags.pluck(:name)).to contain_exactly(tag.name, 'hello')
     end
 
+    it 'raises an error if no tags could be updated' do
+      tag = Fabricate(:tag)
+      other_tag = Fabricate(:tag)
+      tag_group = Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag.name])
+      category = Fabricate(:category, allowed_tag_groups: [tag_group.name])
+      other_category = Fabricate(:category, allowed_tags: [other_tag.name])
+      topic = Fabricate(:topic, category: category)
+
+      result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(admin), [other_tag.name])
+      expect(result).to eq(false)
+      expect(topic.tags.pluck(:name)).to be_blank
+    end
+
     context 'respects category minimum_required_tags setting' do
       fab!(:category) { Fabricate(:category, minimum_required_tags: 2) }
       fab!(:topic) { Fabricate(:topic, category: category) }

GitHub sha: c2c169f5

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there:

With this change, it looks like we do not return a useful error message to users creating topics via the UI. If I create a topic using a tag which is not allowed in the category, I get “Internal Server Error”. Previously, it would filter out the invalid tags and continue anyway.

I think this behaviour is more correct since the input is indeed bad. I will revise the error message to be more clear about the error.

1 Like

That should do it.

2 Likes