Merge branch 'better-priority-sorting' into 'master'

Better priority sorting

Closes #28754

See merge request !9938

app/assets/javascripts/boards/components/modal/filters/milestone.js

@@ -20,6 +20,7 @@ module.exports = Vue.extend({
         data-toggle="dropdown"
         data-show-any="true"
         data-show-upcoming="true"
+        data-show-started="true"
         data-field-name="milestone_title"
         :data-milestones="milestonePath"
         ref="dropdown">

app/assets/javascripts/filtered_search/filtered_search_token_keys.js

@@ -42,6 +42,10 @@
     url: 'milestone_title=%23upcoming',
     tokenKey: 'milestone',
     value: 'upcoming',
+  }, {
+    url: 'milestone_title=%23started',
+    tokenKey: 'milestone',
+    value: 'started',
   }, {
     url: 'label_name[]=No+Label',
     tokenKey: 'label',

app/assets/javascripts/milestone_select.js

@@ -19,7 +19,7 @@
       }
 
       $els.each(function(i, dropdown) {
-        var $block, $dropdown, $loading, $selectbox, $sidebarCollapsedValue, $value, abilityName, collapsedSidebarLabelTemplate, defaultLabel, issuableId, issueUpdateURL, milestoneLinkNoneTemplate, milestoneLinkTemplate, milestonesUrl, projectId, selectedMilestone, showAny, showNo, showUpcoming, useId, showMenuAbove;
+        var $block, $dropdown, $loading, $selectbox, $sidebarCollapsedValue, $value, abilityName, collapsedSidebarLabelTemplate, defaultLabel, issuableId, issueUpdateURL, milestoneLinkNoneTemplate, milestoneLinkTemplate, milestonesUrl, projectId, selectedMilestone, showAny, showNo, showUpcoming, showStarted, useId, showMenuAbove;
         $dropdown = $(dropdown);
         projectId = $dropdown.data('project-id');
         milestonesUrl = $dropdown.data('milestones');
@@ -29,6 +29,7 @@
         showAny = $dropdown.data('show-any');
         showMenuAbove = $dropdown.data('showMenuAbove');
         showUpcoming = $dropdown.data('show-upcoming');
+        showStarted = $dropdown.data('show-started');
         useId = $dropdown.data('use-id');
         defaultLabel = $dropdown.data('default-label');
         issuableId = $dropdown.data('issuable-id');
@@ -71,6 +72,13 @@
                   title: 'Upcoming'
                 });
               }
+              if (showStarted) {
+                extraOptions.push({
+                  id: -3,
+                  name: '#started',
+                  title: 'Started'
+                });
+              }
               if (extraOptions.length) {
                 extraOptions.push('divider');
               }

app/finders/issuable_finder.rb

@@ -310,6 +310,10 @@ class IssuableFinder
     params[:milestone_title] == Milestone::Upcoming.name
   end
 
+  def filter_by_started_milestone?
+    params[:milestone_title] == Milestone::Started.name
+  end
+
   def by_milestone(items)
     if milestones?
       if filter_by_no_milestone?
@@ -317,6 +321,8 @@ class IssuableFinder
       elsif filter_by_upcoming_milestone?
         upcoming_ids = Milestone.upcoming_ids_by_projects(projects(items))
         items = items.left_joins_milestones.where(milestone_id: upcoming_ids)
+      elsif filter_by_started_milestone?
+        items = items.left_joins_milestones.where('milestones.start_date <= NOW()')
       else
         items = items.with_milestone(params[:milestone_title])
         items_projects = projects(items)

app/helpers/issuables_helper.rb

@@ -90,11 +90,14 @@ module IssuablesHelper
   end
 
   def milestone_dropdown_label(milestone_title, default_label = "Milestone")
-    if milestone_title == Milestone::Upcoming.name
-      milestone_title = Milestone::Upcoming.title
+    title =
+      case milestone_title
+      when Milestone::Upcoming.name then Milestone::Upcoming.title
+      when Milestone::Started.name then Milestone::Started.title
+      else milestone_title.presence
       end
 
-    h(milestone_title.presence || default_label)
+    h(title || default_label)
   end
 
   def to_url_reference(issuable)

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

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

app/models/milestone.rb

