Merge branch '17227-upcoming-milestone-is-confusing-when-projects-have-different-milestones' into 'master'

Make upcoming milestone work across projects

Before: we took the next milestone due across all projects in the
search and found issues whose milestone title matched that
one. Problems:

1. The milestone could be closed.
2. Different projects have milestones with different schedules.
3. Different projects have milestones with different titles.
4. Different projects can have milestones with different schedules, but
   the _same_ title. That means we could show issues from a past
   milestone, or one that's far in the future.

After: gather the ID of the next milestone on each project we're looking
at, and find issues with those milestone IDs. Problems:

1. For a lot of projects, this can return a lot of IDs.
2. The SQL query has to be different between Postgres and MySQL, because
   MySQL is much more lenient with HAVING: as well as the columns
   appearing in GROUP BY or in aggregate clauses, MySQL allows them to
   appear in the SELECT list (un-aggregated).

Closes #17227.

See merge request !4125

5 files changed, 161 insertions, 71 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 087a339f3f6..66950b0b0d1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -15,6 +15,7 @@ v 8.8.0 (unreleased)
   - Make build status canceled if any of the jobs was canceled and none failed
   - Upgrade Sidekiq to 4.1.2
   - Added /health_check endpoint for checking service status
+  - Make 'upcoming' filter for milestones work better across projects
   - Sanitize repo paths in new project error message
   - Bump mail_room to 0.7.0 to fix stuck IDLE connections
   - Remove future dates from contribution calendar graph.
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index f00f3f709e9..5849e00662b 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -252,8 +252,8 @@ class IssuableFinder
       if filter_by_no_milestone?
         items = items.where(milestone_id: [-1, nil])
       elsif filter_by_upcoming_milestone?
-        upcoming = Milestone.where(project_id: projects).upcoming
-        items = items.joins(:milestone).where(milestones: { title: upcoming.try(:title) })
+        upcoming_ids = Milestone.upcoming_ids_by_projects(projects)
+        items = items.joins(:milestone).where(milestone_id: upcoming_ids)
       else
         items = items.joins(:milestone).where(milestones: { title: params[:milestone_title] })
 
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index e4fdd23badb..fe9a281f366 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -67,8 +67,18 @@ class Milestone < ActiveRecord::Base
     @link_reference_pattern ||= super("milestones", /(?<milestone>\d+)/)
   end
 
-  def self.upcoming
-    self.where('due_date > ?', Time.now).reorder(due_date: :asc).first
+  def self.upcoming_ids_by_projects(projects)
+    rel = unscoped.of_projects(projects).active.where('due_date > ?', Time.now)
+
+    if Gitlab::Database.postgresql?
+      rel.order(:project_id, :due_date).select('DISTINCT ON (project_id) id')
+    else
+      rel.
+        group(:project_id).
+        having('due_date = MIN(due_date)').
+        pluck(:id, :project_id, :due_date).
+        map(&:first)
+    end
   end
 
   def to_reference(from_project = nil)
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index bc607a29751..ec8809e6926 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe IssuesFinder do
-  let(:user) { create :user }
-  let(:user2) { create :user }
-  let(:project1) { create(:project) }
-  let(:project2) { create(:project) }
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:project1) { create(:empty_project) }
+  let(:project2) { create(:empty_project) }
   let(:milestone) { create(:milestone, project: project1) }
   let(:label) { create(:label, project: project2) }
   let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone) }
@@ -16,101 +16,147 @@ describe IssuesFinder do
     project1.team << [user, :master]
     project2.team << [user, :developer]
     project2.team << [user2, :developer]
+
+    issue1
+    issue2
+    issue3
   end
 
-  describe :execute do
-    before :each do
-      issue1
-      issue2
-      issue3
-    end
+  describe '#execute' do
+    let(:search_user) { user }
+    let(:params) { {} }
+    let(:issues) { IssuesFinder.new(search_user, params.merge(scope: scope, state: 'opened')).execute }
 
     context 'scope: all' do
-      it 'should filter by all' do
-        params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues.size).to eq(3)
+      let(:scope) { 'all' }
+
+      it 'returns all issues' do
+        expect(issues).to contain_exactly(issue1, issue2, issue3)
       end
 
-      it 'should filter by assignee id' do
-        params = { scope: "all", assignee_id: user.id, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues.size).to eq(2)
+      context 'filtering by assignee ID' do
+        let(:params) { { assignee_id: user.id } }
+
+        it 'returns issues assigned to that user' do
+          expect(issues).to contain_exactly(issue1, issue2)
+        end
       end
 
-      it 'should filter by author id' do
-        params = { scope: "all", author_id: user2.id, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues).to eq([issue3])
+      context 'filtering by author ID' do
+        let(:params) { { author_id: user2.id } }
+
+        it 'returns issues created by that user' do
+          expect(issues).to contain_exactly(issue3)
+        end
       end
 
-      it 'should filter by milestone id' do
-        params = { scope: "all", milestone_title: milestone.title, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues).to eq([issue1])
+      context 'filtering by milestone' do
+        let(:params) { { milestone_title: milestone.title } }
+
+        it 'returns issues assigned to that milestone' do
+          expect(issues).to contain_exactly(issue1)
+        end
       end
 
-      it 'should filter by no milestone id' do
-        params = { scope: "all", milestone_title: Milestone::None.title, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues).to match_array([issue2, issue3])
+      context 'filtering by no milestone' do
+        let(:params) { { milestone_title: Milestone::None.title } }
+
+        it 'returns issues with no milestone' do
+          expect(issues).to contain_exactly(issue2, issue3)
+        end
       end
 
