summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBob Van Landuyt <bob@gitlab.com>2019-07-25 07:13:58 +0000
committerBob Van Landuyt <bob@gitlab.com>2019-07-25 07:13:58 +0000
commitba997f3c428d94adfc9a2eb4eb0daaa3d759c4df (patch)
treebc01dd05a44834d37781cb8bd72dad8a1ff34fa8
parenta5eaefc9027696fc65b7d51251902e013c14d161 (diff)
parentf36da45711f7d412951b50c6b9db913a5e324171 (diff)
downloadgitlab-ce-ba997f3c428d94adfc9a2eb4eb0daaa3d759c4df.tar.gz
Merge branch 'make-relative-positioning-module-reusable' into 'master'
Make RelativePositioning reusable See merge request gitlab-org/gitlab-ce!30993
-rw-r--r--app/models/concerns/relative_positioning.rb52
-rw-r--r--app/models/issue.rb6
-rw-r--r--spec/models/concerns/relative_positioning_spec.rb242
-rw-r--r--spec/models/issue_spec.rb8
-rw-r--r--spec/support/shared_examples/relative_positioning_shared_examples.rb253
5 files changed, 300 insertions, 261 deletions
diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index e4fe46d722a..9cd7b8d6258 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -1,5 +1,26 @@
# frozen_string_literal: true
+# This module makes it possible to handle items as a list, where the order of items can be easily altered
+# Requirements:
+#
+# - Only works for ActiveRecord models
+# - relative_position integer field must present on the model
+# - This module uses GROUP BY: the model should have a parent relation, example: project -> issues, project is the parent relation (issues table has a parent_id column)
+#
+# Setup like this in the body of your class:
+#
+# include RelativePositioning
+#
+# # base query used for the position calculation
+# def self.relative_positioning_query_base(issue)
+# where(deleted: false)
+# end
+#
+# # column that should be used in GROUP BY
+# def self.relative_positioning_parent_column
+# :project_id
+# end
+#
module RelativePositioning
extend ActiveSupport::Concern
@@ -93,7 +114,7 @@ module RelativePositioning
return move_after(before) unless after
return move_before(after) unless before
- # If there is no place to insert an issue we need to create one by moving the before issue closer
+ # If there is no place to insert an item we need to create one by moving the before item closer
# to its predecessor. This process will recursively move all the predecessors until we have a place
if (after.relative_position - before.relative_position) < 2
before.move_before
@@ -108,11 +129,11 @@ module RelativePositioning
pos_after = before.next_relative_position
if before.shift_after?
- issue_to_move = self.class.in_parents(parent_ids).find_by!(relative_position: pos_after)
- issue_to_move.move_after
- @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
+ item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_after)
+ item_to_move.move_after
+ @positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
- pos_after = issue_to_move.relative_position
+ pos_after = item_to_move.relative_position
end
self.relative_position = self.class.position_between(pos_before, pos_after)
@@ -123,11 +144,11 @@ module RelativePositioning
pos_before = after.prev_relative_position
if after.shift_before?
- issue_to_move = self.class.in_parents(parent_ids).find_by!(relative_position: pos_before)
- issue_to_move.move_before
- @positionable_neighbours = [issue_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
+ item_to_move = self.class.relative_positioning_query_base(self).find_by!(relative_position: pos_before)
+ item_to_move.move_before
+ @positionable_neighbours = [item_to_move] # rubocop:disable Gitlab/ModuleWithInstanceVariables
- pos_before = issue_to_move.relative_position
+ pos_before = item_to_move.relative_position
end
self.relative_position = self.class.position_between(pos_before, pos_after)
@@ -141,13 +162,13 @@ module RelativePositioning
self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION)
end
- # Indicates if there is an issue that should be shifted to free the place
+ # Indicates if there is an item that should be shifted to free the place
def shift_after?
next_pos = next_relative_position
next_pos && (next_pos - relative_position) == 1
end
- # Indicates if there is an issue that should be shifted to free the place
+ # Indicates if there is an item that should be shifted to free the place
def shift_before?
prev_pos = prev_relative_position
prev_pos && (relative_position - prev_pos) == 1
@@ -159,7 +180,7 @@ module RelativePositioning
def save_positionable_neighbours
return unless @positionable_neighbours
- status = @positionable_neighbours.all? { |issue| issue.save(touch: false) }
+ status = @positionable_neighbours.all? { |item| item.save(touch: false) }
@positionable_neighbours = nil
status
@@ -170,16 +191,15 @@ module RelativePositioning
# When calculating across projects, this is much more efficient than
# MAX(relative_position) without the GROUP BY, due to index usage:
# https://gitlab.com/gitlab-org/gitlab-ce/issues/54276#note_119340977
- relation = self.class
- .in_parents(parent_ids)
+ relation = self.class.relative_positioning_query_base(self)
.order(Gitlab::Database.nulls_last_order('position', 'DESC'))
+ .group(self.class.relative_positioning_parent_column)
.limit(1)
- .group(self.class.parent_column)
relation = yield relation if block_given?
relation
- .pluck(self.class.parent_column, Arel.sql("#{calculation}(relative_position) AS position"))
+ .pluck(self.class.relative_positioning_parent_column, Arel.sql("#{calculation}(relative_position) AS position"))
.first&.
last
end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 8c5dd5e382e..164858dc432 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -91,11 +91,11 @@ class Issue < ApplicationRecord
end
end
- class << self
- alias_method :in_parents, :in_projects
+ def self.relative_positioning_query_base(issue)
+ in_projects(issue.parent_ids)
end
- def self.parent_column
+ def self.relative_positioning_parent_column
:project_id
end
diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb
deleted file mode 100644
index d0ae45f7871..00000000000
--- a/spec/models/concerns/relative_positioning_spec.rb
+++ /dev/null
@@ -1,242 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe RelativePositioning do
- let(:project) { create(:project) }
- let(:issue) { create(:issue, project: project) }
- let(:issue1) { create(:issue, project: project) }
- let(:new_issue) { create(:issue, project: project) }
-
- describe '.move_to_end' do
- it 'moves the object to the end' do
- Issue.move_to_end([issue, issue1])
-
- expect(issue1.prev_relative_position).to eq issue.relative_position
- expect(issue.prev_relative_position).to eq nil
- expect(issue1.next_relative_position).to eq nil
- end
-
- it 'does not perform any moves if all issues have their relative_position set' do
- issue.update!(relative_position: 1)
-
- expect(issue).not_to receive(:save)
-
- Issue.move_to_end([issue])
- end
- end
-
- describe '#max_relative_position' do
- it 'returns maximum position' do
- expect(issue.max_relative_position).to eq issue1.relative_position
- end
- end
-
- describe '#prev_relative_position' do
- it 'returns previous position if there is an issue above' do
- expect(issue1.prev_relative_position).to eq issue.relative_position
- end
-
- it 'returns nil if there is no issue above' do
- expect(issue.prev_relative_position).to eq nil
- end
- end
-
- describe '#next_relative_position' do
- it 'returns next position if there is an issue below' do
- expect(issue.next_relative_position).to eq issue1.relative_position
- end
-
- it 'returns nil if there is no issue below' do
- expect(issue1.next_relative_position).to eq nil
- end
- end
-
- describe '#move_before' do
- it 'moves issue before' do
- [issue1, issue].each(&:move_to_end)
-
- issue.move_before(issue1)
-
- expect(issue.relative_position).to be < issue1.relative_position
- end
- end
-
- describe '#move_after' do
- it 'moves issue after' do
- [issue, issue1].each(&:move_to_end)
-
- issue.move_after(issue1)
-
- expect(issue.relative_position).to be > issue1.relative_position
- end
- end
-
- describe '#move_to_end' do
- before do
- [issue, issue1].each do |issue|
- issue.move_to_end && issue.save
- end
- end
-
- it 'moves issue to the end' do
- new_issue.move_to_end
-
- expect(new_issue.relative_position).to be > issue1.relative_position
- end
- end
-
- describe '#shift_after?' do
- before do
- [issue, issue1].each do |issue|
- issue.move_to_end && issue.save
- end
- end
-
- it 'returns true' do
- issue.update(relative_position: issue1.relative_position - 1)
-
- expect(issue.shift_after?).to be_truthy
- end
-
- it 'returns false' do
- issue.update(relative_position: issue1.relative_position - 2)
-
- expect(issue.shift_after?).to be_falsey
- end
- end
-
- describe '#shift_before?' do
- before do
- [issue, issue1].each do |issue|
- issue.move_to_end && issue.save
- end
- end
-
- it 'returns true' do
- issue.update(relative_position: issue1.relative_position + 1)
-
- expect(issue.shift_before?).to be_truthy
- end
-
- it 'returns false' do
- issue.update(relative_position: issue1.relative_position + 2)
-
- expect(issue.shift_before?).to be_falsey
- end
- end
-
- describe '#move_between' do
- before do
- [issue, issue1].each do |issue|
- issue.move_to_end && issue.save
- end
- end
-
- it 'positions issue between two other' do
- new_issue.move_between(issue, issue1)
-
- expect(new_issue.relative_position).to be > issue.relative_position
- expect(new_issue.relative_position).to be < issue1.relative_position
- end
-
- it 'positions issue between on top' do
- new_issue.move_between(nil, issue)
-
- expect(new_issue.relative_position).to be < issue.relative_position
- end
-
- it 'positions issue between to end' do
- new_issue.move_between(issue1, nil)
-
- expect(new_issue.relative_position).to be > issue1.relative_position
- end
-
- it 'positions issues even when after and before positions are the same' do
- issue1.update relative_position: issue.relative_position
-
- new_issue.move_between(issue, issue1)
-
- expect(new_issue.relative_position).to be > issue.relative_position
- expect(issue.relative_position).to be < issue1.relative_position
- 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)
-
- expect(new_issue.relative_position).to be > issue.relative_position
- expect(issue.relative_position).to be < issue1.relative_position
- end
-
- it 'positions issue in the middle of other two if distance is big enough' do
- issue.update relative_position: 6000
- issue1.update relative_position: 10000
-
- new_issue.move_between(issue, issue1)
-
- expect(new_issue.relative_position).to eq(8000)
- 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)
-
- expect(new_issue.relative_position).to eq(6000 - 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)
-
- expect(new_issue.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
- end
-
- it 'positions issue in the middle of other two if distance is not big enough' do
- issue.update relative_position: 100
- issue1.update relative_position: 400
-
- new_issue.move_between(issue, issue1)
-
- expect(new_issue.relative_position).to eq(250)
- end
-
- it 'positions issue in the middle of other two is there is no place' do
- issue.update relative_position: 100
- issue1.update relative_position: 101
-
- new_issue.move_between(issue, issue1)
-
- expect(new_issue.relative_position).to be_between(issue.relative_position, issue1.relative_position)
- end
-
- it 'uses rebalancing if there is no place' do
- issue.update relative_position: 100
- issue1.update relative_position: 101
- issue2 = create(:issue, relative_position: 102, project: project)
- new_issue.update relative_position: 103
-
- new_issue.move_between(issue1, issue2)
- new_issue.save!
-
- expect(new_issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
- expect(issue.reload.relative_position).not_to eq(100)
- end
-
- it 'positions issue right if we pass none-sequential parameters' do
- issue.update relative_position: 99
- issue1.update relative_position: 101
- issue2 = create(:issue, relative_position: 102, project: project)
- new_issue.update relative_position: 103
-
- new_issue.move_between(issue, issue2)
- new_issue.save!
-
- expect(new_issue.relative_position).to be(100)
- end
- end
-end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index d5b016dc8f6..2e7d78d77a8 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -871,4 +871,12 @@ describe Issue do
expect(issue.labels_hook_attrs).to eq([label.hook_attrs])
end
end
+
+ context "relative positioning" do
+ it_behaves_like "a class that supports relative positioning" do
+ let(:project) { create(:project) }
+ let(:factory) { :issue }
+ let(:default_params) { { project: project } }
+ end
+ end
end
diff --git a/spec/support/shared_examples/relative_positioning_shared_examples.rb b/spec/support/shared_examples/relative_positioning_shared_examples.rb
new file mode 100644
index 00000000000..5ee62644c54
--- /dev/null
+++ b/spec/support/shared_examples/relative_positioning_shared_examples.rb
@@ -0,0 +1,253 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples "a class that supports relative positioning" do
+ let(:item1) { create(factory, default_params) }
+ let(:item2) { create(factory, default_params) }
+ let(:new_item) { create(factory, default_params) }
+
+ def create_item(params)
+ create(factory, params.merge(default_params))
+ end
+
+ describe '.move_to_end' do
+ it 'moves the object to the end' do
+ item1.update(relative_position: 5)
+ item2.update(relative_position: 15)
+
+ described_class.move_to_end([item1, item2])
+
+ expect(item2.prev_relative_position).to eq item1.relative_position
+ expect(item1.prev_relative_position).to eq nil
+ expect(item2.next_relative_position).to eq nil
+ end
+
+ it 'does not perform any moves if all items have their relative_position set' do
+ item1.update!(relative_position: 1)
+
+ expect(item1).not_to receive(:save)
+
+ described_class.move_to_end([item1])
+ end
+ end
+
+ describe '#max_relative_position' do
+ it 'returns maximum position' do
+ expect(item1.max_relative_position).to eq item2.relative_position
+ end
+ end
+
+ describe '#prev_relative_position' do
+ it 'returns previous position if there is an item above' do
+ item1.update(relative_position: 5)
+ item2.update(relative_position: 15)
+
+ expect(item2.prev_relative_position).to eq item1.relative_position
+ end
+
+ it 'returns nil if there is no item above' do
+ expect(item1.prev_relative_position).to eq nil
+ end
+ end
+
+ describe '#next_relative_position' do
+ it 'returns next position if there is an item below' do
+ item1.update(relative_position: 5)
+ item2.update(relative_position: 15)
+
+ expect(item1.next_relative_position).to eq item2.relative_position
+ end
+
+ it 'returns nil if there is no item below' do
+ expect(item2.next_relative_position).to eq nil
+ end
+ end
+
+ describe '#move_before' do
+ it 'moves item before' do
+ [item2, item1].each(&:move_to_end)
+
+ item1.move_before(item2)
+
+ expect(item1.relative_position).to be < item2.relative_position
+ end
+ end
+
+ describe '#move_after' do
+ it 'moves item after' do
+ [item1, item2].each(&:move_to_end)
+
+ item1.move_after(item2)
+
+ expect(item1.relative_position).to be > item2.relative_position
+ end
+ end
+
+ describe '#move_to_end' do
+ before do
+ [item1, item2].each do |item1|
+ item1.move_to_end && item1.save
+ end
+ end
+
+ it 'moves item to the end' do
+ new_item.move_to_end
+
+ expect(new_item.relative_position).to be > item2.relative_position
+ end
+ end
+
+ describe '#shift_after?' do
+ before do
+ [item1, item2].each do |item1|
+ item1.move_to_end && item1.save
+ end
+ end
+
+ it 'returns true' do
+ item1.update(relative_position: item2.relative_position - 1)
+
+ expect(item1.shift_after?).to be_truthy
+ end
+
+ it 'returns false' do
+ item1.update(relative_position: item2.relative_position - 2)
+
+ expect(item1.shift_after?).to be_falsey
+ end
+ end
+
+ describe '#shift_before?' do
+ before do
+ [item1, item2].each do |item1|
+ item1.move_to_end && item1.save
+ end
+ end
+
+ it 'returns true' do
+ item1.update(relative_position: item2.relative_position + 1)
+
+ expect(item1.shift_before?).to be_truthy
+ end
+
+ it 'returns false' do
+ item1.update(relative_position: item2.relative_position + 2)
+
+ expect(item1.shift_before?).to be_falsey
+ end
+ end
+
+ describe '#move_between' do
+ before do
+ [item1, item2].each do |item1|
+ item1.move_to_end && item1.save
+ end
+ end
+
+ it 'positions item between two other' do
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to be > item1.relative_position
+ expect(new_item.relative_position).to be < item2.relative_position
+ end
+
+ it 'positions item between on top' do
+ new_item.move_between(nil, item1)
+
+ expect(new_item.relative_position).to be < item1.relative_position
+ end
+
+ it 'positions item between to end' do
+ new_item.move_between(item2, nil)
+
+ expect(new_item.relative_position).to be > item2.relative_position
+ end
+
+ it 'positions items even when after and before positions are the same' do
+ item2.update relative_position: item1.relative_position
+
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to be > item1.relative_position
+ expect(item1.relative_position).to be < item2.relative_position
+ end
+
+ it 'positions items between other two if distance is 1' do
+ item2.update relative_position: item1.relative_position + 1
+
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to be > item1.relative_position
+ expect(item1.relative_position).to be < item2.relative_position
+ end
+
+ it 'positions item in the middle of other two if distance is big enough' do
+ item1.update relative_position: 6000
+ item2.update relative_position: 10000
+
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to eq(8000)
+ end
+
+ it 'positions item closer to the middle if we are at the very top' do
+ item2.update relative_position: 6000
+
+ new_item.move_between(nil, item2)
+
+ expect(new_item.relative_position).to eq(6000 - RelativePositioning::IDEAL_DISTANCE)
+ end
+
+ it 'positions item closer to the middle if we are at the very bottom' do
+ new_item.update relative_position: 1
+ item1.update relative_position: 6000
+ item2.destroy
+
+ new_item.move_between(item1, nil)
+
+ expect(new_item.relative_position).to eq(6000 + RelativePositioning::IDEAL_DISTANCE)
+ end
+
+ it 'positions item in the middle of other two if distance is not big enough' do
+ item1.update relative_position: 100
+ item2.update relative_position: 400
+
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to eq(250)
+ end
+
+ it 'positions item in the middle of other two is there is no place' do
+ item1.update relative_position: 100
+ item2.update relative_position: 101
+
+ new_item.move_between(item1, item2)
+
+ expect(new_item.relative_position).to be_between(item1.relative_position, item2.relative_position)
+ end
+
+ it 'uses rebalancing if there is no place' do
+ item1.update relative_position: 100
+ item2.update relative_position: 101
+ item3 = create_item(relative_position: 102)
+ new_item.update relative_position: 103
+
+ new_item.move_between(item2, item3)
+ new_item.save!
+
+ expect(new_item.relative_position).to be_between(item2.relative_position, item3.relative_position)
+ expect(item1.reload.relative_position).not_to eq(100)
+ end
+
+ it 'positions item right if we pass none-sequential parameters' do
+ item1.update relative_position: 99
+ item2.update relative_position: 101
+ item3 = create_item(relative_position: 102)
+ new_item.update relative_position: 103
+
+ new_item.move_between(item1, item3)
+ new_item.save!
+
+ expect(new_item.relative_position).to be(100)
+ end
+ end
+end