FIX: if mandatory parent tag is missing, add it

Previous behaviour was to silently remove tags that
belonged to a group with a parent tag that was missing.

The "required parent tag" feature is meant to guide people
to use the correct tags and show scoped results in the tag
input field, and to help create topic lists of related
tags. It isn't meant to be a strict requirement in the
composer that should trigger errors or restrictions.

lib/discourse_tagging.rb

@@ -4,6 +4,13 @@ module DiscourseTagging
   TAGS_FILTER_REGEXP = /[\/\?#\[\]@!\$&'\(\)\*\+,;=\.%\\`^\s|\{\}"<>]+/ # /?#[]@!$&'()*+,;=.%\`^|{}"<>
   TAGS_STAFF_CACHE_KEY = "staff_tag_names"
 
+  TAG_GROUP_TAG_IDS_SQL = <<-SQL
+      SELECT tag_id
+        FROM tag_group_memberships tgm
+  INNER JOIN tag_groups tg
+          ON tgm.tag_group_id = tg.id
+  SQL
+
   def self.tag_topic_by_names(topic, guardian, tag_names_arg, append: false)
     if guardian.can_tag?(topic)
       tag_names = DiscourseTagging.tags_for_saving(tag_names_arg, guardian) || []
@@ -55,6 +62,19 @@ module DiscourseTagging
           end
         end
 
+        # add missing mandatory parent tags
+        parent_tags = TagGroup.includes(:parent_tag).where("tag_groups.id IN (
+          SELECT tg.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)
+
+        parent_tags.reject { |t| tag_names.include?(t.name) }.each do |tag|
+          tags << tag
+        end
+
         # validate minimum required tags for a category
         if !guardian.is_staff? && category && category.minimum_required_tags > 0 && tags.length < category.minimum_required_tags
           topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)
@@ -147,28 +167,19 @@ module DiscourseTagging
         all_staff_tags = DiscourseTagging.staff_tag_names
         query = query.where('tags.name NOT IN (?)', all_staff_tags) if all_staff_tags.present?
       end
+    end
 
+    if opts[:for_input]
       # exclude tag groups that have a parent tag which is missing from selected_tags
 
-      select_sql = <<-SQL
-      SELECT tag_id
-            FROM tag_group_memberships tgm
-      INNER JOIN tag_groups tg
-              ON tgm.tag_group_id = tg.id
-      SQL
-
       if selected_tag_ids.empty?
-        sql = "tags.id NOT IN (#{select_sql} WHERE tg.parent_tag_id IS NOT NULL)"
+        sql = "tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE tg.parent_tag_id IS NOT NULL)"
         query = query.where(sql)
       else
-        # One tag per group restriction
-        exclude_group_ids = TagGroup.where(one_per_topic: true)
-          .joins(:tag_group_memberships)
-          .where('tag_group_memberships.tag_id in (?)', selected_tag_ids)
-          .pluck(:id)
+        exclude_group_ids = one_per_topic_group_ids(selected_tag_ids)
 
         if exclude_group_ids.empty?
-          sql = "tags.id NOT IN (#{select_sql} WHERE tg.parent_tag_id NOT IN (?))"
+          sql = "tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE tg.parent_tag_id NOT IN (?))"
           query = query.where(sql, selected_tag_ids)
         else
           # It's possible that the selected tags violate some one-tag-per-group restrictions,
@@ -177,10 +188,22 @@ module DiscourseTagging
             .where(tag_id: selected_tag_ids)
             .where(tag_group_id: exclude_group_ids)
             .map(&:tag_id)
-          sql = "(tags.id NOT IN (#{select_sql} WHERE (tg.parent_tag_id NOT IN (?) OR tg.id in (?))) OR tags.id IN (?))"
+          sql = "(tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE (tg.parent_tag_id NOT IN (?) OR tg.id in (?))) OR tags.id IN (?))"
           query = query.where(sql, selected_tag_ids, exclude_group_ids, limit_tag_ids)
         end
       end
