diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index f9284f9751c..3e71a284a54 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -32,9 +32,13 @@ 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, for_input: true, - category: category, - selected_tags: tag_names).to_a + tags = filter_allowed_tags( + Tag.where(name: tag_names), + nil, # guardian + for_topic: true, + category: category, + selected_tags: tag_names + ).to_a if tags.size < tag_names.size && (category.nil? || category.tags.count == 0) tag_names.each do |name| @@ -57,13 +61,22 @@ module DiscourseTagging # term: a search term to filter tags by name # 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 = {}) + + selected_tag_ids = opts[:selected_tags] ? Tag.where(name: opts[:selected_tags]).pluck(:id) : [] + + if opts[:for_input] && !selected_tag_ids.empty? + query = query.where('tags.id NOT IN (?)', selected_tag_ids) + end + term = opts[:term] if term.present? term.gsub!("_", "\\_") term = clean_tag(term) query = query.where('tags.name like ?', "%#{term}%") + # query = query.where('tags.id NOT IN (?)', selected_tag_ids) unless selected_tag_ids.empty? end # Filters for category-specific tags: @@ -86,7 +99,7 @@ module DiscourseTagging query = query.where("tags.id IN (SELECT tag_id FROM tag_group_memberships WHERE tag_group_id IN (?))", tag_group_ids) end - elsif opts[:for_input] || category + 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)") @@ -98,9 +111,7 @@ module DiscourseTagging end end - if opts[:for_input] - selected_tag_ids = opts[:selected_tags] ? Tag.where(name: opts[:selected_tags]).pluck(:id) : [] - + if opts[:for_input] || opts[:for_topic] unless guardian.nil? || guardian.is_staff? staff_tag_names = SiteSetting.staff_tags.split("|") query = query.where('tags.name NOT IN (?)', staff_tag_names) if staff_tag_names.present? diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb new file mode 100644 index 00000000000..ab15f0b0d26 --- /dev/null +++ b/spec/components/discourse_tagging_spec.rb @@ -0,0 +1,42 @@ +# encoding: UTF-8 + +require 'rails_helper' +require 'discourse_tagging' + +# More tests are found in the category_tag_spec integration specs + +describe DiscourseTagging do + + let(:user) { Fabricate(:user) } + + let!(:tag1) { Fabricate(:tag, name: "tag1") } + let!(:tag2) { Fabricate(:tag, name: "tag2") } + let!(:tag3) { Fabricate(:tag, name: "tag3") } + + before do + SiteSetting.tagging_enabled = true + SiteSetting.min_trust_to_create_tag = 0 + SiteSetting.min_trust_level_to_tag_topics = 0 + end + + 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), + selected_tags: [tag2.name], + for_input: true, + term: 'tag' + ).map(&:name) + expect(tags).to contain_exactly(tag1.name, tag3.name) + end + + it "doesn't return selected tags if there's no search term" do + tags = DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), + selected_tags: [tag2.name], + for_input: true + ).map(&:name) + expect(tags).to contain_exactly(tag1.name, tag3.name) + end + end + end +end diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 36979b45d5b..2e0abf30b0c 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -13,10 +13,10 @@ describe "category tag restrictions" do DiscourseTagging.filter_allowed_tags(Tag.all, Guardian.new(user), opts) end - let!(:tag1) { Fabricate(:tag) } - let!(:tag2) { Fabricate(:tag) } - let!(:tag3) { Fabricate(:tag) } - let!(:tag4) { Fabricate(:tag) } + let!(:tag1) { Fabricate(:tag, name: 'tag1') } + let!(:tag2) { Fabricate(:tag, name: 'tag2') } + let!(:tag3) { Fabricate(:tag, name: 'tag3') } + let!(:tag4) { Fabricate(:tag, name: 'tag4') } let(:user) { Fabricate(:user) } let(:admin) { Fabricate(:admin) } @@ -37,23 +37,25 @@ describe "category tag restrictions" do it "tags belonging to that category can only be used there" do post = create_post(category: category_with_tags, tags: [tag1.name, tag2.name, tag3.name]) - expect(post.topic.tags.map(&:name).sort).to eq([tag1.name, tag2.name].sort) + expect(post.topic.tags).to contain_exactly(tag1, tag2) post = create_post(category: other_category, tags: [tag1.name, tag2.name, tag3.name]) - expect(post.topic.tags.map(&:name)).to eq([tag3.name]) + expect(post.topic.tags).to contain_exactly(tag3) end 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).pluck(:name).sort).to eq([tag1.name, tag2.name].sort) - expect(filter_allowed_tags(for_input: true).pluck(:name).sort).to eq([tag3.name, tag4.name].sort) + 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) 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.map(&:name)).to eq([tag1.name]) + expect(post.topic.tags).to contain_exactly(tag1) post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"], user: admin) - expect(post.topic.tags.map(&:name)).to eq([tag1.name]) + expect(post.topic.tags).to contain_exactly(tag1) end it "can create new tags in a non-restricted category" do @@ -80,13 +82,13 @@ describe "category tag restrictions" do category.allowed_tag_groups = [tag_group1.name] category.reload - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: category))).to eq(sorted_tag_names([tag1, tag2])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true))).to eq(sorted_tag_names([tag3, tag4])) + 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) tag_group1.tags = [tag2, tag3, tag4] - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: category))).to eq(sorted_tag_names([tag2, tag3, tag4])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true))).to eq(sorted_tag_names([tag1])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: other_category))).to eq(sorted_tag_names([tag1])) + 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) end it "groups and individual tags can be mixed" do @@ -94,9 +96,9 @@ describe "category tag restrictions" do category.allowed_tags = [tag4.name] category.reload - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: category))).to eq(sorted_tag_names([tag1, tag2, tag4])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true))).to eq(sorted_tag_names([tag3])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: other_category))).to eq(sorted_tag_names([tag3])) + 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) end end @@ -104,9 +106,9 @@ describe "category tag restrictions" do it "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(sorted_tag_names(filter_allowed_tags(for_input: true))).to eq(sorted_tag_names([tag1, tag2])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name]))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, selected_tags: [tag1.name, tag3.name]))).to eq(sorted_tag_names([tag1, tag2, 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) end context "and category restrictions" do @@ -137,15 +139,15 @@ describe "category tag restrictions" do it "handles all those rules" do # car tags can't be used outside of car category: - expect(sorted_tag_names(filter_allowed_tags(for_input: true))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: other_category))).to eq(sorted_tag_names([tag1, tag2, tag3, tag4])) + 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) # 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']) # 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', 'honda']) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']))).to eq(['ford', 'honda', 'mustang', 'taurus']) + 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']) end it "can apply the tags to a topic" do @@ -162,9 +164,9 @@ describe "category tag restrictions" do it "can restrict one tag from each group" do expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category))).to eq(['ford', 'honda']) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['honda']))).to eq(['accord', 'civic', 'honda']) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford']))).to eq(['ford', 'mustang', 'taurus']) - expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford', 'mustang']))).to eq(['ford', 'mustang']) + 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']) + expect(sorted_tag_names(filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['ford', 'mustang']))).to eq([]) end it "can apply the tags to a topic" do @@ -176,8 +178,9 @@ describe "category tag restrictions" do # A weird case that input field wouldn't allow. # Only one tag from car makers is allowed, but we're saying that two have been selected. names = filter_allowed_tags(for_input: true, category: car_category, selected_tags: ['honda', 'ford']).map(&:name) - expect(names.include?('honda') && names.include?('ford')).to eq(false) - expect(names.include?('honda') || names.include?('ford')).to eq(true) + expect(names.include?('honda') || names.include?('ford')).to eq(false) + expect(names).to include('civic') + expect(names).to include('mustang') end end end