IntenseWebs/discourse
3
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-23 09:26:54 -06:00
Code Issues Packages Projects Releases Wiki Activity

FIX: tag input should not include tags you've already chosen in the search results

Browse Source
This commit is contained in:
Neil Lalonde 2018-03-13 16:34:28 -04:00
parent c75fd34328
commit 58508e553d
3 changed files with 93 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
lib/discourse_tagging.rb
View File

@ -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?

42
spec/components/discourse_tagging_spec.rb Normal file
View File

@ -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

63
spec/integration/category_tag_spec.rb
View File

@ -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