Merge branch '19706-allow-user-search-less-than-3-chars-when-scoped' into 'master'

Allow searching of users using less than 3 chars

See merge request gitlab-org/gitlab!79927

app/controllers/admin/users_controller.rb

@@ -16,7 +16,7 @@ class Admin::UsersController < Admin::ApplicationController
     return redirect_to admin_cohorts_path if params[:tab] == 'cohorts'
 
     @users = User.filter_items(params[:filter]).order_name_asc
-    @users = @users.search_with_secondary_emails(params[:search_query]) if params[:search_query].present?
+    @users = @users.search(params[:search_query], with_private_emails: true) if params[:search_query].present?
     @users = users_with_included_associations(@users)
     @users = @users.sort_by_attribute(@sort = params[:sort])
     @users = @users.page(params[:page])

app/finders/autocomplete/users_finder.rb

@@ -62,7 +62,7 @@ module Autocomplete
       find_users
         .active
         .reorder_by_name
-        .optionally_search(search)
+        .optionally_search(search, use_minimum_char_limit: use_minimum_char_limit)
         .where_not_in(skip_users)
         .limit_to_todo_authors(
           user: current_user,
@@ -99,6 +99,12 @@ module Autocomplete
       ActiveRecord::Associations::Preloader.new.preload(items, :status)
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def use_minimum_char_limit
+      return if project.blank? && group.blank?  # We return nil so that we use the default defined in the User model
+
+      false
+    end
   end
 end
 

app/models/member.rb

@@ -204,7 +204,7 @@ class Member < ApplicationRecord
 
   class << self
     def search(query)
-      joins(:user).merge(User.search(query))
+      joins(:user).merge(User.search(query, use_minimum_char_limit: false))
     end
 
     def search_invite_email(query)

app/models/user.rb

@@ -668,7 +668,8 @@ class User < ApplicationRecord
 
       sanitized_order_sql = Arel.sql(sanitize_sql_array([order, query: query]))
 
-      scope = options[:with_private_emails] ? search_with_secondary_emails(query) : search_with_public_emails(query)
+      scope = options[:with_private_emails] ? with_primary_or_secondary_email(query) : with_public_email(query)
+      scope = scope.or(search_by_name_or_username(query, use_minimum_char_limit: options[:use_minimum_char_limit]))
 
       scope.reorder(sanitized_order_sql, :name)
     end
@@ -685,50 +686,32 @@ class User < ApplicationRecord
       reorder(:name)
     end
 
-    def search_with_public_emails(query)
-      return none if query.blank?
-
-      query = query.downcase
+    # searches user by given pattern
+    # it compares name and username fields with given pattern
+    # This method uses ILIKE on PostgreSQL.
+    def search_by_name_or_username(query, use_minimum_char_limit: nil)
+      use_minimum_char_limit = user_search_minimum_char_limit if use_minimum_char_limit.nil?
 
       where(
-        fuzzy_arel_match(:name, query, use_minimum_char_limit: user_search_minimum_char_limit)
-          .or(fuzzy_arel_match(:username, query, use_minimum_char_limit: user_search_minimum_char_limit))
-          .or(arel_table[:public_email].eq(query))
+        fuzzy_arel_match(:name, query, use_minimum_char_limit: use_minimum_char_limit)
+          .or(fuzzy_arel_match(:username, query, use_minimum_char_limit: use_minimum_char_limit))
       )
     end
 
-    def search_without_secondary_emails(query)
-      return none if query.blank?
-
-      query = query.downcase
-
-      where(
-        fuzzy_arel_match(:name, query, lower_exact_match: true)
-          .or(fuzzy_arel_match(:username, query, lower_exact_match: true))
-          .or(arel_table[:email].eq(query))
-      )
+    def with_public_email(email_address)
+      where(public_email: email_address)
     end
 
-    # searches user by given pattern
-    # it compares name, email, username fields and user's secondary emails with given pattern
-    # This method uses ILIKE on PostgreSQL.
-
-    def search_with_secondary_emails(query)
-      return none if query.blank?
-
-      query = query.downcase
-
+    def with_primary_or_secondary_email(email_address)
       email_table = Email.arel_table
       matched_by_email_user_id = email_table
         .project(email_table[:user_id])
-        .where(email_table[:email].eq(query))
+        .where(email_table[:email].eq(email_address))
         .take(1) # at most 1 record as there is a unique constraint
 
       where(
-        fuzzy_arel_match(:name, query, use_minimum_char_limit: user_search_minimum_char_limit)
-          .or(fuzzy_arel_match(:username, query, use_minimum_char_limit: user_search_minimum_char_limit))
-          .or(arel_table[:email].eq(query))
-          .or(arel_table[:id].eq(matched_by_email_user_id))
+        arel_table[:email].eq(email_address)
+        .or(arel_table[:id].eq(matched_by_email_user_id))
       )
     end
 

app/models/users_star_project.rb

@@ -32,7 +32,7 @@ class UsersStarProject < ApplicationRecord
     end
 
     def search(query)
-      joins(:user).merge(User.search(query))
+      joins(:user).merge(User.search(query, use_minimum_char_limit: false))
     end
   end
 end

lib/api/helpers/members_helpers.rb

@@ -23,7 +23,7 @@ module API
       def retrieve_members(source, params:, deep: false)
         members = deep ? find_all_members(source) : source_members(source).connected_to_user
         members = members.includes(:user)
-        members = members.references(:user).merge(User.search(params[:query])) if params[:query].present?
+        members = members.references(:user).merge(User.search(params[:query], use_minimum_char_limit: false)) if params[:query].present?
         members = members.where(user_id: params[:user_ids]) if params[:user_ids].present?
         members
       end

spec/finders/autocomplete/users_finder_spec.rb

@@ -7,15 +7,16 @@ RSpec.describe Autocomplete::UsersFinder do
   # https://gitlab.com/gitlab-org/gitlab/-/issues/21432
 
   describe '#execute' do
-    let!(:user1) { create(:user, username: 'johndoe') }
-    let!(:user2) { create(:user, :blocked, username: 'notsorandom') }
-    let!(:external_user) { create(:user, :external) }
-    let!(:omniauth_user) { create(:omniauth_user, provider: 'twitter', extern_uid: '123456') }
+    let_it_be(:user1) { create(:user, name: 'zzzzzname', username: 'johndoe') }
+    let_it_be(:user2) { create(:user, :blocked, username: 'notsorandom') }
+    let_it_be(:external_user) { create(:user, :external) }
+    let_it_be(:omniauth_user) { create(:omniauth_user, provider: 'twitter', extern_uid: '123456') }
+
     let(:current_user) { create(:user) }
     let(:params) { {} }
 
-    let(:project) { nil }
-    let(:group) { nil }
+    let_it_be(:project) { nil }
+    let_it_be(:group) { nil }
 
     subject { described_class.new(params: params, current_user: current_user, project: project, group: group).execute.to_a }
 
@@ -26,7 +27,7 @@ RSpec.describe Autocomplete::UsersFinder do
     end
 
     context 'when project passed' do
-      let(:project) { create(:project) }
+      let_it_be(:project) { create(:project) }
 
       it { is_expected.to match_array([project.first_owner]) }
 
@@ -43,16 +44,36 @@ RSpec.describe Autocomplete::UsersFinder do
           it { is_expected.to match_array([project.first_owner]) }
         end
       end
+
+      context 'searching with less than 3 characters' do
+        let(:params) { { search: 'zz' } }
+
+        before do
+          project.add_guest(user1)
+        end
+
+        it 'allows partial matches' do
+          expect(subject).to contain_exactly(user1)
+        end
+      end
     end
 
     context 'when group passed and project not passed' do
-      let(:group) { create(:group, :public) }
+      let_it_be(:group) { create(:group, :public) }
 
-      before do
+      before_all do
         group.add_users([user1], GroupMember::DEVELOPER)
       end
 
       it { is_expected.to match_array([user1]) }
+
+      context 'searching with less than 3 characters' do
+        let(:params) { { search: 'zz' } }
+
+        it 'allows partial matches' do
+          expect(subject).to contain_exactly(user1)
+        end
+      end
     end
 
     context 'when passed a subgroup' do
@@ -76,6 +97,14 @@ RSpec.describe Autocomplete::UsersFinder do
       let(:params) { { search: 'johndoe' } }
 
       it { is_expected.to match_array([user1]) }
+
+      context 'searching with less than 3 characters' do
+        let(:params) { { search: 'zz' } }
+
+        it 'does not allow partial matches' do
+          expect(subject).to be_empty
+        end
+      end
     end
 
     context 'when filtered by skip_users' do

spec/models/user_spec.rb

@@ -2612,6 +2612,12 @@ RSpec.describe User do
       it 'returns users with a exact matching name shorter than 3 chars regardless of the casing' do
         expect(described_class.search(user3.name.upcase)).to eq([user3])
       end
+
+      context 'when use_minimum_char_limit is false' do
+        it 'returns users with a partially matching name' do
+          expect(described_class.search('u', use_minimum_char_limit: false)).to eq([user3, user, user2])
+        end
+      end
     end
 
     describe 'email matching' do
@@ -2671,204 +2677,20 @@ RSpec.describe User do
       it 'returns users with a exact matching username shorter than 3 chars regardless of the casing' do
         expect(described_class.search(user3.username.upcase)).to eq([user3])
       end
-    end
-
-    it 'returns no matches for an empty string' do
-      expect(described_class.search('')).to be_empty
-    end
-
-    it 'returns no matches for nil' do
-      expect(described_class.search(nil)).to be_empty
-    end
-  end
-
-  describe '.search_without_secondary_emails' do
-    let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) }
-    let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) }
-    let_it_be(:email) { create(:email, user: another_user, email: 'alias@example.com') }
 
