Reverted some updates to simplify MR

Reverted refactoring to be done in another MR.

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,7 +3,6 @@
 class GroupMember < Member
   include FromUnion
   include CreatedAtFilterable
-  include GroupMemberSorting
 
   SOURCE_TYPE = 'Namespace'
 

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"

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 @@ module EE
     # 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 @@ module EE
     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

ee/lib/ee/api/members.rb

@@ -48,7 +48,7 @@ module EE
           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 EE
             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

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

@@ -33,30 +33,4 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
       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

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