Merge branch '15064_issuable_default_sort_order' into 'master'

Sensible state specific default sort order for issues and merge requests

## What does this MR do?

It provides more sensible default sort order for issues and merge requests based on the following table:

    | type           | state  | default sort order |
    |----------------|--------|--------------------|
    | issues         | open   | last created       |
    | issues         | closed | last updated       |
    | issues         | all    | last created       |
    | merge requests | open   | last created       |
    | merge requests | merged | last updated       |
    | merge requests | closed | last updated       |
    | merge requests | all    | last created       |

## Are there points in the code the reviewer needs to double check?

All the bits where `id_desc` was changed to `created_desc`.
 
I hope it's okay, It makes more sense in my opinion.

## Why was this MR needed?

Prior to this MR the issues and merge request were sorted based on `id_desc` by default. 

This MR aims to make the interface more user-friendly by providing state specific sorting defaults most users would expect. 

## What are the relevant issue numbers?

See #15064 

See merge request !5453

6 files changed, 95 insertions, 66 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index a1004d9bcea..634d36a4467 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -243,42 +243,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def set_filters_params
-    set_default_sort
-
-    params[:scope] = 'all' if params[:scope].blank?
-    params[:state] = 'opened' if params[:state].blank?
-
-    @sort = params[:sort]
-    @filter_params = params.dup
-
-    if @project
-      @filter_params[:project_id] = @project.id
-    elsif @group
-      @filter_params[:group_id] = @group.id
-    else
-      # TODO: this filter ignore issues/mr created in public or
-      # internal repos where you are not a member. Enable this filter
-      # or improve current implementation to filter only issues you
-      # created or assigned or mentioned
-      # @filter_params[:authorized_only] = true
-    end
-
-    @filter_params
-  end
-
-  def get_issues_collection
-    set_filters_params
-    @issuable_finder = IssuesFinder.new(current_user, @filter_params)
-    @issuable_finder.execute
-  end
-
-  def get_merge_requests_collection
-    set_filters_params
-    @issuable_finder = MergeRequestsFinder.new(current_user, @filter_params)
-    @issuable_finder.execute
-  end
-
   def import_sources_enabled?
     !current_application_settings.import_sources.empty?
   end
@@ -363,24 +327,4 @@ class ApplicationController < ActionController::Base
   def u2f_app_id
     request.base_url
   end
-
-  private
-
-  def set_default_sort
-    key = if is_a_listing_page_for?('issues') || is_a_listing_page_for?('merge_requests')
-            'issuable_sort'
-          end
-
-    cookies[key]  = params[:sort] if key && params[:sort].present?
-    params[:sort] = cookies[key] if key
-    params[:sort] ||= 'id_desc'
-  end
-
-  def is_a_listing_page_for?(page_type)
-    controller_name, action_name = params.values_at(:controller, :action)
-
-    (controller_name == "projects/#{page_type}" && action_name == 'index') ||
-    (controller_name == 'groups' && action_name == page_type) ||
-    (controller_name == 'dashboard' && action_name == page_type)
-  end
 end
diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb
new file mode 100644
index 00000000000..c802922e0af
--- /dev/null
+++ b/app/controllers/concerns/issuable_collections.rb
@@ -0,0 +1,79 @@
+module IssuableCollections
+  extend ActiveSupport::Concern
+  include SortingHelper
+
+  included do
+    helper_method :issues_finder
+    helper_method :merge_requests_finder
+  end
+
+  private
+
+  def issues_collection
+    issues_finder.execute
+  end
+
+  def merge_requests_collection
+    merge_requests_finder.execute
+  end
+
+  def issues_finder
+    @issues_finder ||= issuable_finder_for(IssuesFinder)
+  end
+
+  def merge_requests_finder
+    @merge_requests_finder ||= issuable_finder_for(MergeRequestsFinder)
+  end
+
+  def issuable_finder_for(finder_class)
+    finder_class.new(current_user, filter_params)
+  end
+
+  def filter_params
+    set_sort_order_from_cookie
+    set_default_scope
+    set_default_state
+
+    @filter_params = params.dup
+    @filter_params[:sort] ||= default_sort_order
+
+    @sort = @filter_params[:sort]
+
+    if @project
+      @filter_params[:project_id] = @project.id
+    elsif @group
+      @filter_params[:group_id] = @group.id
+    else
+      # TODO: this filter ignore issues/mr created in public or
+      # internal repos where you are not a member. Enable this filter
+      # or improve current implementation to filter only issues you
+      # created or assigned or mentioned
+      # @filter_params[:authorized_only] = true
+    end
+
+    @filter_params
+  end
+
+  def set_default_scope
+    params[:scope] = 'all' if params[:scope].blank?
+  end
+
+  def set_default_state
+    params[:state] = 'opened' if params[:state].blank?
+  end
+
+  def set_sort_order_from_cookie
+    key = 'issuable_sort'
+
+    cookies[key] = params[:sort] if params[:sort].present?
+    params[:sort] = cookies[key]
+  end
+
+  def default_sort_order
+    case params[:state]
+    when 'opened', 'all' then sort_value_recently_created
+    when 'merged', 'closed' then sort_value_recently_updated
+    else sort_value_recently_created
+    end
+  end
+end
diff --git a/app/controllers/concerns/issues_action.rb b/app/controllers/concerns/issues_action.rb
index 4feabc32b1c..b89fb94be6e 100644
--- a/app/controllers/concerns/issues_action.rb
+++ b/app/controllers/concerns/issues_action.rb
@@ -1,12 +1,14 @@
 module IssuesAction
   extend ActiveSupport::Concern
+  include IssuableCollections
 
   def issues
-    @issues = get_issues_collection.non_archived
-    @issues = @issues.page(params[:page])
-    @issues = @issues.preload(:author, :project)
+    @label = issues_finder.labels.first
 
-    @label = @issuable_finder.labels.first
+    @issues = issues_collection
+              .non_archived
+              .preload(:author, :project)
+              .page(params[:page])
 
     respond_to do |format|
       format.html
diff --git a/app/controllers/concerns/merge_requests_action.rb b/app/controllers/concerns/merge_requests_action.rb
index 06a6b065e7e..a1b0eee37f9 100644
--- a/app/controllers/concerns/merge_requests_action.rb
+++ b/app/controllers/concerns/merge_requests_action.rb
@@ -1,11 +1,13 @@
 module MergeRequestsAction
   extend ActiveSupport::Concern
+  include IssuableCollections
 
   def merge_requests
-    @merge_requests = get_merge_requests_collection.non_archived
-    @merge_requests = @merge_requests.page(params[:page])
-    @merge_requests = @merge_requests.preload(:author, :target_project)
+    @label = merge_requests_finder.labels.first
 
-    @label = @issuable_finder.labels.first
+    @merge_requests = merge_requests_collection
+                      .non_archived
+                      .preload(:author, :target_project)
+                      .page(params[:page])
   end
 end
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 3c6f29ac0ba..7f5c3ff3d6a 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -3,6 +3,7 @@ class Projects::IssuesController < Projects::ApplicationController
   include ToggleSubscriptionAction
   include IssuableActions
   include ToggleAwardEmoji
+  include IssuableCollections
 
   before_action :module_enabled
   before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests,
@@ -24,7 +25,7 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def index
     terms = params['issue_search']
-    @issues = get_issues_collection
+    @issues = issues_collection
 
     if terms.present?
       if terms =~ /\A#(\d+)\z/
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 47c21a18b33..03166294ddd 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -5,6 +5,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   include IssuableActions
   include NotesHelper
   include ToggleAwardEmoji
+  include IssuableCollections
 
   before_action :module_enabled
   before_action :merge_request, only: [
@@ -29,7 +30,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def index
     terms = params['issue_search']
-    @merge_requests = get_merge_requests_collection
+    @merge_requests = merge_requests_collection
 
     if terms.present?
       if terms =~ /\A[#!](\d+)\z/