From 228c4814be40bbc640391834c9e4a87ec3fbf84d Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 18 Nov 2019 13:20:37 -0500 Subject: [PATCH] FIX: errors when using tags with colons in their name --- app/controllers/tags_controller.rb | 6 ++--- lib/discourse_tagging.rb | 14 +++++++---- spec/components/discourse_tagging_spec.rb | 29 +++++++++++++++++++++++ spec/integration/category_tag_spec.rb | 13 +++++++++- 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 77e1e43e608..e2aee6abe6b 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -203,12 +203,10 @@ class TagsController < ::ApplicationController filter_params[:category] = Category.find_by_id(params[:categoryId]) end - if params[:q] + if !params[:q].blank? clean_name = DiscourseTagging.clean_tag(params[:q]) filter_params[:term] = clean_name - filter_params[:order] = Tag.sanitize_sql_for_order( - ["lower(name) = lower(?) DESC, topic_count DESC", clean_name] - ) + filter_params[:order_search_results] = true else filter_params[:order] = "topic_count DESC" end diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb index 041170580a2..4711a8b74b9 100644 --- a/lib/discourse_tagging.rb +++ b/lib/discourse_tagging.rb @@ -227,15 +227,13 @@ module DiscourseTagging 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)) + builder.where("LOWER(name) IN (:only_tag_names)") + builder_params[:only_tag_names] = opts[:only_tag_names].map(&:downcase) end # parent tag requirements @@ -268,6 +266,7 @@ module DiscourseTagging clean_tag(term) term.downcase! builder.where("LOWER(name) LIKE :term") + builder_params[:cleaned_term] = term builder_params[:term] = "%#{term}%" sql.gsub!("/*and_name_like*/", "AND LOWER(t.name) LIKE :term") else @@ -308,6 +307,13 @@ module DiscourseTagging end end + builder.limit(opts[:limit]) if opts[:limit] + if opts[:order] + builder.order_by(opts[:order]) + elsif opts[:order_search_results] && !term.blank? + builder.order_by("lower(name) = lower(:cleaned_term) DESC, topic_count DESC") + end + result = builder.query(builder_params).uniq { |t| t.id } end diff --git a/spec/components/discourse_tagging_spec.rb b/spec/components/discourse_tagging_spec.rb index 0c3791188c9..0b0bd1ad1ce 100644 --- a/spec/components/discourse_tagging_spec.rb +++ b/spec/components/discourse_tagging_spec.rb @@ -45,6 +45,35 @@ describe DiscourseTagging do expect(tags).to contain_exactly(tag1.name, tag3.name) end + context 'tag with colon' do + fab!(:tag_with_colon) { Fabricate(:tag, name: 'with:colon') } + + it "can use it as selected tag" do + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + selected_tags: [tag_with_colon.name], + for_input: true + ).map(&:name) + expect(tags).to contain_exactly(tag1.name, tag2.name, tag3.name) + end + + it "can search for tags with colons" do + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + for_input: true, + term: 'with:c', + order_search_results: true + ).map(&:name) + expect(tags).to contain_exactly(tag_with_colon.name) + end + + it "can limit results to the tag" do + tags = DiscourseTagging.filter_allowed_tags(Guardian.new(user), + for_topic: true, + only_tag_names: [tag_with_colon.name] + ).map(&:name) + expect(tags).to contain_exactly(tag_with_colon.name) + end + end + context 'with tags visible only to staff' do fab!(:hidden_tag) { Fabricate(:tag) } let!(:staff_tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) } diff --git a/spec/integration/category_tag_spec.rb b/spec/integration/category_tag_spec.rb index 0b5339f1c70..c917ca661f3 100644 --- a/spec/integration/category_tag_spec.rb +++ b/spec/integration/category_tag_spec.rb @@ -21,6 +21,7 @@ describe "category tag restrictions" do fab!(:tag2) { Fabricate(:tag, name: 'tag2') } fab!(:tag3) { Fabricate(:tag, name: 'tag3') } fab!(:tag4) { Fabricate(:tag, name: 'tag4') } + let(:tag_with_colon) { Fabricate(:tag, name: 'with:colon') } fab!(:user) { Fabricate(:user) } fab!(:admin) { Fabricate(:admin) } @@ -58,6 +59,11 @@ describe "category tag restrictions" do expect_same_tag_names(filter_allowed_tags(for_input: true, category: other_category, selected_tags: [tag3.name], term: 'tag'), [tag4]) end + it "search can handle colons in tag names" do + tag_with_colon + expect_same_tag_names(filter_allowed_tags(for_input: true, term: 'with:c'), [tag_with_colon]) + end + it "can't create new tags in a restricted category" do post = create_post(category: category_with_tags, tags: [tag1.name, "newtag"]) expect_same_tag_names(post.topic.tags, [tag1]) @@ -157,6 +163,11 @@ describe "category tag restrictions" do expect(post.topic.tags.map(&:name)).to eq([tag1.name]) end + it "handles colons" do + tag_with_colon + expect_same_tag_names(filter_allowed_tags(for_input: true, term: 'with:c'), [tag_with_colon]) + end + context 'required tags from tag group' do fab!(:tag_group) { Fabricate(:tag_group, tags: [tag1, tag3]) } before { category.update!(required_tag_group: tag_group, min_tags_from_required_group: 1) } @@ -326,7 +337,7 @@ describe "category tag restrictions" do car_category.allowed_tag_groups = [makes.name, honda_group.name, ford_group.name] end - it "handles all those rules", :focus do + it "handles all those rules" do # car tags can't be used outside of car category: 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])