FEATURE: prioritize group search order based on prefix match (#16093)

Our @mention user search prioritized users based on prefix matches.

So if searching for `sa` we will display `sam`, `asam` in that order

Previously, we did not prioritize group matches based on prefix. This change ensures better parity.

Implementation notes:

1. User search only prioritizes based on username prefix, not name prefix. TBD if we want to change that.
2. @mention on client side will show 0 group matches if we fill up all the spots with user matches. TBD if we want to unconditionally show the first / second group match.

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

app/controllers/users_controller.rb

@@ -1171,8 +1171,7 @@ class UsersController < ApplicationController
         end
       end
 
-      groups = Group.search_groups(term, groups: groups)
+      groups = Group.search_groups(term, groups: groups, sort: :auto)
-      groups = groups.order('groups.name asc')
 
       to_render[:groups] = groups.map do |m|
         { name: m.name, full_name: m.full_name }
@@ -1328,7 +1327,7 @@ class UsersController < ApplicationController
     end
 
     render json: success_json
-  rescue Discourse::InvalidAccess => e
+  rescue Discourse::InvalidAccess
     render_json_error(I18n.t("notification_level.#{@error_message}"))
   end
 

app/models/group.rb

@@ -560,12 +560,24 @@ class Group < ActiveRecord::Base
     lookup_group(name) || refresh_automatic_group!(name)
   end
 
-  def self.search_groups(name, groups: nil, custom_scope: {})
+  def self.search_groups(name, groups: nil, custom_scope: {}, sort: :none)
     groups ||= Group
 
-    groups.where(
+    relation = groups.where(
       "name ILIKE :term_like OR full_name ILIKE :term_like", term_like: "%#{name}%"
     )
+
+    if sort == :auto
+      prefix = "#{name.gsub("_", "\\_")}%"
+      relation = relation.reorder(
+        DB.sql_fragment(
+          "CASE WHEN name ILIKE :like OR full_name ILIKE :like THEN 0 ELSE 1 END ASC, name ASC",
+          like: prefix
+        )
+      )
+    end
+
+    relation
   end
 
   def self.lookup_group(name)

spec/models/group_spec.rb

@@ -923,21 +923,36 @@ describe Group do
   end
 
   describe '.search_groups' do
-    fab!(:group) { Fabricate(:group, name: 'tEsT_more_things', full_name: 'Abc something awesome') }
+
-    let(:messageable_group) { Fabricate(:group, name: "MessageableGroup", messageable_level: Group::ALIAS_LEVELS[:everyone]) }
+    def search_group_names(name)
+      Group.search_groups(name, sort: :auto).map(&:name)
+    end
 
     it 'should return the right groups' do
-      group
+      group_name = Fabricate(:group, name: 'tEsT_more_things', full_name: 'Abc something awesome').name
 
-      expect(Group.search_groups('te')).to eq([group])
+      expect(search_group_names('te')).to eq([group_name])
-      expect(Group.search_groups('TE')).to eq([group])
+      expect(search_group_names('TE')).to eq([group_name])
-      expect(Group.search_groups('es')).to eq([group])
+      expect(search_group_names('es')).to eq([group_name])
-      expect(Group.search_groups('ES')).to eq([group])
+      expect(search_group_names('ES')).to eq([group_name])
-      expect(Group.search_groups('ngs')).to eq([group])
+      expect(search_group_names('ngs')).to eq([group_name])
-      expect(Group.search_groups('sOmEthi')).to eq([group])
+      expect(search_group_names('sOmEthi')).to eq([group_name])
-      expect(Group.search_groups('abc')).to eq([group])
+      expect(search_group_names('abc')).to eq([group_name])
-      expect(Group.search_groups('sOmEthi')).to eq([group])
+      expect(search_group_names('sOmEthi')).to eq([group_name])
-      expect(Group.search_groups('test2')).to eq([])
+      expect(search_group_names('test2')).to eq([])
+    end
+
+    it "should prioritize prefix matches on group's name or fullname" do
+      Fabricate(:group, name: 'pears_11', full_name: 'fred apple')
+      Fabricate(:group, name: 'apples', full_name: 'jane orange')
+      Fabricate(:group, name: 'oranges2', full_name: 'nothing')
+      Fabricate(:group, name: 'oranges1', full_name: 'ms fred')
+
+      expect(search_group_names('ap')).to eq(['apples', 'pears_11'])
+      expect(search_group_names('fr')).to eq(['pears_11', 'oranges1'])
+      expect(search_group_names('oran')).to eq(['oranges1', 'oranges2', 'apples'])
+
+      expect(search_group_names('pearsX11')).to eq([])
     end
   end
 

spec/requests/users_controller_spec.rb

@@ -1788,7 +1788,7 @@ describe UsersController do
           inviter = Fabricate(:user, trust_level: 2)
           sign_in(inviter)
           invite = Fabricate(:invite, invited_by: inviter)
-          invited_user = Fabricate(:invited_user, invite: invite, user: invitee)
+          _invited_user = Fabricate(:invited_user, invite: invite, user: invitee)
 
           get "/u/#{inviter.username}/invited.json"
           expect(response.status).to eq(200)
@@ -1848,7 +1848,7 @@ describe UsersController do
 
           it 'allows admin to see invites' do
             inviter = Fabricate(:user, trust_level: 2)
-            admin = sign_in(Fabricate(:admin))
+            _admin = sign_in(Fabricate(:admin))
             invite = Fabricate(:invite, invited_by: inviter,  email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required])
 
             get "/u/#{inviter.username}/invited/pending.json"
@@ -1863,7 +1863,7 @@ describe UsersController do
 
         context 'without permission to see invite links' do
           it 'does not return invites' do
-            user = Fabricate(:user, trust_level: 2)
+            _user = Fabricate(:user, trust_level: 2)
             inviter = admin
             Fabricate(:invite, invited_by: inviter,  email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required])
 
