Refactored SortingOptions module

Updated rspecs based on review.

app/helpers/member_sort_options_helper.rb

+# frozen_string_literal: true
+
+module MemberSortOptionsHelper
+  include SortingTitlesValuesHelper
+
+  def member_sort_options_hash
+    {
+      sort_value_access_level_asc  => sort_title_access_level_asc,
+      sort_value_access_level_desc => sort_title_access_level_desc,
+      sort_value_last_joined       => sort_title_last_joined,
+      sort_value_name              => sort_title_name_asc,
+      sort_value_name_desc         => sort_title_name_desc,
+      sort_value_oldest_joined     => sort_title_oldest_joined,
+      sort_value_oldest_signin     => sort_title_oldest_signin,
+      sort_value_recently_signin   => sort_title_recently_signin
+    }
+  end
+end

app/helpers/sorting_helper.rb

 # frozen_string_literal: true
 
 module SortingHelper
+  include SortingTitlesValuesHelper
+  include MemberSortOptionsHelper
+
   def sort_options_hash
     {
       sort_value_created_date      => sort_title_created_date,
@@ -113,19 +116,6 @@ module SortingHelper
     )
   end
 
-  def member_sort_options_hash
-    {
-      sort_value_access_level_asc  => sort_title_access_level_asc,
-      sort_value_access_level_desc => sort_title_access_level_desc,
-      sort_value_last_joined       => sort_title_last_joined,
-      sort_value_name              => sort_title_name_asc,
-      sort_value_name_desc         => sort_title_name_desc,
-      sort_value_oldest_joined     => sort_title_oldest_joined,
-      sort_value_oldest_signin     => sort_title_oldest_signin,
-      sort_value_recently_signin   => sort_title_recently_signin
-    }
-  end
-
   def milestone_sort_options_hash
     {
       sort_value_name             => sort_title_name_asc,
@@ -276,340 +266,6 @@ module SortingHelper
     sort_direction_button(url, reverse_sort, sort_value)
   end
 
-  # Titles.
-  def sort_title_access_level_asc
-    s_('SortOptions|Access level, ascending')
-  end
-
-  def sort_title_access_level_desc
-    s_('SortOptions|Access level, descending')
-  end
-
-  def sort_title_created_date
-    s_('SortOptions|Created date')
-  end
-
-  def sort_title_downvotes
-    s_('SortOptions|Least popular')
-  end
-
-  def sort_title_due_date
-    s_('SortOptions|Due date')
-  end
-
-  def sort_title_due_date_later
-    s_('SortOptions|Due later')
-  end
-
-  def sort_title_due_date_soon
-    s_('SortOptions|Due soon')
-  end
-
-  def sort_title_label_priority
-    s_('SortOptions|Label priority')
-  end
-
-  def sort_title_largest_group
-    s_('SortOptions|Largest group')
-  end
-
-  def sort_title_largest_repo
-    s_('SortOptions|Largest repository')
-  end
-
-  def sort_title_last_joined
-    s_('SortOptions|Last joined')
-  end
-
-  def sort_title_latest_activity
-    s_('SortOptions|Last updated')
-  end
-
-  def sort_title_milestone
-    s_('SortOptions|Milestone due date')
-  end
-
-  def sort_title_milestone_later
-    s_('SortOptions|Milestone due later')
-  end
-
-  def sort_title_milestone_soon
-    s_('SortOptions|Milestone due soon')
-  end
-
-  def sort_title_name
-    s_('SortOptions|Name')
-  end
-
-  def sort_title_name_asc
-    s_('SortOptions|Name, ascending')
-  end
-
-  def sort_title_name_desc
-    s_('SortOptions|Name, descending')
-  end
-
-  def sort_title_oldest_activity
-    s_('SortOptions|Oldest updated')
-  end
-
-  def sort_title_oldest_created
-    s_('SortOptions|Oldest created')
-  end
-
-  def sort_title_oldest_joined
-    s_('SortOptions|Oldest joined')
-  end
-
-  def sort_title_oldest_signin
-    s_('SortOptions|Oldest sign in')
-  end
-
-  def sort_title_oldest_starred
-    s_('SortOptions|Oldest starred')
-  end
-
-  def sort_title_oldest_updated
-    s_('SortOptions|Oldest updated')
-  end
-
-  def sort_title_popularity
-    s_('SortOptions|Popularity')
-  end
-
-  def sort_title_priority
-    s_('SortOptions|Priority')
-  end
-
-  def sort_title_recently_created
-    s_('SortOptions|Last created')
-  end
-
-  def sort_title_recently_signin
-    s_('SortOptions|Recent sign in')
-  end
-
-  def sort_title_recently_starred
-    s_('SortOptions|Recently starred')
-  end
-
-  def sort_title_recently_updated
-    s_('SortOptions|Last updated')
-  end
-
-  def sort_title_start_date_later
-    s_('SortOptions|Start later')
-  end
-
-  def sort_title_start_date_soon
-    s_('SortOptions|Start soon')
-  end
-
-  def sort_title_upvotes
-    s_('SortOptions|Most popular')
-  end
-
-  def sort_title_contacted_date
-    s_('SortOptions|Last Contact')
-  end
-
-  def sort_title_most_stars
-    s_('SortOptions|Most stars')
-  end
-
-  def sort_title_stars
-    s_('SortOptions|Stars')
-  end
-
-  def sort_title_oldest_last_activity
-    s_('SortOptions|Oldest last activity')
-  end
-
-  def sort_title_recently_last_activity
-    s_('SortOptions|Recent last activity')
-  end
-
-  def sort_title_relative_position
-    s_('SortOptions|Manual')
-  end
-
-  def sort_title_size
-    s_('SortOptions|Size')
-  end
-
-  def sort_title_expire_date
-    s_('SortOptions|Expired date')
-  end
-
-  def sort_title_relevant
-    s_('SortOptions|Relevant')
-  end
-
-  # Values.
-  def sort_value_access_level_asc
-    'access_level_asc'
-  end
-
-  def sort_value_access_level_desc
-    'access_level_desc'
-  end
-
-  def sort_value_created_date
-    'created_date'
-  end
-
-  def sort_value_downvotes
-    'downvotes_desc'
-  end
-
-  def sort_value_due_date
-    'due_date'
-  end
-
-  def sort_value_due_date_later
-    'due_date_desc'
-  end
-
-  def sort_value_due_date_soon
-    'due_date_asc'
-  end
-
-  def sort_value_label_priority
-    'label_priority'
-  end
-
-  def sort_value_largest_group
-    'storage_size_desc'
-  end
-
-  def sort_value_largest_repo
-    'storage_size_desc'
-  end
-
-  def sort_value_last_joined
-    'last_joined'
-  end
-
-  def sort_value_latest_activity
-    'latest_activity_desc'
-  end
-
-  def sort_value_milestone
-    'milestone'
-  end
-
-  def sort_value_milestone_later
-    'milestone_due_desc'
-  end
-
-  def sort_value_milestone_soon
-    'milestone_due_asc'
-  end
-
-  def sort_value_name
-    'name_asc'
-  end
-
-  def sort_value_name_desc
-    'name_desc'
-  end
-
-  def sort_value_oldest_activity
-    'latest_activity_asc'
-  end
-
-  def sort_value_oldest_created
-    'created_asc'
-  end
-
-  def sort_value_oldest_signin
-    'oldest_sign_in'
-  end
-
-  def sort_value_oldest_joined
-    'oldest_joined'
-  end
-
-  def sort_value_oldest_updated
-    'updated_asc'
-  end
-
-  def sort_value_popularity
-    'popularity'
-  end
-
-  def sort_value_most_popular
-    'popularity_desc'
-  end
-
-  def sort_value_least_popular
-    'popularity_asc'
-  end
-
-  def sort_value_priority
-    'priority'
-  end
-
-  def sort_value_recently_created
-    'created_desc'
-  end
-
-  def sort_value_recently_signin
-    'recent_sign_in'
-  end
-
-  def sort_value_recently_updated
-    'updated_desc'
-  end
-
-  def sort_value_start_date_later
-    'start_date_desc'
-  end
-
-  def sort_value_start_date_soon
-    'start_date_asc'
-  end
-
-  def sort_value_upvotes
-    'upvotes_desc'
-  end
-
-  def sort_value_contacted_date
-    'contacted_asc'
-  end
-
-  def sort_value_stars_desc
-    'stars_desc'
-  end
-
-  def sort_value_stars_asc
-    'stars_asc'
-  end
-
-  def sort_value_oldest_last_activity
-    'last_activity_on_asc'
-  end
-
-  def sort_value_recently_last_activity
-    'last_activity_on_desc'
-  end
-
-  def sort_value_relative_position
-    'relative_position'
-  end
-
-  def sort_value_size
-    'size_desc'
-  end
-
-  def sort_value_expire_date
-    'expired_asc'
-  end
-
-  def sort_value_relevant
-    'relevant'
-  end
-
   def packages_sort_options_hash
     {
       sort_value_recently_created  => sort_title_created_date,

app/helpers/sorting_titles_values_helper.rb

+# frozen_string_literal: true
+
+module SortingTitlesValuesHelper
+  # Titles.
+  def sort_title_access_level_asc
+    s_('SortOptions|Access level, ascending')
+  end
+
+  def sort_title_access_level_desc
+    s_('SortOptions|Access level, descending')
+  end
+
+  def sort_title_created_date
+    s_('SortOptions|Created date')
+  end
+
+  def sort_title_downvotes
+    s_('SortOptions|Least popular')
+  end
+
+  def sort_title_due_date
+    s_('SortOptions|Due date')
+  end
+
+  def sort_title_due_date_later
+    s_('SortOptions|Due later')
+  end
+
+  def sort_title_due_date_soon
+    s_('SortOptions|Due soon')
+  end
+
+  def sort_title_label_priority
+    s_('SortOptions|Label priority')
+  end
+
+  def sort_title_largest_group
+    s_('SortOptions|Largest group')
+  end
+
+  def sort_title_largest_repo
+    s_('SortOptions|Largest repository')
+  end
+
+  def sort_title_last_joined
+    s_('SortOptions|Last joined')
+  end
+
+  def sort_title_latest_activity
+    s_('SortOptions|Last updated')
+  end
+
+  def sort_title_milestone
+    s_('SortOptions|Milestone due date')
+  end
+
+  def sort_title_milestone_later
+    s_('SortOptions|Milestone due later')
+  end
+
+  def sort_title_milestone_soon
+    s_('SortOptions|Milestone due soon')
+  end
+
+  def sort_title_name
+    s_('SortOptions|Name')
+  end
+
+  def sort_title_name_asc
+    s_('SortOptions|Name, ascending')
+  end
+
+  def sort_title_name_desc
+    s_('SortOptions|Name, descending')
+  end
+
+  def sort_title_oldest_activity
+    s_('SortOptions|Oldest updated')
+  end
+
+  def sort_title_oldest_created
+    s_('SortOptions|Oldest created')
+  end
+
+  def sort_title_oldest_joined
+    s_('SortOptions|Oldest joined')
+  end
+
+  def sort_title_oldest_signin
+    s_('SortOptions|Oldest sign in')
+  end
+
+  def sort_title_oldest_starred
+    s_('SortOptions|Oldest starred')
+  end
+
+  def sort_title_oldest_updated
+    s_('SortOptions|Oldest updated')
+  end
+
+  def sort_title_popularity
+    s_('SortOptions|Popularity')
+  end
+
+  def sort_title_priority
+    s_('SortOptions|Priority')
+  end
+
+  def sort_title_recently_created
+    s_('SortOptions|Last created')
+  end
+
+  def sort_title_recently_signin
+    s_('SortOptions|Recent sign in')
+  end
+
+  def sort_title_recently_starred
+    s_('SortOptions|Recently starred')
+  end
+
+  def sort_title_recently_updated
+    s_('SortOptions|Last updated')
+  end
+
+  def sort_title_start_date_later
+    s_('SortOptions|Start later')
+  end
+
+  def sort_title_start_date_soon
+    s_('SortOptions|Start soon')
+  end
+
+  def sort_title_upvotes
+    s_('SortOptions|Most popular')
+  end
+
+  def sort_title_contacted_date
+    s_('SortOptions|Last Contact')
+  end
+
+  def sort_title_most_stars
+    s_('SortOptions|Most stars')
+  end
+
+  def sort_title_stars
+    s_('SortOptions|Stars')
+  end
+
+  def sort_title_oldest_last_activity
+    s_('SortOptions|Oldest last activity')
+  end
+
+  def sort_title_recently_last_activity
+    s_('SortOptions|Recent last activity')
+  end
+
+  def sort_title_relative_position
+    s_('SortOptions|Manual')
+  end
+
+  def sort_title_size
+    s_('SortOptions|Size')
+  end
+
+  def sort_title_expire_date
+    s_('SortOptions|Expired date')
+  end
+
+  def sort_title_relevant
+    s_('SortOptions|Relevant')
+  end
+
+  # Values.
+  def sort_value_access_level_asc
+    'access_level_asc'
+  end
+
+  def sort_value_access_level_desc
+    'access_level_desc'
+  end
+
+  def sort_value_created_date
+    'created_date'
+  end
+
+  def sort_value_downvotes
+    'downvotes_desc'
+  end
+
+  def sort_value_due_date
+    'due_date'
+  end
+
+  def sort_value_due_date_later
+    'due_date_desc'
+  end
+
+  def sort_value_due_date_soon
+    'due_date_asc'
+  end
+
+  def sort_value_label_priority
+    'label_priority'
+  end
+
+  def sort_value_largest_group
+    'storage_size_desc'
+  end
+
+  def sort_value_largest_repo
+    'storage_size_desc'
+  end
+
+  def sort_value_last_joined
+    'last_joined'
+  end
+
+  def sort_value_latest_activity
+    'latest_activity_desc'
+  end
+
+  def sort_value_milestone
+    'milestone'
+  end
+
+  def sort_value_milestone_later
+    'milestone_due_desc'
+  end
+
+  def sort_value_milestone_soon
+    'milestone_due_asc'
+  end
+
+  def sort_value_name
+    'name_asc'
+  end
+
+  def sort_value_name_desc
+    'name_desc'
+  end
+
+  def sort_value_oldest_activity
+    'latest_activity_asc'
+  end
+
+  def sort_value_oldest_created
+    'created_asc'
+  end
+
+  def sort_value_oldest_signin
+    'oldest_sign_in'
+  end
+
+  def sort_value_oldest_joined
+    'oldest_joined'
+  end
+
+  def sort_value_oldest_updated
+    'updated_asc'
+  end
+
+  def sort_value_popularity
+    'popularity'
+  end
+
+  def sort_value_most_popular
+    'popularity_desc'
+  end
+
+  def sort_value_least_popular
+    'popularity_asc'
+  end
+
+  def sort_value_priority
+    'priority'
+  end
+
+  def sort_value_recently_created
+    'created_desc'
+  end
+
+  def sort_value_recently_signin
+    'recent_sign_in'
+  end
+
+  def sort_value_recently_updated
+    'updated_desc'
+  end
+
+  def sort_value_start_date_later
+    'start_date_desc'
+  end
+
+  def sort_value_start_date_soon
+    'start_date_asc'
+  end
+
+  def sort_value_upvotes
+    'upvotes_desc'
+  end
+
+  def sort_value_contacted_date
+    'contacted_asc'
+  end
+
+  def sort_value_stars_desc
+    'stars_desc'
+  end
+
+  def sort_value_stars_asc
+    'stars_asc'
+  end
+
+  def sort_value_oldest_last_activity
+    'last_activity_on_asc'
+  end
+
+  def sort_value_recently_last_activity
+    'last_activity_on_desc'
+  end
+
+  def sort_value_relative_position
+    'relative_position'
+  end
+
+  def sort_value_size
+    'size_desc'
+  end
+
+  def sort_value_expire_date
+    'expired_asc'
+  end
+
+  def sort_value_relevant
+    'relevant'
+  end
+end

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

@@ -8,7 +8,7 @@ module EE
         extend ::Gitlab::Utils::Override
 
         class << self
-          include ::SortingHelper
+          include ::MemberSortOptionsHelper
 
           def member_sort_options
             member_sort_options_hash.keys
@@ -84,11 +84,11 @@ module EE
           ).for_member(member).security_event
         end
 
-        def billed_users_for(group, search_term, order_by)
+        def billed_users_for(group, search_term, order_by: 'name_asc')
           users = ::User.id_in(group.billed_user_ids)
           users = users.search(search_term) if search_term
 
-          users.sort_by_attribute(order_by || 'name_asc')
+          users.sort_by_attribute(order_by)
         end
       end
     end

ee/lib/ee/api/members.rb

@@ -57,7 +57,7 @@ module EE
             bad_request!(nil) unless ::Ability.allowed?(current_user, :admin_group_member, group)
 
             sorting = params[:sort] || 'id_asc'
-            users = paginate(billed_users_for(group, params[:search], sorting))
+            users = paginate(billed_users_for(group, params[:search], order_by: sorting))
 
             present users, with: ::API::Entities::UserBasic, current_user: current_user
           end

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

@@ -43,7 +43,7 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
     let(:search_term) { nil }
     let(:order_by) { nil }
 
-    subject { members_helpers.billed_users_for(group, search_term, order_by) }
+    subject { members_helpers.billed_users_for(group, search_term, order_by: order_by) }
 
     context 'when a search parameter is present' do
       let(:search_term) { 'John' }
@@ -66,6 +66,8 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
     end
 
     context 'when a search parameter is not present' do
+      subject { members_helpers.billed_users_for(group, search_term) }
+
       it 'returns expected users in name asc order' do
         allow(group).to receive(:billed_user_members).and_return([john_doe, john_smith, sophie, maria])
 
@@ -75,6 +77,8 @@ RSpec.describe EE::API::Helpers::MembersHelpers do
       context 'and when a sorting parameter is provided (eg name descending)' do
         let(:order_by) { 'name_desc' }
 
+        subject { members_helpers.billed_users_for(group, search_term, order_by: order_by) }
+
         it 'sorts results accordingly' do
           expect(subject).to eq([sophie, maria, john_smith, john_doe].map(&:user))
         end