Merge branch '60798-follow-up-simplify-sort-direction-logic' into 'master'

Resolve "Follow up: Simplify sort direction logic"

Closes #60798

See merge request gitlab-org/gitlab-ce!30443

2 files changed, 181 insertions, 50 deletions

diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb
index 26692934456..d5e5b472115 100644
--- a/app/helpers/sorting_helper.rb
+++ b/app/helpers/sorting_helper.rb
@@ -31,22 +31,24 @@ module SortingHelper
   end
 
   def projects_sort_options_hash
-    Feature.enabled?(:project_list_filter_bar) && !current_controller?('admin/projects') ? projects_sort_common_options_hash : old_projects_sort_options_hash
-  end
+    use_old_sorting = Feature.disabled?(:project_list_filter_bar) || current_controller?('admin/projects')
 
-  # TODO: Simplify these sorting options
-  # https://gitlab.com/gitlab-org/gitlab-ce/issues/60798
-  # https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/11209#note_162234858
-  def old_projects_sort_options_hash
     options = {
       sort_value_latest_activity  => sort_title_latest_activity,
+      sort_value_recently_created => sort_title_created_date,
       sort_value_name             => sort_title_name,
-      sort_value_oldest_activity  => sort_title_oldest_activity,
-      sort_value_oldest_created   => sort_title_oldest_created,
-      sort_value_recently_created => sort_title_recently_created,
-      sort_value_stars_desc       => sort_title_most_stars
+      sort_value_stars_desc       => sort_title_stars
     }
 
+    if use_old_sorting
+      options = options.merge({
+        sort_value_oldest_activity  => sort_title_oldest_activity,
+        sort_value_oldest_created   => sort_title_oldest_created,
+        sort_value_recently_created => sort_title_recently_created,
+        sort_value_stars_desc       => sort_title_most_stars
+      })
+    end
+
     if current_controller?('admin/projects')
       options[sort_value_largest_repo] = sort_title_largest_repo
     end
@@ -54,26 +56,14 @@ module SortingHelper
     options
   end
 
-  def projects_sort_common_options_hash
-    {
-      sort_value_latest_activity  => sort_title_latest_activity,
-      sort_value_recently_created => sort_title_created_date,
-      sort_value_name             => sort_title_name,
-      sort_value_stars_desc       => sort_title_stars
-    }
-  end
-
   def projects_sort_option_titles
-    {
-      sort_value_latest_activity  => sort_title_latest_activity,
-      sort_value_recently_created => sort_title_created_date,
-      sort_value_name             => sort_title_name,
-      sort_value_stars_desc       => sort_title_stars,
+    # Only used for the project filter search bar
+    projects_sort_options_hash.merge({
       sort_value_oldest_activity  => sort_title_latest_activity,
       sort_value_oldest_created   => sort_title_created_date,
       sort_value_name_desc        => sort_title_name,
       sort_value_stars_asc        => sort_title_stars
-    }
+    })
   end
 
   def projects_reverse_sort_options_hash
@@ -210,47 +200,42 @@ module SortingHelper
     sort_options_hash[sort_value]
   end
 
-  def issuable_sort_icon_suffix(sort_value)
+  def sort_direction_icon(sort_value)
     case sort_value
     when sort_value_milestone, sort_value_due_date, /_asc\z/
-      'lowest'
+      'sort-lowest'
     else
-      'highest'
+      'sort-highest'
     end
   end
 
-  # TODO: dedupicate issuable and project sort direction
-  # https://gitlab.com/gitlab-org/gitlab-ce/issues/60798
-  def issuable_sort_direction_button(sort_value)
+  def sort_direction_button(reverse_url, reverse_sort, sort_value)
     link_class = 'btn btn-default has-tooltip reverse-sort-btn qa-reverse-sort'
-    reverse_sort = issuable_reverse_sort_order_hash[sort_value]
+    icon = sort_direction_icon(sort_value)
+    url = reverse_url
 
