From f7809207591c6ec48d137df9b7fc05514aa80dd5 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Tue, 6 Aug 2019 17:57:45 +1000 Subject: [PATCH] FEATURE: mention in secure category to prioritize groups This feature allows @ mentions to prioritize showing members of a group who have explicit permission to a category. This makes it far easier to @ mention group member when composing topics in categories where only the group has access. For example: If Sam, Jane an Joan have access to bugs category. Then `@` will auto complete to (jane,joan,sam) ordered on last seen at This feature works on new topics and existing topics. There is an explicit exclusion of trust level 0,1,2 groups cause they get too big. --- .../components/composer-editor.js.es6 | 22 +- .../discourse/lib/user-search.js.es6 | 14 +- app/controllers/users_controller.rb | 26 ++- app/models/user_search.rb | 55 ++++- spec/models/user_search_spec.rb | 198 +++++++++++------- test/javascripts/lib/user-search-test.js.es6 | 32 ++- 6 files changed, 247 insertions(+), 100 deletions(-) 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");