@@ -3984,7 +3984,7 @@ describe UsersController do
           mentionable_level: Group::ALIAS_LEVELS[:everyone],
           messageable_level: Group::ALIAS_LEVELS[:nobody],
           visibility_level: Group.visibility_levels[:public],
-          name: 'aaa1'
+          name: 'aaa1bbb'
         )
       end
 
@@ -3993,7 +3993,7 @@ describe UsersController do
           mentionable_level: Group::ALIAS_LEVELS[:everyone],
           messageable_level: Group::ALIAS_LEVELS[:nobody],
           visibility_level: Group.visibility_levels[:logged_on_users],
-          name: 'aaa2'
+          name: 'bbb1aaa'
         )
       end
 
@@ -4002,7 +4002,7 @@ describe UsersController do
           mentionable_level: Group::ALIAS_LEVELS[:nobody],
           messageable_level: Group::ALIAS_LEVELS[:everyone],
           visibility_level: Group.visibility_levels[:logged_on_users],
-          name: 'aaa3'
+          name: 'ccc1aaa'
         )
       end
 
@@ -4011,7 +4011,7 @@ describe UsersController do
           mentionable_level: Group::ALIAS_LEVELS[:members_mods_and_admins],
           messageable_level: Group::ALIAS_LEVELS[:members_mods_and_admins],
           visibility_level: Group.visibility_levels[:members],
-          name: 'aaa4'
+          name: 'ddd1aaa'
         )
       end
 
@@ -4020,6 +4020,15 @@ describe UsersController do
           sign_in(user)
         end
 
+        it "correctly sorts on prefix" do
+          get "/u/search/users.json", params: { include_groups: "true", term: 'bbb' }
+
+          expect(response.status).to eq(200)
+          groups = response.parsed_body["groups"]
+
+          expect(groups.map { |g| g["name"] }).to eq(["bbb1aaa", "aaa1bbb"])
+        end
+
         it "does not search for groups if there is no term" do
           get "/u/search/users.json", params: { include_groups: "true" }
 
@@ -4612,7 +4621,7 @@ describe UsersController do
       it "creates a security key for the user" do
         simulate_localhost_webauthn_challenge
         create_second_factor_security_key
-        response_parsed = response.parsed_body
+        _response_parsed = response.parsed_body
 
         post "/u/register_second_factor_security_key.json", params: valid_security_key_create_post_data
 
@@ -4626,7 +4635,7 @@ describe UsersController do
       it "shows a security key error and does not create a key" do
         stub_as_dev_localhost
         create_second_factor_security_key
-        response_parsed = response.parsed_body
+        _response_parsed = response.parsed_body
 
         post "/u/register_second_factor_security_key.json", params: {
           id: "bad id",
@@ -4651,8 +4660,8 @@ describe UsersController do
       end
       context 'when user has a registered totp and security key' do
         before do
-          totp_second_factor = Fabricate(:user_second_factor_totp, user: user1)
+          _totp_second_factor = Fabricate(:user_second_factor_totp, user: user1)
-          security_key_second_factor = Fabricate(:user_security_key, user: user1, factor_type: UserSecurityKey.factor_types[:second_factor])
+          _security_key_second_factor = Fabricate(:user_security_key, user: user1, factor_type: UserSecurityKey.factor_types[:second_factor])
         end
 
         it 'should disable all totp and security keys' do