From 7711df40e657586d24d65cefb3853ae91bcaad16 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Tue, 12 Nov 2019 14:28:44 -0500 Subject: [PATCH] REFACTOR: redo DiscourseTagging.filter_allowed_tags (#8328) This method had grown into a monster. Its query had bugs that I couldn't fix, and new features would be hard to add. Also I don't understand how it all works anymore... Replace it with common table expressions that can be queried to generate the results we need, instead of subtracting results using lots of "NOT IN" clauses. Fixed are bugs with tag schemas that use combinations of tag groups, parent tags, and one-tag-per-topic restrictions. For example: https://meta.discourse.org/t/130991/6 --- app/controllers/tags_controller.rb | 12 +- app/controllers/topics_controller.rb | 3 +- lib/discourse_tagging.rb | 270 ++++++++++++---------- spec/components/discourse_tagging_spec.rb | 34 +-- spec/integration/category_tag_spec.rb | 196 +++++++++------- spec/requests/topics_controller_spec.rb | 12 +- 6 files changed, 294 insertions(+), 233 deletions(-) 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