diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 4ef31938b5e..b759bd646a4 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -155,21 +155,29 @@ export default Ember.Component.extend({ }; }, + userSearchTerm(term) { + const topicId = this.get("topic.id"); + // maybe this is a brand new topic, so grab category from composer + const categoryId = + this.get("topic.category_id") || this.get("composer._categoryId"); + + return userSearch({ + term, + topicId, + categoryId, + includeMentionableGroups: true + }); + }, + @on("didInsertElement") _composerEditorInit() { - const topicId = this.get("topic.id"); const $input = $(this.element.querySelector(".d-editor-input")); const $preview = $(this.element.querySelector(".d-editor-preview-wrapper")); if (this.siteSettings.enable_mentions) { $input.autocomplete({ template: findRawTemplate("user-selector-autocomplete"), - dataSource: term => - userSearch({ - term, - topicId, - includeMentionableGroups: true - }), + dataSource: term => this.userSearchTerm.call(this, term), key: "@", transformComplete: v => v.username || v.name, afterComplete() { diff --git a/app/assets/javascripts/discourse/lib/user-search.js.es6 b/app/assets/javascripts/discourse/lib/user-search.js.es6 index 753460f1fa5..1cb70232e98 100644 --- a/app/assets/javascripts/discourse/lib/user-search.js.es6 +++ b/app/assets/javascripts/discourse/lib/user-search.js.es6 @@ -4,7 +4,7 @@ import { userPath } from "discourse/lib/url"; import { emailValid } from "discourse/lib/utilities"; var cache = {}, - cacheTopicId, + cacheKey, cacheTime, currentTerm, oldSearch; @@ -12,6 +12,7 @@ var cache = {}, function performSearch( term, topicId, + categoryId, includeGroups, includeMentionableGroups, includeMessageableGroups, @@ -28,7 +29,7 @@ function performSearch( // 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) { + if (term === "" && !topicId && !categoryId) { return []; } @@ -37,6 +38,7 @@ function performSearch( data: { term: term, topic_id: topicId, + category_id: categoryId, include_groups: includeGroups, include_mentionable_groups: includeMentionableGroups, include_messageable_groups: includeMessageableGroups, @@ -140,6 +142,7 @@ export default function userSearch(options) { includeMessageableGroups = options.includeMessageableGroups, allowedUsers = options.allowedUsers, topicId = options.topicId, + categoryId = options.categoryId, groupMembersOf = options.groupMembersOf; if (oldSearch) { @@ -150,11 +153,13 @@ export default function userSearch(options) { currentTerm = term; return new Ember.RSVP.Promise(function(resolve) { - if (new Date() - cacheTime > 30000 || cacheTopicId !== topicId) { + const newCacheKey = `${topicId}-${categoryId}`; + + if (new Date() - cacheTime > 30000 || cacheKey !== newCacheKey) { cache = {}; } - cacheTopicId = topicId; + cacheKey = newCacheKey; var clearPromise = setTimeout(function() { resolve(CANCELLED_STATUS); @@ -168,6 +173,7 @@ export default function userSearch(options) { debouncedSearch( term, topicId, + categoryId, includeGroups, includeMentionableGroups, includeMessageableGroups, diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c7853e83c6e..da400520d2c 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -852,8 +852,13 @@ class UsersController < ApplicationController def search_users term = params[:term].to_s.strip + topic_id = params[:topic_id] topic_id = topic_id.to_i if topic_id + + category_id = params[:category_id] + category_id = category_id.to_i if category_id + topic_allowed_users = params[:topic_allowed_users] || false group_names = params[:groups] || [] @@ -862,12 +867,21 @@ class UsersController < ApplicationController @groups = Group.where(name: group_names) end - results = UserSearch.new(term, - topic_id: topic_id, - topic_allowed_users: topic_allowed_users, - searching_user: current_user, - groups: @groups - ).search + options = { + topic_allowed_users: topic_allowed_users, + searching_user: current_user, + groups: @groups + } + + if topic_id + options[:topic_id] = topic_id + end + + if category_id + options[:category_id] = category_id + end + + results = UserSearch.new(term, options).search user_fields = [:username, :upload_avatar_template] user_fields << :name if SiteSetting.enable_names? diff --git a/app/models/user_search.rb b/app/models/user_search.rb index 7b9f0b9a4dd..85c042e9530 100644 --- a/app/models/user_search.rb +++ b/app/models/user_search.rb @@ -9,6 +9,7 @@ class UserSearch @term = term @term_like = "#{term.downcase.gsub("_", "\\_")}%" @topic_id = opts[:topic_id] + @category_id = opts[:category_id] @topic_allowed_users = opts[:topic_allowed_users] @searching_user = opts[:searching_user] @include_staged_users = opts[:include_staged_users] || false @@ -81,7 +82,8 @@ class UserSearch # 2. in topic if @topic_id - in_topic = 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) @@ -96,8 +98,55 @@ class UserSearch return users.to_a if users.length >= @limit - # 3. global matches - if !@topic_id || @term.present? + secure_category_id = nil + + if @category_id + secure_category_id = DB.query_single(<<~SQL, @category_id).first + SELECT id FROM categories + WHERE read_restricted AND id = ? + SQL + elsif @topic_id + secure_category_id = DB.query_single(<<~SQL, @topic_id).first + SELECT id FROM categories + WHERE read_restricted AND id IN ( + SELECT category_id FROM topics + WHERE id = ? + ) + SQL + end + + # 3. category matches + # 10,11,12: trust level groups (tl0/1/2) explicitly bypassed + # may amend this in future to allow them if count in the group + # is small enough + if secure_category_id + in_category = filtered_by_term_users + .where(<<~SQL, secure_category_id) + users.id IN ( + SELECT gu.user_id + FROM group_users gu + WHERE group_id IN ( + SELECT group_id FROM category_groups + WHERE category_id = ? + ) AND group_id NOT IN (10,11,12) + LIMIT 200 + ) + SQL + + if @searching_user.present? + in_category = in_category.where('users.id <> ?', @searching_user.id) + end + + in_category + .order('last_seen_at DESC') + .limit(@limit - users.length) + .pluck(:id) + .each { |id| users << id } + end + + return users.to_a if users.length >= @limit + # 4. global matches + if @term.present? filtered_by_term_users.order('last_seen_at DESC') .limit(@limit - users.length) .pluck(:id) diff --git a/spec/models/user_search_spec.rb b/spec/models/user_search_spec.rb index 6d1ae9f8d81..582a0b9bc15 100644 --- a/spec/models/user_search_spec.rb +++ b/spec/models/user_search_spec.rb @@ -21,22 +21,47 @@ describe UserSearch do before do SearchIndexer.enable - - Fabricate :post, user: user1, topic: topic - Fabricate :post, user: user2, topic: topic2 - Fabricate :post, user: user3, topic: topic - Fabricate :post, user: user4, topic: topic - Fabricate :post, user: user5, topic: topic3 - Fabricate :post, user: user6, topic: topic - Fabricate :post, user: staged, topic: topic4 - - user6.update(suspended_at: 1.day.ago, suspended_till: 1.year.from_now) end def search_for(*args) UserSearch.new(*args).search end + it 'finds users in secure category' do + group = Fabricate(:group) + user = Fabricate(:user) + group.add(user) + group.save + + category = Fabricate.build( + :category, + read_restricted: true, + user: user + ) + # TODO: maybe we amend all category fabrication to skip this? + # test is half a second faster + category.skip_category_definition = true + category.save! + + Fabricate(:category_group, category: category, group: group) + + results = search_for("", category_id: category.id) + + expect(user.username).to eq(results[0].username) + expect(results.length).to eq(1) + + topic = Fabricate(:topic, category: category) + _post = Fabricate(:post, user: topic.user, topic: topic) + + results = search_for("", topic_id: topic.id) + + expect(results.length).to eq(2) + + expect(results.map(&:username)).to contain_exactly( + user.username, topic.user.username + ) + end + it 'allows for correct underscore searching' do Fabricate(:user, username: 'Under_Score') Fabricate(:user, username: 'undertaker') @@ -67,95 +92,110 @@ describe UserSearch do expect(results.count).to eq(2) end - # this is a seriously expensive integration test, - # re-creating this entire test db is too expensive reuse - it "operates correctly" do - # normal search - results = search_for(user1.name.split(" ").first) - expect(results.size).to eq(1) - expect(results.first.username).to eq(user1.username) + context "with seed data" do - # lower case - results = search_for(user1.name.split(" ").first.downcase) - expect(results.size).to eq(1) - expect(results.first).to eq(user1) + before do - # username - results = search_for(user4.username) - expect(results.size).to eq(1) - expect(results.first).to eq(user4) + Fabricate :post, user: user1, topic: topic + Fabricate :post, user: user2, topic: topic2 + Fabricate :post, user: user3, topic: topic + Fabricate :post, user: user4, topic: topic + Fabricate :post, user: user5, topic: topic3 + Fabricate :post, user: user6, topic: topic + Fabricate :post, user: staged, topic: topic4 - # case insensitive - results = search_for(user4.username.upcase) - expect(results.size).to eq(1) - expect(results.first).to eq(user4) + user6.update(suspended_at: 1.day.ago, suspended_till: 1.year.from_now) + end - # substrings - # only staff members see suspended users in results - results = search_for("mr") - expect(results.size).to eq(5) - expect(results).not_to include(user6) - expect(search_for("mr", searching_user: user1).size).to eq(5) + # this is a seriously expensive integration test, + # re-creating this entire test db is too expensive reuse + it "operates correctly" do + # normal search + results = search_for(user1.name.split(" ").first) + expect(results.size).to eq(1) + expect(results.first.username).to eq(user1.username) - results = search_for("mr", searching_user: admin) - expect(results.size).to eq(6) - expect(results).to include(user6) - expect(search_for("mr", searching_user: moderator).size).to eq(6) + # lower case + results = search_for(user1.name.split(" ").first.downcase) + expect(results.size).to eq(1) + expect(results.first).to eq(user1) - results = search_for(user1.username, searching_user: admin) - expect(results.size).to eq(3) + # username + results = search_for(user4.username) + expect(results.size).to eq(1) + expect(results.first).to eq(user4) - results = search_for("MR", searching_user: admin) - expect(results.size).to eq(6) + # case insensitive + results = search_for(user4.username.upcase) + expect(results.size).to eq(1) + expect(results.first).to eq(user4) - results = search_for("MRB", searching_user: admin, limit: 2) - expect(results.size).to eq(2) + # substrings + # only staff members see suspended users in results + results = search_for("mr") + expect(results.size).to eq(5) + expect(results).not_to include(user6) + expect(search_for("mr", searching_user: user1).size).to eq(5) - # topic priority - results = search_for(user1.username, topic_id: topic.id) - expect(results.first).to eq(user1) + results = search_for("mr", searching_user: admin) + expect(results.size).to eq(6) + expect(results).to include(user6) + expect(search_for("mr", searching_user: moderator).size).to eq(6) - results = search_for(user1.username, topic_id: topic2.id) - expect(results[1]).to eq(user2) + results = search_for(user1.username, searching_user: admin) + expect(results.size).to eq(3) - results = search_for(user1.username, topic_id: topic3.id) - expect(results[1]).to eq(user5) + results = search_for("MR", searching_user: admin) + expect(results.size).to eq(6) - # When searching by name is enabled, it returns the record - SiteSetting.enable_names = true - results = search_for("Tarantino") - expect(results.size).to eq(1) + results = search_for("MRB", searching_user: admin, limit: 2) + expect(results.size).to eq(2) - results = search_for("coding") - expect(results.size).to eq(0) + # topic priority + results = search_for(user1.username, topic_id: topic.id) + expect(results.first).to eq(user1) - results = search_for("z") - expect(results.size).to eq(0) + results = search_for(user1.username, topic_id: topic2.id) + expect(results[1]).to eq(user2) - # When searching by name is disabled, it will not return the record - SiteSetting.enable_names = false - results = search_for("Tarantino") - expect(results.size).to eq(0) + results = search_for(user1.username, topic_id: topic3.id) + expect(results[1]).to eq(user5) - # find an exact match first - results = search_for("mrB") - expect(results.first.username).to eq(user1.username) + # When searching by name is enabled, it returns the record + SiteSetting.enable_names = true + results = search_for("Tarantino") + expect(results.size).to eq(1) - # don't return inactive users - results = search_for(inactive.username) - expect(results).to be_blank + results = search_for("coding") + expect(results.size).to eq(0) - # don't return staged users - results = search_for(staged.username) - expect(results).to be_blank + results = search_for("z") + expect(results.size).to eq(0) - results = search_for(staged.username, include_staged_users: true) - expect(results.first.username).to eq(staged.username) + # When searching by name is disabled, it will not return the record + SiteSetting.enable_names = false + results = search_for("Tarantino") + expect(results.size).to eq(0) - results = search_for("", topic_id: topic.id, searching_user: user1) + # find an exact match first + results = search_for("mrB") + expect(results.first.username).to eq(user1.username) - # mrb is omitted, mrb is current user - expect(results.map(&:username)).to eq(["mrpink", "mrorange"]) + # don't return inactive users + results = search_for(inactive.username) + expect(results).to be_blank + + # don't return staged users + results = search_for(staged.username) + expect(results).to be_blank + + 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 - end diff --git a/test/javascripts/lib/user-search-test.js.es6 b/test/javascripts/lib/user-search-test.js.es6 index c0d3cca04e2..f061c2ec614 100644 --- a/test/javascripts/lib/user-search-test.js.es6 +++ b/test/javascripts/lib/user-search-test.js.es6 @@ -7,7 +7,22 @@ QUnit.module("lib:user-search", { }; // prettier-ignore - server.get("/u/search/users", () => { //eslint-disable-line + server.get("/u/search/users", request => { //eslint-disable-line + + // special responder for per category search + const categoryMatch = request.url.match(/category_id=([0-9]+)/); + if (categoryMatch) { + return response({ + users: [ + { + username: `category_${categoryMatch[1]}`, + name: "category user", + avatar_template: + "https://avatars.discourse.org/v3/letter/t/41988e/{size}.png" + } + ]}); + } + return response({ users: [ { @@ -62,6 +77,21 @@ QUnit.module("lib:user-search", { } }); +QUnit.test("it flushes cache when switching categories", async assert => { + let results = await userSearch({ term: "hello", categoryId: 1 }); + assert.equal(results[0].username, "category_1"); + assert.equal(results.length, 1); + + // this is cached ... so let's check the cache is good + results = await userSearch({ term: "hello", categoryId: 1 }); + assert.equal(results[0].username, "category_1"); + assert.equal(results.length, 1); + + results = await userSearch({ term: "hello", categoryId: 2 }); + assert.equal(results[0].username, "category_2"); + assert.equal(results.length, 1); +}); + QUnit.test("it places groups unconditionally for exact match", async assert => { let results = await userSearch({ term: "Team" }); assert.equal(results[results.length - 1]["name"], "team");