summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAdam Niedzielski <adamsunday@gmail.com>2017-01-06 13:47:18 +0100
committerAdam Niedzielski <adamsunday@gmail.com>2017-01-06 13:47:18 +0100
commitf13c650c161b07bf8d6ad5db849fe9f442f6d0ac (patch)
treef8517deb61d8e485f8fbe1b6d87142f4c5b43567
parentaec04a47c16665f1dfb1fb61647c3f78a4dde20f (diff)
downloadgitlab-ce-f13c650c161b07bf8d6ad5db849fe9f442f6d0ac.tar.gz
Speed up group milestone index by passing group_id to IssuesFinder
-rw-r--r--app/controllers/concerns/global_milestones.rb20
-rw-r--r--app/controllers/dashboard/milestones_controller.rb13
-rw-r--r--app/controllers/groups/milestones_controller.rb11
-rw-r--r--app/models/concerns/milestoneish.rb10
-rw-r--r--app/models/global_milestone.rb19
-rw-r--r--app/models/group_milestone.rb19
-rw-r--r--app/models/milestone.rb4
-rw-r--r--changelogs/unreleased/speed-up-group-milestone-index.yml4
-rw-r--r--spec/models/global_milestone_spec.rb86
-rw-r--r--spec/models/group_milestone_spec.rb32
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