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

3 files changed, 190 insertions, 35 deletions

diff --git a/spec/features/issuables/default_sort_order_spec.rb b/spec/features/issuables/default_sort_order_spec.rb
new file mode 100644
index 00000000000..0d495cd04aa
--- /dev/null
+++ b/spec/features/issuables/default_sort_order_spec.rb
@@ -0,0 +1,171 @@
+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
diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb
index 93dcb2ec3fc..9c92b52898c 100644
--- a/spec/features/issues_spec.rb
+++ b/spec/features/issues_spec.rb
@@ -1,6 +1,7 @@
 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
diff --git a/spec/features/merge_requests/user_lists_merge_requests_spec.rb b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
index 1c130057c56..cabb8e455f9 100644
--- a/spec/features/merge_requests/user_lists_merge_requests_spec.rb
+++ b/spec/features/merge_requests/user_lists_merge_requests_spec.rb
@@ -1,6 +1,7 @@
 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