From b694cfae56da0127deefee0796ad2eb3e61017b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com>
Date: Tue, 24 Nov 2020 13:33:10 +0100
Subject: [PATCH 1/5] Updated group member search

Added search and sort params.
Updated rspec accordingly.
---
 ee/lib/ee/api/helpers/members_helpers.rb      | 22 +++++++++-
 ee/lib/ee/api/members.rb                      |  4 +-
 .../ee/api/helpers/members_helpers_spec.rb    | 41 ++++++++++++++++++-
 ee/spec/requests/api/members_spec.rb          | 27 ++++++++++--
 4 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb
index f805205cda8ab87c..726b727b4a1565de 100644
--- a/ee/lib/ee/api/helpers/members_helpers.rb
+++ b/ee/lib/ee/api/helpers/members_helpers.rb
@@ -6,6 +6,7 @@ module Helpers
       module MembersHelpers
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
+        include ::SortingHelper
 
         prepended do
           params :optional_filter_params_ee do
@@ -76,8 +77,10 @@ def log_audit_event(member)
           ).for_member(member).security_event
         end
 
-        def paginate_billable_from_user_ids(user_ids)
-          paginated = paginate(::Kaminari.paginate_array(user_ids.sort))
+        def paginate_billable_from_user_ids(user_ids, params = {})
+          sorted_ids = params[:search].present? ? user_ids : user_ids.sort
+
+          paginated = paginate(::Kaminari.paginate_array(sorted_ids))
 
           users_as_hash = ::User.id_in(paginated).index_by(&:id)
 
@@ -85,6 +88,21 @@ def paginate_billable_from_user_ids(user_ids)
           # 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
+
+            ::GroupMember
+              .with_user(group.billed_user_ids)
+              .search(params[:search])
+              .sort_by_attribute(sorting)
+              .map(&:user_id)
+              .uniq
+          else
+            group.billed_user_ids
+          end
+        end
       end
     end
   end
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index 1277cf81f98c34a5..516826b2b39eac0f 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -47,6 +47,8 @@ module Members
           end
           params do
             use :pagination
+            optional :search, type: String, desc: 'The name of the subscribed member'
+            optional :sort, type: String, desc: 'The sorting option'
           end
           get ":id/billable_members" do
             group = find_group!(params[:id])
@@ -56,7 +58,7 @@ module Members
             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)
+            users = paginate_billable_from_user_ids(group_billed_user_ids_for(group, params), params)
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end
diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
index 2fcf27f4cc5340dc..2cb3e1da5a806586 100644
--- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
+++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe EE::API::Helpers::MembersHelpers do
-  subject(:members_helpers) { Class.new.include(described_class).new }
+  let(:members_helpers) { Class.new.include(described_class).new }
 
   before do
     allow(members_helpers).to receive(:current_user).and_return(create(:user))
@@ -21,6 +21,8 @@
   end
 
   describe '#log_audit_event' do
+    subject { members_helpers }
+
     it_behaves_like 'creates security_event', 'group' do
       let(:source) { create(:group) }
       let(:member) { create(:group_member, :owner, group: source, user: create(:user)) }
@@ -65,4 +67,41 @@
       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
diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb
index 97229f0a2f00c8e9..646fbe26747a0ef0 100644
--- a/ee/spec/requests/api/members_spec.rb
+++ b/ee/spec/requests/api/members_spec.rb
@@ -339,7 +339,7 @@
       end
     end
 
-    let_it_be(:nested_user) { create(:user) }
+    let_it_be(:nested_user) { create(:user, name: 'Scott Anderson') }
     let_it_be(:nested_group) do
       create(:group, parent: group) do |nested_group|
         nested_group.add_developer(nested_user)
@@ -347,9 +347,10 @@
     end
 
     let(:url) { "/groups/#{group.id}/billable_members" }
+    let(:params) { {} }
 
     subject do
-      get api(url, owner)
+      get api(url, owner), params: params
       json_response
     end
 
@@ -361,7 +362,7 @@
         end
       end
 
