diff --git a/app/assets/javascripts/discourse/lib/user-search.js.es6 b/app/assets/javascripts/discourse/lib/user-search.js.es6 index 932d93b3c02..c64ec385de3 100644 --- a/app/assets/javascripts/discourse/lib/user-search.js.es6 +++ b/app/assets/javascripts/discourse/lib/user-search.js.es6 @@ -23,7 +23,11 @@ function performSearch( resultsFn(cached); return; } - if (term === "") { + + // I am not strongly against unconditionally returning + // however this allows us to return a list of probable + // users we want to mention, early on a topic + if (term === "" && !topicId) { return []; } @@ -108,6 +112,18 @@ function organizeResults(r, options) { return results; } +// all punctuations except for . which is allowed in usernames +// note: these are valid in names, but will end up tripping search anyway so just skip +// this means searching for `sam saffron` is OK but if my name is `sam$ saffron` autocomplete +// will not find me, which is a reasonable compromise +// +// we also ignore if we notice a double space or a string that is only a space +const ignoreRegex = /([\u2000-\u206F\u2E00-\u2E7F\\'!"#$%&()*+,\-\/:;<=>?@\[\]^_`{|}~])|\s\s|^\s$/; + +function skipSearch(term) { + return !!term.match(ignoreRegex); +} + export default function userSearch(options) { if (options.term && options.term.length > 0 && options.term[0] === "@") { options.term = options.term.substring(1); @@ -121,10 +137,6 @@ export default function userSearch(options) { topicId = options.topicId, group = options.group; - if (/[^\w.-]/.test(term)) { - term = ""; - } - if (oldSearch) { oldSearch.abort(); oldSearch = null; @@ -143,6 +155,11 @@ export default function userSearch(options) { resolve(CANCELLED_STATUS); }, 5000); + if (skipSearch(term)) { + resolve([]); + return; + } + debouncedSearch( term, topicId, diff --git a/app/models/user_search.rb b/app/models/user_search.rb index 3acdf917c3e..98855b585ec 100644 --- a/app/models/user_search.rb +++ b/app/models/user_search.rb @@ -80,7 +80,13 @@ class UserSearch # 2. in topic if @topic_id - filtered_by_term_users.where('users.id IN (SELECT p.user_id FROM posts p WHERE topic_id = ?)', @topic_id) + in_topic = filtered_by_term_users.where('users.id IN (SELECT p.user_id FROM posts p WHERE topic_id = ?)', @topic_id) + + if @searching_user.present? + in_topic = in_topic.where('users.id <> ?', @searching_user.id) + end + + in_topic .order('last_seen_at DESC') .limit(@limit - users.length) .pluck(:id) @@ -90,10 +96,12 @@ class UserSearch return users.to_a if users.length >= @limit # 3. global matches - filtered_by_term_users.order('last_seen_at DESC') - .limit(@limit - users.length) - .pluck(:id) - .each { |id| users << id } + if !@topic_id || @term.present? + filtered_by_term_users.order('last_seen_at DESC') + .limit(@limit - users.length) + .pluck(:id) + .each { |id| users << id } + end users.to_a end diff --git a/spec/models/user_search_spec.rb b/spec/models/user_search_spec.rb index 73b85885eca..2f4b9c484ad 100644 --- a/spec/models/user_search_spec.rb +++ b/spec/models/user_search_spec.rb @@ -137,6 +137,11 @@ describe UserSearch do results = search_for(staged.username, include_staged_users: true) expect(results.first.username).to eq(staged.username) + + results = search_for("", topic_id: topic.id, searching_user: user1) + + # mrb is omitted, mrb is current user + expect(results.map(&:username)).to eq(["mrpink", "mrorange"]) end end diff --git a/test/javascripts/lib/user-search-test.js.es6 b/test/javascripts/lib/user-search-test.js.es6 index 647a0ab50d4..55b512c9ae9 100644 --- a/test/javascripts/lib/user-search-test.js.es6 +++ b/test/javascripts/lib/user-search-test.js.es6 @@ -1,5 +1,4 @@ import userSearch from "discourse/lib/user-search"; -import { CANCELLED_STATUS } from "discourse/lib/autocomplete"; QUnit.module("lib:user-search", { beforeEach() { @@ -73,7 +72,29 @@ QUnit.test("it strips @ from the beginning", async assert => { assert.equal(results[results.length - 1]["name"], "team"); }); -QUnit.test("it does not search for invalid usernames", async assert => { - let results = await userSearch({ term: "foo, " }); - assert.equal(results, CANCELLED_STATUS); +QUnit.test("it skips a search depending on punctuations", async assert => { + let skippedTerms = [ + "@sam s", // double space is not allowed + "@sam;", + "@sam,", + "@sam:" + ]; + + skippedTerms.forEach(async term => { + let results = await userSearch({ term }); + assert.equal(results.length, 0); + }); + + let allowedTerms = [ + "@sam sam", // double space is not allowed + "@sam.sam", + "@" + ]; + + let topicId = 100; + + allowedTerms.forEach(async term => { + let results = await userSearch({ term, topicId }); + assert.equal(results.length, 6); + }); });