-    it 'returns users with a matching name' do
-      expect(described_class.search_without_secondary_emails(user.name)).to eq([user])
-    end
-
-    it 'returns users with a partially matching name' do
-      expect(described_class.search_without_secondary_emails(user.name[0..2])).to eq([user])
-    end
-
-    it 'returns users with a matching name regardless of the casing' do
-      expect(described_class.search_without_secondary_emails(user.name.upcase)).to eq([user])
-    end
-
-    it 'returns users with a matching email' do
-      expect(described_class.search_without_secondary_emails(user.email)).to eq([user])
-    end
-
-    it 'does not return users with a partially matching email' do
-      expect(described_class.search_without_secondary_emails(user.email[1...-1])).to be_empty
-    end
-
-    it 'returns users with a matching email regardless of the casing' do
-      expect(described_class.search_without_secondary_emails(user.email.upcase)).to eq([user])
-    end
-
-    it 'returns users with a matching username' do
-      expect(described_class.search_without_secondary_emails(user.username)).to eq([user])
-    end
-
-    it 'returns users with a partially matching username' do
-      expect(described_class.search_without_secondary_emails(user.username[0..2])).to eq([user])
-    end
-
-    it 'returns users with a matching username regardless of the casing' do
-      expect(described_class.search_without_secondary_emails(user.username.upcase)).to eq([user])
-    end
-
-    it 'does not return users with a matching whole secondary email' do
-      expect(described_class.search_without_secondary_emails(email.email)).not_to include(email.user)
-    end
-
-    it 'does not return users with a matching part of secondary email' do
-      expect(described_class.search_without_secondary_emails(email.email[1...-1])).to be_empty
-    end
-
-    it 'returns no matches for an empty string' do
-      expect(described_class.search_without_secondary_emails('')).to be_empty
-    end
-
-    it 'returns no matches for nil' do
-      expect(described_class.search_without_secondary_emails(nil)).to be_empty
-    end
-  end
-
-  describe '.search_with_public_emails' do
-    let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) }
-    let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) }
-    let_it_be(:public_email) do
-      create(:email, :confirmed, user: another_user, email: 'alias@example.com').tap do |email|
-        another_user.update!(public_email: email.email)
+      context 'when use_minimum_char_limit is false' do
+        it 'returns users with a partially matching username' do
+          expect(described_class.search('se', use_minimum_char_limit: false)).to eq([user3, user, user2])
+        end
       end
     end
 