-      let!(:linked_group_user) { create(:user) }
+      let!(:linked_group_user) { create(:user, name: 'Scott McNeil') }
       let!(:linked_group) do
         create(:group) do |linked_group|
           linked_group.add_developer(linked_group_user)
@@ -375,6 +376,26 @@
 
         expect_paginated_array_response(*[owner, maintainer, nested_user, project_user, linked_group_user].map(&:id))
       end
+
+      context 'with seach params provided' do
+        let(:params) { { search: nested_user.name } }
+
+        it 'returns the relevant billable users' do
+          subject
+
+          expect_paginated_array_response([nested_user.id])
+        end
+      end
+
+      context 'with search and sort params 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
 
     context 'when feature is disabled' do
-- 
GitLab


From fcf75d779fc5bae51f547b2c6ee6ae7266e8fe03 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com>
Date: Mon, 30 Nov 2020 16:29:22 +0100
Subject: [PATCH 2/5] Applied comments from code reviewer

Adjusted newly introduced queries.
---
 app/models/concerns/group_member_sorting.rb   | 13 +++++
 app/models/members/group_member.rb            |  2 +
 doc/api/members.md                            |  5 ++
 ee/app/models/ee/group.rb                     | 13 +++++
 ee/lib/ee/api/helpers/members_helpers.rb      | 25 ++++------
 ee/lib/ee/api/members.rb                      |  6 +--
 .../ee/api/helpers/members_helpers_spec.rb    | 47 +------------------
 ee/spec/models/group_spec.rb                  | 45 ++++++++++++++++++
 ee/spec/requests/api/members_spec.rb          | 29 ++++++++++--
 .../concerns/group_member_sorting_spec.rb     | 26 ++++++++++
 10 files changed, 142 insertions(+), 69 deletions(-)
 create mode 100644 app/models/concerns/group_member_sorting.rb
 create mode 100644 spec/models/concerns/group_member_sorting_spec.rb

diff --git a/app/models/concerns/group_member_sorting.rb b/app/models/concerns/group_member_sorting.rb
new file mode 100644
index 0000000000000000..768112b17dfbe908
--- /dev/null
+++ b/app/models/concerns/group_member_sorting.rb
@@ -0,0 +1,13 @@
+# 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
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 2bbcdbbe5ced78a6..e3645891e7bf943e 100644
--- a/app/models/members/group_member.rb
+++ b/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?
diff --git a/doc/api/members.md b/doc/api/members.md
index fe7fffeab5df57be..5b08e9cdcd981634 100644
--- a/doc/api/members.md
+++ b/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"
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 96272ecf3daf7470..6cbb96fb1d6f3eeb 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -448,6 +448,19 @@ def releases_percentage
       )
     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
diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb
index 726b727b4a1565de..2e5dc895bab27960 100644
--- a/ee/lib/ee/api/helpers/members_helpers.rb
+++ b/ee/lib/ee/api/helpers/members_helpers.rb
@@ -6,7 +6,6 @@ module Helpers
       module MembersHelpers
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
-        include ::SortingHelper
 
         prepended do
           params :optional_filter_params_ee do
@@ -78,29 +77,23 @@ def log_audit_event(member)
         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
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index 516826b2b39eac0f..5cf0588bf26680c0 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -47,8 +47,8 @@ module Members
           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 Members
             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
diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
index 2cb3e1da5a806586..7cd29e4ab6d2a442 100644
--- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
+++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
@@ -48,60 +48,15 @@
       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
diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb
index b32eaf21b2106fb4..f36ed649e1a123da 100644
--- a/ee/spec/models/group_spec.rb
+++ b/ee/spec/models/group_spec.rb
@@ -1223,4 +1223,49 @@
       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
diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb
index 646fbe26747a0ef0..2048614b1b79e8b1 100644
--- a/ee/spec/requests/api/members_spec.rb
+++ b/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 @@
       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
diff --git a/spec/models/concerns/group_member_sorting_spec.rb b/spec/models/concerns/group_member_sorting_spec.rb
new file mode 100644
index 0000000000000000..a54476b535834f76
--- /dev/null
+++ b/spec/models/concerns/group_member_sorting_spec.rb
@@ -0,0 +1,26 @@
+# 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
-- 
GitLab