+    elsif opts[:for_topic] && !selected_tag_ids.empty?
+      # One tag per group restriction
+      exclude_group_ids = one_per_topic_group_ids(selected_tag_ids)
+
+      unless exclude_group_ids.empty?
+        limit_tag_ids = TagGroupMembership.select('distinct on (tag_group_id) tag_id')
+          .where(tag_id: selected_tag_ids)
+          .where(tag_group_id: exclude_group_ids)
+          .map(&:tag_id)
+        sql = "(tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE (tg.id in (?))) OR tags.id IN (?))"
+        query = query.where(sql, exclude_group_ids, limit_tag_ids)
+      end
     end
 
     if guardian.nil? || guardian.is_staff?
@@ -190,6 +213,13 @@ module DiscourseTagging
     end
   end
 
+  def self.one_per_topic_group_ids(selected_tag_ids)
+    TagGroup.where(one_per_topic: true)
+      .joins(:tag_group_memberships)
+      .where('tag_group_memberships.tag_id in (?)', selected_tag_ids)
+      .pluck(:id)
+  end
+
   def self.filter_visible(query, guardian = nil)
     guardian&.is_staff? ? query : query.where("tags.id NOT IN (#{hidden_tags_query.select(:id).to_sql})")
   end

spec/components/discourse_tagging_spec.rb

@@ -158,6 +158,39 @@ describe DiscourseTagging do
         expect(topic.reload.tags).to eq([hidden_tag])
       end
     end
+
+    context 'tag group with parent tag' do
+      let(:topic) { Fabricate(:topic, user: user) }
+      let(:post) { Fabricate(:post, user: user, topic: topic, post_number: 1) }
+
+      before do
+        tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id)
+        tag_group.tags = [tag3]
+      end
+
+      it "can tag with parent" do
+        valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name])
+        expect(valid).to eq(true)
+        expect(topic.reload.tags.map(&:name)).to eq([tag1.name])
+      end
+
+      it "can tag with parent and a tag" do
+        valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag1.name, tag3.name])
+        expect(valid).to eq(true)
+        expect(topic.reload.tags.map(&:name)).to contain_exactly(*[tag1, tag3].map(&:name))
+      end
+
+      it "adds all parent tags that are missing" do
+        parent_tag = Fabricate(:tag, name: 'parent')
+        tag_group2 = Fabricate(:tag_group, parent_tag_id: parent_tag.id)
+        tag_group2.tags = [tag2]
+        valid = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [tag3.name, tag2.name])
+        expect(valid).to eq(true)
+        expect(topic.reload.tags.map(&:name)).to contain_exactly(
+          *[tag1, tag2, tag3, parent_tag].map(&:name)
+        )
+      end
+    end
   end
 
   describe '#tags_for_saving' do

spec/integration/category_tag_spec.rb

@@ -196,7 +196,7 @@ describe "category tag restrictions" do
   end
 
   context "tag groups with parent tag" do
-    it "filter_allowed_tags returns results based on whether parent tag is present or not" do
+    it "for input field, filter_allowed_tags returns results based on whether parent tag is present or not" do
       tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id)
       tag_group.tags = [tag3, tag4]
       expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1, tag2)
@@ -204,6 +204,14 @@ describe "category tag restrictions" do
       expect(filter_allowed_tags(for_input: true, selected_tags: [tag1.name, tag3.name])).to contain_exactly(tag2, tag4)
     end
 
+    it "for tagging a topic, filter_allowed_tags allows tags without parent tag" do
+      tag_group = Fabricate(:tag_group, parent_tag_id: tag1.id)
+      tag_group.tags = [tag3, tag4]
+      expect(filter_allowed_tags(for_topic: true)).to contain_exactly(tag1, tag2, tag3, tag4)
+      expect(filter_allowed_tags(for_topic: true, selected_tags: [tag1.name])).to contain_exactly(tag1, tag2, tag3, tag4)
+      expect(filter_allowed_tags(for_topic: true, selected_tags: [tag1.name, tag3.name])).to contain_exactly(tag1, tag2, tag3, tag4)
+    end
+
     context "and category restrictions" do
       let(:car_category)    { Fabricate(:category) }
       let(:other_category)  { Fabricate(:category) }