Add group milestones in upcoming filter

4 files changed, 76 insertions, 24 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index b73a3fa6e01..a144db9fcd1 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -149,6 +149,18 @@ class IssuableFinder
       end
   end
 
+  def related_groups
+    if project? && project && project.group && Ability.allowed?(current_user, :read_group, project.group)
+      project.group.self_and_ancestors
+    elsif group
+      [group]
+    elsif current_user
+      Gitlab::GroupHierarchy.new(current_user.authorized_groups, current_user.groups).all_groups
+    else
+      []
+    end
+  end
+
   def project?
     params[:project_id].present?
   end
@@ -163,8 +175,10 @@ class IssuableFinder
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
-  def projects(items = nil)
-    return @projects = project if project?
+  def projects
+    return @projects if defined?(@projects)
+
+    return @projects = [project] if project?
 
     projects =
       if current_user && params[:authorized_only].presence && !current_user_related?
@@ -459,7 +473,7 @@ class IssuableFinder
       elsif filter_by_any_milestone?
         items = items.any_milestone
       elsif filter_by_upcoming_milestone?
-        upcoming_ids = Milestone.upcoming_ids_by_projects(projects(items))
+        upcoming_ids = Milestone.upcoming_ids(projects, related_groups)
         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()')
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 6dc0fca68e6..30b5884d405 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -40,6 +40,7 @@ class Milestone < ActiveRecord::Base
 
   scope :for_projects_and_groups, -> (project_ids, group_ids) do
     conditions = []
+
     conditions << arel_table[:project_id].in(project_ids) if project_ids&.compact&.any?
     conditions << arel_table[:group_id].in(group_ids) if group_ids&.compact&.any?
 
@@ -129,18 +130,29 @@ class Milestone < ActiveRecord::Base
     @link_reference_pattern ||= super("milestones", /(?<milestone>\d+)/)
   end
 
-  def self.upcoming_ids_by_projects(projects)
-    rel = unscoped.of_projects(projects).active.where('due_date > ?', Time.now)
+  def self.upcoming_ids(projects, groups)
+    rel = unscoped
+            .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id))
+            .active.where('milestones.due_date > NOW()')
 
     if Gitlab::Database.postgresql?
-      rel.order(:project_id, :due_date).select('DISTINCT ON (project_id) id')
+      rel.order(:project_id, :group_id, :due_date).select('DISTINCT ON (project_id, group_id) id')
     else
+      # We need to use MySQL's NULL-safe comparison operator `<=>` here
+      # because one of `project_id` or `group_id` is always NULL
+      join_clause = <<~HEREDOC
+        LEFT OUTER JOIN milestones earlier_milestones
+          ON milestones.project_id <=> earlier_milestones.project_id
+            AND milestones.group_id <=> earlier_milestones.group_id
+            AND milestones.due_date > earlier_milestones.due_date
+            AND earlier_milestones.due_date > NOW()
+            AND earlier_milestones.state = 'active'
+      HEREDOC
+
       rel
-        .group(:project_id, :due_date, :id)
-        .having('due_date = MIN(due_date)')
-        .pluck(:id, :project_id, :due_date)
-        .uniq(&:second)
-        .map(&:first)
+        .joins(join_clause)
+        .where('earlier_milestones.id IS NULL')
+        .select(:id)
     end
   end
 
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 80f7232f282..682fae06434 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -174,9 +174,13 @@ describe IssuesFinder do
       context 'filtering by upcoming milestone' do
         let(:params) { { milestone_title: Milestone::Upcoming.name } }
 
+        let!(:group) { create(:group, :public) }
+        let!(:group_member) { create(:group_member, group: group, user: user) }
+
         let(:project_no_upcoming_milestones) { create(:project, :public) }
         let(:project_next_1_1) { create(:project, :public) }
         let(:project_next_8_8) { create(:project, :public) }
+        let(:project_in_group) { create(:project, :public, namespace: group) }
 
         let(:yesterday) { Date.today - 1.day }
         let(:tomorrow) { Date.today + 1.day }
@@ -187,21 +191,22 @@ describe IssuesFinder 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)
+            create(:milestone, project: project_next_1_1, title: '8.9', due_date: ten_days_from_now),
+            create(:milestone, project: project_next_8_8, title: '1.2', due_date: yesterday),
+            create(:milestone, project: project_next_8_8, title: '8.8', due_date: tomorrow),
+            create(:milestone, group: group, title: '9.9', due_date: tomorrow)
           ]
         end
 
         before do
           milestones.each do |milestone|
-            create(:issue, project: milestone.project, milestone: milestone, author: user, assignees: [user])
+            create(:issue, project: milestone.project || project_in_group, milestone: milestone, author: user, assignees: [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)
+        it 'returns issues in the upcoming milestone for each project or group' do
+          expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8', '9.9')
+          expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now, tomorrow)
         end
       end
 
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index b3d31e65c85..23f9f054673 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -240,7 +240,22 @@ describe Milestone do
     end
   end
 
-  describe '.upcoming_ids_by_projects' do
+  describe '.upcoming_ids' do
+    let(:group_1) { create(:group) }
+    let(:group_2) { create(:group) }
+    let(:group_3) { create(:group) }
+    let(:groups) { [group_1, group_2, group_3] }
+
+    let!(:past_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now - 1.day) }
+    let!(:current_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 1.day) }
+    let!(:future_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 2.days) }
+
+    let!(:past_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now - 1.day) }
+    let!(:closed_milestone_group_2) { create(:milestone, :closed, group: group_2, due_date: Time.now + 1.day) }
+    let!(:current_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now + 2.days) }
+
+    let!(:past_milestone_group_3) { create(:milestone, group: group_3, due_date: Time.now - 1.day) }
+
     let(:project_1) { create(:project) }
     let(:project_2) { create(:project) }
     let(:project_3) { create(:project) }
@@ -258,14 +273,20 @@ describe Milestone do
 
     # 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) { described_class.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)
+    let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map { |id| id.try(:id) || id } }
+
+    it 'returns the next upcoming open milestone ID for each project and group' do
+      expect(milestone_ids).to contain_exactly(
+        current_milestone_project_1.id,
+        current_milestone_project_2.id,
+        current_milestone_group_1.id,
+        current_milestone_group_2.id
+      )
     end
 
-    context 'when the projects have no open upcoming milestones' do
+    context 'when the projects and groups have no open upcoming milestones' do
       let(:projects) { [project_3] }
+      let(:groups) { [group_3] }
 
       it 'returns no results' do
         expect(milestone_ids).to be_empty