@@ -5,6 +5,7 @@ class Milestone < ActiveRecord::Base
   None = MilestoneStruct.new('No Milestone', 'No Milestone', 0)
   Any = MilestoneStruct.new('Any Milestone', '', -1)
   Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2)
+  Started = MilestoneStruct.new('Started', '#started', -3)
 
   include CacheMarkdownField
   include InternalId

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

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

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

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'

app/views/shared/issuable/_filter.html.haml

@@ -24,7 +24,7 @@
             placeholder: "Search assignee", data: { any_user: "Any Assignee", first_user: current_user.try(:username), null_user: true, current_user: true, project_id: @project.try(:id), selected: params[:assignee_id], field_name: "assignee_id", default_label: "Assignee" } })
 
         .filter-item.inline.milestone-filter
-          = render "shared/issuable/milestone_dropdown", selected: finder.milestones.try(:first), name: :milestone_title, show_any: true, show_upcoming: true
+          = render "shared/issuable/milestone_dropdown", selected: finder.milestones.try(:first), name: :milestone_title, show_any: true, show_upcoming: true, show_started: true
 
         .filter-item.inline.labels-filter
           = render "shared/issuable/label_dropdown", selected: finder.labels.select(:title).uniq, use_id: false, selected_toggle: params[:label_name], data_options: { field_name: "label_name[]" }

app/views/shared/issuable/_milestone_dropdown.html.haml

@@ -6,7 +6,7 @@
 - if selected.present? || params[:milestone_title].present?
   = hidden_field_tag(name, name == :milestone_title ? selected_text : selected.id)
 = dropdown_tag(milestone_dropdown_label(selected_text), options: { title: dropdown_title, toggle_class: "js-milestone-select js-filter-submit #{extra_class}", filter: true, dropdown_class: "dropdown-menu-selectable dropdown-menu-milestone",
-  placeholder: "Search milestones", footer_content: project.present?, data: { show_no: true, show_menu_above: show_menu_above, show_any: show_any, show_upcoming: show_upcoming, field_name: name, selected: selected.try(:title), project_id: project.try(:id), milestones: milestones_filter_dropdown_path, default_label: "Milestone" } }) do
+  placeholder: "Search milestones", footer_content: project.present?, data: { show_no: true, show_menu_above: show_menu_above, show_any: show_any, show_upcoming: show_upcoming, show_started: show_started, field_name: name, selected: selected.try(:title), project_id: project.try(:id), milestones: milestones_filter_dropdown_path, default_label: "Milestone" } }) do
   - if project
     %ul.dropdown-footer-list
       - if can? current_user, :admin_milestone, project

app/views/shared/issuable/_search_bar.html.haml

@@ -68,6 +68,9 @@
               %li.filter-dropdown-item{ data: { value: 'upcoming' } }
                 %button.btn.btn-link
                   Upcoming
+              %li.filter-dropdown-item{ 'data-value' => 'started' }
+                %button.btn.btn-link
+                  Started
               %li.divider
             %ul.filter-dropdown{ data: { dynamic: true, dropdown: true } }
               %li.filter-dropdown-item

app/views/shared/issuable/form/_metadata.html.haml

@@ -21,7 +21,7 @@
       = form.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}"
       .col-sm-10{ class: ("col-lg-8" if has_due_date) }
         .issuable-form-select-holder
-          = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone"
+          = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, show_started: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input", dropdown_title: "Select milestone"
     .form-group
       - has_labels = @labels && @labels.any?
       = form.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}"

changelogs/unreleased/better-priority-sorting-2.yml

+---
+title: Allow filtering by all started milestones
+merge_request:
+author:

changelogs/unreleased/better-priority-sorting.yml

+---
+title: Allow sorting by due date and priority
+merge_request:
+author:

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)
-

doc/workflow/milestones.md

@@ -11,3 +11,18 @@ You can create a milestone for several projects in the same group simultaneously
 On the group's milestones page, you will be able to see the status of that milestone across all of the selected projects.
 
 ![group milestone form](milestones/group_form.png)