-    if reverse_sort
-      reverse_url = page_filter_path(sort: reverse_sort)
-    else
-      reverse_url = '#'
+    unless reverse_sort
+      url = '#'
       link_class += ' disabled'
     end
 
-    link_to(reverse_url, type: 'button', class: link_class, title: s_('SortOptions|Sort direction')) do
-      sprite_icon("sort-#{issuable_sort_icon_suffix(sort_value)}", size: 16)
+    link_to(url, type: 'button', class: link_class, title: s_('SortOptions|Sort direction')) do
+      sprite_icon(icon, size: 16)
     end
   end
 
+  def issuable_sort_direction_button(sort_value)
+    reverse_sort = issuable_reverse_sort_order_hash[sort_value]
+    url = page_filter_path(sort: reverse_sort)
+
+    sort_direction_button(url, reverse_sort, sort_value)
+  end
+
   def project_sort_direction_button(sort_value)
-    link_class = 'btn btn-default has-tooltip reverse-sort-btn qa-reverse-sort'
     reverse_sort = projects_reverse_sort_options_hash[sort_value]
+    url = filter_projects_path(sort: reverse_sort)
 
-    if reverse_sort
-      reverse_url = filter_projects_path(sort: reverse_sort)
-    else
-      reverse_url = '#'
-      link_class += ' disabled'
-    end
-
-    link_to(reverse_url, type: 'button', class: link_class, title: s_('SortOptions|Sort direction')) do
-      sprite_icon("sort-#{issuable_sort_icon_suffix(sort_value)}", size: 16)
-    end
+    sort_direction_button(url, reverse_sort, sort_value)
   end
 
   # Titles.
diff --git a/spec/helpers/sorting_helper_spec.rb b/spec/helpers/sorting_helper_spec.rb
index f405268d198..5397a47b3dd 100644
--- a/spec/helpers/sorting_helper_spec.rb
+++ b/spec/helpers/sorting_helper_spec.rb
@@ -4,6 +4,11 @@ require 'spec_helper'
 describe SortingHelper do
   include ApplicationHelper
   include IconsHelper
+  include ExploreHelper
+
+  def set_sorting_url(option)
+    allow(self).to receive(:request).and_return(double(path: 'http://test.com', query_parameters: { label_name: option }))
+  end
 
   describe '#issuable_sort_option_title' do
     it 'returns correct title for issuable_sort_option_overrides key' do
@@ -21,7 +26,7 @@ describe SortingHelper do
 
   describe '#issuable_sort_direction_button' do
     before do
-      allow(self).to receive(:request).and_return(double(path: 'http://test.com', query_parameters: { label_name: 'test_label' }))
+      set_sorting_url 'test_label'
     end
 
     it 'keeps label filter param' do
@@ -44,4 +49,145 @@ describe SortingHelper do
       expect(issuable_sort_direction_button('due_date')).to include('sort-lowest')
     end
   end