From c0920003c6b1823b0ae00ee4f283ad0b481b8a57 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com>
Date: Mon, 30 Nov 2020 21:39:13 +0100
Subject: [PATCH 3/5] Applied suggestion from reviewer

Did some refactoring in Group class.
---
 ee/app/models/ee/group.rb                | 56 +++++++++++++++---------
 ee/lib/ee/api/helpers/members_helpers.rb | 13 ------
 ee/lib/ee/api/members.rb                 |  2 +-
 ee/spec/models/group_spec.rb             | 35 ++++++++-------
 4 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 6cbb96fb1d6f3eeb..db4c55ab379f704e 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -299,23 +299,43 @@ def billable_members_count(requested_hosted_plan = nil)
     # We are plucking the user_ids from the "Members" table in an array and
     # converting the array of user_ids to a Set which will have unique user_ids.
     def billed_user_ids(requested_hosted_plan = nil)
+      billed_user_members.pluck(:user_id).to_set
+    end
+
+    def billed_user_members(requested_hosted_plan = nil)
       if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD)
-        strong_memoize(:gold_billed_user_ids) do
-          (billed_group_members.non_guests.distinct.pluck(:user_id) +
-          billed_project_members.non_guests.distinct.pluck(:user_id) +
-          billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) +
-          billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set
+        strong_memoize(:gold_billed_users) do
+          gold_billed_members
         end
       else
-        strong_memoize(:non_gold_billed_user_ids) do
-          (billed_group_members.distinct.pluck(:user_id) +
-          billed_project_members.distinct.pluck(:user_id) +
-          billed_shared_group_members.distinct.pluck(:user_id) +
-          billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set
+        strong_memoize(:non_gold_billed_users) do
+          non_gold_billed_members
         end
       end
     end
 
+    def non_gold_billed_members
+      ::Member.from_union(
+        [
+          billed_group_members,
+          billed_project_members,
+          billed_shared_group_members,
+          billed_invited_group_to_project_members
+        ]
+      ).distinct
+    end
+
+    def gold_billed_members
+      Member.from_union(
+        [
+          billed_group_members.non_guests,
+          billed_project_members.non_guests,
+          billed_shared_non_guests_group_members.non_guests,
+          billed_invited_non_guests_group_to_project_members.non_guests
+        ]
+      ).distinct
+    end
+
     override :supports_events?
     def supports_events?
       feature_available?(:epics)
@@ -448,17 +468,11 @@ def releases_percentage
       )
     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
+    def billed_users_for(search_term, order_by)
+      users = ::User.id_in(billed_user_members.pluck(:user_id).uniq)
+      users = users.search(search_term) if search_term
+
+      users.sort_by_attribute(::GroupMember.sorting_for(order_by))
     end
 
     private
diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb
index 2e5dc895bab27960..d67fb617b7bbb543 100644
--- a/ee/lib/ee/api/helpers/members_helpers.rb
+++ b/ee/lib/ee/api/helpers/members_helpers.rb
@@ -76,19 +76,6 @@ def log_audit_event(member)
           ).for_member(member).security_event
         end
 
-        def paginate_billable_from_user_ids(user_ids, params = {})
-          paginated = paginate(::Kaminari.paginate_array(user_ids))
-
-          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
-
         class << self
           include ::SortingHelper
 
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index 5cf0588bf26680c0..409496878d8a7195 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -58,7 +58,7 @@ module Members
             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(params[:search], params[:sort]), params)
+            users = paginate(group.billed_users_for(params[:search], params[:sort]))
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end
diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb
index f36ed649e1a123da..1743cd1a1037ebef 100644
--- a/ee/spec/models/group_spec.rb
+++ b/ee/spec/models/group_spec.rb
@@ -1224,47 +1224,50 @@
     end
   end
 
