Applied comments from code reviewer

Adjusted newly introduced queries.

app/models/concerns/group_member_sorting.rb

+# frozen_string_literal: true
+
+module GroupMemberSorting
+  extend ActiveSupport::Concern
+
+  class_methods do
+    include SortingHelper
+
+    def sorting_for(sort_value)
+      sort_value || sort_value_name
+    end
+  end
+end

app/models/members/group_member.rb

@@ -3,6 +3,7 @@
 class GroupMember < Member
   include FromUnion
   include CreatedAtFilterable
+  include GroupMemberSorting
 
   SOURCE_TYPE = 'Namespace'
 
@@ -22,6 +23,7 @@ class GroupMember < Member
   scope :of_ldap_type, -> { where(ldap: true) }
   scope :count_users_by_group_id, -> { group(:source_id).count }
   scope :with_user, -> (user) { where(user: user) }
+  scope :get_user_id, -> { pluck(:user_id) }
 
   after_create :update_two_factor_requirement, unless: :invite?
   after_destroy :update_two_factor_requirement, unless: :invite?

doc/api/members.md

@@ -241,6 +241,9 @@ Unlike other API endpoints, billable members is updated once per day at 12:00 UT
 
 This function takes [pagination](README.md#pagination) parameters `page` and `per_page` to restrict the list of users.
 
+[Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/262875) in GitLab 13.7, the `search` and `sort`
+parameters allows to search for billable group members by their name as well as specifying result sorting.
+
 ```plaintext
 GET /groups/:id/billable_members
 ```
@@ -248,6 +251,8 @@ GET /groups/:id/billable_members
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `search`  | string         | no  | A query string to search for group members by their name |
+| `sort`    | string         | no  | A query string specifying the sorting attribute and order |
 
 ```shell
 curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/billable_members"

ee/app/models/ee/group.rb

@@ -448,6 +448,19 @@ module EE
       )
     end
 
+    def billed_user_ids_for(search_term, order_by)
+      if search_term.present?
+        ::GroupMember
+          .with_user(billed_user_ids)
+          .search(search_term)
+          .sort_by_attribute(::GroupMember.sorting_for(order_by))
+          .get_user_id
+          .uniq
+      else
+        billed_user_ids.sort
+      end
+    end
+
     private
 
     def custom_project_templates_group_allowed

ee/lib/ee/api/helpers/members_helpers.rb

@@ -6,7 +6,6 @@ module EE
       module MembersHelpers
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
-        include ::SortingHelper
 
         prepended do
           params :optional_filter_params_ee do
@@ -78,29 +77,23 @@ module EE
         end
 
         def paginate_billable_from_user_ids(user_ids, params = {})
-          sorted_ids = params[:search].present? ? user_ids : user_ids.sort
+          paginated = paginate(::Kaminari.paginate_array(user_ids))
 
-          paginated = paginate(::Kaminari.paginate_array(sorted_ids))
-
-          users_as_hash = ::User.id_in(paginated).index_by(&:id)
+          users_as_hash = ::User
+            .id_in(paginated)
+            .sort_by_attribute(::GroupMember.sorting_for(params[:sort]))
+            .index_by(&:id)
 
           # map! ensures same paginatable array is manipulated
           # instead of creating a new non-paginatable array
           paginated.map! { |user_id| users_as_hash[user_id] }
         end
 
-        def group_billed_user_ids_for(group, params)
-          if params[:search].present?
-            sorting = params[:sort] || sort_value_name
+        class << self
+          include ::SortingHelper
 
-            ::GroupMember
-              .with_user(group.billed_user_ids)
-              .search(params[:search])
-              .sort_by_attribute(sorting)
-              .map(&:user_id)
-              .uniq
-          else
-            group.billed_user_ids
+          def member_sort_options
+            member_sort_options_hash.keys
           end
         end
       end

ee/lib/ee/api/members.rb

@@ -47,8 +47,8 @@ module EE
           end
           params do
             use :pagination
-            optional :search, type: String, desc: 'The name of the subscribed member'
-            optional :sort, type: String, desc: 'The sorting option'
+            optional :search, type: String, desc: 'The exact name of the subscribed member'
+            optional :sort, type: String, desc: 'The sorting option', values: Helpers::MembersHelpers.member_sort_options, default: 'name_asc'
           end
           get ":id/billable_members" do
             group = find_group!(params[:id])
@@ -58,7 +58,7 @@ module EE
             bad_request!(nil) if group.subgroup?
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
-            users = paginate_billable_from_user_ids(group_billed_user_ids_for(group, params), params)
+            users = paginate_billable_from_user_ids(group.billed_user_ids_for(params[:search], params[:sort]), params)
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end

ee/spec/lib/ee/api/helpers/members_helpers_spec.rb

@@ -48,60 +48,15 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
       allow(members_helpers).to receive(:request).and_return(double(:request, url: ''))
     end
 
-    it 'returns paginated User array in asc order' do
-      results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse)
-
-      expect(results).to all be_a(User)
-      expect(results.size).to eq(per_page)
-      expect(results.map { |result| result.id }).to eq(user_ids.first(2))
-    end
-
     context 'when page is 2' do
       let(:page) { 2 }
 
       it 'returns User as paginated array' do
-        results = members_helpers.paginate_billable_from_user_ids(user_ids.reverse)
+        results = members_helpers.paginate_billable_from_user_ids(user_ids)
 
         expect(results.size).to eq(1)
         expect(results.map { |result| result.id }).to contain_exactly(user_ids.last)
       end
     end
   end
-
-  describe '#group_billed_user_ids_for' do
-    let_it_be(:group) { create(:group) }
-    let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
-    let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
-    let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
-    let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
-    let(:params) { {} }
-
-    subject { members_helpers.group_billed_user_ids_for(group, params).to_a }
-
-    context 'when a search parameter is present' do
-      let(:params) { { search: 'John', sort: sort } }
-
-      context 'when a sorting parameter is provided (eg name descending)' do
-        let(:sort) { 'name_desc' }
-
-        it 'sorts results accordingly' do
-          expect(subject).to eq([gm_2, gm_3].map(&:user_id))
-        end
-      end
-
-      context 'when a sorting parameter is not provided' do
-        let(:sort) { nil }
-
-        it 'sorts results by name ascending' do
-          expect(subject).to eq([gm_3, gm_2].map(&:user_id))
-        end
-      end
-    end
-
-    context 'when a search parameter is not present' do
-      it 'returns the expected group member user ids' do
-        expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id))
-      end
-    end
-  end
 end

ee/spec/models/group_spec.rb

@@ -1223,4 +1223,49 @@ RSpec.describe Group do
       end
     end
   end
+
+  describe '#billed_user_ids_for' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:gm_1) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
+    let_it_be(:gm_2) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
+    let_it_be(:gm_3) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
+    let_it_be(:gm_4) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
+    let(:search_term) { nil }
+    let(:order_by) { nil }
+
+    subject { group.billed_user_ids_for(search_term, order_by) }
+
+    context 'when a search parameter is present' do
+      let(:search_term) { 'John' }
+      let(:order_by) { sort }
+
+      context 'when a sorting parameter is provided (eg name descending)' do
+        let(:sort) { 'name_desc' }
+
+        it 'sorts results accordingly' do
+          expect(subject).to eq([gm_2, gm_3].map(&:user_id))
+        end
+      end
+
+      context 'when a sorting parameter is not provided' do
+        let(:sort) { nil }
+
+        it 'sorts results by name ascending' do
+          expect(subject).to eq([gm_3, gm_2].map(&:user_id))
+        end
+      end
+    end
+
+    context 'when a search parameter is not present' do
+      it 'returns the expected group member user ids' do
+        expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id))
+      end
+
+      it 'returns user array in asc order' do
+        allow(group).to receive(:billed_user_ids).and_return([gm_3, gm_2, gm_4, gm_1].map(&:user_id))
+
+        expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id))
+      end
+    end
+  end
 end

ee/spec/requests/api/members_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe API::Members do
+  include EE::API::Helpers::MembersHelpers
+
   context 'group members endpoints for group with minimal access feature' do
     let_it_be(:group) { create(:group) }
     let_it_be(:minimal_access_member) { create(:group_member, :minimal_access, source: group) }
@@ -388,12 +390,31 @@ RSpec.describe API::Members do
       end
 
       context 'with search and sort params provided' do
-        let(:params) { { search: 'Scott', sort: 'name_desc' } }
+        it 'accepts only sorting options defined in a list' do
+          EE::API::Helpers::MembersHelpers.member_sort_options.each do |sorting|
+            get api(url, owner), params: { search: 'name', sort: sorting }
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
 
-        it 'returns the relevant billable users' do
-          subject
+        it 'does not accept query string not defined in a list' do
+          defined_query_strings = EE::API::Helpers::MembersHelpers.member_sort_options
+          sorting = 'fake_sorting'
+
+          get api(url, owner), params: { search: 'name', sort: sorting }
 
-          expect_paginated_array_response(*[linked_group_user, nested_user].map(&:id))
+          expect(defined_query_strings).not_to include(sorting)
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        context 'when a specific sorting is provided' do
+          let(:params) { { search: 'Scott', sort: 'name_desc' } }
+
+          it 'returns the relevant billable users' do
+            subject
+
+            expect_paginated_array_response(*[linked_group_user, nested_user].map(&:id))
+          end
         end
       end
     end

spec/models/concerns/group_member_sorting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GroupMemberSorting do
+  describe '.sorting_for' do
+    let(:sort_value) { nil }
+    let(:sortable_class) { GroupMember }
+
+    subject { sortable_class.sorting_for(sort_value) }
+
+    context 'sorting is not passed in' do
+      it 'returns name_asc' do
+        expect(subject).to eq('name_asc')
+      end
+    end
+
+    context 'sorting is passed in' do
+      let(:sort_value) { 'name_desc' }
+
+      it 'returns that sorting' do
+        expect(subject).to eq(sort_value)
+      end
+    end
+  end
+end