Allow sorting by due date and label priority

10 files changed, 106 insertions, 15 deletions

diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb
index 18734f1411f..959ee310867 100644
--- a/app/helpers/sorting_helper.rb
+++ b/app/helpers/sorting_helper.rb
@@ -16,7 +16,8 @@ module SortingHelper
       sort_value_oldest_signin => sort_title_oldest_signin,
       sort_value_downvotes => sort_title_downvotes,
       sort_value_upvotes => sort_title_upvotes,
-      sort_value_priority => sort_title_priority
+      sort_value_priority => sort_title_priority,
+      sort_value_label_priority => sort_title_label_priority
     }
   end
 
@@ -50,6 +51,10 @@ module SortingHelper
   end
 
   def sort_title_priority
+    'Priority'
+  end
+
+  def sort_title_label_priority
     'Label priority'
   end
 
@@ -161,6 +166,10 @@ module SortingHelper
     'priority'
   end
 
+  def sort_value_label_priority
+    'label_priority'
+  end
+
   def sort_value_oldest_updated
     'updated_asc'
   end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 3cf4c67d7e7..3b2c6a178e7 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -144,7 +144,8 @@ module Issuable
                when 'milestone_due_desc' then order_milestone_due_desc
                when 'downvotes_desc' then order_downvotes_desc
                when 'upvotes_desc' then order_upvotes_desc
-               when 'priority' then order_labels_priority(excluded_labels: excluded_labels)
+               when 'label_priority' then order_labels_priority(excluded_labels: excluded_labels)
+               when 'priority' then order_due_date_and_labels_priority(excluded_labels: excluded_labels)
                when 'position_asc' then  order_position_asc
                else
                  order_by(method)
@@ -154,7 +155,28 @@ module Issuable
       sorted.order(id: :desc)
     end
 