-  describe '#billed_user_ids_for' do
+  describe '#billed_users_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_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
+    let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
+    let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
+    let_it_be(:sophie) { 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) }
+    subject { group.billed_users_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' }
+        let(:order_by) { 'name_desc' }
 
         it 'sorts results accordingly' do
-          expect(subject).to eq([gm_2, gm_3].map(&:user_id))
+          expect(subject).to eq([john_smith, john_doe].map(&:user))
         end
       end
 
       context 'when a sorting parameter is not provided' do
-        let(:sort) { nil }
+        let(:order_by) { nil }
 
         it 'sorts results by name ascending' do
-          expect(subject).to eq([gm_3, gm_2].map(&:user_id))
+          expect(subject).to eq([john_doe, john_smith].map(&:user))
         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))
+      it 'returns expected users in name asc order' do
+        allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
+
+        expect(subject).to eq([john_doe, john_smith, maria, sophie].map(&:user))
       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))
+      context 'and when a sorting parameter is provided (eg name descending)' do
+        let(:order_by) { 'name_desc' }
 
-        expect(subject).to eq([gm_1, gm_2, gm_3, gm_4].map(&:user_id))
+        it 'sorts results accordingly' do
+          expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+        end
       end
     end
   end
-- 
GitLab


From e592e1fbdab4262d94f21815f278ef0a14a8b8da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com>
Date: Wed, 2 Dec 2020 12:39:00 +0100
Subject: [PATCH 4/5] Reverted some updates to simplify MR

Reverted refactoring to be done in another MR.
---
 app/models/concerns/group_member_sorting.rb   | 13 ------
 app/models/members/group_member.rb            |  1 -
 doc/api/members.md                            | 22 +++++++--
 ee/app/models/ee/group.rb                     | 45 ++++++-------------
 ee/lib/ee/api/members.rb                      |  5 ++-
 .../ee/api/helpers/members_helpers_spec.rb    | 26 -----------
 .../concerns/group_member_sorting_spec.rb     | 26 -----------
 7 files changed, 34 insertions(+), 104 deletions(-)
 delete mode 100644 app/models/concerns/group_member_sorting.rb
 delete mode 100644 spec/models/concerns/group_member_sorting_spec.rb

diff --git a/app/models/concerns/group_member_sorting.rb b/app/models/concerns/group_member_sorting.rb
deleted file mode 100644
index 768112b17dfbe908..0000000000000000
--- a/app/models/concerns/group_member_sorting.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-# 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
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index e3645891e7bf943e..03fb2194687486dc 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -3,7 +3,6 @@
 class GroupMember < Member
   include FromUnion
   include CreatedAtFilterable
-  include GroupMemberSorting
 
   SOURCE_TYPE = 'Namespace'
 
diff --git a/doc/api/members.md b/doc/api/members.md
index 5b08e9cdcd981634..bf445ed62d4edad5 100644
--- a/doc/api/members.md
+++ b/doc/api/members.md
@@ -241,8 +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.
+[Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/262875) in GitLab 13.7, the `search` and
+`sort` parameters allow you to search for billable group members by name and sort the results,
+respectively.
 
 ```plaintext
 GET /groups/:id/billable_members
@@ -251,8 +252,21 @@ 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 |
+| `search`  | string         | no  | A query string to search for group members by name, username, or email. |
+| `sort`    | string         | no  | A query string containing parameters that specify the sort attribute and order. See supported values below.|
+
+The supported values for the `sort` attribute are:
+
+| Value               | Description              |
+| ------------------- | ------------------------ |
+| `access_level_asc`  | Access level, ascending  |
+| `access_level_desc` | Access level, descending |
+| `last_joined`       | Last joined              |
+| `name_asc`          | Name, ascending          |
+| `name_desc`         | Name, descending         |
+| `oldest_joined`     | Oldest joined            |
+| `oldest_sign_in`    | Oldest sign in           |
+| `recent_sign_in`    | Recent sign in           |
 
 ```shell
 curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups/:id/billable_members"
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index db4c55ab379f704e..91515d1300784b03 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -8,6 +8,7 @@ module EE
   module Group
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
+    include ::SortingHelper
 
     prepended do
       include TokenAuthenticatable