+
+## Special milestone filters
+
+In addition to the milestones that exist in the project or group, there are some
+special options available when filtering by milestone:
+
+* **No Milestone** - only show issues or merge requests without a milestone.
+* **Upcoming** - show issues or merge request that belong to the next open
+  milestone with a due date, by project. (For example: if project A has
+  milestone v1 due in three days, and project B has milestone v2 due in a week,
+  then this will show issues or merge requests from milestone v1 in project A
+  and milestone v2 in project B.)
+* **Started** - show issues or merge requests from any milestone with a start
+  date less than today. Note that this can return results from several
+  milestones in the same project.

spec/features/issues/filtered_search/dropdown_milestone_spec.rb

@@ -202,6 +202,14 @@ describe 'Dropdown milestone', :feature, :js do
       expect_tokens([{ name: 'milestone', value: 'upcoming' }])
       expect_filtered_search_input_empty
     end
+
+    it 'selects `started milestones`' do
+      click_static_milestone('Started')
+
+      expect(page).to have_css(js_dropdown_milestone, visible: false)
+      expect_tokens([{ name: 'milestone', value: 'started' }])
+      expect_filtered_search_input_empty
+    end
   end
 
   describe 'input has existing content' do

spec/features/issues/filtered_search/filter_issues_spec.rb

@@ -8,13 +8,12 @@ describe 'Filter issues', js: true, feature: true do
   let!(:project) { create(:project, group: group) }
   let!(:user) { create(:user) }
   let!(:user2) { create(:user) }
-  let!(:milestone) { create(:milestone, project: project) }
   let!(:label) { create(:label, project: project) }
   let!(:wontfix) { create(:label, project: project, title: "Won't fix") }
 
   let!(:bug_label) { create(:label, project: project, title: 'bug') }
   let!(:caps_sensitive_label) { create(:label, project: project, title: 'CAPS_sensitive') }
-  let!(:milestone) { create(:milestone, title: "8", project: project) }
+  let!(:milestone) { create(:milestone, title: "8", project: project, start_date: 2.days.ago) }
   let!(:multiple_words_label) { create(:label, project: project, title: "Two words") }
 
   let!(:closed_issue) { create(:issue, title: 'bug that is closed', project: project, state: :closed) }
@@ -505,6 +504,14 @@ describe 'Filter issues', js: true, feature: true do
         expect_filtered_search_input_empty
       end
 
+      it 'filters issues by started milestones' do
+        input_filtered_search("milestone:started")
+
+        expect_tokens([{ name: 'milestone', value: 'started' }])
+        expect_issues_list_count(5)
+        expect_filtered_search_input_empty
+      end
+
       it 'filters issues by invalid milestones' do
         skip('to be tested, issue #26546')
       end

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')
 

spec/finders/issues_finder_spec.rb

@@ -101,6 +101,41 @@ describe IssuesFinder do
         end
       end
 
+      context 'filtering by started milestone' do
+        let(:params) { { milestone_title: Milestone::Started.name } }
+
+        let(:project_no_started_milestones) { create(:empty_project, :public) }
+        let(:project_started_1_and_2) { create(:empty_project, :public) }
+        let(:project_started_8) { create(:empty_project, :public) }
+
+        let(:yesterday) { Date.today - 1.day }
+        let(:tomorrow) { Date.today + 1.day }
+        let(:two_days_ago) { Date.today - 2.days }
+
+        let(:milestones) do
+          [
+            create(:milestone, project: project_no_started_milestones, start_date: tomorrow),
+            create(:milestone, project: project_started_1_and_2, title: '1.0', start_date: two_days_ago),
+            create(:milestone, project: project_started_1_and_2, title: '2.0', start_date: yesterday),
+            create(:milestone, project: project_started_1_and_2, title: '3.0', start_date: tomorrow),
+            create(:milestone, project: project_started_8, title: '7.0'),
+            create(:milestone, project: project_started_8, title: '8.0', start_date: yesterday),
+            create(:milestone, project: project_started_8, title: '9.0', start_date: tomorrow)
+          ]
+        end
+
+        before do
+          milestones.each do |milestone|
+            create(:issue, project: milestone.project, milestone: milestone, author: user, assignee: user)
+          end
+        end
+
+        it 'returns issues in the started milestones for each project' do
+          expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.0', '2.0', '8.0')
+          expect(issues.map { |issue| issue.milestone.start_date }).to contain_exactly(two_days_ago, yesterday, yesterday)
+        end
+      end
+
       context 'filtering by label' do
         let(:params) { { label_name: label.title } }
 

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) }