Speed up group milestone index by passing group_id to IssuesFinder

10 files changed, 172 insertions, 46 deletions

diff --git a/app/controllers/concerns/global_milestones.rb b/app/controllers/concerns/global_milestones.rb
deleted file mode 100644
index 5c503c5b698..00000000000
--- a/app/controllers/concerns/global_milestones.rb
+++ /dev/null
@@ -1,20 +0,0 @@
-module GlobalMilestones
-  extend ActiveSupport::Concern
-
-  def milestones
-    epoch = DateTime.parse('1970-01-01')
-    @milestones = MilestonesFinder.new.execute(@projects, params)
-    @milestones = GlobalMilestone.build_collection(@milestones)
-    @milestones = @milestones.sort_by { |x| x.due_date.nil? ? epoch : x.due_date }
-  end
-
-  def milestone
-    milestones = Milestone.of_projects(@projects).where(title: params[:title])
-
-    if milestones.present?
-      @milestone = GlobalMilestone.new(params[:title], milestones)
-    else
-      render_404
-    end
-  end
-end
diff --git a/app/controllers/dashboard/milestones_controller.rb b/app/controllers/dashboard/milestones_controller.rb
index fa9c6c054f0..7051652d109 100644
--- a/app/controllers/dashboard/milestones_controller.rb
+++ b/app/controllers/dashboard/milestones_controller.rb
@@ -1,6 +1,4 @@
 class Dashboard::MilestonesController < Dashboard::ApplicationController
-  include GlobalMilestones
-
   before_action :projects
   before_action :milestone, only: [:show]
 
@@ -17,4 +15,15 @@ class Dashboard::MilestonesController < Dashboard::ApplicationController
 
   def show
   end
+
+  private
+
+  def milestones
+    @milestones = GlobalMilestone.build_collection(@projects, params)
+  end
+
+  def milestone
+    @milestone = GlobalMilestone.build(@projects, params[:title])
+    render_404 unless @milestone
+  end
 end
diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index 24ec4eec3f2..0d872c86c8a 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -1,6 +1,4 @@
 class Groups::MilestonesController < Groups::ApplicationController
-  include GlobalMilestones
-
   before_action :group_projects
   before_action :milestone, only: [:show, :update]
   before_action :authorize_admin_milestones!, only: [:new, :create, :update]
@@ -73,4 +71,13 @@ class Groups::MilestonesController < Groups::ApplicationController
   def milestone_path(title)
     group_milestone_path(@group, title.to_slug.to_s, title: title)
   end
+
+  def milestones
+    @milestones = GroupMilestone.build_collection(@group, @projects, params)
+  end
+
+  def milestone
+    @milestone = GroupMilestone.build(@group, @projects, params[:title])
+    render_404 unless @milestone
+  end
 end
diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb
index 8f02c226f0b..fcc8feddb39 100644
--- a/app/models/concerns/milestoneish.rb
+++ b/app/models/concerns/milestoneish.rb
@@ -36,8 +36,8 @@ module Milestoneish
 
   def issues_visible_to_user(user)
     memoize_per_user(user, :issues_visible_to_user) do
-      params = try(:project_id) ? { project_id: project_id } : {}
-      IssuesFinder.new(user, params).execute.where(milestone_id: milestoneish_ids)
+      IssuesFinder.new(user, issues_finder_params)
+        .execute.where(milestone_id: milestoneish_ids)
     end
   end
 
@@ -72,4 +72,10 @@ module Milestoneish
     @memoized[method_name] ||= {}
     @memoized[method_name][user.try!(:id)] ||= yield
   end
+
+  # override in a class that includes this module to get a faster query
+  # from IssuesFinder
+  def issues_finder_params
+    {}
+  end
 end
diff --git a/app/models/global_milestone.rb b/app/models/global_milestone.rb
index a54e478f5f8..b991d78e27f 100644
--- a/app/models/global_milestone.rb
+++ b/app/models/global_milestone.rb
@@ -1,6 +1,8 @@
 class GlobalMilestone
   include Milestoneish
 
+  EPOCH = DateTime.parse('1970-01-01')
+
   attr_accessor :title, :milestones
   alias_attribute :name, :title
 
@@ -8,13 +10,22 @@ class GlobalMilestone
     @first_milestone
   end
 
-  def self.build_collection(milestones)
-    milestones = milestones.group_by(&:title)
+  def self.build_collection(projects, params)
+    child_milestones = MilestonesFinder.new.execute(projects, params)
 
-    milestones.map do |title, milestones|
-      milestones_relation = Milestone.where(id: milestones.map(&:id))
+    milestones = child_milestones.select(:id, :title).group_by(&:title).map do |title, grouped|
+      milestones_relation = Milestone.where(id: grouped.map(&:id))
       new(title, milestones_relation)
     end
+
+    milestones.sort_by { |milestone| milestone.due_date || EPOCH }
+  end
+
+  def self.build(projects, title)
+    child_milestones = Milestone.of_projects(projects).where(title: title)
+    return if child_milestones.blank?
+
+    new(title, child_milestones)
   end
 
   def initialize(title, milestones)
