diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index 839fa6e7d22..ba517c0b0de 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -38,9 +38,11 @@ const Category = RestModel.extend({ @discourseComputed("required_tag_groups", "minimum_required_tags") minimumRequiredTags() { if (this.required_tag_groups?.length > 0) { - return this.required_tag_groups.reduce( - (sum, rtg) => sum + rtg.min_count, - 0 + // it should require the max between the bare minimum set in the category and the sum of the min_count of the + // required_tag_groups + return Math.max( + this.required_tag_groups.reduce((sum, rtg) => sum + rtg.min_count, 0), + this.minimum_required_tags || 0 ); } else { return this.minimum_required_tags > 0 ? this.minimum_required_tags : null; diff --git a/app/assets/javascripts/discourse/tests/unit/models/category-test.js b/app/assets/javascripts/discourse/tests/unit/models/category-test.js index 69ad24f03e7..f7dac2d5eac 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/category-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/category-test.js @@ -263,6 +263,27 @@ module("Unit | Model | category", function (hooks) { }); assert.strictEqual(quux.minimumRequiredTags, null); + + const foobar = store.createRecord("category", { + id: 1, + slug: "foo", + minimum_required_tags: 2, + required_tag_groups: [{ name: "bar", min_count: 1 }], + }); + + assert.strictEqual(foobar.minimumRequiredTags, 2); + + const barfoo = store.createRecord("category", { + id: 1, + slug: "foo", + minimum_required_tags: 2, + required_tag_groups: [ + { name: "foo", min_count: 1 }, + { name: "bar", min_count: 2 }, + ], + }); + + assert.strictEqual(barfoo.minimumRequiredTags, 3); }); test("search with category name", function (assert) { diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 4730b43f566..577b8e29283 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4916,6 +4916,7 @@ en: 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}." + limited_to_one_tag_from_group: "The tags %{tags} cannot be used simultaneously. Please include only one of them." invalid_target_tag: "cannot be a synonym of a synonym" synonyms_exist: "is not allowed while synonyms exist" rss_by_tag: "Topics tagged %{tag}" diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 878f05ba15d..03567f44bb4 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -102,6 +102,12 @@ module DiscourseTagging end end + # tests if there are conflicts between tags on tag groups that only allow one tag from the group before adding + # mandatory parent tags because later we want to test if the mandatory parent tags introduce any conflicts + # and be able to pinpoint the tag that is introducing it + # guardian like above is nil to prevent stripping tags that already passed validation + return false unless validate_one_tag_from_group_per_topic(nil, topic, category, tags) + # add missing mandatory parent tags tag_ids = tags.map(&:id) @@ -132,7 +138,57 @@ module DiscourseTagging .compact .uniq - tags = tags + Tag.where(id: missing_parent_tag_ids).all unless missing_parent_tag_ids.empty? + missing_parent_tags = Tag.where(id: missing_parent_tag_ids).all + + tags = tags + missing_parent_tags unless missing_parent_tags.empty? + + parent_tag_conflicts = + filter_tags_violating_one_tag_from_group_per_topic( + nil, # guardian like above is nil to prevent stripping tags that already passed validation + topic.category, + tags, + ) + + if parent_tag_conflicts.present? + # we need to get the original tag names that introduced conflicting missing parent tags to return an useful + # error message + parent_child_names_map = {} + parent_tags_map.each do |tag_id, parent_tag_ids| + next if (tag_ids & parent_tag_ids).size > 0 # tag already has a parent tag + + parent_tag = tags.select { |t| t.id == parent_tag_ids.first }.first + original_child_tag = tags.select { |t| t.id == tag_id }.first + + next unless parent_tag.present? && original_child_tag.present? + parent_child_names_map[parent_tag.name] = original_child_tag.name + end + + # replaces the added missing parent tags with the original tag + parent_tag_conflicts.map do |_, conflicting_tags| + topic.errors.add( + :base, + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: + conflicting_tags + .map do |tag| + tag_name = tag.name + + if parent_child_names_map[tag_name].present? + parent_child_names_map[tag_name] + else + tag_name + end + end + .uniq + .sort + .join(", "), + ), + ) + end + + return false + end return false unless validate_min_required_tags_for_category(guardian, topic, category, tags) return false unless validate_required_tags_from_group(guardian, topic, category, tags) @@ -163,6 +219,19 @@ module DiscourseTagging false end + def self.validate_category_tags(guardian, model, category, tags = []) + existing_tags = tags.present? ? Tag.where(name: tags) : [] + valid_tags = guardian.can_create_tag? ? tags : existing_tags + + # all add to model (topic) errors + valid = validate_min_required_tags_for_category(guardian, model, category, valid_tags) + valid &&= validate_required_tags_from_group(guardian, model, category, existing_tags) + valid &&= validate_category_restricted_tags(guardian, model, category, valid_tags) + valid &&= validate_one_tag_from_group_per_topic(guardian, model, category, valid_tags) + + valid + end + def self.validate_min_required_tags_for_category(guardian, model, category, tags = []) if !guardian.is_staff? && category && category.minimum_required_tags > 0 && tags.length < category.minimum_required_tags @@ -250,6 +319,54 @@ module DiscourseTagging true end + def self.validate_one_tag_from_group_per_topic(guardian, model, category, tags = []) + tags_cant_be_used = filter_tags_violating_one_tag_from_group_per_topic(guardian, category, tags) + + return true if tags_cant_be_used.blank? + + tags_cant_be_used.each do |_, incompatible_tags| + model.errors.add( + :base, + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: incompatible_tags.map(&:name).sort.join(", "), + ), + ) + end + + false + end + + def self.filter_tags_violating_one_tag_from_group_per_topic(guardian, category, tags = []) + return [] if tags.size < 2 + + # ensures that tags are a list of tag names + tags = tags.map(&:name) if Tag === tags[0] + + allowed_tags = + filter_allowed_tags( + guardian, + category: category, + only_tag_names: tags, + for_topic: true, + order_search_results: true, + ) + + return {} if allowed_tags.size < 2 + + tags_by_group_map = + allowed_tags + .sort_by { |tag| [tag.tag_group_id || -1, tag.name] } + .inject({}) do |hash, tag| + next hash unless tag.one_per_topic + + hash[tag.tag_group_id] = (hash[tag.tag_group_id] || []) << tag + hash + end + + tags_by_group_map.select { |_, group_tags| group_tags.size > 1 } + 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, @@ -457,7 +574,7 @@ module DiscourseTagging if !one_tag_per_group_ids.empty? builder.where( - "tag_group_id IS NULL OR tag_group_id NOT IN (?) OR id IN (:selected_tag_ids)", + "t.id NOT IN (SELECT DISTINCT tag_id FROM tag_group_restrictions WHERE tag_group_id IN (?)) OR id IN (:selected_tag_ids)", one_tag_per_group_ids, ) end diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 7f9d55c1b60..71ece6f4d14 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -90,12 +90,7 @@ class PostRevisor elsif new_category.nil? || tc.guardian.can_move_topic_to_category?(new_category_id) tags = fields[:tags] || tc.topic.tags.map(&:name) if new_category && - !DiscourseTagging.validate_min_required_tags_for_category( - tc.guardian, - tc.topic, - new_category, - tags, - ) + !DiscourseTagging.validate_category_tags(tc.guardian, tc.topic, new_category, tags) tc.check_result(false) next end diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index f9cdc99b441..24245291c54 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -28,18 +28,9 @@ class TopicCreator category = find_category if category.present? && guardian.can_tag?(topic) tags = @opts[:tags].presence || [] - existing_tags = tags.present? ? Tag.where(name: tags) : [] - valid_tags = guardian.can_create_tag? ? tags : existing_tags - # 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) + # adds topic.errors + DiscourseTagging.validate_category_tags(guardian, topic, category, tags) end DiscourseEvent.trigger(:after_validate_topic, topic, self) diff --git a/spec/lib/discourse_tagging_spec.rb b/spec/lib/discourse_tagging_spec.rb index 02782eefd5d..c767e03a1eb 100644 --- a/spec/lib/discourse_tagging_spec.rb +++ b/spec/lib/discourse_tagging_spec.rb @@ -97,6 +97,312 @@ RSpec.describe DiscourseTagging do end end + describe "#validate_one_tag_from_group_per_topic" do + fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2, tag3], one_per_topic: true) } + fab!(:topic) { Fabricate(:topic) } + fab!(:category) { Fabricate(:category, allowed_tag_groups: [tag_group.name]) } + + it "returns true if the topic doesn't belong to a category" do + result = + DiscourseTagging.validate_one_tag_from_group_per_topic(guardian, topic, nil, [tag1, tag2]) + expect(result).to eq(true) + end + + it "returns true if only one tag is provided" do + result = + DiscourseTagging.validate_one_tag_from_group_per_topic(guardian, topic, category, [tag1]) + expect(result).to eq(true) + + result = + DiscourseTagging.validate_one_tag_from_group_per_topic(guardian, topic, category, [tag2]) + expect(result).to eq(true) + end + + it "returns true if only one tag in the group matches" do + tag4 = Fabricate(:tag, name: "fun4") + + result = + DiscourseTagging.validate_one_tag_from_group_per_topic( + guardian, + topic, + category, + [tag1, tag4], + ) + expect(result).to eq(true) + end + + context "when it fails" do + it "returns false if more than one tag from the group is provided" do + result = + DiscourseTagging.validate_one_tag_from_group_per_topic( + guardian, + topic, + category, + [tag1, tag2], + ) + + expect(topic.errors[:base]&.first).to eq( + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: [tag1.name, tag2.name].sort.join(", "), + ), + ) + expect(result).to eq(false) + end + + it "returns multiple errors when incompatible sets from more then one group are detected" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], one_per_topic: true) + category2 = Fabricate(:category, allowed_tag_groups: [tag_group.name, tag_group2.name]) + + result = + DiscourseTagging.validate_one_tag_from_group_per_topic( + guardian, + topic, + category2, + [tag1, tag2, tag4, tag5], + ) + + expect(topic.errors[:base]).to contain_exactly( + *[[tag1.name, tag2.name], [tag4.name, tag5.name]].map do |failed_set| + I18n.t("tags.limited_to_one_tag_from_group", tags: failed_set.sort.join(", ")) + end, + ) + expect(result).to eq(false) + end + end + end + + describe "#filter_tags_violating_one_tag_from_group_per_topic" do + fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2, tag3], one_per_topic: true) } + + context "when the topic doesn't belong to a category" do + it "does not return tags that are not violating the one tag from group per topic rule" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + invalid_tags = + DiscourseTagging.filter_tags_violating_one_tag_from_group_per_topic( + guardian, + nil, + [tag4, tag5], + ) + expect(invalid_tags).to be_empty + end + + it "returns tags that are violating the one tag from group per topic rule when there is only one group" do + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic(guardian, nil, [tag1, tag2]) + .values + .first + .map(&:name) + expect(invalid_tags).to contain_exactly(tag1.name, tag2.name) + end + end + + context "when the topic belongs to a category" do + context "when the category only allows tags from some tag groups" do + it "returns tags that are violating the one tag from group per topic rule when there is only one group" do + category = Fabricate(:category, allowed_tag_groups: [tag_group.name]) + + [[tag1, tag2], [tag1, tag3], [tag2, tag3], [tag1, tag2, tag3]].each do |test_values| + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic(guardian, category, test_values) + .values + .first + .map(&:name) + expect(invalid_tags).to contain_exactly(*test_values.map(&:name)) + end + end + + it "returns tags that are violating the one tag from group per topic rule when there are multiple groups" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], one_per_topic: true) + + category = Fabricate(:category, allowed_tag_groups: [tag_group.name, tag_group2.name]) + + [ + [tag1, tag2], + [tag1, tag3], + [tag2, tag3], + [tag1, tag2, tag3], + [tag4, tag5], + ].each do |test_values| + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic(guardian, category, test_values) + .values + .first + .map(&:name) + expect(invalid_tags).to contain_exactly(*test_values.map(&:name)) + end + + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag1, tag2, tag4, tag5], + ) + .values + .map { |tags| tags.map(&:name) } + expect(invalid_tags).to contain_exactly([tag1.name, tag2.name], [tag4.name, tag5.name]) + end + + it "returns an empty array when only one tag is provided" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], one_per_topic: true) + + category = Fabricate(:category, allowed_tag_groups: [tag_group.name, tag_group2.name]) + + [tag1, tag2, tag3, tag4, tag5].each do |tag| + invalid_tags = + DiscourseTagging.filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag], + ) + expect(invalid_tags).to be_empty + end + end + + it "returns and empty array if the tags don't belong to a tag group" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + category = Fabricate(:category, allowed_tag_groups: [tag_group.name]) + + invalid_tags = + DiscourseTagging.filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag4, tag5], + ) + expect(invalid_tags).to be_empty + end + end + + context "when some tag groups are required in the category" do + it "returns tags that are violating the one tag from group per topic rule when there is only one group" do + category = + Fabricate( + :category, + category_required_tag_groups: [ + CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1), + ], + ) + + [[tag1, tag2], [tag1, tag3], [tag2, tag3], [tag1, tag2, tag3]].each do |test_values| + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic(guardian, category, test_values) + .values + .first + .map(&:name) + expect(invalid_tags).to contain_exactly(*test_values.map(&:name)) + end + end + + it "returns tags that are violating the one tag from group per topic rule when there are multiple groups" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], one_per_topic: true) + + category = + Fabricate( + :category, + category_required_tag_groups: [ + CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1), + CategoryRequiredTagGroup.new(tag_group: tag_group2, min_count: 1), + ], + ) + + [ + [tag1, tag2], + [tag1, tag3], + [tag2, tag3], + [tag1, tag2, tag3], + [tag4, tag5], + ].each do |test_values| + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic(guardian, category, test_values) + .values + .first + .map(&:name) + expect(invalid_tags).to contain_exactly(*test_values.map(&:name)) + end + + invalid_tags = + DiscourseTagging + .filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag1, tag2, tag4, tag5], + ) + .values + .map { |tags| tags.map(&:name) } + expect(invalid_tags).to contain_exactly([tag1.name, tag2.name], [tag4.name, tag5.name]) + end + + it "returns an empty array when only one tag is provided" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], one_per_topic: true) + + category = + Fabricate( + :category, + category_required_tag_groups: [ + CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1), + CategoryRequiredTagGroup.new(tag_group: tag_group2, min_count: 1), + ], + ) + + [tag1, tag2, tag3, tag4, tag5].each do |tag| + invalid_tags = + DiscourseTagging.filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag], + ) + expect(invalid_tags).to be_empty + end + end + + it "returns and empty array if the tags don't belong to a tag group" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + category = + Fabricate( + :category, + category_required_tag_groups: [ + CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1), + ], + ) + + invalid_tags = + DiscourseTagging.filter_tags_violating_one_tag_from_group_per_topic( + guardian, + category, + [tag4, tag5], + ) + expect(invalid_tags).to be_empty + end + end + end + end + describe "filter_allowed_tags" do context "for input fields" do it "doesn't return selected tags if there's a search term" do @@ -317,6 +623,139 @@ RSpec.describe DiscourseTagging do end end + context "with tag groups restricted to the category in which the number of tags per topic is limited to one" do + fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag2, tag3], one_per_topic: true) } + fab!(:tag_group2) { Fabricate(:tag_group, tags: [tag1, tag2], one_per_topic: true) } + + it "doesn't return tags leaked from other tag groups containing the same tags" do + # this tests covers the bug described in + # https://meta.discourse.org/t/limiting-tags-to-categories-not-working-as-expected/263143 + + category = Fabricate(:category, tag_groups: [tag_group]) + + # In the beginning, show tags for tag_group + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3])) + + # Once a tag has been selected it should not return any tags from the same group + # this is where the problem reported in the bug linked above was happening + # If the user selected a tag that belonged only to the tag group restricted to the category but other tags + # from the same tag group were also present in other tag groups, they were being returned because they were + # bleeding from the tag list as the filter performed in the query was scoping only the category to apply the + # restriction. Since the join was done on tag_id, it was returning all tags with the same ids even if they + # actually belonged to other tag groups that should not be returned because the category was not restricted + # to them + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag3.name], + ).to_a + expect(sorted_tag_names(tags)).to be_empty + end + + it "doesn't return a tag excluded from a tag group even if also belongs to another allowed one" do + tag4 = Fabricate(:tag) + tag5 = Fabricate(:tag) + tag_group3 = Fabricate(:tag_group, tags: [tag3, tag4], one_per_topic: true) + + category = Fabricate(:category, tag_groups: [tag_group, tag_group2, tag_group3]) + + # In the beginning, show all expected tags + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + + # tag3 belongs to tag_group1 and tag_group3. no tags from both groups should be returned + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag3.name], + ).to_a + expect(sorted_tag_names(tags)).to be_empty + + # tag4 only belong belongs to tag_group3. tag1 and tag2 should be returned because they belong to tag_group1 + # and tag_group2 but don't belong to tag_group3 + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag4.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2])) + end + + it "returns correctly tags from other restricted tag groups when they're not limited to one" do + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + tag_group3 = Fabricate(:tag_group, tags: [tag4, tag5]) + + category = Fabricate(:category, tag_groups: [tag_group, tag_group2, tag_group3]) + + # In the beginning, show all expected tags + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, tag4, tag5])) + + # Once a tag from a limited group has been selected it should not return any tags from the same group + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag4, tag5])) + + # if a tag from the group not limited to one tag is also selected the other should be returned + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name, tag4.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag5])) + + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name, tag5.name], + ).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag4])) + + # finally if all the tags from the group not limited to one tag are also selected, then there is no other + # tag to return + tags = + DiscourseTagging.filter_allowed_tags( + Guardian.new(user), + for_input: true, + category: category, + selected_tags: [tag1.name, tag4.name, tag5.name], + ).to_a + expect(sorted_tag_names(tags)).to be_empty + end + end + context "with many required tags in a tag group" do fab!(:tag4) { Fabricate(:tag, name: "T4") } fab!(:tag5) { Fabricate(:tag, name: "T5") } @@ -682,6 +1121,194 @@ RSpec.describe DiscourseTagging do ) end + it "fails when parent tag missing will conflict with another tag from tag group set to one tag per topic" do + parent_tag = Fabricate(:tag, name: "parent-1") + parent_tag2 = Fabricate(:tag, name: "parent-2") + parent_tag_group = + Fabricate(:tag_group, tags: [parent_tag, parent_tag2], one_per_topic: true) + + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + child_tag_group = + Fabricate(:tag_group, tags: [tag1, tag2, tag3], parent_tag_id: parent_tag.id) + child_tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], parent_tag_id: parent_tag2.id) + + tag_limited_category = + Fabricate( + :category, + allowed_tag_groups: [ + parent_tag_group.name, + child_tag_group.name, + child_tag_group2.name, + ], + ) + + topic = Fabricate(:topic, category: tag_limited_category) + + # tag2 will insert parent_tag which is missing. parent_tag will conflict with parent_tag2 + valid = + DiscourseTagging.tag_topic_by_names( + topic, + Guardian.new(user), + [parent_tag2.name, tag2.name], + ) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: [parent_tag2.name, tag2.name].sort.join(", "), + ), + ) + + topic = Fabricate(:topic, category: tag_limited_category) + + # tag4 will insert parent_tag2 which is missing. parent_tag will conflict with parent_tag2 + valid = + DiscourseTagging.tag_topic_by_names( + topic, + Guardian.new(user), + [parent_tag.name, tag1.name, tag4.name], + ) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: [parent_tag.name, tag4.name].sort.join(", "), + ), + ) + end + + it "fails when multiple parent tag missing will conflict with another tag from tag group set to one tag per topic" do + parent_tag = Fabricate(:tag, name: "parent-1") + parent_tag2 = Fabricate(:tag, name: "parent-2") + parent_tag3 = Fabricate(:tag, name: "parent-3") + parent_tag_group = + Fabricate(:tag_group, tags: [parent_tag, parent_tag2, parent_tag3], one_per_topic: true) + + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + child_tag_group = + Fabricate(:tag_group, tags: [tag1, tag2, tag3], parent_tag_id: parent_tag.id) + child_tag_group2 = Fabricate(:tag_group, tags: [tag4], parent_tag_id: parent_tag2.id) + child_tag_group3 = Fabricate(:tag_group, tags: [tag5], parent_tag_id: parent_tag3.id) + + tag_limited_category = + Fabricate( + :category, + allowed_tag_groups: [ + parent_tag_group.name, + child_tag_group.name, + child_tag_group2.name, + child_tag_group3.name, + ], + ) + + topic = Fabricate(:topic, category: tag_limited_category) + + # tag4 and tag5 will insert parent_tag2 and parent_tag3 which are missing. they will conflict with parent_tag + valid = + DiscourseTagging.tag_topic_by_names( + topic, + Guardian.new(user), + [parent_tag.name, tag4.name, tag5.name], + ) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: [parent_tag.name, tag4.name, tag5.name].sort.join(", "), + ), + ) + end + + it "fails when multiple parent tag missing will conflict in tag group set to one tag per topic" do + parent_tag = Fabricate(:tag, name: "parent-1") + parent_tag2 = Fabricate(:tag, name: "parent-2") + parent_tag_group = + Fabricate(:tag_group, tags: [parent_tag, parent_tag2], one_per_topic: true) + + tag4 = Fabricate(:tag, name: "fun4") + tag5 = Fabricate(:tag, name: "fun5") + + child_tag_group = + Fabricate(:tag_group, tags: [tag1, tag2, tag3], parent_tag_id: parent_tag.id) + child_tag_group2 = Fabricate(:tag_group, tags: [tag4, tag5], parent_tag_id: parent_tag2.id) + + tag_limited_category = + Fabricate( + :category, + allowed_tag_groups: [ + parent_tag_group.name, + child_tag_group.name, + child_tag_group2.name, + ], + ) + + topic = Fabricate(:topic, category: tag_limited_category) + + # tag1 and tag4 will insert parent_tag and parent_tag2 which are missing. they will conflict with each other + valid = + DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name, tag4.name]) + expect(valid).to eq(false) + expect(topic.errors[:base]&.first).to eq( + I18n.t( + "tags.limited_to_one_tag_from_group", + tags: [tag1.name, tag4.name].sort.join(", "), + ), + ) + end + + it "fails with multiple errors when parent tag missing will conflict in more than one tag group set to one tag per topic" do + parent_tag = Fabricate(:tag, name: "parent-1") + parent_tag2 = Fabricate(:tag, name: "parent-2") + parent_tag_group = + Fabricate(:tag_group, tags: [parent_tag, parent_tag2], one_per_topic: true) + + parent_tag3 = Fabricate(:tag, name: "parent-3") + parent_tag4 = Fabricate(:tag, name: "parent-4") + parent_tag_group2 = + Fabricate(:tag_group, tags: [parent_tag3, parent_tag4], one_per_topic: true) + + tag4 = Fabricate(:tag, name: "fun4") + + child_tag_group = Fabricate(:tag_group, tags: [tag1], parent_tag_id: parent_tag.id) + child_tag_group2 = Fabricate(:tag_group, tags: [tag2], parent_tag_id: parent_tag2.id) + child_tag_group3 = Fabricate(:tag_group, tags: [tag3], parent_tag_id: parent_tag3.id) + child_tag_group4 = Fabricate(:tag_group, tags: [tag4], parent_tag_id: parent_tag4.id) + + tag_limited_category = + Fabricate( + :category, + allowed_tag_groups: [ + parent_tag_group.name, + parent_tag_group2.name, + child_tag_group.name, + child_tag_group2.name, + child_tag_group3.name, + child_tag_group4.name, + ], + ) + + topic = Fabricate(:topic, category: tag_limited_category) + + # tag2 will insert parent_tag2 which is missing. it will conflict with parent_tag + # tag4 will insert parent_tag4 which is missing. it will conflict with parent_tag3 + valid = + DiscourseTagging.tag_topic_by_names( + topic, + Guardian.new(user), + [parent_tag.name, tag2.name, parent_tag3.name, tag4.name], + ) + expect(valid).to eq(false) + expect(topic.errors[:base]).to contain_exactly( + *[[parent_tag.name, tag2.name], [parent_tag3.name, tag4.name]].map do |conflicting_tags| + I18n.t("tags.limited_to_one_tag_from_group", tags: conflicting_tags.sort.join(", ")) + end, + ) + end + it "adds only the necessary parent tags" do common = Fabricate(:tag, name: "common") tag_group.tags = [tag3, common] @@ -832,7 +1459,8 @@ RSpec.describe DiscourseTagging do ) end - it "only sanitizes new tags" do # for backwards compat + it "only sanitizes new tags" do + # for backwards compat Tag.new(name: "math=fun").save(validate: false) expect( described_class.tags_for_saving(%w[math=fun fun*2@gmail.com], guardian).try(:sort), diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 1696471dcda..54303de8e2f 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/spec/lib/post_revisor_spec.rb @@ -120,6 +120,80 @@ RSpec.describe PostRevisor do post.revise(post.user, category_id: new_category.id, tags: ["test_tag"]) expect(post.reload.topic.category_id).to eq(new_category.id) end + + it "returns an error if the topic does not have minimum amount of tags that the new category requires" do + SiteSetting.min_trust_to_create_tag = 0 + SiteSetting.min_trust_level_to_tag_topics = 0 + + old_category = Fabricate(:category, minimum_required_tags: 0) + new_category = Fabricate(:category, minimum_required_tags: 1) + + post = create_post(category: old_category) + topic = post.topic + + post.revise(post.user, category_id: new_category.id) + expect(topic.errors.full_messages).to eq([I18n.t("tags.minimum_required_tags", count: 1)]) + end + + it "returns an error if the topic has tags not allowed in the new category" do + SiteSetting.min_trust_to_create_tag = 0 + SiteSetting.min_trust_level_to_tag_topics = 0 + + tag1 = Fabricate(:tag) + tag2 = Fabricate(:tag) + tag_group = Fabricate(:tag_group, tags: [tag1]) + tag_group2 = Fabricate(:tag_group, tags: [tag2]) + + old_category = Fabricate(:category, tag_groups: [tag_group]) + new_category = Fabricate(:category, tag_groups: [tag_group2]) + + post = create_post(category: old_category, tags: [tag1.name]) + topic = post.topic + + post.revise(post.user, category_id: new_category.id) + expect(topic.errors.full_messages).to eq( + [ + I18n.t( + "tags.forbidden.restricted_tags_cannot_be_used_in_category", + count: 1, + tags: tag1.name, + category: new_category.name, + ), + ], + ) + end + + it "returns an error if the topic is missing tags required from a tag group in the new category" do + SiteSetting.min_trust_to_create_tag = 0 + SiteSetting.min_trust_level_to_tag_topics = 0 + + tag1 = Fabricate(:tag) + tag_group = Fabricate(:tag_group, tags: [tag1]) + + old_category = Fabricate(:category) + new_category = + Fabricate( + :category, + category_required_tag_groups: [ + CategoryRequiredTagGroup.new(tag_group: tag_group, min_count: 1), + ], + ) + + post = create_post(category: old_category) + topic = post.topic + + post.revise(post.user, category_id: new_category.id) + expect(topic.errors.full_messages).to eq( + [ + I18n.t( + "tags.required_tags_from_group", + count: 1, + tag_group_name: tag_group.name, + tags: tag1.name, + ), + ], + ) + end end describe "editing tags" do @@ -1377,20 +1451,20 @@ RSpec.describe PostRevisor do let(:image3) { Fabricate(:upload) } let(:image4) { Fabricate(:upload) } let(:post_args) { { user: user, topic: topic, raw: <<~RAW } } - This is a post with multiple uploads - ![image1](#{image1.short_url}) - ![image2](#{image2.short_url}) - RAW + This is a post with multiple uploads + ![image1](#{image1.short_url}) + ![image2](#{image2.short_url}) + RAW it "updates linked post uploads" do post.link_post_uploads expect(post.upload_references.pluck(:upload_id)).to contain_exactly(image1.id, image2.id) subject.revise!(user, raw: <<~RAW) - This is a post with multiple uploads - ![image2](#{image2.short_url}) - ![image3](#{image3.short_url}) - ![image4](#{image4.short_url}) + This is a post with multiple uploads + ![image2](#{image2.short_url}) + ![image3](#{image3.short_url}) + ![image4](#{image4.short_url}) RAW expect(post.reload.upload_references.pluck(:upload_id)).to contain_exactly( @@ -1413,8 +1487,8 @@ RSpec.describe PostRevisor do it "updates the upload secure status, which is secure by default from the composer. set to false for a public topic" do stub_image_size subject.revise!(user, raw: <<~RAW) - This is a post with a secure upload - ![image5](#{image5.short_url}) + This is a post with a secure upload + ![image5](#{image5.short_url}) RAW expect(image5.reload.secure).to eq(false) @@ -1427,8 +1501,8 @@ RSpec.describe PostRevisor do post.topic.update(category: Fabricate(:private_category, group: Fabricate(:group))) stub_image_size subject.revise!(user, raw: <<~RAW) - This is a post with a secure upload - ![image5](#{image5.short_url}) + This is a post with a secure upload + ![image5](#{image5.short_url}) RAW expect(image5.reload.secure).to eq(true)