@@ -299,43 +300,23 @@ def billable_members_count(requested_hosted_plan = nil)
     # We are plucking the user_ids from the "Members" table in an array and
     # converting the array of user_ids to a Set which will have unique user_ids.
     def billed_user_ids(requested_hosted_plan = nil)
-      billed_user_members.pluck(:user_id).to_set
-    end
-
-    def billed_user_members(requested_hosted_plan = nil)
       if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD)
-        strong_memoize(:gold_billed_users) do
-          gold_billed_members
+        strong_memoize(:gold_billed_user_ids) do
+          (billed_group_members.non_guests.distinct.pluck(:user_id) +
+           billed_project_members.non_guests.distinct.pluck(:user_id) +
+           billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) +
+           billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set
         end
       else
-        strong_memoize(:non_gold_billed_users) do
-          non_gold_billed_members
+        strong_memoize(:non_gold_billed_user_ids) do
+          (billed_group_members.distinct.pluck(:user_id) +
+           billed_project_members.distinct.pluck(:user_id) +
+           billed_shared_group_members.distinct.pluck(:user_id) +
+           billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set
         end
       end
     end
 
-    def non_gold_billed_members
-      ::Member.from_union(
-        [
-          billed_group_members,
-          billed_project_members,
-          billed_shared_group_members,
-          billed_invited_group_to_project_members
-        ]
-      ).distinct
-    end
-
-    def gold_billed_members
-      Member.from_union(
-        [
-          billed_group_members.non_guests,
-          billed_project_members.non_guests,
-          billed_shared_non_guests_group_members.non_guests,
-          billed_invited_non_guests_group_to_project_members.non_guests
-        ]
-      ).distinct
-    end
-
     override :supports_events?
     def supports_events?
       feature_available?(:epics)
@@ -469,10 +450,10 @@ def releases_percentage
     end
 
     def billed_users_for(search_term, order_by)
-      users = ::User.id_in(billed_user_members.pluck(:user_id).uniq)
+      users = ::User.id_in(billed_user_ids)
       users = users.search(search_term) if search_term
 
-      users.sort_by_attribute(::GroupMember.sorting_for(order_by))
+      users.sort_by_attribute(order_by || sort_value_name)
     end
 
     private
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index 409496878d8a7195..a7db813fd6510725 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -48,7 +48,7 @@ module Members
           params do
             use :pagination
             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'
+            optional :sort, type: String, desc: 'The sorting option', values: Helpers::MembersHelpers.member_sort_options
           end
           get ":id/billable_members" do
             group = find_group!(params[:id])
@@ -58,7 +58,8 @@ module Members
             bad_request!(nil) if group.subgroup?
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
-            users = paginate(group.billed_users_for(params[:search], params[:sort]))
+            sorting = params[:sort] || 'id_asc'
+            users = paginate(group.billed_users_for(params[:search], sorting))
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end
diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
index 7cd29e4ab6d2a442..8807de54cf33f238 100644
--- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
+++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
@@ -33,30 +33,4 @@
       let(:member) { create(:project_member, project: source, user: create(:user)) }
     end
   end
-
-  describe '#paginate_billable_from_user_ids' do
-    subject(:members_helpers) { Class.new.include(described_class, API::Helpers::Pagination).new }
-
-    let_it_be(:users) { create_list(:user, 3) }
-    let(:user_ids) { users.map(&:id) }
-    let(:page) { 1 }
-    let(:per_page) { 2 }
-
-    before do
-      allow(members_helpers).to receive(:params).and_return({ page: page, per_page: per_page })
-      allow(members_helpers).to receive(:header) { }
-      allow(members_helpers).to receive(:request).and_return(double(:request, url: ''))
-    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)
-
-        expect(results.size).to eq(1)
-        expect(results.map { |result| result.id }).to contain_exactly(user_ids.last)
-      end
-    end
-  end
 end
diff --git a/spec/models/concerns/group_member_sorting_spec.rb b/spec/models/concerns/group_member_sorting_spec.rb
deleted file mode 100644
index a54476b535834f76..0000000000000000
--- a/spec/models/concerns/group_member_sorting_spec.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-# 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
-- 
GitLab