+
+  def stub_controller_path(value)
+    allow(helper.controller).to receive(:controller_path).and_return(value)
+  end
+
+  def project_common_options
+    {
+      sort_value_latest_activity  => sort_title_latest_activity,
+      sort_value_recently_created => sort_title_created_date,
+      sort_value_name             => sort_title_name,
+      sort_value_stars_desc       => sort_title_stars
+    }
+  end
+
+  describe 'with `admin/projects` controller' do
+    before do
+      stub_controller_path 'admin/projects'
+    end
+
+    describe '#projects_sort_options_hash' do
+      it 'returns a hash of available sorting options' do
+        admin_options = project_common_options.merge({
+          sort_value_oldest_activity  => sort_title_oldest_activity,
+          sort_value_oldest_created   => sort_title_oldest_created,
+          sort_value_recently_created => sort_title_recently_created,
+          sort_value_stars_desc       => sort_title_most_stars,
+          sort_value_largest_repo     => sort_title_largest_repo
+        })
+
+        expect(projects_sort_options_hash).to eq(admin_options)
+      end
+    end
+  end
+
+  describe 'with `projects` controller' do
+    before do
+      stub_controller_path 'projects'
+    end
+
+    describe '#projects_sort_options_hash' do
+      it 'returns a hash of available sorting options' do
+        expect(projects_sort_options_hash).to include(project_common_options)
+      end
+    end
+
+    describe '#projects_reverse_sort_options_hash' do
+      context 'returns a reversed hash of available sorting options' do
+        using RSpec::Parameterized::TableSyntax
+
+        where(:sort_key, :reverse_sort_title) do
+          sort_value_latest_activity  | sort_value_oldest_activity
+          sort_value_recently_created | sort_value_oldest_created
+          sort_value_name             | sort_value_name_desc
+          sort_value_stars_desc       | sort_value_stars_asc
+          sort_value_oldest_activity  | sort_value_latest_activity
+          sort_value_oldest_created   | sort_value_recently_created
+          sort_value_name_desc        | sort_value_name
+          sort_value_stars_asc        | sort_value_stars_desc
+        end
+
+        with_them do
+          it do
+            reverse_hash = projects_reverse_sort_options_hash
+
+            expect(reverse_hash).to include(sort_key)
+            expect(reverse_hash[sort_key]).to eq(reverse_sort_title)
+          end
+        end
+      end
+    end
+
+    describe '#project_sort_direction_button' do
+      context 'returns the correct icon for each sort option' do
+        using RSpec::Parameterized::TableSyntax
+
+        sort_lowest_icon = 'sort-lowest'
+        sort_highest_icon = 'sort-highest'
+
+        where(:selected_sort, :icon) do
+          sort_value_latest_activity  | sort_highest_icon
+          sort_value_recently_created | sort_highest_icon
+          sort_value_name_desc        | sort_highest_icon
+          sort_value_stars_desc       | sort_highest_icon
+          sort_value_oldest_activity  | sort_lowest_icon
+          sort_value_oldest_created   | sort_lowest_icon
+          sort_value_name             | sort_lowest_icon
+          sort_value_stars_asc        | sort_lowest_icon
+        end
+
+        with_them do
+          it do
+            set_sorting_url selected_sort
+
+            expect(project_sort_direction_button(selected_sort)).to include(icon)
+          end
+        end
+      end
+
+      it 'returns the correct link to reverse the current sort option' do
+        sort_options_links = projects_reverse_sort_options_hash
+
+        sort_options_links.each do |selected_sort, reverse_sort|
+          set_sorting_url selected_sort
+
+          expect(project_sort_direction_button(selected_sort)).to include(reverse_sort)
+        end
+      end
+    end
+
+    describe '#projects_sort_option_titles' do
+      it 'returns a hash of titles for the sorting options' do
+        options = project_common_options.merge({
+          sort_value_oldest_activity  => sort_title_latest_activity,
+          sort_value_oldest_created   => sort_title_created_date,
+          sort_value_name_desc        => sort_title_name,
+          sort_value_stars_asc        => sort_title_stars
+        })
+
+        expect(projects_sort_option_titles).to eq(options)
+      end
+    end
+
+    describe 'with project_list_filter_bar off' do
+      before do
+        stub_feature_flags(project_list_filter_bar: false)
+      end
+
+      describe '#projects_sort_options_hash' do
+        it 'returns a hash of available sorting options' do
+          options = project_common_options.merge({
+            sort_value_oldest_activity  => sort_title_oldest_activity,
+            sort_value_oldest_created   => sort_title_oldest_created,
+            sort_value_recently_created => sort_title_recently_created,
+            sort_value_stars_desc       => sort_title_most_stars
+          })
+
+          expect(projects_sort_options_hash).to eq(options)
+        end
+      end
+    end
+  end
 end