-    def order_labels_priority(excluded_labels: [])
+    def order_due_date_and_labels_priority(excluded_labels: [])
+      # The order_ methods also modify the query in other ways:
+      #
+      # - For milestones, we add a JOIN.
+      # - For label priority, we change the SELECT, and add a GROUP BY.#
+      #
+      # After doing those, we need to reorder to the order we want. The existing
+      # ORDER BYs won't work because:
+      #
+      # 1. We need milestone due date first.
+      # 2. We can't ORDER BY a column that isn't in the GROUP BY and doesn't
+      #    have an aggregate function applied, so we do a useless MIN() instead.
+      #
+      milestones_due_date = 'MIN(milestones.due_date)'
+
+      order_milestone_due_asc.
+        order_labels_priority(excluded_labels: excluded_labels, extra_select_columns: [milestones_due_date]).
+        reorder(Gitlab::Database.nulls_last_order(milestones_due_date, 'ASC'),
+                Gitlab::Database.nulls_last_order('highest_priority', 'ASC'))
+    end
+
+    def order_labels_priority(excluded_labels: [], extra_select_columns: [])
       params = {
         target_type: name,
         target_column: "#{table_name}.id",
@@ -164,7 +186,12 @@ module Issuable
 
       highest_priority = highest_label_priority(params).to_sql
 
-      select("#{table_name}.*, (#{highest_priority}) AS highest_priority").
+      select_columns = [
+        "#{table_name}.*",
+        "(#{highest_priority}) AS highest_priority"
+      ] + extra_select_columns
+
+      select(select_columns.join(', ')).
         group(arel_table[:id]).
         reorder(Gitlab::Database.nulls_last_order('highest_priority', 'ASC'))
     end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 47789a21133..da3fa7277c2 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -48,8 +48,14 @@ class Todo < ActiveRecord::Base
   after_save :keep_around_commit
 
   class << self
+    # Priority sorting isn't displayed in the dropdown, because we don't show
+    # milestones, but still show something if the user has a URL with that
+    # selected.
     def sort(method)
-      method == "priority" ? order_by_labels_priority : order_by(method)
+      case method.to_s
+      when 'priority', 'label_priority' then order_by_labels_priority
+      else order_by(method)
+      end
     end
 
     # Order by priority depending on which issue/merge request the Todo belongs to
diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml
index d7e0a8e4b2c..3ed67d9258c 100644
--- a/app/views/dashboard/todos/index.html.haml
+++ b/app/views/dashboard/todos/index.html.haml
@@ -57,8 +57,8 @@
               = icon('chevron-down')
             %ul.dropdown-menu.dropdown-menu-sort
               %li
-                = link_to todos_filter_path(sort: sort_value_priority) do
-                  = sort_title_priority
+                = link_to todos_filter_path(sort: sort_value_label_priority) do
+                  = sort_title_label_priority
                 = link_to todos_filter_path(sort: sort_value_recently_created) do
                   = sort_title_recently_created
                 = link_to todos_filter_path(sort: sort_value_oldest_created) do
diff --git a/app/views/shared/_sort_dropdown.html.haml b/app/views/shared/_sort_dropdown.html.haml
index 0ce0d759e86..367aa550a78 100644
--- a/app/views/shared/_sort_dropdown.html.haml
+++ b/app/views/shared/_sort_dropdown.html.haml
@@ -10,6 +10,8 @@
     %li
       = link_to page_filter_path(sort: sort_value_priority, label: true) do
         = sort_title_priority
+      = link_to page_filter_path(sort: sort_value_label_priority, label: true) do
+        = sort_title_label_priority
       = link_to page_filter_path(sort: sort_value_recently_created, label: true) do
         = sort_title_recently_created
       = link_to page_filter_path(sort: sort_value_oldest_created, label: true) do
diff --git a/app/views/shared/empty_states/_labels.html.haml b/app/views/shared/empty_states/_labels.html.haml
index ba5c2dae09d..00fb77bdb3b 100644
--- a/app/views/shared/empty_states/_labels.html.haml
+++ b/app/views/shared/empty_states/_labels.html.haml
@@ -5,7 +5,7 @@
   .col-xs-12.col-sm-6
     .text-content
       %h4 Labels can be applied to issues and merge requests to categorize them.
-      %p You can also star label to make it a priority label.
+      %p You can also star a label to make it a priority label.
       - if can?(current_user, :admin_label, @project)
         = link_to 'New label', new_namespace_project_label_path(@project.namespace, @project), class: 'btn btn-new', title: 'New label', id: 'new_label_link'
         = link_to 'Generate a default set of labels', generate_namespace_project_labels_path(@project.namespace, @project), method: :post, class: 'btn btn-success btn-inverted', title: 'Generate a default set of labels', id: 'generate_labels_link'
diff --git a/changelogs/unreleased/better-priority-sorting.yml b/changelogs/unreleased/better-priority-sorting.yml
new file mode 100644
index 00000000000..a44cd090ceb
--- /dev/null
+++ b/changelogs/unreleased/better-priority-sorting.yml
@@ -0,0 +1,4 @@
+---
+title: Allow sorting by due date and priority
+merge_request:
+author:
diff --git a/doc/user/project/labels.md b/doc/user/project/labels.md
index cf1d9cbe69c..8ec7adad172 100644
--- a/doc/user/project/labels.md
+++ b/doc/user/project/labels.md
@@ -65,7 +65,7 @@ issues and merge requests assigned to each label.
 >   https://gitlab.com/gitlab-org/gitlab-ce/issues/18554.
 
 Prioritized labels are like any other label, but sorted by priority. This allows
-you to sort issues and merge requests by priority.
+you to sort issues and merge requests by label priority.
 
 To prioritize labels, navigate to your project's **Issues > Labels** and click
 on the star icon next to them to put them in the priority list. Click on the
@@ -77,9 +77,13 @@ having their priority set to null.
 
 ![Prioritize labels](img/labels_prioritize.png)
 
-Now that you have labels prioritized, you can use the 'Priority' filter in the
-issues or merge requests tracker. Those with the highest priority label, will
-appear on top.
+Now that you have labels prioritized, you can use the 'Priority' and 'Label
+priority' filters in the issues or merge requests tracker.
+
+The 'Label priority' filter puts issues with the highest priority label on top.
+
+The 'Priority' filter sorts issues by their soonest milestone due date, then by
+label priority.
 
 ![Filter labels by priority](img/labels_filter_by_priority.png)
 
@@ -156,4 +160,3 @@ mouse over the label in the issue tracker or wherever else the label is
 rendered.
 
 ![Label tooltips](img/labels_description_tooltip.png)
-
diff --git a/spec/features/projects/labels/issues_sorted_by_priority_spec.rb b/spec/features/projects/labels/issues_sorted_by_priority_spec.rb
index de3c6eceb82..e2911a37e40 100644
--- a/spec/features/projects/labels/issues_sorted_by_priority_spec.rb
+++ b/spec/features/projects/labels/issues_sorted_by_priority_spec.rb
@@ -29,7 +29,7 @@ feature 'Issue prioritization', feature: true do
       issue_1.labels << label_5
 
       login_as user
-      visit namespace_project_issues_path(project.namespace, project, sort: 'priority')
+      visit namespace_project_issues_path(project.namespace, project, sort: 'label_priority')
 
       # Ensure we are indicating that issues are sorted by priority
       expect(page).to have_selector('.dropdown-toggle', text: 'Label priority')
@@ -68,7 +68,7 @@ feature 'Issue prioritization', feature: true do
       issue_6.labels << label_5 # 8 - No priority
 
       login_as user
-      visit namespace_project_issues_path(project.namespace, project, sort: 'priority')
+      visit namespace_project_issues_path(project.namespace, project, sort: 'label_priority')
 
       expect(page).to have_selector('.dropdown-toggle', text: 'Label priority')
 
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 545a11912e3..31ae0dce140 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -344,6 +344,46 @@ describe Issue, "Issuable" do
     end
   end
 
+  describe '.order_due_date_and_labels_priority' do
+    let(:project) { create(:empty_project) }
+
+    def create_issue(milestone, labels)
+      create(:labeled_issue, milestone: milestone, labels: labels, project: project)
+    end
+
+    it 'sorts issues in order of milestone due date, then label priority' do
+      first_priority = create(:label, project: project, priority: 1)
+      second_priority = create(:label, project: project, priority: 2)
+      no_priority = create(:label, project: project)
+
+      first_milestone = create(:milestone, project: project, due_date: Time.now)
+      second_milestone = create(:milestone, project: project, due_date: Time.now + 1.month)
+      third_milestone = create(:milestone, project: project)
+
+      # The issues here are ordered by label priority, to ensure that we don't
+      # accidentally just sort by creation date.
+      second_milestone_first_priority = create_issue(second_milestone, [first_priority, second_priority, no_priority])
+      third_milestone_first_priority = create_issue(third_milestone, [first_priority, second_priority, no_priority])
+      first_milestone_second_priority = create_issue(first_milestone, [second_priority, no_priority])
+      second_milestone_second_priority = create_issue(second_milestone, [second_priority, no_priority])
+      no_milestone_second_priority = create_issue(nil, [second_priority, no_priority])
+      first_milestone_no_priority = create_issue(first_milestone, [no_priority])
+      second_milestone_no_labels = create_issue(second_milestone, [])
+      third_milestone_no_priority = create_issue(third_milestone, [no_priority])
+
+      result = Issue.order_due_date_and_labels_priority
+
+      expect(result).to eq([first_milestone_second_priority,
+                            first_milestone_no_priority,
+                            second_milestone_first_priority,
+                            second_milestone_second_priority,
+                            second_milestone_no_labels,
+                            third_milestone_first_priority,
+                            no_milestone_second_priority,
+                            third_milestone_no_priority])
+    end
+  end
+
   describe '.order_labels_priority' do
     let(:label_1) { create(:label, title: 'label_1', project: issue.project, priority: 1) }
     let(:label_2) { create(:label, title: 'label_2', project: issue.project, priority: 2) }