From c2a8a2bc9738f31cc607c14004da8de6f644e5e2 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Fri, 26 Apr 2019 14:39:39 -0400 Subject: [PATCH] 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 | 60 +++++++++++++++++------ spec/components/discourse_tagging_spec.rb | 33 +++++++++++++ spec/integration/category_tag_spec.rb | 10 +++- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 1b26986707..ffd8b143e5 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 c9ee958a70..52b52fd95b 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 80f76705e6..e855786a6c 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) } let(:other_category) { Fabricate(:category) }