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

CHANGELOG

@@ -38,6 +38,7 @@ v 8.11.0 (unreleased)
   - Make error pages responsive (Takuya Noguchi)
   - Change requests_profiles resource constraint to catch virtually any file
   - Reduce number of queries made for merge_requests/:id/diffs
+  - Sensible state specific default sort order for issues and merge requests !5453 (tomb0y)
 
 v 8.10.3 (unreleased)
   - Fix hooks missing on imported GitLab projects

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

app/controllers/concerns/issuable_collections.rb

+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

app/controllers/concerns/issues_action.rb

 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

app/controllers/concerns/merge_requests_action.rb

 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

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/

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/

app/finders/issuable_finder.rb

@@ -109,7 +109,7 @@ class IssuableFinder
 
         scope.where(title: params[:milestone_title])
       else
-        nil
+        Milestone.none
       end
   end
 

app/helpers/application_helper.rb

@@ -245,7 +245,6 @@ module ApplicationHelper
       milestone_title: params[:milestone_title],
       assignee_id: params[:assignee_id],
       author_id: params[:author_id],
-      sort: params[:sort],
       issue_search: params[:issue_search],
       label_name: params[:label_name]
     }

app/helpers/sorting_helper.rb

@@ -102,11 +102,11 @@ module SortingHelper
   end
 
   def sort_value_oldest_created
-    'id_asc'
+    'created_asc'
   end
 
   def sort_value_recently_created
-    'id_desc'
+    'created_desc'
   end
 
   def sort_value_milestone_soon

app/views/projects/issues/index.html.haml

@@ -19,7 +19,13 @@
               Subscribe
         = render 'shared/issuable/search_form', path: namespace_project_issues_path(@project.namespace, @project)
         - if can? current_user, :create_issue, @project
-          = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { assignee_id: @issuable_finder.assignee.try(:id), milestone_id: @issuable_finder.milestones.try(:first).try(:id) }), class: "btn btn-new", title: "New Issue", id: "new_issue_link" do
+          = link_to new_namespace_project_issue_path(@project.namespace,
+                                                     @project,
+                                                     issue: { assignee_id: issues_finder.assignee.try(:id),
+                                                              milestone_id: issues_finder.milestones.first.try(:id) }),
+                                                     class: "btn btn-new",
+                                                     title: "New Issue",
+                                                     id: "new_issue_link" do
             New Issue
     = render 'shared/issuable/filter', type: :issues
 

spec/features/issuables/default_sort_order_spec.rb

