FIX: tag cannot be used if it belongs to two tag groups with parent tag

FIX: tag cannot be used if it belongs to two tag groups with parent tag

If two tag groups exist with a mandatory parent tag, and one tag is added to both tag groups, then the tag couldn’t be used on any topics.

diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index 0213eb73fc..e8bb6b96ec 100644
--- a/lib/discourse_tagging.rb
+++ b/lib/discourse_tagging.rb
@@ -65,16 +65,27 @@ module DiscourseTagging
         end
 
         # add missing mandatory parent tags
-        parent_tags = TagGroup.includes(:parent_tag).where("tag_groups.id IN (
-          SELECT tg.id
+        tag_ids = tags.map(&:id)
+
+        parent_tags_map = DB.query("
+          SELECT tgm.tag_id, tg.parent_tag_id
             FROM tag_groups tg
       INNER JOIN tag_group_memberships tgm
               ON tgm.tag_group_id = tg.id
            WHERE tg.parent_tag_id IS NOT NULL
-             AND tgm.tag_id IN (?))", tags.map(&:id)).map(&:parent_tag)
+             AND tgm.tag_id IN (?)
+        ", tag_ids).inject({}) do |h, v|
+          h[v.tag_id] ||= []
+          h[v.tag_id] << v.parent_tag_id
+          h
+        end
+
+        missing_parent_tag_ids = parent_tags_map.map do |_, parent_tag_ids|
+          (tag_ids & parent_tag_ids).size == 0 ? parent_tag_ids.first : nil
+        end.compact.uniq
 
-        parent_tags.reject { |t| tag_names.include?(t.name) }.each do |tag|
-          tags << tag
+        unless missing_parent_tag_ids.empty?
+          tags = tags + Tag.where(id: missing_parent_tag_ids).all
         end
 
         # validate minimum required tags for a category
@@ -187,8 +198,18 @@ module DiscourseTagging
         exclude_group_ids = one_per_topic_group_ids(selected_tag_ids)
 
         if exclude_group_ids.empty?
-          sql = "tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE tg.parent_tag_id NOT IN (?))"
-          query = query.where(sql, selected_tag_ids)
+          # tags that don't belong to groups that require a parent tag,
+          # and tags that belong to groups with parent tag selected
+          query = query.where(<<~SQL, selected_tag_ids, selected_tag_ids)
+            tags.id NOT IN (
+              #{TAG_GROUP_TAG_IDS_SQL}
+              WHERE tg.parent_tag_id NOT IN (?)
+            )
+            OR tags.id IN (
+              #{TAG_GROUP_TAG_IDS_SQL}
+              WHERE tg.parent_tag_id IN (?)
+            )
+          SQL
         else
           # It's possible that the selected tags violate some one-tag-per-group restrictions,
           # so filter them out by picking one from each group.
diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb
index 3dfa17662c..d269ffd5ec 100644
--- a/spec/components/discourse_tagging_spec.rb
+++ b/spec/components/discourse_tagging_spec.rb
@@ -194,9 +194,9 @@ describe DiscourseTagging do
     context 'tag group with parent tag' do
       let(:topic) { Fabricate(:topic, user: user) }
       let(:post) { Fabricate(:post, user: user, topic: topic, post_number: 1) }
+      let(:tag_group) { Fabricate(:tag_group, parent_tag_id: tag1.id) }
 
       before do
-        tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id)
         tag_group.tags = [tag3]
       end
 
@@ -222,6 +222,19 @@ describe DiscourseTagging do
           *[tag1, tag2, tag3, parent_tag].map(&:name)
         )
       end
+
+      it "adds only the necessary parent tags" do
+        common = Fabricate(:tag, name: 'common')
+        tag_group.tags = [tag3, common]
+
+        parent_tag = Fabricate(:tag, name: 'parent')
+        tag_group2 = Fabricate(:tag_group, parent_tag_id: parent_tag.id)
+        tag_group2.tags = [tag2, common]
+
+        valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [parent_tag.name, common.name])
+        expect(valid).to eq(true)
+        expect(topic.reload.tags.map(&:name)).to contain_exactly(*[parent_tag, common].map(&:name))
+      end
     end
   end
 
diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb
index b20829778b..9fddfb35cc 100644
--- a/spec/integration/category_tag_spec.rb
+++ b/spec/integration/category_tag_spec.rb
@@ -212,6 +212,23 @@ describe "category tag restrictions" do
       expect(filter_allowed_tags(for_topic: true, selected_tags: [tag1.name, tag3.name])).to contain_exactly(tag1, tag2, tag3, tag4)
     end
 
+    it "filter_allowed_tags returns tags common to more than one tag group with parent tag" do
+      common = Fabricate(:tag, name: 'common')
+      tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id)
+      tag_group.tags = [tag2, common]
+      tag_group = Fabricate(:tag_group, parent_tag_id: tag3.id)
+
+      tag_group.tags = [tag4]
+      expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1, tag3)
+      expect(filter_allowed_tags(for_input: true, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, common)
+      expect(filter_allowed_tags(for_input: true, selected_tags: [tag3.name])).to contain_exactly(tag4, tag1)
+
+      tag_group.tags = [tag4, common]
+      expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1, tag3)
+      expect(filter_allowed_tags(for_input: true, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, common)
+      expect(filter_allowed_tags(for_input: true, selected_tags: [tag3.name])).to contain_exactly(tag4, tag1, common)
+    end
+
     context "and category restrictions" do
       fab!(:car_category)    { Fabricate(:category) }
       fab!(:other_category)  { Fabricate(:category) }

GitHub sha: 5ef49692

1 Like

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