FIX: if mandatory parent tag is missing, add it

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.

diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index 1b26986..ffd8b14 100644
--- a/lib/discourse_tagging.rb
+++ b/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
diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb
index c9ee958..52b52fd 100644
--- a/spec/components/discourse_tagging_spec.rb
+++ b/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
diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb
index 80f7670..e855786 100644
--- a/spec/integration/category_tag_spec.rb
+++ b/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) }

[... diff too long, it was truncated ...]

GitHub sha: c2a8a2bc

1 Like