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.

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 =>
+        dataSource: term => this.userSearchTerm.call(this, term),
-          userSearch({
-            term,
-            topicId,
-            includeMentionableGroups: true
-          }),
         key: "@",
         transformComplete: v => v.username || v.name,
         afterComplete() {

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,

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,
+    options = {
-                             topic_id: topic_id,
+     topic_allowed_users: topic_allowed_users,
-                             topic_allowed_users: topic_allowed_users,
+     searching_user: current_user,
-                             searching_user: current_user,
+     groups: @groups
-                             groups: @groups
+    }
-                            ).search
+
+    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?

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
+    secure_category_id = nil
-    if !@topic_id || @term.present?
+
+    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)

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,
+  context "with seed data" do
-  # 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)
 
-    # lower case
+    before do
-    results = search_for(user1.name.split(" ").first.downcase)
-    expect(results.size).to eq(1)
-    expect(results.first).to eq(user1)
 
-    # username
+      Fabricate :post, user: user1, topic: topic
-    results = search_for(user4.username)
+      Fabricate :post, user: user2, topic: topic2
-    expect(results.size).to eq(1)
+      Fabricate :post, user: user3, topic: topic
-    expect(results.first).to eq(user4)
+      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
+      user6.update(suspended_at: 1.day.ago, suspended_till: 1.year.from_now)
-    results = search_for(user4.username.upcase)
+    end
-    expect(results.size).to eq(1)
-    expect(results.first).to eq(user4)
 
-    # substrings
+    # this is a seriously expensive integration test,
-    # only staff members see suspended users in results
+    # re-creating this entire test db is too expensive reuse
-    results = search_for("mr")
+    it "operates correctly" do
-    expect(results.size).to eq(5)
+      # normal search
-    expect(results).not_to include(user6)
+      results = search_for(user1.name.split(" ").first)
-    expect(search_for("mr", searching_user: user1).size).to eq(5)
+      expect(results.size).to eq(1)
+      expect(results.first.username).to eq(user1.username)
 
-    results = search_for("mr", searching_user: admin)
+      # lower case
-    expect(results.size).to eq(6)
+      results = search_for(user1.name.split(" ").first.downcase)
-    expect(results).to include(user6)
+      expect(results.size).to eq(1)
-    expect(search_for("mr", searching_user: moderator).size).to eq(6)
+      expect(results.first).to eq(user1)
 
-    results = search_for(user1.username, searching_user: admin)
+      # username
-    expect(results.size).to eq(3)
+      results = search_for(user4.username)
+      expect(results.size).to eq(1)
+      expect(results.first).to eq(user4)
 
-    results = search_for("MR", searching_user: admin)
+      # case insensitive
-    expect(results.size).to eq(6)
+      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)
+      # substrings
-    expect(results.size).to eq(2)
+      # 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("mr", searching_user: admin)
-    results = search_for(user1.username, topic_id: topic.id)
+      expect(results.size).to eq(6)
-    expect(results.first).to eq(user1)
+      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)
+      results = search_for(user1.username, searching_user: admin)
-    expect(results[1]).to eq(user2)
+      expect(results.size).to eq(3)
 
-    results = search_for(user1.username, topic_id: topic3.id)
+      results = search_for("MR", searching_user: admin)
-    expect(results[1]).to eq(user5)
+      expect(results.size).to eq(6)
 
-    # When searching by name is enabled, it returns the record
+      results = search_for("MRB", searching_user: admin, limit: 2)
-    SiteSetting.enable_names = true
+      expect(results.size).to eq(2)
-    results = search_for("Tarantino")
-    expect(results.size).to eq(1)
 
-    results = search_for("coding")
+      # topic priority
-    expect(results.size).to eq(0)
+      results = search_for(user1.username, topic_id: topic.id)
+      expect(results.first).to eq(user1)
 
-    results = search_for("z")
+      results = search_for(user1.username, topic_id: topic2.id)
-    expect(results.size).to eq(0)
+      expect(results[1]).to eq(user2)
 
-    # When searching by name is disabled, it will not return the record
+      results = search_for(user1.username, topic_id: topic3.id)
-    SiteSetting.enable_names = false
+      expect(results[1]).to eq(user5)
-    results = search_for("Tarantino")
-    expect(results.size).to eq(0)
 
-    # find an exact match first
+      # When searching by name is enabled, it returns the record
-    results = search_for("mrB")
+      SiteSetting.enable_names = true
-    expect(results.first.username).to eq(user1.username)
+      results = search_for("Tarantino")
+      expect(results.size).to eq(1)
 
-    # don't return inactive users
+      results = search_for("coding")
-    results = search_for(inactive.username)
+      expect(results.size).to eq(0)
-    expect(results).to be_blank
 
-    # don't return staged users
+      results = search_for("z")
-    results = search_for(staged.username)
+      expect(results.size).to eq(0)
-    expect(results).to be_blank
 
-    results = search_for(staged.username, include_staged_users: true)
+      # When searching by name is disabled, it will not return the record
-    expect(results.first.username).to eq(staged.username)
+      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
+      # don't return inactive users
-    expect(results.map(&:username)).to eq(["mrpink", "mrorange"])
+      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

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");