FIX: Miscellaneous tagging errors (#21490)

* FIX: Displaying the wrong number of minimum tags in the composer

When the minimum number of tags set for the category is larger than the minimum number of tags
set in the category tag-groups, the composer was displaying the wrong value.

This commit fixes the value displayed in the composer to show the max value between the required
for the category and the tag-groups set for the category.

This bug was reported on Meta in https://meta.discourse.org/t/tags-from-multiple-tag-groups-required-only-suggest-select-at-least-one-tag/263817

* FIX: Limiting tags in categories not working as expected

When a category was restricted to a tag group A, which was set to only allow
one tag from the group per topic, selecting a tag belonging only to A returned
other tags from A that also belonged to other group/s (if any).

Example:

Tag group A: alpha, beta, gamma, epsilon, delta
Tag group B: alpha, beta, gamma

Both tag groups set to only allow one tag from the group per topic.

If Category 1 was set to only allow tags from the tag group A, and the first tag
selected was epsilon, then, because they also belonged to tag group B, the tags
alpha, beta, and gamma were still returned as valid options when they should not be.

This commit ensures that once a tag from a tag group that restricts its tags to
one per topic is selected, no other tag from this group is returned.

This bug was reported on Meta in https://meta.discourse.org/t/limiting-tags-to-categories-not-working-as-expected/263143.

* FIX: Moving topics does not prompt to add required tag for new category

When a topic moved from a category to another, the tag requirements
of the new category were not being checked.

This allowed a topic to be created and moved to a category:

- that limited the tags to a tag group, with the topic containing tags
not allowed.
- that required N tags from a tag group, with the topic not containing
the required tags.

This bug was reported on Meta in https://meta.discourse.org/t/moving-tagged-topics-does-not-prompt-to-add-required-tag-for-new-category/264138.

* FIX: Editing topics with tag groups from parents allows incorrect tagging

When there was a combination between parent tags defined in a tag group
set to allow only one tag from the group per topic, and other tag groups
relying on this restriction to combine the children tag types with the
parent tag, editing a topic could allow the user to insert an invalid
combination of these tags.

Example:

Automakers tag group: landhover, toyota
  - group set to limit one tag from the group per topic

Toyota models group: land-cruiser, hilux, corolla

Landhover models group: evoque, defender, discovery

If a topic was initially set up with the tags toyota, land-cruiser it was
possible to edit it by removing the tag toyota and adding the tag landhover
and other landhover model tags like evoque for example.

In this case, the topic would end up with the tags toyota, land-cruiser,
landhover, evoque because Discourse will automatically insert the
missing parent tag toyota when it detects the tag land-cruiser.

This combination of tags would violate the restriction specified in
the Automakers tag group resulting in an invalid combination of tags.

This commit enforces that the "one tag from the group per topic"
restriction is verified before updating the topic tags and also
make sure the verification checks the compatibility of parent tags that
would be automatically inserted.

After the changes, the user will receive an error similar to:
The tags land-cruiser, landhover cannot be used simultaneously.
Please include only one of them.

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;

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) {

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}"

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

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

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)

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),

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)