From 778f2456fe65caf6008027a2058beb10e10374c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Etienne=20Baqu=C3=A9?= <ebaque@gitlab.com>
Date: Fri, 4 Dec 2020 17:06:59 +0100
Subject: [PATCH 5/5] Applied maintainer reviewer recommendation

Reorganized code based on review.
---
 app/models/members/group_member.rb            |  1 -
 ee/app/models/ee/group.rb                     | 20 +++-----
 ee/lib/ee/api/helpers/members_helpers.rb      | 17 +++++--
 ee/lib/ee/api/members.rb                      |  2 +-
 .../ee/api/helpers/members_helpers_spec.rb    | 48 +++++++++++++++++++
 ee/spec/models/group_spec.rb                  | 48 -------------------
 6 files changed, 67 insertions(+), 69 deletions(-)

diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 03fb2194687486dc..2bbcdbbe5ced78a6 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -22,7 +22,6 @@ 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?
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index 91515d1300784b03..96272ecf3daf7470 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -8,7 +8,6 @@ module EE
   module Group
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
-    include ::SortingHelper
 
     prepended do
       include TokenAuthenticatable
@@ -303,16 +302,16 @@ def billed_user_ids(requested_hosted_plan = nil)
       if [actual_plan_name, requested_hosted_plan].include?(::Plan::GOLD)
         strong_memoize(:gold_billed_user_ids) do
           (billed_group_members.non_guests.distinct.pluck(:user_id) +
-           billed_project_members.non_guests.distinct.pluck(:user_id) +
-           billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) +
-           billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set
+          billed_project_members.non_guests.distinct.pluck(:user_id) +
+          billed_shared_non_guests_group_members.non_guests.distinct.pluck(:user_id) +
+          billed_invited_non_guests_group_to_project_members.non_guests.distinct.pluck(:user_id)).to_set
         end
       else
         strong_memoize(:non_gold_billed_user_ids) do
           (billed_group_members.distinct.pluck(:user_id) +
-           billed_project_members.distinct.pluck(:user_id) +
-           billed_shared_group_members.distinct.pluck(:user_id) +
-           billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set
+          billed_project_members.distinct.pluck(:user_id) +
+          billed_shared_group_members.distinct.pluck(:user_id) +
+          billed_invited_group_to_project_members.distinct.pluck(:user_id)).to_set
         end
       end
     end
@@ -449,13 +448,6 @@ def releases_percentage
       )
     end
 
-    def billed_users_for(search_term, order_by)
-      users = ::User.id_in(billed_user_ids)
-      users = users.search(search_term) if search_term
-
-      users.sort_by_attribute(order_by || sort_value_name)
-    end
-
     private
 
     def custom_project_templates_group_allowed
diff --git a/ee/lib/ee/api/helpers/members_helpers.rb b/ee/lib/ee/api/helpers/members_helpers.rb
index d67fb617b7bbb543..12cea6b7d7cf1936 100644
--- a/ee/lib/ee/api/helpers/members_helpers.rb
+++ b/ee/lib/ee/api/helpers/members_helpers.rb
@@ -7,6 +7,14 @@ module MembersHelpers
         extend ActiveSupport::Concern
         extend ::Gitlab::Utils::Override
 
+        class << self
+          include ::SortingHelper
+
+          def member_sort_options
+            member_sort_options_hash.keys
+          end
+        end
+
         prepended do
           params :optional_filter_params_ee do
             optional :with_saml_identity, type: Grape::API::Boolean, desc: "List only members with linked SAML identity"
@@ -76,12 +84,11 @@ def log_audit_event(member)
           ).for_member(member).security_event
         end
 
-        class << self
-          include ::SortingHelper
+        def billed_users_for(group, search_term, order_by)
+          users = ::User.id_in(group.billed_user_ids)
+          users = users.search(search_term) if search_term
 
-          def member_sort_options
-            member_sort_options_hash.keys
-          end
+          users.sort_by_attribute(order_by || 'name_asc')
         end
       end
     end
