github-mirror/discourse
github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-04-17 03:11:32 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FEATURE: Mixed case tagging (#6454)

Browse Source 
- By default, behaviour is not changed: tags are made lowercase upon creation and edit.

- If force_lowercase_tags is disabled, then mixed case tags are allowed.

- Tags must remain case-insensitively unique. This is enforced by ActiveRecord and Postgres.

- A migration is added to provide a `UNIQUE` index on `lower(name)`. Migration includes a safety to correct any current tags that do not meet the criteria.

- A `where_name` scope is added to `models/tag.rb`, to allow easy case-insensitive lookups. This is used instead of `Tag.where(name: "blah")`.

- URLs remain lowercase. Mixed case URLs are functional, but have the lowercase equivalent as the canonical.
This commit is contained in:
David Taylor 2018-10-05 10:23:52 +01:00 committed by GitHub
parent 8430ea927e
commit 9bf522f227
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 137 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/assets/javascripts/discourse/lib/render-tag.js.es6
View File

@ -1,6 +1,7 @@
export default function renderTag(tag, params) {
params = params || {};
tag = Handlebars.Utils.escapeExpression(tag);
const visibleName = Handlebars.Utils.escapeExpression(tag);
tag = visibleName.toLowerCase();
const classes = ["discourse-tag"];
const tagName = params.tagName || "a";
let path;
@ -29,7 +30,7 @@ export default function renderTag(tag, params) {
" class='" +
classes.join(" ") +
"'>" +
tag +
visibleName +
"</" +
tagName +
">";

13
app/assets/javascripts/discourse/routes/tags-show.js.es6
View File

@ -49,10 +49,12 @@ export default Discourse.Route.extend({
if (tag && tag.get("id") !== "none" && this.get("currentUser")) {
// If logged in, we should get the tag's user settings
return this.store.find("tagNotification", tag.get("id")).then(tn => {
this.set("tagNotification", tn);
return tag;
});
return this.store
.find("tagNotification", tag.get("id").toLowerCase())
.then(tn => {
this.set("tagNotification", tn);
return tag;
});
}
return tag;
@ -67,7 +69,7 @@ export default Discourse.Route.extend({
const categorySlug = this.get("categorySlug");
const parentCategorySlug = this.get("parentCategorySlug");
const filter = this.get("navMode");
const tag_id = tag ? tag.id : "none";
const tag_id = tag ? tag.id.toLowerCase() : "none";
if (categorySlug) {
var category = Discourse.Category.findBySlug(
@ -100,6 +102,7 @@ export default Discourse.Route.extend({
params,
{}
).then(list => {
tag.set("id", list.topic_list.tags[0].name); // Update name of tag (case might be different)
controller.setProperties({
list: list,
canCreateTopic: list.get("can_create_topic"),

1
app/assets/javascripts/select-kit/components/mini-tag-chooser.js.es6
View File

@ -225,7 +225,6 @@ export default ComboBox.extend(TagsMixin, {
case "string":
// See lib/discourse_tagging#clean_tag.
return content
.toLowerCase()
.trim()
.replace(/\s+/, "-")
.replace(/[\/\?#\[\]@!\$&'\(\)\*\+,;=\.%\\`^\s|\{\}"<>]+/, "")

2
app/assets/javascripts/select-kit/components/tag-drop.js.es6
View File

@ -145,7 +145,7 @@ export default ComboBoxComponent.extend(TagsMixin, {
if (this.get("currentCategory")) {
url += this.get("currentCategory.url");
}
url = `${url}/${tagId}`;
url = `${url}/${tagId.toLowerCase()}`;
DiscourseURL.routeTo(url);
},

8
app/assets/javascripts/select-kit/mixins/tags.js.es6
View File

@ -52,12 +52,16 @@ export default Ember.Mixin.create({
return false;
}
const toLowerCaseOrUndefined = string => {
return string === undefined ? undefined : string.toLowerCase();
};
const inCollection = this.get("collectionComputedContent")
.map(c => get(c, "id"))
.map(c => toLowerCaseOrUndefined(get(c, "id")))
.includes(term);
const inSelection = this.get("selection")
.map(s => get(s, "value").toLowerCase())
.map(s => toLowerCaseOrUndefined(get(s, "value")))
.includes(term);
if (inCollection || inSelection) {
return false;

10
app/controllers/tags_controller.rb
View File

@ -89,7 +89,7 @@ class TagsController < ::ApplicationController
path_name = url_method(params.slice(:category, :parent_category))
canonical_url "#{Discourse.base_url_no_prefix}#{public_send(path_name, *(params.slice(:parent_category, :category, :tag_id).values.map { |t| t.force_encoding("UTF-8") }))}"
if @list.topics.size == 0 && params[:tag_id] != 'none' && !Tag.where(name: @tag_id).exists?
if @list.topics.size == 0 && params[:tag_id] != 'none' && !Tag.where_name(@tag_id).exists?
raise Discourse::NotFound.new("tag not found", check_permalinks: true)
else
respond_with_list(@list)
@ -162,7 +162,7 @@ class TagsController < ::ApplicationController
json_response = { results: tags }
if Tag.where(name: params[:q]).exists? && !tags.find { |h| h[:id] == params[:q] }
if Tag.where_name(params[:q]).exists? && !tags.find { |h| h[:id] == params[:q] }
# filter_allowed_tags determined that the tag entered is not allowed
json_response[:forbidden] = params[:q]
end
@ -171,7 +171,7 @@ class TagsController < ::ApplicationController
end
def notifications
tag = Tag.find_by_name(params[:tag_id])
tag = Tag.where_name(params[:tag_id]).first
raise Discourse::NotFound unless tag
level = tag.tag_users.where(user: current_user).first.try(:notification_level) || TagUser.notification_levels[:regular]
render json: { tag_notification: { id: tag.name, notification_level: level.to_i } }
@ -186,9 +186,7 @@ class TagsController < ::ApplicationController
end
def check_hashtag
tag_values = params[:tag_values].each(&:downcase!)
valid_tags = Tag.where(name: tag_values).map do |tag|
valid_tags = Tag.where_name(params[:tag_values]).map do |tag|
{ value: tag.name, url: tag.full_url }
end.compact

11
app/models/tag.rb
View File

@ -2,7 +2,12 @@ class Tag < ActiveRecord::Base
include Searchable
include HasDestroyedWebHook
validates :name, presence: true, uniqueness: true
validates :name, presence: true, uniqueness: { case_sensitive: false }
scope :where_name, ->(name) do
name = Array(name).map(&:downcase)
where("lower(name) IN (?)", name)
end
has_many :tag_users # notification settings
@ -59,6 +64,10 @@ class Tag < ActiveRecord::Base
SQL
end
def self.find_by_name(name)
self.find_by('lower(name) = ?', name.downcase)
end
def self.top_tags(limit_arg: nil, category: nil, guardian: nil)
limit = limit_arg || SiteSetting.max_tags_in_filter_list
scope_category_ids = (guardian || Guardian.new).allowed_category_ids

2
app/models/tag_user.rb
View File

@ -19,7 +19,7 @@ class TagUser < ActiveRecord::Base
records = TagUser.where(user: user, notification_level: notification_levels[level])
old_ids = records.pluck(:tag_id)
tag_ids = tags.empty? ? [] : Tag.where('name in (?)', tags).pluck(:id)
tag_ids = tags.empty? ? [] : Tag.where_name(tags).pluck(:id)
remove = (old_ids - tag_ids)
if remove.present?

2
app/services/search_indexer.rb
View File

@ -105,7 +105,7 @@ class SearchIndexer
end
def self.update_tags_index(tag_id, name)
update_index(table: 'tag', id: tag_id, raw_data: [name])
update_index(table: 'tag', id: tag_id, raw_data: [name.downcase])
end
def self.queue_post_reindex(topic_id)

1
config/locales/server.en.yml
View File

@ -1798,6 +1798,7 @@ en:
min_trust_level_to_tag_topics: "Minimum trust level required to tag topics"
suppress_overlapping_tags_in_list: "If tags match exact words in topic titles, don't show the tag"
remove_muted_tags_from_latest: "Don't show topics tagged with muted tags in the latest topic list."
force_lowercase_tags: "Force all new tags to be entirely lowercase."
company_short_name: "Company Name (short)"
company_full_name: "Company Name (full)"

2
config/site_settings.yml
View File

@ -1788,3 +1788,5 @@ tags:
client: true
remove_muted_tags_from_latest:
default: false
force_lowercase_tags:
default: true

17
db/migrate/20180928105835_add_index_to_tags.rb Normal file
View File

@ -0,0 +1,17 @@
class AddIndexToTags < ActiveRecord::Migration[5.2]
def up
# Append ID to any tags that already have duplicate names
# Super rare case, as this is not possible to do via the UI
# Might affect some imports
execute <<~SQL
UPDATE tags
SET name = name || id
WHERE EXISTS(SELECT * FROM tags t WHERE lower(t.name) = lower(tags.name) AND t.id < tags.id)
SQL
add_index :tags, 'lower(name)', unique: true
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

18
lib/discourse_tagging.rb
View File

@ -39,7 +39,7 @@ 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),
Tag.where_name(tag_names),
nil, # guardian
for_topic: true,
category: category,
@ -48,7 +48,7 @@ module DiscourseTagging
if tags.size < tag_names.size && (category.nil? || (category.tags.count == 0 && category.tag_groups.count == 0))
tag_names.each do |name|
unless Tag.where(name: name).exists?
unless Tag.where_name(name).exists?
tags << Tag.create(name: name)
end
end
@ -82,8 +82,7 @@ module DiscourseTagging
# 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) : []
selected_tag_ids = opts[:selected_tags] ? Tag.where_name(opts[:selected_tags]).pluck(:id) : []
if !opts[:for_topic] && !selected_tag_ids.empty?
query = query.where('tags.id NOT IN (?)', selected_tag_ids)
@ -92,8 +91,8 @@ module DiscourseTagging
term = opts[:term]
if term.present?
term.gsub!("_", "\\_")
term = clean_tag(term)
query = query.where('tags.name like ?', "%#{term}%")
term = clean_tag(term).downcase
query = query.where('lower(tags.name) like ?', "%#{term}%")
end
# Filters for category-specific tags:
@ -203,7 +202,8 @@ module DiscourseTagging
end
def self.clean_tag(tag)
tag.downcase.strip
tag.downcase! if SiteSetting.force_lowercase_tags
tag.strip
.gsub(/\s+/, '-').squeeze('-')
.gsub(TAGS_FILTER_REGEXP, '')[0...SiteSetting.max_tag_length]
end
@ -212,7 +212,7 @@ module DiscourseTagging
return [] unless guardian.can_tag_topics? && tags_arg.present?
tag_names = Tag.where(name: tags_arg).pluck(:name)
tag_names = Tag.where_name(tags_arg).pluck(:name)
if guardian.can_create_tag?
tag_names += (tags_arg - tag_names).map { |t| clean_tag(t) }
@ -226,7 +226,7 @@ module DiscourseTagging
def self.add_or_create_tags_by_name(taggable, tag_names_arg, opts = {})
tag_names = DiscourseTagging.tags_for_saving(tag_names_arg, Guardian.new(Discourse.system_user), opts) || []
if taggable.tags.pluck(:name).sort != tag_names.sort
taggable.tags = Tag.where(name: tag_names).all
taggable.tags = Tag.where_name(tag_names).all
if taggable.tags.size < tag_names.size
new_tag_names = tag_names - taggable.tags.map(&:name)
new_tag_names.each do |name|

8
lib/search.rb
View File

@ -407,7 +407,7 @@ class Search
posts.where("topics.category_id IN (?)", category_ids)
else
# try a possible tag match
tag_id = Tag.where(name: slug[0]).pluck(:id).first
tag_id = Tag.where_name(slug[0]).pluck(:id).first
if (tag_id)
posts.where("topics.id IN (
SELECT DISTINCT(tt.topic_id)
@ -496,7 +496,7 @@ class Search
def search_tags(posts, match, positive:)
return if match.nil?
match.downcase!
modifier = positive ? "" : "NOT"
if match.include?('+')
@ -507,7 +507,7 @@ class Search
FROM topic_tags tt, tags
WHERE tt.tag_id = tags.id
GROUP BY tt.topic_id
HAVING to_tsvector(#{default_ts_config}, array_to_string(array_agg(tags.name), ' ')) @@ to_tsquery(#{default_ts_config}, ?)
HAVING to_tsvector(#{default_ts_config}, array_to_string(array_agg(lower(tags.name)), ' ')) @@ to_tsquery(#{default_ts_config}, ?)
)", tags.join('&'))
else
tags = match.split(",")
@ -515,7 +515,7 @@ class Search
posts.where("topics.id #{modifier} IN (
SELECT DISTINCT(tt.topic_id)
FROM topic_tags tt, tags
WHERE tt.tag_id = tags.id AND tags.name IN (?)
WHERE tt.tag_id = tags.id AND lower(tags.name) IN (?)
)", tags)
end
end

5
lib/topic_query.rb
View File

@ -634,11 +634,12 @@ class TopicQuery
result = result.preload(:tags)
if @options[:tags] && @options[:tags].size > 0
@options[:tags].each { |t| t.downcase! if t.is_a? String }
if @options[:match_all_tags]
# ALL of the given tags:
tags_count = @options[:tags].length
@options[:tags] = Tag.where(name: @options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer)
@options[:tags] = Tag.where_name(@options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer)
if tags_count == @options[:tags].length
@options[:tags].each_with_index do |tag, index|
@ -654,7 +655,7 @@ class TopicQuery
if @options[:tags][0].is_a?(Integer)
result = result.where("tags.id in (?)", @options[:tags])
else
result = result.where("tags.name in (?)", @options[:tags])
result = result.where("lower(tags.name) in (?)", @options[:tags])
end
end
elsif @options[:no_tags]

2
script/bulk_import/discourse_merger.rb
View File

@ -271,7 +271,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base
@raw_connection.copy_data(sql, @encoder) do
source_raw_connection.exec("SELECT #{columns.map { |c| "\"#{c}\"" }.join(', ')} FROM tags").each do |row|
if existing = Tag.where(name: row['name']).first
if existing = Tag.where_name(row['name']).first
@tags[row['id']] = existing.id
next
end

13
spec/components/discourse_tagging_spec.rb
View File

@ -13,7 +13,7 @@ describe DiscourseTagging do
let!(:tag1) { Fabricate(:tag, name: "fun") }
let!(:tag2) { Fabricate(:tag, name: "fun2") }
let!(:tag3) { Fabricate(:tag, name: "fun3") }
let!(:tag3) { Fabricate(:tag, name: "Fun3") }
before do
SiteSetting.tagging_enabled = true
@ -186,7 +186,8 @@ describe DiscourseTagging do
it "returns only existing tag names" do
Fabricate(:tag, name: 'oldtag')
expect(described_class.tags_for_saving(['newtag', 'oldtag'], guardian).try(:sort)).to eq(['oldtag'])
Fabricate(:tag, name: 'oldTag2')
expect(described_class.tags_for_saving(['newtag', 'oldtag', 'oldtag2'], guardian)).to contain_exactly('oldtag', 'oldTag2')
end
end
@ -205,5 +206,13 @@ describe DiscourseTagging do
expect(described_class.tags_for_saving(['math=fun', 'fun*2@gmail.com'], guardian).try(:sort)).to eq(['math=fun', 'fun2gmailcom'].sort)
end
end
describe "clean_tag" do
it "downcases new tags if setting enabled" do
expect(DiscourseTagging.clean_tag("HeLlO")).to eq("hello")
SiteSetting.force_lowercase_tags = false
expect(DiscourseTagging.clean_tag("HeLlO")).to eq("HeLlO")
end
end
end
end

9
spec/components/search_spec.rb
View File

@ -407,6 +407,7 @@ describe Search do
end
let!(:tag) { Fabricate(:tag) }
let!(:uppercase_tag) { Fabricate(:tag, name: "HeLlO") }
let(:tag_group) { Fabricate(:tag_group) }
let(:category) { Fabricate(:category) }
@ -415,7 +416,7 @@ describe Search do
SiteSetting.tagging_enabled = true
post = Fabricate(:post, raw: 'I am special post')
DiscourseTagging.tag_topic_by_names(post.topic, Guardian.new(Fabricate.build(:admin)), [tag.name])
DiscourseTagging.tag_topic_by_names(post.topic, Guardian.new(Fabricate.build(:admin)), [tag.name, uppercase_tag.name])
post.topic.save
# we got to make this index (it is deferred)
@ -424,6 +425,9 @@ describe Search do
result = Search.execute(tag.name)
expect(result.posts.length).to eq(1)
result = Search.execute("hElLo")
expect(result.posts.length).to eq(1)
SiteSetting.tagging_enabled = false
result = Search.execute(tag.name)
@ -822,9 +826,10 @@ describe Search do
expect(Search.execute("sams post #sub-category").posts.length).to eq(1)
# tags
topic.tags = [Fabricate(:tag, name: 'alpha'), Fabricate(:tag, name: 'привет')]
topic.tags = [Fabricate(:tag, name: 'alpha'), Fabricate(:tag, name: 'привет'), Fabricate(:tag, name: 'HeLlO')]
expect(Search.execute('this is a test #alpha').posts.map(&:id)).to eq([post.id])
expect(Search.execute('this is a test #привет').posts.map(&:id)).to eq([post.id])
expect(Search.execute('this is a test #hElLo').posts.map(&:id)).to eq([post.id])
expect(Search.execute('this is a test #beta').posts.size).to eq(0)
end

5
spec/components/topic_query_spec.rb
View File

@ -160,6 +160,7 @@ describe TopicQuery do
context 'tag filter' do
let(:tag) { Fabricate(:tag) }
let(:other_tag) { Fabricate(:tag) }
let(:uppercase_tag) { Fabricate(:tag, name: "HeLlO") }
before do
SiteSetting.tagging_enabled = true
@ -169,6 +170,7 @@ describe TopicQuery do
let!(:tagged_topic1) { Fabricate(:topic, tags: [tag]) }
let!(:tagged_topic2) { Fabricate(:topic, tags: [other_tag]) }
let!(:tagged_topic3) { Fabricate(:topic, tags: [tag, other_tag]) }
let!(:tagged_topic4) { Fabricate(:topic, tags: [uppercase_tag]) }
let!(:no_tags_topic) { Fabricate(:topic) }
it "returns topics with the tag when filtered to it" do
@ -186,6 +188,9 @@ describe TopicQuery do
expect(TopicQuery.new(moderator, tags: [tag.id, other_tag.id]).list_latest.topics)
.to contain_exactly(tagged_topic1, tagged_topic2, tagged_topic3)
expect(TopicQuery.new(moderator, tags: ["hElLo"]).list_latest.topics)
.to contain_exactly(tagged_topic4)
end
it "can return topics with all specified tags" do

6
spec/models/tag_spec.rb
View File

@ -27,6 +27,12 @@ describe Tag do
expect(event[:event_name]).to eq(:tag_created)
expect(event[:params].first).to eq(subject)
end
it 'prevents case-insensitive duplicates' do
Fabricate.build(:tag, name: "hello").save!
expect { Fabricate.build(:tag, name: "hElLo").save! }.to raise_error(ActiveRecord::RecordInvalid)
end
end
describe 'destroy' do

13
test/javascripts/acceptance/tags-test.js.es6
View File

@ -32,7 +32,7 @@ QUnit.test("list the tags in groups", async assert => {
id: 2,
name: "Ford Cars",
tags: [
{ id: "escort", text: "escort", count: 1, pm_count: 0 },
{ id: "Escort", text: "Escort", count: 1, pm_count: 0 },
{ id: "focus", text: "focus", count: 3, pm_count: 0 }
]
},
@ -79,7 +79,16 @@ QUnit.test("list the tags in groups", async assert => {
.map(i => {
return $(i).text();
}),
["focus", "escort"],
["focus", "Escort"],
"shows the tags in default sort (by count)"
);
assert.deepEqual(
$(".tag-list:first .discourse-tag")
.toArray()
.map(i => {
return $(i).attr("href");
}),
["/tags/focus", "/tags/escort"],
"always uses lowercase URLs for mixed case tags"
);
});

12
test/javascripts/components/mini-tag-chooser-test.js.es6
View File

@ -28,7 +28,7 @@ componentTest("default", {
});
}
if (params.queryParams.q === "joffrey" || params.queryParams.q === "invalid'tag" || params.queryParams.q === "01234567890123456789012345") {
if (params.queryParams.q.toLowerCase() === "joffrey" || params.queryParams.q === "invalid'tag" || params.queryParams.q === "01234567890123456789012345") {
return response({results: []});
}
@ -74,6 +74,16 @@ componentTest("default", {
"it creates the tag"
);
await this.get("subject").expand();
await this.get("subject").fillInFilter("Joffrey");
await this.get("subject").keyboard("enter");
await this.get("subject").collapse();
assert.deepEqual(
this.get("tags"),
["jeff", "neil", "arpit", "régis", "joffrey"],
"it does not allow case insensitive duplicate tags"
);
await this.get("subject").expand();
await this.get("subject").fillInFilter("invalid'tag");
await this.get("subject").keyboard("enter");

15
test/javascripts/components/tag-drop-test.js.es6
View File

@ -1,4 +1,5 @@
import componentTest from "helpers/component-test";
import DiscourseURL from "discourse/lib/url";
moduleForComponent("tag-drop", {
integration: true,
@ -26,6 +27,12 @@ componentTest("default", {
{ "id": "régis", "name": "régis", "count": 2, "pm_count": 0 }
]
});
}else if (params.queryParams.q === "dav") {
return response({
"results": [
{ "id": "David", "name": "David", "count": 2, "pm_count": 0 }
]
});
}
});
},
@ -66,5 +73,13 @@ componentTest("default", {
"jeff",
"it returns top tags for an empty search"
);
sandbox.stub(DiscourseURL, "routeTo");
await this.get("subject").fillInFilter("dav");
await this.get("subject").keyboard("enter");
assert.ok(
DiscourseURL.routeTo.calledWith("/tags/david"),
"it uses lowercase URLs for tags"
);
}
});