diff --git a/app/controllers/concerns/global_milestones.rb b/app/controllers/concerns/global_milestones.rb
deleted file mode 100644
index 5c503c5b69896c4e9d92477ed1817307bc9c94b7..0000000000000000000000000000000000000000
--- 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 fa9c6c054f0fbb3da50aa224cf42aa345d30c9d7..7051652d109341a2576a73926797c4854cce791a 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 24ec4eec3f28ef52214ad849423f1c8c91ff2aec..0d872c86c8a7942496f72984b7648a36dd18bced 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 8f02c226f0b4831fb0462e04139f922a84456d6e..fcc8feddb39427ab4cfc2ead3c68afa07bf8ee43 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 a54e478f5f8e98e1916b3ae9ae025208dec729d7..b991d78e27fc22bbd503300c83c52e39b4ed02f1 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 0000000000000000000000000000000000000000..7b6db2634b7107a4cebb2ae55d9961f4fe909a2d
--- /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 8a11f47dd6796d872ae1cb3947d0b701b8807b88..7331000a9f2de4bd43ebc6f2dd2cf573eb54c40d 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 0000000000000000000000000000000000000000..b5181fa66da4aca59bf014deb723d34ad6408ea6
--- /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 dd03348052796f33d31e517349713f2dcc7abacf..d87684fd49eb5c75d3ac968ded162e5db2f64e0b 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 0000000000000000000000000000000000000000..601167c3bd325335ffa709de16a94166ac05f7ad
--- /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