diff --git a/app/models/category.rb b/app/models/category.rb index fc9d1377570..6a0e0d5f338 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -933,6 +933,10 @@ class Category < ActiveRecord::Base end end + def has_restricted_tags? + tags.count > 0 || tag_groups.count > 0 + end + private def should_update_reviewables? diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 25c7e2678df..24645833a75 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4810,6 +4810,12 @@ en: other: '"%{tag_name}" is restricted to the following categories: %{category_names}' synonym: 'Synonyms are not allowed. Use "%{tag_name}" instead.' has_synonyms: '"%{tag_name}" cannot be used because it has synonyms.' + restricted_tags_cannot_be_used_in_category: + one: 'The "%{tags}" tag cannot be used in the "%{category}" category. Please remove it.' + other: 'The following tags cannot be used in the "%{category}" category: %{tags}. Please remove them.' + category_does_not_allow_tags: + one: 'The "%{category}" category does not allow the "%{tags}" tag. Please remove it.' + other: 'The "%{category}" category does not allow the following tags: "%{tags}". Please remove them.' required_tags_from_group: one: "You must include at least %{count} %{tag_group_name} tag. The tags in this group are: %{tags}." other: "You must include at least %{count} %{tag_group_name} tags. The tags in this group are: %{tags}." diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 4b2cb3db435..28fa7fe0592 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -175,6 +175,51 @@ module DiscourseTagging end end + def self.validate_category_restricted_tags(guardian, model, category, tags = []) + return true if tags.blank? || category.blank? + + tags = tags.map(&:name) if Tag === tags[0] + tags_restricted_to_categories = Hash.new { |h, k| h[k] = Set.new } + + query = Tag.where(name: tags) + query.joins(tag_groups: :categories).pluck(:name, 'categories.id').each do |(tag, cat_id)| + tags_restricted_to_categories[tag] << cat_id + end + query.joins(:categories).pluck(:name, 'categories.id').each do |(tag, cat_id)| + tags_restricted_to_categories[tag] << cat_id + end + + unallowed_tags = tags_restricted_to_categories.keys.select do |tag| + !tags_restricted_to_categories[tag].include?(category.id) + end + + if unallowed_tags.present? + msg = I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: unallowed_tags.size, + tags: unallowed_tags.sort.join(", "), + category: category.name + ) + model.errors.add(:base, msg) + return false + end + + if !category.allow_global_tags && category.has_restricted_tags? + unrestricted_tags = tags - tags_restricted_to_categories.keys + if unrestricted_tags.present? + msg = I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: unrestricted_tags.size, + tags: unrestricted_tags.sort.join(", "), + category: category.name + ) + model.errors.add(:base, msg) + return false + end + end + true + end + TAG_GROUP_RESTRICTIONS_SQL ||= <<~SQL tag_group_restrictions AS ( SELECT t.id as tag_id, tgm.id as tgm_id, tg.id as tag_group_id, tg.parent_tag_id as parent_tag_id, diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 9119ec95511..c7e812f8160 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -30,9 +30,10 @@ class TopicCreator existing_tags = tags.present? ? Tag.where(name: tags) : [] valid_tags = guardian.can_create_tag? ? tags : existing_tags - # both add to topic.errors + # all add to topic.errors DiscourseTagging.validate_min_required_tags_for_category(guardian, topic, category, valid_tags) DiscourseTagging.validate_required_tags_from_group(guardian, topic, category, existing_tags) + DiscourseTagging.validate_category_restricted_tags(guardian, topic, category, valid_tags) end DiscourseEvent.trigger(:after_validate_topic, topic, self) diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 1f3fa9fbc24..6319f0e6393 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -31,11 +31,25 @@ describe "category tag restrictions" do end it "tags belonging to that category can only be used there" do - post = create_post(category: category_with_tags, tags: [tag1.name, tag2.name, tag3.name]) - expect(post.topic.tags).to contain_exactly(tag1, tag2) + msg = I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + tags: tag3.name, + category: category_with_tags.name + ) + expect { + create_post(category: category_with_tags, tags: [tag1.name, tag2.name, tag3.name]) + }.to raise_error(StandardError, msg) - post = create_post(category: other_category, tags: [tag1.name, tag2.name, tag3.name]) - expect(post.topic.tags).to contain_exactly(tag3) + msg = I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 2, + tags: [tag1, tag2].map(&:name).sort.join(", "), + category: other_category.name + ) + expect { + create_post(category: other_category, tags: [tag1.name, tag2.name, tag3.name]) + }.to raise_error(StandardError, msg) end it "search can show only permitted tags" do @@ -55,10 +69,19 @@ describe "category tag restrictions" do end it "can't create new tags in a restricted category" do - post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"]) - expect_same_tag_names(post.topic.tags, [tag1]) - post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin) - expect_same_tag_names(post.topic.tags, [tag1]) + msg = I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + tags: "newtag", + category: category_with_tags.name + ) + expect { + create_post(category: category_with_tags, tags: [tag1.name, "newtag"]) + }.to raise_error(StandardError, msg) + + expect { + create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin) + }.to raise_error(StandardError, msg) end it "can create new tags in a non-restricted category" do @@ -149,8 +172,15 @@ describe "category tag restrictions" do end it "enforces restrictions when creating a topic" do - post = create_post(category: category, tags: [tag1.name, "newtag"]) - expect(post.topic.tags.map(&:name)).to eq([tag1.name]) + msg = I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + tags: "newtag", + category: category.name + ) + expect { + create_post(category: category, tags: [tag1.name, "newtag"]) + }.to raise_error(StandardError, msg) end it "handles colons" do diff --git a/spec/lib/new_post_manager_spec.rb b/spec/lib/new_post_manager_spec.rb index 5344e328d07..532a6f1008c 100644 --- a/spec/lib/new_post_manager_spec.rb +++ b/spec/lib/new_post_manager_spec.rb @@ -52,6 +52,53 @@ describe NewPostManager do expect(result.post).to be_a(Post) end + it "doesn't enqueue topic if it doesn't meet the tag restrictions of the category" do + tag1 = Fabricate(:tag) + tag2 = Fabricate(:tag) + tag3 = Fabricate(:tag) + tag_group = Fabricate(:tag_group, tags: [tag2]) + category = Fabricate(:category, tags: [tag1], tag_groups: [tag_group]) + category.custom_fields[Category::REQUIRE_TOPIC_APPROVAL] = true + category.save! + + manager = NewPostManager.new( + user, + raw: 'this is a new post', + title: 'this is a new post', + category: category.id, + tags: [tag1.name, tag3.name] + ) + result = manager.perform + expect(result.success?).to eq(false) + expect(result.reviewable.persisted?).to eq(false) + expect(result.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + category: category.name, + tags: tag3.name + ) + ) + + manager = NewPostManager.new( + user, + raw: 'this is a new post', + title: 'this is a new post', + category: category.id, + tags: [tag2.name, tag3.name] + ) + result = manager.perform + expect(result.success?).to eq(false) + expect(result.reviewable.persisted?).to eq(false) + expect(result.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + category: category.name, + tags: tag3.name + ) + ) + end end context "default handler" do diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb index d311e23d350..c5bdf02f96f 100644 --- a/spec/lib/topic_creator_spec.rb +++ b/spec/lib/topic_creator_spec.rb @@ -84,6 +84,11 @@ describe TopicCreator do context 'tags' do fab!(:tag1) { Fabricate(:tag, name: "fun") } fab!(:tag2) { Fabricate(:tag, name: "fun2") } + fab!(:tag3) { Fabricate(:tag, name: "fun3") } + fab!(:tag4) { Fabricate(:tag, name: "fun4") } + fab!(:tag5) { Fabricate(:tag, name: "fun5") } + fab!(:tag_group1) { Fabricate(:tag_group, tags: [tag1]) } + fab!(:tag_group2) { Fabricate(:tag_group, tags: [tag2]) } before do SiteSetting.tagging_enabled = true @@ -180,6 +185,203 @@ describe TopicCreator do ).to be_truthy end end + + context "when category has restricted tags or tag groups" do + fab!(:category) { Fabricate(:category, tags: [tag3], tag_groups: [tag_group1]) } + + it "allows topics without any tags" do + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic without tags", + raw: "hello this is a test topic without tags", + category: category.id, + ) + expect(tc.valid?).to eq(true) + expect(tc.errors).to be_empty + topic = tc.create + expect(topic.tags).to be_empty + end + + it "allows topics if they use tags only from the tags set that the category restricts" do + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag3.name] + ) + expect(tc.valid?).to eq(true) + expect(tc.errors).to be_empty + topic = tc.create + expect(topic.tags).to contain_exactly(tag1, tag3) + end + + it "allows topics to use tags that are restricted in multiple categories" do + category2 = Fabricate(:category, tags: [tag5], tag_groups: [tag_group1]) + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category2.id, + tags: [tag1.name, tag5.name] + ) + expect(tc.valid?).to eq(true) + expect(tc.errors).to be_empty + topic = tc.create + expect(topic.tags).to contain_exactly(tag1, tag5) + + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags 1", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag3.name] + ) + expect(tc.valid?).to eq(true) + expect(tc.errors).to be_empty + topic = tc.create + expect(topic.tags).to contain_exactly(tag1, tag3) + + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags 2", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag5.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 1, + tags: tag5.name, + category: category.name + ) + ) + end + + it "rejects topics if they use a tag outside the set of tags that the category restricts" do + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag2.name, tag1.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 1, + tags: tag2.name, + category: category.name + ) + ) + + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag2.name, tag5.name, tag3.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.category_does_not_allow_tags", + count: 2, + tags: [tag2, tag5].map(&:name).sort.join(", "), + category: category.name + ) + ) + end + + it "rejects topics in other categories if a restricted tag of a category are used" do + category2 = Fabricate(:category) + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category2.id, + tags: [tag1.name, tag2.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 1, + tags: tag1.name, + category: category2.name + ) + ) + end + + context "and allows other tags" do + before { category.update!(allow_global_tags: true) } + + it "allows topics to use tags that aren't restricted by any category" do + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag2.name, tag3.name, tag5.name] + ) + expect(tc.valid?).to eq(true) + expect(tc.errors).to be_empty + topic = tc.create + expect(topic.tags).to contain_exactly(tag1, tag2, tag3, tag5) + end + + it "rejects topics if they use restricted tags of another category" do + Fabricate(:category, tags: [tag5], tag_groups: [tag_group2]) + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag5.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 1, + tags: tag5.name, + category: category.name + ) + ) + + tc = TopicCreator.new( + user, + Guardian.new(user), + title: "hello this is a test topic with tags", + raw: "hello this is a test topic with tags", + category: category.id, + tags: [tag1.name, tag2.name, tag5.name] + ) + expect(tc.valid?).to eq(false) + expect(tc.errors.full_messages).to contain_exactly( + I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 2, + tags: [tag2, tag5].map(&:name).sort.join(", "), + category: category.name + ) + ) + end + end + end end context 'personal message' do