Commit 13547860 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'milestone-issue-sorting-performance' into 'master'

Improve performance of sorting milestone issues

This will fix the problem described in #3066.

See merge request !1609
parents 8adeda37 4ff75e31
...@@ -4,6 +4,7 @@ v 8.2.0 (unreleased) ...@@ -4,6 +4,7 @@ v 8.2.0 (unreleased)
- Show last project commit to default branch on project home page - Show last project commit to default branch on project home page
- Highlight comment based on anchor in URL - Highlight comment based on anchor in URL
- Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw) - Adds ability to remove the forked relationship from project settings screen. (Han Loong Liauw)
- Improved performance of sorting milestone issues
v 8.1.0 (unreleased) v 8.1.0 (unreleased)
- Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge. - Fix bug preventing mentioned issued from being closed when MR is merged using fast-forward merge.
......
...@@ -75,11 +75,7 @@ class Projects::MilestonesController < Projects::ApplicationController ...@@ -75,11 +75,7 @@ class Projects::MilestonesController < Projects::ApplicationController
end end
def sort_issues def sort_issues
@issues = @milestone.issues.where(id: params['sortable_issue']) @milestone.sort_issues(params['sortable_issue'].map(&:to_i))
@issues.each do |issue|
issue.position = params['sortable_issue'].index(issue.id.to_s) + 1
issue.save
end
render json: { saved: true } render json: { saved: true }
end end
......
...@@ -105,4 +105,36 @@ class Milestone < ActiveRecord::Base ...@@ -105,4 +105,36 @@ class Milestone < ActiveRecord::Base
def author_id def author_id
nil nil
end end
# Sorts the issues for the given IDs.
#
# This method runs a single SQL query using a CASE statement to update the
# position of all issues in the current milestone (scoped to the list of IDs).
#
# Given the ids [10, 20, 30] this method produces a SQL query something like
# the following:
#
# UPDATE issues
# SET position = CASE
# WHEN id = 10 THEN 1
# WHEN id = 20 THEN 2
# WHEN id = 30 THEN 3
# ELSE position
# END
# WHERE id IN (10, 20, 30);
#
# This method expects that the IDs given in `ids` are already Fixnums.
def sort_issues(ids)
pairs = []
ids.each_with_index do |id, index|
pairs << id
pairs << index + 1
end
conditions = 'WHEN id = ? THEN ? ' * ids.length
issues.where(id: ids).
update_all(["position = CASE #{conditions} ELSE position END", *pairs])
end
end end
require 'spec_helper'
describe Milestone, benchmark: true do
describe '#sort_issues' do
let(:milestone) { create(:milestone) }
let(:issue1) { create(:issue, milestone: milestone) }
let(:issue2) { create(:issue, milestone: milestone) }
let(:issue3) { create(:issue, milestone: milestone) }
let(:issue_ids) { [issue3.id, issue2.id, issue1.id] }
benchmark_subject { milestone.sort_issues(issue_ids) }
it { is_expected.to iterate_per_second(500) }
end
end
...@@ -140,4 +140,32 @@ describe Milestone do ...@@ -140,4 +140,32 @@ describe Milestone do
end end
end end
describe '#sort_issues' do
let(:milestone) { create(:milestone) }
let(:issue1) { create(:issue, milestone: milestone, position: 1) }
let(:issue2) { create(:issue, milestone: milestone, position: 2) }
let(:issue3) { create(:issue, milestone: milestone, position: 3) }
let(:issue4) { create(:issue, position: 42) }
it 'sorts the given issues' do
milestone.sort_issues([issue3.id, issue2.id, issue1.id])
issue1.reload
issue2.reload
issue3.reload
expect(issue1.position).to eq(3)
expect(issue2.position).to eq(2)
expect(issue3.position).to eq(1)
end
it 'ignores issues not part of the milestone' do
milestone.sort_issues([issue3.id, issue2.id, issue1.id, issue4.id])
issue4.reload
expect(issue4.position).to eq(42)
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment