Skip to content

Commit

Permalink
FIX: Fail if none of our tags could be updated
Browse files Browse the repository at this point in the history
For example, if a category has a tag restriction and the API tries to
attempt to update it but cannot.

See:
https://meta.discourse.org/t/unallowed-tag-in-conversation-returns-200/122170
  • Loading branch information
eviltrout committed Jul 5, 2019
1 parent 5494e17 commit c2c169f
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 2 deletions.
1 change: 1 addition & 0 deletions lib/discourse_tagging.rb
Expand Up @@ -83,6 +83,7 @@ def self.tag_topic_by_names(topic, guardian, tag_names_arg, append: false)
return false
end

return false if tags.size == 0
topic.tags = tags
else
# validate minimum required tags for a category
Expand Down
19 changes: 17 additions & 2 deletions spec/components/discourse_tagging_spec.rb
Expand Up @@ -116,14 +116,29 @@
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) }
Expand Down

4 comments on commit c2c169f

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://meta.discourse.org/t/unallowed-tag-in-conversation-returns-200/122170/2

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

David Taylor posted:

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.

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robin Ward posted:

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.

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robin Ward posted:

a727968

That should do it.

Please sign in to comment.