Merge branch 'issue_15221_2'

4 files changed, 40 insertions, 7 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b515d2f09b8..017dff0fa81 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.9.0 (unreleased)
   - Redesign navigation for project pages
   - Use gitlab-shell v3.0.0
   - Changed the Slack build message to use the singular duration if necessary (Aran Koning)
+  - Fix issues filter when ordering by milestone
 
 v 8.8.2 (unreleased)
   - Fix Error 500 when accessing application settings due to nil disabled OAuth sign-in sources
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 5849e00662b..8ed3ccf1c02 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -250,12 +250,12 @@ class IssuableFinder
   def by_milestone(items)
     if milestones?
       if filter_by_no_milestone?
-        items = items.where(milestone_id: [-1, nil])
+        items = items.left_joins_milestones.where(milestone_id: [-1, nil])
       elsif filter_by_upcoming_milestone?
         upcoming_ids = Milestone.upcoming_ids_by_projects(projects)
-        items = items.joins(:milestone).where(milestone_id: upcoming_ids)
+        items = items.left_joins_milestones.where(milestone_id: upcoming_ids)
       else
-        items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] })
+        items = items.with_milestone(params[:milestone_title])
 
         if projects
           items = items.where(milestones: { project_id: projects })
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index c1248b53031..91315b3459f 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -31,18 +31,21 @@ module Issuable
     scope :unassigned, -> { where("assignee_id IS NULL") }
     scope :of_projects, ->(ids) { where(project_id: ids) }
     scope :of_milestones, ->(ids) { where(milestone_id: ids) }
+    scope :with_milestone, ->(title) { left_joins_milestones.where(milestones: { title: title }) }
     scope :opened, -> { with_state(:opened, :reopened) }
     scope :only_opened, -> { with_state(:opened) }
     scope :only_reopened, -> { with_state(:reopened) }
     scope :closed, -> { with_state(:closed) }
-    scope :order_milestone_due_desc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date DESC, milestones.id DESC') }
-    scope :order_milestone_due_asc, -> { outer_join_milestone.reorder('milestones.due_date IS NULL ASC, milestones.due_date ASC, milestones.id ASC') }
-    scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
 
+    scope :left_joins_milestones,    -> { joins("LEFT OUTER JOIN milestones ON #{table_name}.milestone_id = milestones.id") }
+    scope :order_milestone_due_desc, -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date DESC') }
+    scope :order_milestone_due_asc,  -> { left_joins_milestones.reorder('milestones.due_date IS NULL, milestones.id IS NULL, milestones.due_date ASC') }
+
+    scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) }
     scope :join_project, -> { joins(:project) }
     scope :references_project, -> { references(:project) }
     scope :non_archived, -> { join_project.where(projects: { archived: false }) }
-    scope :outer_join_milestone, -> { joins("LEFT OUTER JOIN milestones ON milestones.id = #{table_name}.milestone_id") }
+
 
     delegate :name,
              :email,
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 4a4cd093435..c7e98e8309b 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -114,6 +114,35 @@ describe Issue, "Issuable" do
     end
   end
 
+  describe "#sort" do
+    let(:project) { build_stubbed(:empty_project) }
+
+    context "by milestone due date" do
+      #Correct order is:
+      #Issues/MRs with milestones ordered by date
+      #Issues/MRs with milestones without dates
+      #Issues/MRs without milestones
+
+      let!(:issue) { create(:issue, project: project) }
+      let!(:early_milestone) { create(:milestone, project: project, due_date: 10.days.from_now) }
+      let!(:late_milestone) { create(:milestone, project: project, due_date: 30.days.from_now) }
+      let!(:issue1) { create(:issue, project: project, milestone: early_milestone) }
+      let!(:issue2) { create(:issue, project: project, milestone: late_milestone) }
+      let!(:issue3) { create(:issue, project: project) }
+
+      it "sorts desc" do
+        issues = project.issues.sort('milestone_due_desc')
+        expect(issues).to match_array([issue2, issue1, issue, issue3])
+      end
+
+      it "sorts asc" do
+        issues = project.issues.sort('milestone_due_asc')
+        expect(issues).to match_array([issue1, issue2, issue, issue3])
+      end
+    end
+  end
+
+
   describe '#subscribed?' do
     context 'user is not a participant in the issue' do
       before { allow(issue).to receive(:participants).with(user).and_return([]) }