+require 'spec_helper'
+
+describe 'Projects > Issuables > Default sort order', feature: true do
+  let(:project) { create(:empty_project, :public) }
+
+  let(:first_created_issuable) { issuables.order_created_asc.first }
+  let(:last_created_issuable) { issuables.order_created_desc.first }
+
+  let(:first_updated_issuable) { issuables.order_updated_asc.first }
+  let(:last_updated_issuable) { issuables.order_updated_desc.first }
+
+  context 'for merge requests' do
+    include MergeRequestHelpers
+
+    let!(:issuables) do
+      timestamps = [{ created_at: 3.minutes.ago, updated_at: 20.seconds.ago },
+                    { created_at: 2.minutes.ago, updated_at: 30.seconds.ago },
+                    { created_at: 4.minutes.ago, updated_at: 10.seconds.ago }]
+
+      timestamps.each_with_index do |ts, i|
+        create issuable_type, { title: "#{issuable_type}_#{i}",
+                                source_branch: "#{issuable_type}_#{i}",
+                                source_project: project }.merge(ts)
+      end
+
+      MergeRequest.all
+    end
+
+    context 'in the "merge requests" tab', js: true do
+      let(:issuable_type) { :merge_request }
+
+      it 'is "last created"' do
+        visit_merge_requests project
+
+        expect(first_merge_request).to include(last_created_issuable.title)
+        expect(last_merge_request).to include(first_created_issuable.title)
+      end
+    end
+
+    context 'in the "merge requests / open" tab', js: true do
+      let(:issuable_type) { :merge_request }
+
+      it 'is "last created"' do
+        visit_merge_requests_with_state(project, 'open')
+
+        expect(selected_sort_order).to eq('last created')
+        expect(first_merge_request).to include(last_created_issuable.title)
+        expect(last_merge_request).to include(first_created_issuable.title)
+      end
+    end
+
+    context 'in the "merge requests / merged" tab', js: true do
+      let(:issuable_type) { :merged_merge_request }
+
+      it 'is "last updated"' do
+        visit_merge_requests_with_state(project, 'merged')
+
+        expect(selected_sort_order).to eq('last updated')
+        expect(first_merge_request).to include(last_updated_issuable.title)
+        expect(last_merge_request).to include(first_updated_issuable.title)
+      end
+    end
+
+    context 'in the "merge requests / closed" tab', js: true do
+      let(:issuable_type) { :closed_merge_request }
+
+      it 'is "last updated"' do
+        visit_merge_requests_with_state(project, 'closed')
+
+        expect(selected_sort_order).to eq('last updated')
+        expect(first_merge_request).to include(last_updated_issuable.title)
+        expect(last_merge_request).to include(first_updated_issuable.title)
+      end
+    end
+
+    context 'in the "merge requests / all" tab', js: true do
+      let(:issuable_type) { :merge_request }
+
+      it 'is "last created"' do
+        visit_merge_requests_with_state(project, 'all')
+
+        expect(selected_sort_order).to eq('last created')
+        expect(first_merge_request).to include(last_created_issuable.title)
+        expect(last_merge_request).to include(first_created_issuable.title)
+      end
+    end
+  end
+
+  context 'for issues' do
+    include IssueHelpers
+
+    let!(:issuables) do
+      timestamps = [{ created_at: 3.minutes.ago, updated_at: 20.seconds.ago },
+                    { created_at: 2.minutes.ago, updated_at: 30.seconds.ago },
+                    { created_at: 4.minutes.ago, updated_at: 10.seconds.ago }]
+
+      timestamps.each_with_index do |ts, i|
+        create issuable_type, { title: "#{issuable_type}_#{i}",
+                                project: project }.merge(ts)
+      end
+
+      Issue.all
+    end
+
+    context 'in the "issues" tab', js: true do
+      let(:issuable_type) { :issue }
+
+      it 'is "last created"' do
+        visit_issues project
+
+        expect(selected_sort_order).to eq('last created')
+        expect(first_issue).to include(last_created_issuable.title)
+        expect(last_issue).to include(first_created_issuable.title)
+      end
+    end
+
+    context 'in the "issues / open" tab', js: true do
+      let(:issuable_type) { :issue }
+
+      it 'is "last created"' do
+        visit_issues_with_state(project, 'open')
+
+        expect(selected_sort_order).to eq('last created')
+        expect(first_issue).to include(last_created_issuable.title)
+        expect(last_issue).to include(first_created_issuable.title)
+      end
+    end
+
+    context 'in the "issues / closed" tab', js: true do
+      let(:issuable_type) { :closed_issue }
+
+      it 'is "last updated"' do
+        visit_issues_with_state(project, 'closed')
+
+        expect(selected_sort_order).to eq('last updated')
+        expect(first_issue).to include(last_updated_issuable.title)
+        expect(last_issue).to include(first_updated_issuable.title)
+      end
+    end
+
+    context 'in the "issues / all" tab', js: true do
+      let(:issuable_type) { :issue }
+
+      it 'is "last created"' do
+        visit_issues_with_state(project, 'all')
+
+        expect(selected_sort_order).to eq('last created')
+        expect(first_issue).to include(last_created_issuable.title)
+        expect(last_issue).to include(first_created_issuable.title)
+      end
+    end
+  end
+
+  def selected_sort_order
+    find('.pull-right .dropdown button').text.downcase
+  end
+
+  def visit_merge_requests_with_state(project, state)
+    visit_merge_requests project
+    visit_issuables_with_state state
+  end
+
+  def visit_issues_with_state(project, state)
+    visit_issues project
+    visit_issuables_with_state state
+  end
+
+  def visit_issuables_with_state(state)
+    within('.issues-state-filters') { find("span", text: state.titleize).click }
+  end
+end

spec/features/issues_spec.rb

 require 'spec_helper'
 
 describe 'Issues', feature: true do
+  include IssueHelpers
   include SortingHelper
 
   let(:project) { create(:project) }
@@ -186,15 +187,15 @@ describe 'Issues', feature: true do
     it 'sorts by newest' do
       visit namespace_project_issues_path(project.namespace, project, sort: sort_value_recently_created)
 
-      expect(first_issue).to include('baz')
-      expect(last_issue).to include('foo')
+      expect(first_issue).to include('foo')
+      expect(last_issue).to include('baz')
     end
 
     it 'sorts by oldest' do
       visit namespace_project_issues_path(project.namespace, project, sort: sort_value_oldest_created)
 
-      expect(first_issue).to include('foo')
-      expect(last_issue).to include('baz')
+      expect(first_issue).to include('baz')
+      expect(last_issue).to include('foo')
     end
 
     it 'sorts by most recently updated' do
@@ -350,8 +351,8 @@ describe 'Issues', feature: true do
                                             sort: sort_value_oldest_created,
                                             assignee_id: user2.id)
 