diff --git a/ee/lib/ee/api/members.rb b/ee/lib/ee/api/members.rb
index a7db813fd6510725..910b6324fd6e97b3 100644
--- a/ee/lib/ee/api/members.rb
+++ b/ee/lib/ee/api/members.rb
@@ -59,7 +59,7 @@ module Members
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
             sorting = params[:sort] || 'id_asc'
-            users = paginate(group.billed_users_for(params[:search], sorting))
+            users = paginate(billed_users_for(group, params[:search], sorting))
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end
diff --git a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
index 8807de54cf33f238..a3cc9672aa8e3ac8 100644
--- a/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
+++ b/ee/spec/lib/ee/api/helpers/members_helpers_spec.rb
@@ -33,4 +33,52 @@
       let(:member) { create(:project_member, project: source, user: create(:user)) }
     end
   end
+
+  describe '#billed_users_for' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
+    let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
+    let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
+    let_it_be(:sophie) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
+    let(:search_term) { nil }
+    let(:order_by) { nil }
+
+    subject { members_helpers.billed_users_for(group, search_term, order_by) }
+
+    context 'when a search parameter is present' do
+      let(:search_term) { 'John' }
+
+      context 'when a sorting parameter is provided (eg name descending)' do
+        let(:order_by) { 'name_desc' }
+
+        it 'sorts results accordingly' do
+          expect(subject).to eq([john_smith, john_doe].map(&:user))
+        end
+      end
+
+      context 'when a sorting parameter is not provided' do
+        let(:order_by) { nil }
+
+        it 'sorts results by name ascending' do
+          expect(subject).to eq([john_doe, john_smith].map(&:user))
+        end
+      end
+    end
+
+    context 'when a search parameter is not present' do
+      it 'returns expected users in name asc order' do
+        allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
+
+        expect(subject).to eq([john_doe, john_smith, maria, sophie].map(&:user))
+      end
+
+      context 'and when a sorting parameter is provided (eg name descending)' do
+        let(:order_by) { 'name_desc' }
+
+        it 'sorts results accordingly' do
+          expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user))
+        end
+      end
+    end
+  end
 end
diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb
index 1743cd1a1037ebef..b32eaf21b2106fb4 100644
--- a/ee/spec/models/group_spec.rb
+++ b/ee/spec/models/group_spec.rb
@@ -1223,52 +1223,4 @@
       end
     end
   end
-
-  describe '#billed_users_for' do
-    let_it_be(:group) { create(:group) }
-    let_it_be(:maria) { create(:group_member, group: group, user: create(:user, name: 'Maria Gomez')) }
-    let_it_be(:john_smith) { create(:group_member, group: group, user: create(:user, name: 'John Smith')) }
-    let_it_be(:john_doe) { create(:group_member, group: group, user: create(:user, name: 'John Doe')) }
-    let_it_be(:sophie) { create(:group_member, group: group, user: create(:user, name: 'Sophie Dupont')) }
-    let(:search_term) { nil }
-    let(:order_by) { nil }
-
-    subject { group.billed_users_for(search_term, order_by) }
-
-    context 'when a search parameter is present' do
-      let(:search_term) { 'John' }
-
-      context 'when a sorting parameter is provided (eg name descending)' do
-        let(:order_by) { 'name_desc' }
-
-        it 'sorts results accordingly' do
-          expect(subject).to eq([john_smith, john_doe].map(&:user))
-        end
-      end
-
-      context 'when a sorting parameter is not provided' do
-        let(:order_by) { nil }
-
-        it 'sorts results by name ascending' do
-          expect(subject).to eq([john_doe, john_smith].map(&:user))
-        end
-      end
-    end
-
-    context 'when a search parameter is not present' do
-      it 'returns expected users in name asc order' do
-        allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
-
-        expect(subject).to eq([john_doe, john_smith, maria, sophie].map(&:user))
-      end
-
-      context 'and when a sorting parameter is provided (eg name descending)' do
-        let(:order_by) { 'name_desc' }
-
-        it 'sorts results accordingly' do
-          expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user))
-        end
-      end
-    end
-  end
 end
-- 
GitLab