diff --git a/app/models/group_milestone.rb b/app/models/group_milestone.rb
new file mode 100644
index 00000000000..7b6db2634b7
--- /dev/null
+++ b/app/models/group_milestone.rb
@@ -0,0 +1,19 @@
+class GroupMilestone < GlobalMilestone
+  attr_accessor :group
+
+  def self.build_collection(group, projects, params)
+    super(projects, params).each do |milestone|
+      milestone.group = group
+    end
+  end
+
+  def self.build(group, projects, title)
+    super(projects, title).tap do |milestone|
+      milestone.group = group if milestone
+    end
+  end
+
+  def issues_finder_params
+    { group_id: group.id }
+  end
+end
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 8a11f47dd67..7331000a9f2 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -202,4 +202,8 @@ class Milestone < ActiveRecord::Base
       errors.add(:start_date, "Can't be greater than due date")
     end
   end
+
+  def issues_finder_params
+    { project_id: project_id }
+  end
 end
diff --git a/changelogs/unreleased/speed-up-group-milestone-index.yml b/changelogs/unreleased/speed-up-group-milestone-index.yml
new file mode 100644
index 00000000000..b5181fa66da
--- /dev/null
+++ b/changelogs/unreleased/speed-up-group-milestone-index.yml
@@ -0,0 +1,4 @@
+---
+title: Speed up group milestone index by passing group_id to IssuesFinder
+merge_request: 8363
+author:
diff --git a/spec/models/global_milestone_spec.rb b/spec/models/global_milestone_spec.rb
index dd033480527..d87684fd49e 100644
--- a/spec/models/global_milestone_spec.rb
+++ b/spec/models/global_milestone_spec.rb
@@ -7,26 +7,72 @@ describe GlobalMilestone, models: true do
   let(:project1) { create(:project, group: group) }
   let(:project2) { create(:project, path: 'gitlab-ci', group: group) }
   let(:project3) { create(:project, path: 'cookbook-gitlab', group: group) }
-  let(:milestone1_project1) { create(:milestone, title: "Milestone v1.2", project: project1) }
-  let(:milestone1_project2) { create(:milestone, title: "Milestone v1.2", project: project2) }
-  let(:milestone1_project3) { create(:milestone, title: "Milestone v1.2", project: project3) }
-  let(:milestone2_project1) { create(:milestone, title: "VD-123", project: project1) }
-  let(:milestone2_project2) { create(:milestone, title: "VD-123", project: project2) }
-  let(:milestone2_project3) { create(:milestone, title: "VD-123", project: project3) }
 
   describe '.build_collection' do
+    let(:milestone1_due_date) { 2.weeks.from_now.to_date }
+
+    let!(:milestone1_project1) do
+      create(
+        :milestone,
+        title: "Milestone v1.2",
+        project: project1,
+        due_date: milestone1_due_date
+      )
+    end
+
+    let!(:milestone1_project2) do
+      create(
+        :milestone,
+        title: "Milestone v1.2",
+        project: project2,
+        due_date: milestone1_due_date
+      )
+    end
+
+    let!(:milestone1_project3) do
+      create(
+        :milestone,
+        title: "Milestone v1.2",
+        project: project3,
+        due_date: milestone1_due_date
+      )
+    end
+
+    let!(:milestone2_project1) do
+      create(
+        :milestone,
+        title: "VD-123",
+        project: project1,
+        due_date: nil
+      )
+    end
+
+    let!(:milestone2_project2) do
+      create(
+        :milestone,
+        title: "VD-123",
+        project: project2,
+        due_date: nil
+      )
+    end
+
+    let!(:milestone2_project3) do
+      create(
+        :milestone,
+        title: "VD-123",
+        project: project3,
+        due_date: nil
+      )
+    end
+
     before do
-      milestones =
-        [
-          milestone1_project1,
-          milestone1_project2,
-          milestone1_project3,
-          milestone2_project1,
-          milestone2_project2,
-          milestone2_project3
-        ]
+      projects = [
+        project1,
+        project2,
+        project3
+      ]
 
-      @global_milestones = GlobalMilestone.build_collection(milestones)
+      @global_milestones = GlobalMilestone.build_collection(projects, {})
     end
 
     it 'has all project milestones' do
@@ -40,9 +86,17 @@ describe GlobalMilestone, models: true do
     it 'has all project milestones' do
       expect(@global_milestones.map { |group_milestone| group_milestone.milestones.count }.sum).to eq(6)
     end
+
+    it 'sorts collection by due date' do
+      expect(@global_milestones.map(&:due_date)).to eq [nil, milestone1_due_date]
+    end
   end
 
   describe '#initialize' do
+    let(:milestone1_project1) { create(:milestone, title: "Milestone v1.2", project: project1) }
+    let(:milestone1_project2) { create(:milestone, title: "Milestone v1.2", project: project2) }
+    let(:milestone1_project3) { create(:milestone, title: "Milestone v1.2", project: project3) }
+
     before do
       milestones =
         [
diff --git a/spec/models/group_milestone_spec.rb b/spec/models/group_milestone_spec.rb
new file mode 100644
index 00000000000..601167c3bd3
--- /dev/null
+++ b/spec/models/group_milestone_spec.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe GroupMilestone, models: true do
+  let(:group) { create(:group) }
+  let(:project) { create(:project, group: group) }
+  let(:project_milestone) do
+    create(:milestone, title: "Milestone v1.2", project: project)
+  end
+
+  describe '.build' do
+    it 'returns milestone with group assigned' do
+      milestone = GroupMilestone.build(
+        group,
+        [project],
+        project_milestone.title
+      )
+
+      expect(milestone.group).to eq group
+    end
+  end
+
+  describe '.build_collection' do
+    before do
+      project_milestone
+    end
+
+    it 'returns array of milestones, each with group assigned' do
+      milestones = GroupMilestone.build_collection(group, [project], {})
+      expect(milestones).to all(have_attributes(group: group))
+    end
+  end
+end