diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b329dd53cf2..f79e297603e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1171,8 +1171,7 @@ class UsersController < ApplicationController end end - groups = Group.search_groups(term, groups: groups) - groups = groups.order('groups.name asc') + groups = Group.search_groups(term, groups: groups, sort: :auto) 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 diff --git a/app/models/group.rb b/app/models/group.rb index 3c64ab49d14..e6dc796f506 100644 --- a/app/models/group.rb +++ b/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) diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index dff03766023..cdc6f6243cf 100644 --- a/spec/models/group_spec.rb +++ b/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(Group.search_groups('TE')).to eq([group]) - expect(Group.search_groups('es')).to eq([group]) - expect(Group.search_groups('ES')).to eq([group]) - expect(Group.search_groups('ngs')).to eq([group]) - expect(Group.search_groups('sOmEthi')).to eq([group]) - expect(Group.search_groups('abc')).to eq([group]) - expect(Group.search_groups('sOmEthi')).to eq([group]) - expect(Group.search_groups('test2')).to eq([]) + expect(search_group_names('te')).to eq([group_name]) + expect(search_group_names('TE')).to eq([group_name]) + expect(search_group_names('es')).to eq([group_name]) + expect(search_group_names('ES')).to eq([group_name]) + expect(search_group_names('ngs')).to eq([group_name]) + expect(search_group_names('sOmEthi')).to eq([group_name]) + expect(search_group_names('abc')).to eq([group_name]) + expect(search_group_names('sOmEthi')).to eq([group_name]) + 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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 906db0a0b40..78de7a391b8 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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) - security_key_second_factor = Fabricate(:user_security_key, user: user1, factor_type: UserSecurityKey.factor_types[:second_factor]) + _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]) end it 'should disable all totp and security keys' do