Commit 890fe7e3 authored by Alex Kalderimis's avatar Alex Kalderimis Committed by Mayra Cabrera

Relative positioning improvements

This expands the usable range of positions to the negative numbers,
uses more precisely chosen gap sizes, and deals more effectively with
edge cases.
parent bbd574e3
......@@ -1324,7 +1324,6 @@ Rails/SaveBang:
- 'spec/support/shared_examples/models/members_notifications_shared_example.rb'
- 'spec/support/shared_examples/models/mentionable_shared_examples.rb'
- 'spec/support/shared_examples/models/project_latest_successful_build_for_shared_examples.rb'
- 'spec/support/shared_examples/models/relative_positioning_shared_examples.rb'
- 'spec/support/shared_examples/models/slack_mattermost_notifications_shared_examples.rb'
- 'spec/support/shared_examples/models/update_project_statistics_shared_examples.rb'
- 'spec/support/shared_examples/models/with_uploads_shared_examples.rb'
......
This diff is collapsed.
---
title: Performance and robustness improvements for relative positioning
merge_request: 37724
author:
type: performance
......@@ -8,8 +8,8 @@ module EpicTreeSorting
class_methods do
def relative_positioning_query_base(object)
from_union([
EpicIssue.select("id, relative_position, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
Epic.select("id, relative_position, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
EpicIssue.select("id, relative_position, epic_id as parent_id, epic_id, 'epic_issue' as object_type").in_epic(object.parent_ids),
Epic.select("id, relative_position, parent_id, parent_id as epic_id, 'epic' as object_type").where(parent_id: object.parent_ids)
])
end
......@@ -19,11 +19,14 @@ module EpicTreeSorting
end
included do
def move_sequence(start_pos, end_pos, delta)
def move_sequence(start_pos, end_pos, delta, include_self = false)
items_to_update = scoped_items
.select(:id, :object_type)
.where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
unless include_self
items_to_update = items_to_update.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
end
items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
ids = group_items.map(&:id)
......
......@@ -28,9 +28,49 @@ RSpec.describe EpicIssue do
context "relative positioning" do
it_behaves_like "a class that supports relative positioning" do
let(:epic) { create(:epic) }
let_it_be(:epic) { create(:epic) }
let(:factory) { :epic_issue }
let(:default_params) { { epic: epic } }
end
context 'with a mixed tree level' do
let_it_be(:epic) { create(:epic) }
let_it_be_with_reload(:left) { create(:epic_issue, epic: epic, relative_position: 100) }
let_it_be_with_reload(:middle) { create(:epic, group: epic.group, parent: epic, relative_position: 101) }
let_it_be_with_reload(:right) { create(:epic_issue, epic: epic, relative_position: 102) }
it 'can create space by using move_sequence_after' do
left.move_sequence_after
[left, middle, right].each(&:reset)
expect(middle.relative_position - left.relative_position).to be > 1
expect(left.relative_position).to be < middle.relative_position
expect(middle.relative_position).to be < right.relative_position
end
it 'can create space by using move_sequence_before' do
right.move_sequence_before
[left, middle, right].each(&:reset)
expect(right.relative_position - middle.relative_position).to be > 1
expect(left.relative_position).to be < middle.relative_position
expect(middle.relative_position).to be < right.relative_position
end
it 'moves nulls to the end' do
leaves = create_list(:epic_issue, 2, epic: epic, relative_position: nil)
nested = create(:epic, group: epic.group, parent: epic, relative_position: nil)
moved = [*leaves, nested]
level = [nested, *leaves, right]
expect do
EpicIssue.move_nulls_to_end(level)
end.not_to change { right.reset.relative_position }
moved.each(&:reset)
expect(moved.map(&:relative_position)).to all(be > right.relative_position)
end
end
end
end
......@@ -616,12 +616,22 @@ RSpec.describe Epic do
end
context "relative positioning" do
context 'there is no parent' do
it_behaves_like "a class that supports relative positioning" do
let(:factory) { :epic }
let(:default_params) { {} }
end
end
context 'there is a parent' do
it_behaves_like "a class that supports relative positioning" do
let_it_be(:parent) { create(:epic) }
let(:factory) { :epic }
let(:default_params) { { parent: parent, group: parent.group } }
end
end
end
describe '.related_issues' do
it 'returns epic issues ordered by relative position' do
epic1 = create(:epic, group: group)
......
......@@ -454,20 +454,14 @@ RSpec.describe Issue do
end
describe 'relative positioning with group boards' do
let(:group) { create(:group) }
let!(:board) { create(:board, group: group) }
let(:project) { create(:project, namespace: group) }
let(:project1) { create(:project, namespace: group) }
let(:issue) { build(:issue, project: project) }
let(:issue1) { build(:issue, project: project1) }
let_it_be(:group) { create(:group) }
let_it_be(:board) { create(:board, group: group) }
let_it_be(:project) { create(:project, namespace: group) }
let_it_be(:project1) { create(:project, namespace: group) }
let_it_be_with_reload(:issue) { create(:issue, project: project) }
let_it_be_with_reload(:issue1) { create(:issue, project: project1, relative_position: issue.relative_position + RelativePositioning::IDEAL_DISTANCE) }
let(:new_issue) { build(:issue, project: project1, relative_position: nil) }
before do
[issue, issue1].each do |issue|
issue.move_to_end && issue.save
end
end
describe '#max_relative_position' do
it 'returns maximum position' do
expect(issue.max_relative_position).to eq issue1.relative_position
......@@ -546,18 +540,20 @@ RSpec.describe Issue do
issue1.update relative_position: issue.relative_position
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
expect(new_issue.relative_position).to be > issue.relative_position
expect(issue.relative_position).to be < issue1.relative_position
expect(new_issue.relative_position)
.to be_between(issue.relative_position, issue1.relative_position).exclusive
end
it 'positions issues between other two if distance is 1' do
issue1.update relative_position: issue.relative_position + 1
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
expect(new_issue.relative_position).to be > issue.relative_position
expect(issue.relative_position).to be < issue1.relative_position
expect(new_issue.relative_position)
.to be_between(issue.relative_position, issue1.relative_position).exclusive
end
it 'positions issue in the middle of other two if distance is big enough' do
......@@ -566,24 +562,20 @@ RSpec.describe Issue do
new_issue.move_between(issue, issue1)
expect(new_issue.relative_position).to eq(8000)
expect(new_issue.relative_position)
.to be_between(issue.relative_position, issue1.relative_position).exclusive
end
it 'positions issue closer to the middle if we are at the very top' do
issue1.update relative_position: 6000
new_issue.move_between(nil, issue1)
new_issue.move_between(nil, issue)
expect(new_issue.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE)
expect(new_issue.relative_position).to eq(issue.relative_position - RelativePositioning::IDEAL_DISTANCE)
end
it 'positions issue closer to the middle if we are at the very bottom' do
issue.update relative_position: 6000
issue1.update relative_position: nil
new_issue.move_between(issue, nil)
new_issue.move_between(issue1, nil)
expect(new_issue.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
expect(new_issue.relative_position).to eq(issue1.relative_position + RelativePositioning::IDEAL_DISTANCE)
end
it 'positions issue in the middle of other two if distance is not big enough' do
......@@ -600,8 +592,10 @@ RSpec.describe Issue do
issue1.update relative_position: 101
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
expect(new_issue.relative_position).to be_between(issue.relative_position, issue1.relative_position)
expect(new_issue.relative_position)
.to be_between(issue.relative_position, issue1.relative_position).exclusive
end
it 'uses rebalancing if there is no place' do
......@@ -612,12 +606,15 @@ RSpec.describe Issue do
new_issue.move_between(issue1, issue2)
new_issue.save!
[issue, issue1, issue2].each(&:reset)
expect(new_issue.relative_position)
.to be_between(issue1.relative_position, issue2.relative_position).exclusive
expect(new_issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
expect(issue.reload.relative_position).not_to eq(100)
expect([issue, issue1, issue2, new_issue].map(&:relative_position).uniq).to have_attributes(size: 4)
end
it 'positions issue right if we pass none-sequential parameters' do
it 'positions issue right if we pass non-sequential parameters' do
issue.update relative_position: 99
issue1.update relative_position: 101
issue2 = create(:issue, relative_position: 102, project: project)
......
......@@ -169,7 +169,9 @@ RSpec.describe EpicIssues::CreateService do
end
context 'when multiple valid issues are given' do
subject { assign_issue([issue.to_reference(full: true), issue2.to_reference(full: true)]) }
let(:references) { [issue, issue2].map { |i| i.to_reference(full: true) } }
subject { assign_issue(references) }
let(:created_link1) { EpicIssue.find_by!(issue_id: issue.id) }
let(:created_link2) { EpicIssue.find_by!(issue_id: issue2.id) }
......@@ -181,13 +183,18 @@ RSpec.describe EpicIssues::CreateService do
expect(created_link2).to have_attributes(epic: epic)
end
it 'places each issue at the start' do
subject
expect(created_link2.relative_position).to be < created_link1.relative_position
end
it 'orders the epic issues to the first place and moves the existing ones down' do
existing_link = create(:epic_issue, epic: epic, issue: issue3)
subject
expect(created_link2.relative_position).to be < created_link1.relative_position
expect(created_link1.relative_position).to be < existing_link.reload.relative_position
expect([created_link1, created_link2].map(&:relative_position))
.to all(be < existing_link.reset.relative_position)
end
it 'returns success status' do
......
......@@ -22,6 +22,7 @@ module Gitlab
# https://www.postgresql.org/docs/9.2/static/datatype-numeric.html
MAX_INT_VALUE = 2147483647
MIN_INT_VALUE = -2147483648
# The max value between MySQL's TIMESTAMP and PostgreSQL's timestampz:
# https://www.postgresql.org/docs/9.1/static/datatype-datetime.html
......
......@@ -21,7 +21,7 @@ RSpec.describe 'Issue Boards', :js do
end
context 'un-ordered issues' do
let!(:issue4) { create(:labeled_issue, project: project, labels: [label]) }
let!(:issue4) { create(:labeled_issue, project: project, labels: [label], relative_position: nil) }
before do
visit project_board_path(project, board)
......
......@@ -23,7 +23,7 @@ RSpec.describe Analytics::CycleAnalytics::ProjectStage do
context 'relative positioning' do
it_behaves_like 'a class that supports relative positioning' do
let(:project) { build(:project) }
let_it_be(:project) { create(:project) }
let(:factory) { :cycle_analytics_project_stage }
let(:default_params) { { project: project } }
end
......
......@@ -6,6 +6,7 @@ RSpec.describe Issue do
include ExternalAuthorizationServiceHelpers
let_it_be(:user) { create(:user) }
let_it_be(:reusable_project) { create(:project) }
describe "Associations" do
it { is_expected.to belong_to(:milestone) }
......@@ -145,13 +146,13 @@ RSpec.describe Issue do
end
describe '#order_by_position_and_priority' do
let(:project) { create :project }
let(:project) { reusable_project }
let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }
let(:p2) { create(:label, title: 'P2', project: project, priority: 2) }
let!(:issue1) { create(:labeled_issue, project: project, labels: [p1]) }
let!(:issue2) { create(:labeled_issue, project: project, labels: [p2]) }
let!(:issue3) { create(:issue, project: project, relative_position: 100) }
let!(:issue4) { create(:issue, project: project, relative_position: 200) }
let!(:issue3) { create(:issue, project: project, relative_position: -200) }
let!(:issue4) { create(:issue, project: project, relative_position: -100) }
it 'returns ordered list' do
expect(project.issues.order_by_position_and_priority)
......@@ -160,10 +161,10 @@ RSpec.describe Issue do
end
describe '#sort' do
let(:project) { create(:project) }
let(:project) { reusable_project }
context "by relative_position" do
let!(:issue) { create(:issue, project: project) }
let!(:issue) { create(:issue, project: project, relative_position: nil) }
let!(:issue2) { create(:issue, project: project, relative_position: 2) }
let!(:issue3) { create(:issue, project: project, relative_position: 1) }
......@@ -1027,7 +1028,7 @@ RSpec.describe Issue do
context "relative positioning" do
it_behaves_like "a class that supports relative positioning" do
let(:project) { create(:project) }
let_it_be(:project) { create(:project) }
let(:factory) { :issue }
let(:default_params) { { project: project } }
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