-    let_it_be(:secondary_email) do
-      create(:email, :confirmed, user: another_user, email: 'secondary@example.com')
-    end
-
-    it 'returns users with a matching name' do
-      expect(described_class.search_with_public_emails(user.name)).to match_array([user])
-    end
-
-    it 'returns users with a partially matching name' do
-      expect(described_class.search_with_public_emails(user.name[0..2])).to match_array([user])
-    end
-
-    it 'returns users with a matching name regardless of the casing' do
-      expect(described_class.search_with_public_emails(user.name.upcase)).to match_array([user])
-    end
-
-    it 'returns users with a matching public email' do
-      expect(described_class.search_with_public_emails(another_user.public_email)).to match_array([another_user])
-    end
-
-    it 'does not return users with a partially matching email' do
-      expect(described_class.search_with_public_emails(another_user.public_email[1...-1])).to be_empty
-    end
-
-    it 'returns users with a matching email regardless of the casing' do
-      expect(described_class.search_with_public_emails(another_user.public_email.upcase)).to match_array([another_user])
-    end
-
-    it 'returns users with a matching username' do
-      expect(described_class.search_with_public_emails(user.username)).to match_array([user])
-    end
-
-    it 'returns users with a partially matching username' do
-      expect(described_class.search_with_public_emails(user.username[0..2])).to match_array([user])
-    end
-
-    it 'returns users with a matching username regardless of the casing' do
-      expect(described_class.search_with_public_emails(user.username.upcase)).to match_array([user])
-    end
-
-    it 'does not return users with a matching whole private email' do
-      expect(described_class.search_with_public_emails(user.email)).not_to include(user)
-    end
-
-    it 'does not return users with a matching whole private email' do
-      expect(described_class.search_with_public_emails(secondary_email.email)).to be_empty
-    end
-
-    it 'does not return users with a matching part of secondary email' do
-      expect(described_class.search_with_public_emails(secondary_email.email[1...-1])).to be_empty
-    end
-
-    it 'does not return users with a matching part of private email' do
-      expect(described_class.search_with_public_emails(user.email[1...-1])).to be_empty
-    end
-
-    it 'returns no matches for an empty string' do
-      expect(described_class.search_with_public_emails('')).to be_empty
-    end
-
-    it 'returns no matches for nil' do
-      expect(described_class.search_with_public_emails(nil)).to be_empty
-    end
-  end
-
-  describe '.search_with_secondary_emails' do
-    let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) }
-    let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) }
-    let_it_be(:email) { create(:email, user: another_user, email: 'alias@example.com') }
-
-    it 'returns users with a matching name' do
-      expect(described_class.search_with_secondary_emails(user.name)).to eq([user])
-    end
-
-    it 'returns users with a partially matching name' do
-      expect(described_class.search_with_secondary_emails(user.name[0..2])).to eq([user])
-    end
-
-    it 'returns users with a matching name regardless of the casing' do
-      expect(described_class.search_with_secondary_emails(user.name.upcase)).to eq([user])
-    end
-
-    it 'returns users with a matching email' do
-      expect(described_class.search_with_secondary_emails(user.email)).to eq([user])
-    end
-
-    it 'does not return users with a partially matching email' do
-      expect(described_class.search_with_secondary_emails(user.email[1...-1])).to be_empty
-    end
-
-    it 'returns users with a matching email regardless of the casing' do
-      expect(described_class.search_with_secondary_emails(user.email.upcase)).to eq([user])
-    end
-
-    it 'returns users with a matching username' do
-      expect(described_class.search_with_secondary_emails(user.username)).to eq([user])
-    end
-
-    it 'returns users with a partially matching username' do
-      expect(described_class.search_with_secondary_emails(user.username[0..2])).to eq([user])
-    end
-
-    it 'returns users with a matching username regardless of the casing' do
-      expect(described_class.search_with_secondary_emails(user.username.upcase)).to eq([user])
-    end
-
-    it 'returns users with a matching whole secondary email' do
-      expect(described_class.search_with_secondary_emails(email.email)).to eq([email.user])
-    end
-
-    it 'does not return users with a matching part of secondary email' do
-      expect(described_class.search_with_secondary_emails(email.email[1...-1])).to be_empty
-    end
-
     it 'returns no matches for an empty string' do
-      expect(described_class.search_with_secondary_emails('')).to be_empty
+      expect(described_class.search('')).to be_empty
     end
 
     it 'returns no matches for nil' do
-      expect(described_class.search_with_secondary_emails(nil)).to be_empty
+      expect(described_class.search(nil)).to be_empty
     end
   end