-      it 'should filter by label name' do
-        params = { scope: "all", label_name: label.title, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues).to eq([issue2])
+      context 'filtering by upcoming milestone' do
+        let(:params) { { milestone_title: Milestone::Upcoming.name } }
+
+        let(:project_no_upcoming_milestones) { create(:empty_project, :public) }
+        let(:project_next_1_1) { create(:empty_project, :public) }
+        let(:project_next_8_8) { create(:empty_project, :public) }
+
+        let(:yesterday) { Date.today - 1.day }
+        let(:tomorrow) { Date.today + 1.day }
+        let(:two_days_from_now) { Date.today + 2.days }
+        let(:ten_days_from_now) { Date.today + 10.days }
+
+        let(:milestones) do
+          [
+            create(:milestone, :closed, project: project_no_upcoming_milestones),
+            create(:milestone, project: project_next_1_1, title: '1.1', due_date: two_days_from_now),
+            create(:milestone, project: project_next_1_1, title: '8.8', due_date: ten_days_from_now),
+            create(:milestone, project: project_next_8_8, title: '1.1', due_date: yesterday),
+            create(:milestone, project: project_next_8_8, title: '8.8', due_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 upcoming milestone for each project' do
+          expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8')
+          expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now)
+        end
       end
 
-      it 'returns unique issues when filtering by multiple labels' do
-        label2 = create(:label, project: project2)
+      context 'filtering by label' do
+        let(:params) { { label_name: label.title } }
 
-        create(:label_link, label: label2, target: issue2)
+        it 'returns issues with that label' do
+          expect(issues).to contain_exactly(issue2)
+        end
+      end
 
-        params = {
-          scope:      'all',
-          label_name: [label.title, label2.title].join(','),
-          state:      'opened'
-        }
+      context 'filtering by multiple labels' do
+        let(:params) { { label_name: [label.title, label2.title].join(',') } }
+        let(:label2) { create(:label, project: project2) }
 
-        issues = IssuesFinder.new(user, params).execute
+        before { create(:label_link, label: label2, target: issue2) }
 
-        expect(issues).to eq([issue2])
+        it 'returns the unique issues with any of those labels' do
+          expect(issues).to contain_exactly(issue2)
+        end
       end
 
-      it 'should filter by no label name' do
-        params = { scope: "all", label_name: Label::None.title, state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues).to match_array([issue1, issue3])
+      context 'filtering by no label' do
+        let(:params) { { label_name: Label::None.title } }
+
+        it 'returns issues with no labels' do
+          expect(issues).to contain_exactly(issue1, issue3)
+        end
       end
 
-      it 'should be empty for unauthorized user' do
-        params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new(nil, params).execute
-        expect(issues.size).to be_zero
+      context 'when the user is unauthorized' do
+        let(:search_user) { nil }
+
+        it 'returns no results' do
+          expect(issues).to be_empty
+        end
       end
 
-      it 'should not include unauthorized issues' do
-        params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new(user2, params).execute
-        expect(issues.size).to eq(2)
-        expect(issues).not_to include(issue1)
-        expect(issues).to include(issue2)
-        expect(issues).to include(issue3)
+      context 'when the user can see some, but not all, issues' do
+        let(:search_user) { user2 }
+
+        it 'returns only issues they can see' do
+          expect(issues).to contain_exactly(issue2, issue3)
+        end
       end
     end
 
     context 'personal scope' do
-      it 'should filter by assignee' do
-        params = { scope: "assigned-to-me", state: 'opened' }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues.size).to eq(2)
+      let(:scope) { 'assigned-to-me' }
+
+      it 'returns issue assigned to the user' do
+        expect(issues).to contain_exactly(issue1, issue2)
       end
 
-      it 'should filter by project' do
-        params = { scope: "assigned-to-me", state: 'opened', project_id: project1.id }
-        issues = IssuesFinder.new(user, params).execute
-        expect(issues.size).to eq(1)
+      context 'filtering by project' do
+        let(:params) { { project_id: project1.id } }
+
+        it 'returns issues assigned to the user in that project' do
+          expect(issues).to contain_exactly(issue1)
+        end
       end
     end
   end
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index 247a9fa9910..1e18c788b50 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -204,4 +204,37 @@ describe Milestone, models: true do
         to eq([milestone])
     end
   end
+
+  describe '.upcoming_ids_by_projects' do
+    let(:project_1) { create(:empty_project) }
+    let(:project_2) { create(:empty_project) }
+    let(:project_3) { create(:empty_project) }
+    let(:projects) { [project_1, project_2, project_3] }
+
+    let!(:past_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now - 1.day) }
+    let!(:current_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now + 1.day) }
+    let!(:future_milestone_project_1) { create(:milestone, project: project_1, due_date: Time.now + 2.days) }
+
+    let!(:past_milestone_project_2) { create(:milestone, project: project_2, due_date: Time.now - 1.day) }
+    let!(:closed_milestone_project_2) { create(:milestone, :closed, project: project_2, due_date: Time.now + 1.day) }
+    let!(:current_milestone_project_2) { create(:milestone, project: project_2, due_date: Time.now + 2.days) }
+
+    let!(:past_milestone_project_3) { create(:milestone, project: project_3, due_date: Time.now - 1.day) }
+
+    # The call to `#try` is because this returns a relation with a Postgres DB,
+    # and an array of IDs with a MySQL DB.
+    let(:milestone_ids) { Milestone.upcoming_ids_by_projects(projects).map { |id| id.try(:id) || id } }
+
+    it 'returns the next upcoming open milestone ID for each project' do
+      expect(milestone_ids).to contain_exactly(current_milestone_project_1.id, current_milestone_project_2.id)
+    end
+
+    context 'when the projects have no open upcoming milestones' do
+      let(:projects) { [project_3] }
+
+      it 'returns no results' do
+        expect(milestone_ids).to be_empty
+      end
+    end
+  end
 end