diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 8aabc8f3db3..0a4d7cd9fb5 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -195,7 +195,8 @@ class TagsController < ::ApplicationController def search filter_params = { for_input: params[:filterForInput], - selected_tags: params[:selected_tags] + selected_tags: params[:selected_tags], + limit: params[:limit] } if params[:categoryId] @@ -205,19 +206,14 @@ class TagsController < ::ApplicationController if params[:q] clean_name = DiscourseTagging.clean_tag(params[:q]) filter_params[:term] = clean_name - - # Prioritize exact matches when ordering - order_query = Tag.sanitize_sql_for_order( + filter_params[:order] = Tag.sanitize_sql_for_order( ["lower(name) = lower(?) DESC, topic_count DESC", clean_name] ) - - tag_query = Tag.order(order_query).limit(params[:limit]) else - tag_query = Tag.limit(params[:limit]) + filter_params[:order] = "topic_count DESC" end tags_with_counts = DiscourseTagging.filter_allowed_tags( - tag_query, guardian, filter_params ) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 2c29594e66b..416e84f19a5 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -327,10 +327,9 @@ class TopicsController < ApplicationController if category && topic_tags = (params[:tags] || topic.tags.pluck(:name)).reject { |c| c.empty? } if topic_tags.present? allowed_tags = DiscourseTagging.filter_allowed_tags( - Tag.all, guardian, category: category - ).pluck("tags.name") + ).map(&:name) invalid_tags = topic_tags - allowed_tags diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 1ef8da2abc2..753c1988aff 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -2,11 +2,11 @@ module DiscourseTagging - TAGS_FIELD_NAME = "tags" - TAGS_FILTER_REGEXP = /[\/\?#\[\]@!\$&'\(\)\*\+,;=\.%\\`^\s|\{\}"<>]+/ # /?#[]@!$&'()*+,;=.%\`^|{}"<> - TAGS_STAFF_CACHE_KEY = "staff_tag_names" + TAGS_FIELD_NAME ||= "tags" + TAGS_FILTER_REGEXP ||= /[\/\?#\[\]@!\$&'\(\)\*\+,;=\.%\\`^\s|\{\}"<>]+/ # /?#[]@!$&'()*+,;=.%\`^|{}"<> + TAGS_STAFF_CACHE_KEY ||= "staff_tag_names" - TAG_GROUP_TAG_IDS_SQL = <<-SQL + TAG_GROUP_TAG_IDS_SQL ||= <<-SQL SELECT tag_id FROM tag_group_memberships tgm INNER JOIN tag_groups tg @@ -49,12 +49,14 @@ module DiscourseTagging # guardian is explicitly nil cause we don't want to strip all # staff tags that already passed validation tags = filter_allowed_tags( - Tag.where_name(tag_names), nil, # guardian for_topic: true, category: category, - selected_tags: tag_names - ).to_a + selected_tags: tag_names, + only_tag_names: tag_names + ) + + tags = Tag.where(id: tags.map(&:id)).all.to_a if tags.size > 0 if tags.size < tag_names.size && (category.nil? || category.allow_global_tags || (category.tags.count == 0 && category.tag_groups.count == 0)) tag_names.each do |name| @@ -141,17 +143,123 @@ module DiscourseTagging end end + TAG_GROUP_RESTRICTIONS_SQL ||= <<~SQL + tag_group_restrictions AS ( + SELECT t.name as tag_name, t.id as tag_id, tgm.id as tgm_id, tg.id as tag_group_id, tg.parent_tag_id as parent_tag_id, + tg.one_per_topic as one_per_topic + FROM tags t + LEFT OUTER JOIN tag_group_memberships tgm ON tgm.tag_id = t.id /*and_name_like*/ + LEFT OUTER JOIN tag_groups tg ON tg.id = tgm.tag_group_id + ) + SQL + + CATEGORY_RESTRICTIONS_SQL ||= <<~SQL + category_restrictions AS ( + SELECT t.name as tag_name, t.id as tag_id, ct.id as ct_id, ct.category_id as category_id + FROM tags t + INNER JOIN category_tags ct ON t.id = ct.tag_id /*and_name_like*/ + + UNION + + SELECT t.name as tag_name, t.id as tag_id, ctg.id as ctg_id, ctg.category_id as category_id + FROM tags t + INNER JOIN tag_group_memberships tgm ON tgm.tag_id = t.id /*and_name_like*/ + INNER JOIN category_tag_groups ctg ON tgm.tag_group_id = ctg.tag_group_id + ) + SQL + + PERMITTED_TAGS_SQL ||= <<~SQL + permitted_tag_groups AS ( + SELECT tg.id as tag_group_id, tgp.group_id as group_id, tgp.permission_type as permission_type + FROM tags t + INNER JOIN tag_group_memberships tgm ON tgm.tag_id = t.id /*and_name_like*/ + INNER JOIN tag_groups tg ON tg.id = tgm.tag_group_id + INNER JOIN tag_group_permissions tgp + ON tg.id = tgp.tag_group_id + AND tgp.group_id = #{Group::AUTO_GROUPS[:everyone]} + AND tgp.permission_type = #{TagGroupPermission.permission_types[:full]} + ) + SQL + # Options: # term: a search term to filter tags by name + # order: order by for the query + # limit: max number of results # category: a Category to which the object being tagged belongs # for_input: result is for an input field, so only show permitted tags # for_topic: results are for tagging a topic # selected_tags: an array of tag names that are in the current selection - def self.filter_allowed_tags(query, guardian, opts = {}) + # only_tag_names: limit results to tags with these names + def self.filter_allowed_tags(guardian, opts = {}) selected_tag_ids = opts[:selected_tags] ? Tag.where_name(opts[:selected_tags]).pluck(:id) : [] + category = opts[:category] + category_has_restricted_tags = category ? (category.tags.count > 0 || category.tag_groups.count > 0) : false - if !opts[:for_topic] && !selected_tag_ids.empty? - query = query.where('tags.id NOT IN (?)', selected_tag_ids) + # If guardian is nil, it means the caller doesn't want tags to be filtered + # based on guardian rules. Use the same rules as for staff users. + filter_for_non_staff = !guardian.nil? && !guardian.is_staff? + + builder_params = {} + + unless selected_tag_ids.empty? + builder_params[:selected_tag_ids] = selected_tag_ids + end + + sql = +"WITH #{TAG_GROUP_RESTRICTIONS_SQL}, #{CATEGORY_RESTRICTIONS_SQL}" + if (opts[:for_input] || opts[:for_topic]) && filter_for_non_staff + sql << ", #{PERMITTED_TAGS_SQL} " + end + + outer_join = category.nil? || category.allow_global_tags || !category_has_restricted_tags + + sql << <<~SQL + SELECT t.id, t.name, t.topic_count, t.pm_topic_count, + tgr.tgm_id as tgm_id, tgr.tag_group_id as tag_group_id, tgr.parent_tag_id as parent_tag_id, + tgr.one_per_topic as one_per_topic + FROM tags t + INNER JOIN tag_group_restrictions tgr ON tgr.tag_id = t.id + #{outer_join ? "LEFT OUTER" : "INNER"} + JOIN category_restrictions cr ON t.id = cr.tag_id + /*where*/ + /*order_by*/ + /*limit*/ + SQL + + builder = DB.build(sql) + + builder.limit(opts[:limit]) if opts[:limit] + builder.order_by(opts[:order]) if opts[:order] + + if !opts[:for_topic] && builder_params[:selected_tag_ids] + builder.where("id NOT IN (:selected_tag_ids)") + end + + if opts[:only_tag_names] + builder.where("LOWER(name) IN (?)", opts[:only_tag_names].map(&:downcase)) + end + + # parent tag requirements + if opts[:for_input] + builder.where( + builder_params[:selected_tag_ids] ? + "tgm_id IS NULL OR parent_tag_id IS NULL OR parent_tag_id IN (:selected_tag_ids)" : + "tgm_id IS NULL OR parent_tag_id IS NULL" + ) + end + + if category && category_has_restricted_tags + builder.where( + category.allow_global_tags ? "category_id = ? OR category_id IS NULL" : "category_id = ?", + category.id + ) + elsif category || opts[:for_input] || opts[:for_topic] + # tags not restricted to any categories + builder.where("category_id IS NULL") + end + + if filter_for_non_staff && (opts[:for_input] || opts[:for_topic]) + # exclude staff-only tag groups + builder.where("tag_group_id IS NULL OR tag_group_id IN (SELECT tag_group_id FROM permitted_tag_groups)") end term = opts[:term] @@ -159,128 +267,48 @@ module DiscourseTagging term = term.gsub("_", "\\_") clean_tag(term) term.downcase! - query = query.where('lower(tags.name) like ?', "%#{term}%") + builder.where("LOWER(name) LIKE :term") + builder_params[:term] = "%#{term}%" + sql.gsub!("/*and_name_like*/", "AND LOWER(t.name) LIKE :term") + else + sql.gsub!("/*and_name_like*/", "") end - # Filters for category-specific tags: - category = opts[:category] - - if opts[:for_input] && !guardian.nil? && !guardian.is_staff? && category&.required_tag_group + if opts[:for_input] && filter_for_non_staff && category&.required_tag_group required_tag_ids = category.required_tag_group.tags.pluck(:id) if (required_tag_ids & selected_tag_ids).size < category.min_tags_from_required_group - query = query.where('tags.id IN (?)', required_tag_ids) + builder.where("id IN (?)", required_tag_ids) end end - if category && (category.tags.count > 0 || category.tag_groups.count > 0) - if category.allow_global_tags - # Select tags that: - # * are restricted to the given category - # * belong to no tag groups and aren't restricted to other categories - # * belong to tag groups that are not restricted to any categories - query = query.where(<<~SQL, category.tag_groups.pluck(:id), category.id) - tags.id IN ( - SELECT t.id FROM tags t - LEFT JOIN category_tags ct ON t.id = ct.tag_id - LEFT JOIN ( - SELECT xtgm.tag_id, xtgm.tag_group_id - FROM tag_group_memberships xtgm - INNER JOIN category_tag_groups ctg - ON xtgm.tag_group_id = ctg.tag_group_id - ) AS tgm ON t.id = tgm.tag_id - WHERE (tgm.tag_group_id IS NULL AND ct.category_id IS NULL) - OR tgm.tag_group_id IN (?) - OR ct.category_id = ? - ) - SQL - else - # Select only tags that are restricted to the given category - query = query.where(<<~SQL, category.id, category.tag_groups.pluck(:id)) - tags.id IN ( - SELECT tag_id FROM category_tags WHERE category_id = ? - UNION - SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?) - ) - SQL - end - elsif opts[:for_input] || opts[:for_topic] || category - # exclude tags that are restricted to other categories - if CategoryTag.exists? - query = query.where("tags.id NOT IN (SELECT tag_id FROM category_tags)") - end + if filter_for_non_staff + # remove hidden tags + builder.where(<<~SQL, Group::AUTO_GROUPS[:everyone]) + id NOT IN ( + SELECT tag_id + FROM tag_group_memberships tgm + WHERE tag_group_id NOT IN (SELECT tag_group_id FROM tag_group_permissions WHERE group_id = ?) + ) + SQL + end - if CategoryTagGroup.exists? - tag_group_ids = CategoryTagGroup.pluck(:tag_group_id).uniq - query = query.where("tags.id NOT IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?))", tag_group_ids) + if builder_params[:selected_tag_ids] && (opts[:for_input] || opts[:for_topic]) + one_tag_per_group_ids = DB.query(<<~SQL, builder_params[:selected_tag_ids]).map(&:id) + SELECT DISTINCT(tg.id) + FROM tag_groups tg + INNER JOIN tag_group_memberships tgm ON tg.id = tgm.tag_group_id AND tgm.tag_id IN (?) + WHERE tg.one_per_topic + SQL + + if !one_tag_per_group_ids.empty? + builder.where( + "tag_group_id IS NULL OR tag_group_id NOT IN (?) OR id IN (:selected_tag_ids)", + one_tag_per_group_ids + ) end end - if opts[:for_input] || opts[:for_topic] - unless guardian.nil? || guardian.is_staff? - 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 - - if selected_tag_ids.empty? - sql = "tags.id NOT IN (#{TAG_GROUP_TAG_IDS_SQL} WHERE tg.parent_tag_id IS NOT NULL)" - query = query.where(sql) - else - exclude_group_ids = one_per_topic_group_ids(selected_tag_ids) - - if exclude_group_ids.empty? - # 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. - 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.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? - query - else - filter_visible(query, guardian) - 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) + result = builder.query(builder_params).uniq { |t| t.id } end def self.filter_visible(query, guardian = nil) diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb index 7605b09fa58..0c3791188c9 100644 --- a/spec/components/discourse_tagging_spec.rb +++ b/spec/components/discourse_tagging_spec.rb @@ -8,6 +8,10 @@ require 'discourse_tagging' describe DiscourseTagging do + def sorted_tag_names(tag_records) + tag_records.map(&:name).sort + end + fab!(:admin) { Fabricate(:admin) } fab!(:user) { Fabricate(:user) } let(:guardian) { Guardian.new(user) } @@ -25,7 +29,7 @@ describe DiscourseTagging do describe 'filter_allowed_tags' do context 'for input fields' do it "doesn't return selected tags if there's a search term" do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), selected_tags: [tag2.name], for_input: true, term: 'fun' @@ -34,7 +38,7 @@ describe DiscourseTagging do end it "doesn't return selected tags if there's no search term" do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), selected_tags: [tag2.name], for_input: true ).map(&:name) @@ -46,15 +50,13 @@ describe DiscourseTagging do let!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) } it 'should return all tags to staff' do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(admin)).to_a - expect(tags).to contain_exactly(tag1, tag2, tag3, hidden_tag) - expect(tags.size).to eq(4) + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(admin)).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3, hidden_tag])) end it 'should not return hidden tag to non-staff' do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user)).to_a - expect(tags).to contain_exactly(tag1, tag2, tag3) - expect(tags.size).to eq(3) + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user)).to_a + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3])) end end @@ -63,42 +65,42 @@ describe DiscourseTagging do fab!(:category) { Fabricate(:category, required_tag_group: tag_group, min_tags_from_required_group: 1) } it "returns the required tags if none have been selected" do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), for_input: true, category: category, term: 'fun' ).to_a - expect(tags).to contain_exactly(tag1, tag2) + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2])) end it "returns all allowed tags if a required tag is selected" do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), for_input: true, category: category, selected_tags: [tag1.name], term: 'fun' ).to_a - expect(tags).to contain_exactly(tag2, tag3) + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag2, tag3])) end it "returns required tags if not enough are selected" do category.update!(min_tags_from_required_group: 2) - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), for_input: true, category: category, selected_tags: [tag1.name], term: 'fun' ).to_a - expect(tags).to contain_exactly(tag2) + expect(sorted_tag_names(tags)).to contain_exactly(tag2.name) end it "let's staff ignore the requirement" do - tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(admin), + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(admin), for_input: true, category: category, term: 'fun' ).to_a - expect(tags).to contain_exactly(tag1, tag2, tag3) + expect(sorted_tag_names(tags)).to eq(sorted_tag_names([tag1, tag2, tag3])) end end end diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 60ea91bbb32..0b5339f1c70 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -6,11 +6,15 @@ require 'rails_helper' describe "category tag restrictions" do def sorted_tag_names(tag_records) - tag_records.map(&:name).sort + tag_records.map { |t| t.is_a?(String) ? t : t.name }.sort + end + + def expect_same_tag_names(a, b) + expect(sorted_tag_names(a)).to eq(sorted_tag_names(b)) end def filter_allowed_tags(opts = {}) - DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), opts) + DiscourseTagging.filter_allowed_tags(Guardian.new(user), opts) end fab!(:tag1) { Fabricate(:tag, name: 'tag1') } @@ -45,25 +49,25 @@ describe "category tag restrictions" do it "search can show only permitted tags" do expect(filter_allowed_tags.count).to eq(Tag.count) - expect(filter_allowed_tags(for_input: true, category: category_with_tags)).to contain_exactly(tag1, tag2) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name])).to contain_exactly(tag2) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name], term: 'tag')).to contain_exactly(tag2) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name])).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag')).to contain_exactly(tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1, tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name]), [tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name], term: 'tag'), [tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name]), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag'), [tag4]) end it "can't create new tags in a restricted category" do post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"]) - expect(post.topic.tags).to contain_exactly(tag1) + expect_same_tag_names(post.topic.tags, [tag1]) post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin) - expect(post.topic.tags).to contain_exactly(tag1) + expect_same_tag_names(post.topic.tags, [tag1]) end it "can create new tags in a non-restricted category" do post = create_post(category: other_category, tags: [tag3.name, "newtag"]) - expect(post.topic.tags.map(&:name).sort).to eq([tag3.name, "newtag"].sort) + expect_same_tag_names(post.topic.tags, [tag3.name, "newtag"]) end it "can create tags when changing category settings" do @@ -76,9 +80,9 @@ describe "category tag restrictions" do before { category_with_tags.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } it "search only returns the allowed tags" do - expect(filter_allowed_tags(for_input: true, category: category_with_tags)).to contain_exactly(tag1) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name])).to contain_exactly(tag2) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag2.name])).to contain_exactly(tag1) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name]), [tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag2.name]), [tag1]) end end @@ -89,20 +93,20 @@ describe "category tag restrictions" do it "search can show the permitted tags" do expect(filter_allowed_tags.count).to eq(Tag.count) - expect(filter_allowed_tags(for_input: true, category: category_with_tags)).to contain_exactly(tag1, tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name], term: 'tag')).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name])).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag')).to contain_exactly(tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name]), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name], term: 'tag'), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name]), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag'), [tag4]) end it "works if no tags are restricted to the category" do other_category.update!(allow_global_tags: true) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name])).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag')).to contain_exactly(tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name]), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag'), [tag4]) end context 'required tags from tag group' do @@ -110,9 +114,9 @@ describe "category tag restrictions" do before { category_with_tags.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } it "search only returns the allowed tags" do - expect(filter_allowed_tags(for_input: true, category: category_with_tags)).to contain_exactly(tag1, tag3) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag2.name])).to contain_exactly(tag1, tag3) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag1.name]), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category_with_tags, selected_tags: [tag2.name]), [tag1, tag3]) end end end @@ -130,22 +134,22 @@ describe "category tag restrictions" do end it "tags in the group are used by category tag restrictions" do - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag3, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag3, tag4]) tag_group1.tags = [tag2, tag3, tag4] - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag1) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag1]) end it "groups and individual tags can be mixed" do category.allowed_tags = [tag4.name] category.reload - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag4) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag3) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3]) end it "enforces restrictions when creating a topic" do @@ -158,9 +162,9 @@ describe "category tag restrictions" do before { category.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } it "search only returns the allowed tags" do - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name])).to contain_exactly(tag2) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name])).to contain_exactly(tag1) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name]), [tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name]), [tag1]) end end @@ -170,21 +174,21 @@ describe "category tag restrictions" do end it 'filters tags correctly' do - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3, tag4]) tag_group1.tags = [tag2, tag3, tag4] - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag1) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag1]) end it "works if no tags are restricted to the category" do other_category.update!(allow_global_tags: true) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag3, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag3, tag4]) tag_group1.tags = [tag2, tag3, tag4] - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag1) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag1]) end context 'required tags from tag group' do @@ -192,9 +196,9 @@ describe "category tag restrictions" do before { category.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } it "search only returns the allowed tags" do - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag3) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name])).to contain_exactly(tag1, tag3) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name]), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name]), [tag1, tag3]) end end @@ -209,19 +213,19 @@ describe "category tag restrictions" do end it 'filters tags correctly' do - expect(filter_allowed_tags(for_input: true, category: category2)).to contain_exactly(tag2, tag3) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category2), [tag2, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag4]) end it "doesn't care about tags in a group that isn't used in a category" do unused_tag_group = Fabricate(:tag_group) unused_tag_group.tags = [tag4] - expect(filter_allowed_tags(for_input: true, category: category2)).to contain_exactly(tag2, tag3) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category2), [tag2, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag4]) end end @@ -230,10 +234,10 @@ describe "category tag restrictions" do it "doesn't filter tags that are also restricted in another category" do category2.tags = [tag2, tag3] - expect(filter_allowed_tags(for_input: true, category: category2)).to contain_exactly(tag2, tag3) - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag4) - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category2), [tag2, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2, tag4]) end end end @@ -243,17 +247,17 @@ describe "category tag restrictions" 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) - expect(filter_allowed_tags(for_input: true, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, selected_tags: [tag1.name, tag3.name])).to contain_exactly(tag2, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name, tag3.name]), [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) + expect_same_tag_names(filter_allowed_tags(for_topic: true), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_topic: true, selected_tags: [tag1.name]), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_topic: true, selected_tags: [tag1.name, tag3.name]), [tag1, tag2, tag3, tag4]) end it "filter_allowed_tags returns tags common to more than one tag group with parent tag" do @@ -263,14 +267,24 @@ describe "category tag restrictions" do 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) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]), [tag2, tag3, common]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag3.name]), [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) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]), [tag2, tag3, common]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag3.name]), [tag4, tag1, common]) + + parent_tag_group = Fabricate(:tag_group, tags: [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]), [tag2, tag3, common]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag3.name]), [tag4, tag1, common]) + + parent_tag_group.update!(one_per_topic: true) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag3]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]), [tag2, common]) + expect_same_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag3.name]), [tag4, common]) end context 'required tags from tag group' do @@ -279,10 +293,10 @@ describe "category tag restrictions" do it "search only returns the allowed tags" do tag_group_with_parent = Fabricate(:tag_group, parent_tag_id: tag1.id, tags: [tag3, tag4]) - expect(filter_allowed_tags(for_input: true, category: category)).to contain_exactly(tag1, tag2) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name])).to contain_exactly(tag1) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name])).to contain_exactly(tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name, tag2.name])).to contain_exactly(tag3, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category), [tag1, tag2]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag2.name]), [tag1]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name]), [tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: category, selected_tags: [tag1.name, tag2.name]), [tag3, tag4]) end end @@ -312,10 +326,10 @@ describe "category tag restrictions" do car_category.allowed_tag_groups = [makes.name, honda_group.name, ford_group.name] end - it "handles all those rules" do + it "handles all those rules", :focus do # car tags can't be used outside of car category: - expect(filter_allowed_tags(for_input: true)).to contain_exactly(tag1, tag2, tag3, tag4) - expect(filter_allowed_tags(for_input: true, category: other_category)).to contain_exactly(tag1, tag2, tag3, tag4) + expect_same_tag_names(filter_allowed_tags(for_input: true), [tag1, tag2, tag3, tag4]) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category), [tag1, tag2, tag3, tag4]) # in car category, a make tag must be given first: expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category))).to eq(['ford', 'honda']) @@ -323,6 +337,26 @@ describe "category tag restrictions" do # model tags depend on which make is chosen: expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['honda']))).to eq(['accord', 'civic', 'ford']) expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']))).to eq(['honda', 'mustang', 'taurus']) + + makes.update!(one_per_topic: true) + expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['honda']))).to eq(['accord', 'civic']) + expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']))).to eq(['mustang', 'taurus']) + + honda_group.update!(one_per_topic: true) + ford_group.update!(one_per_topic: true) + expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['honda']))).to eq(['accord', 'civic']) + expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']))).to eq(['mustang', 'taurus']) + + car_category.update!(allow_global_tags: true) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: car_category), + ['ford', 'honda', tag1, tag2, tag3, tag4] + ) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']), + ['mustang', 'taurus', tag1, tag2, tag3, tag4] + ) + expect_same_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford', 'mustang']), + [tag1, tag2, tag3, tag4] + ) end it "can apply the tags to a topic" do diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index afbc542e3be..3e7c784452b 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1103,6 +1103,7 @@ RSpec.describe TopicsController do fab!(:restricted_category) { Fabricate(:category) } fab!(:tag1) { Fabricate(:tag) } fab!(:tag2) { Fabricate(:tag) } + let(:tag3) { Fabricate(:tag) } let!(:tag_group_1) { Fabricate(:tag_group, tag_names: [tag1.name]) } fab!(:tag_group_2) { Fabricate(:tag_group) } @@ -1187,7 +1188,8 @@ RSpec.describe TopicsController do end it 'allows category change when topic has a read-only tag' do - Fabricate(:tag_group, permissions: { "staff" => 1, "everyone" => 3 }, tag_names: [tag1.name]) + Fabricate(:tag_group, permissions: { "staff" => 1, "everyone" => 3 }, tag_names: [tag3.name]) + topic.update!(tags: [tag3]) put "/t/#{topic.slug}/#{topic.id}.json", params: { category_id: category.id @@ -1195,21 +1197,21 @@ RSpec.describe TopicsController do result = ::JSON.parse(response.body) expect(response.status).to eq(200) - expect(topic.reload.tags).to include(tag1) + expect(topic.reload.tags).to contain_exactly(tag3) end it 'does not leak tag name when trying to use a staff tag' do - Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag2.name]) + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag3.name]) put "/t/#{topic.slug}/#{topic.id}.json", params: { - tags: [tag2.name], + tags: [tag3.name], category_id: category.id } result = ::JSON.parse(response.body) expect(response.status).to eq(422) expect(result['errors']).to be_present - expect(result['errors'][0]).not_to include(tag2.name) + expect(result['errors'][0]).not_to include(tag3.name) end it 'will clean tag params' do