FEATURE: mention in secure category to prioritize groups

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.

diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6
index 4ef3193..b759bd6 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 753460f..1cb7023 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 c7853e8..da40052 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 7b9f0b9..85c042e 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

[... diff too long, it was truncated ...]

GitHub sha: f7809207

2 Likes

We should use the AUTO_GROUPS constants here instead of hard coding the ids.

1 Like

Nice use of pretender!

Is _categoryId private for a reason? I always try to be careful about code relying on stuff like that.

1 Like

DEV: followup on secure category mention

Thanks for the review, I addressed both concerns in the followup commit.

1 Like