diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 1b26986707c..ffd8b143e54 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 c9ee958a709..52b52fd95b3 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 80f76705e64..e855786a6cf 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) }