-        expect(first_issue).to include('foo')
-        expect(last_issue).to include('bar')
+        expect(first_issue).to include('bar')
+        expect(last_issue).to include('foo')
         expect(page).not_to have_content 'baz'
       end
     end
@@ -590,14 +591,6 @@ describe 'Issues', feature: true do
     end
   end
 
-  def first_issue
-    page.all('ul.issues-list > li').first.text
-  end
-
-  def last_issue
-    page.all('ul.issues-list > li').last.text
-  end
-
   def drop_in_dropzone(file_path)
     # Generate a fake input selector
     page.execute_script <<-JS

spec/features/merge_requests/user_lists_merge_requests_spec.rb

 require 'spec_helper'
 
 describe 'Projects > Merge requests > User lists merge requests', feature: true do
+  include MergeRequestHelpers
   include SortingHelper
 
   let(:project) { create(:project, :public) }
@@ -23,10 +24,12 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
            milestone: create(:milestone, due_date: '2013-12-12'),
            created_at: 2.minutes.ago,
            updated_at: 2.minutes.ago)
+    # lfs in itself is not a great choice for the title if one wants to match the whole body content later on
+    # just think about the scenario when faker generates 'Chester Runolfsson' as the user's name
     create(:merge_request,
-           title: 'lfs',
+           title: 'merge_lfs',
            source_project: project,
-           source_branch: 'lfs',
+           source_branch: 'merge_lfs',
            created_at: 3.minutes.ago,
            updated_at: 10.seconds.ago)
   end
@@ -35,7 +38,7 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
     visit_merge_requests(project, assignee_id: IssuableFinder::NONE)
 
     expect(current_path).to eq(namespace_project_merge_requests_path(project.namespace, project))
-    expect(page).to have_content 'lfs'
+    expect(page).to have_content 'merge_lfs'
     expect(page).not_to have_content 'fix'
     expect(page).not_to have_content 'markdown'
     expect(count_merge_requests).to eq(1)
@@ -44,7 +47,7 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
   it 'filters on a specific assignee' do
     visit_merge_requests(project, assignee_id: user.id)
 
-    expect(page).not_to have_content 'lfs'
+    expect(page).not_to have_content 'merge_lfs'
     expect(page).to have_content 'fix'
     expect(page).to have_content 'markdown'
     expect(count_merge_requests).to eq(2)
@@ -53,23 +56,23 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
   it 'sorts by newest' do
     visit_merge_requests(project, sort: sort_value_recently_created)
 
-    expect(first_merge_request).to include('lfs')
-    expect(last_merge_request).to include('fix')
+    expect(first_merge_request).to include('fix')
+    expect(last_merge_request).to include('merge_lfs')
     expect(count_merge_requests).to eq(3)
   end
 
   it 'sorts by oldest' do
     visit_merge_requests(project, sort: sort_value_oldest_created)
 
-    expect(first_merge_request).to include('fix')
-    expect(last_merge_request).to include('lfs')
+    expect(first_merge_request).to include('merge_lfs')
+    expect(last_merge_request).to include('fix')
     expect(count_merge_requests).to eq(3)
   end
 
   it 'sorts by last updated' do
     visit_merge_requests(project, sort: sort_value_recently_updated)
 
-    expect(first_merge_request).to include('lfs')
+    expect(first_merge_request).to include('merge_lfs')
     expect(count_merge_requests).to eq(3)
   end
 
@@ -143,18 +146,6 @@ describe 'Projects > Merge requests > User lists merge requests', feature: true
     end
   end
 
-  def visit_merge_requests(project, opts = {})
-    visit namespace_project_merge_requests_path(project.namespace, project, opts)
-  end
-
-  def first_merge_request
-    page.all('ul.mr-list > li').first.text
-  end
-
-  def last_merge_request
-    page.all('ul.mr-list > li').last.text
-  end
-
   def count_merge_requests
     page.all('ul.mr-list > li').count
   end

spec/support/issue_helpers.rb

+module IssueHelpers
+  def visit_issues(project, opts = {})
+    visit namespace_project_issues_path project.namespace, project, opts
+  end
+
+  def first_issue
+    page.all('ul.issues-list > li').first.text
+  end
+
+  def last_issue
+    page.all('ul.issues-list > li').last.text
+  end
+end

spec/support/merge_request_helpers.rb

+module MergeRequestHelpers
+  def visit_merge_requests(project, opts = {})
+    visit namespace_project_merge_requests_path project.namespace, project, opts
+  end
+
+  def first_merge_request
+    page.all('ul.mr-list > li').first.text
+  end
+
+  def last_merge_request
+    page.all('ul.mr-list > li').last.text
+  end
+end