diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 5a1fc68a915e51d261ed40250bb6c4ac5849bbab..c2e8bfa1140086ffd37f0dc143775cdbc4bb4920 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -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'
diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index ca64907f04bf35d52a7c2f30918722f729bc6d5b..afc818b28ab4de6e157add33f02053494ad5fb3c 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -3,11 +3,15 @@
# 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)
+# The model must have the following named columns:
+# - id: integer
+# - relative_position: integer
#
-# Setup like this in the body of your class:
+# The model must support a concept of siblings via a child->parent relationship,
+# to enable rebalancing and `GROUP BY` in queries.
+# - example: project -> issues, project is the parent relation (issues table has a parent_id column)
+#
+# Two class methods must be defined when including this concern:
#
# include RelativePositioning
#
@@ -24,66 +28,162 @@
module RelativePositioning
extend ActiveSupport::Concern
- MIN_POSITION = 0
- START_POSITION = Gitlab::Database::MAX_INT_VALUE / 2
- MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
- IDEAL_DISTANCE = 500
+ STEPS = 10
+ IDEAL_DISTANCE = 2**(STEPS - 1) + 1
- class_methods do
- def move_nulls_to_end(objects)
- objects = objects.reject(&:relative_position)
- return if objects.empty?
+ MIN_POSITION = Gitlab::Database::MIN_INT_VALUE
+ START_POSITION = 0
+ MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
- self.transaction do
- max_relative_position = objects.first.max_relative_position
+ MAX_GAP = IDEAL_DISTANCE * 2
+ MIN_GAP = 2
- objects.each do |object|
- relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
- object.update_column(:relative_position, relative_position)
+ NoSpaceLeft = Class.new(StandardError)
- max_relative_position = relative_position
- end
- end
+ class_methods do
+ def move_nulls_to_end(objects)
+ move_nulls(objects, at_end: true)
end
def move_nulls_to_start(objects)
- objects = objects.reject(&:relative_position)
- return if objects.empty?
-
- self.transaction do
- min_relative_position = objects.first.min_relative_position
-
- objects.reverse_each do |object|
- relative_position = position_between(MIN_POSITION, min_relative_position || START_POSITION)
- object.update_column(:relative_position, relative_position)
-
- min_relative_position = relative_position
- end
- end
+ move_nulls(objects, at_end: false)
end
# This method takes two integer values (positions) and
# calculates the position between them. The range is huge as
- # the maximum integer value is 2147483647. We are incrementing position by IDEAL_DISTANCE * 2 every time
- # when we have enough space. If distance is less than IDEAL_DISTANCE, we are calculating an average number.
+ # the maximum integer value is 2147483647.
+ #
+ # We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION].
+ #
+ # Then we handle one of three cases:
+ # - If the gap is too small, we raise NoSpaceLeft
+ # - If the gap is larger than MAX_GAP, we place the new position at most
+ # IDEAL_DISTANCE from the edge of the gap.
+ # - otherwise we place the new position at the midpoint.
+ #
+ # The new position will always satisfy: pos_before <= midpoint <= pos_after
+ #
+ # As a precondition, the gap between pos_before and pos_after MUST be >= 2.
+ # If the gap is too small, NoSpaceLeft is raised.
+ #
+ # This class method should only be called by instance methods of this module, which
+ # include handling for minimum gap size.
+ #
+ # @raises NoSpaceLeft
+ # @api private
def position_between(pos_before, pos_after)
pos_before ||= MIN_POSITION
pos_after ||= MAX_POSITION
pos_before, pos_after = [pos_before, pos_after].sort
- halfway = (pos_after + pos_before) / 2
- distance_to_halfway = pos_after - halfway
+ gap_width = pos_after - pos_before
+ midpoint = [pos_after - 1, pos_before + (gap_width / 2)].min
- if distance_to_halfway < IDEAL_DISTANCE
- halfway
- else
+ if gap_width < MIN_GAP
+ raise NoSpaceLeft
+ elsif gap_width > MAX_GAP
if pos_before == MIN_POSITION
pos_after - IDEAL_DISTANCE
elsif pos_after == MAX_POSITION
pos_before + IDEAL_DISTANCE
else
- halfway
+ midpoint
+ end
+ else
+ midpoint
+ end
+ end
+
+ private
+
+ # @api private
+ def gap_size(object, gaps:, at_end:, starting_from:)
+ total_width = IDEAL_DISTANCE * gaps
+ size = if at_end && starting_from + total_width >= MAX_POSITION
+ (MAX_POSITION - starting_from) / gaps
+ elsif !at_end && starting_from - total_width <= MIN_POSITION
+ (starting_from - MIN_POSITION) / gaps
+ else
+ IDEAL_DISTANCE
+ end
+
+ # Shift max elements leftwards if there isn't enough space
+ return [size, starting_from] if size >= MIN_GAP
+
+ order = at_end ? :desc : :asc
+ terminus = object
+ .send(:relative_siblings) # rubocop:disable GitlabSecurity/PublicSend
+ .where('relative_position IS NOT NULL')
+ .order(relative_position: order)
+ .first
+
+ if at_end
+ terminus.move_sequence_before(true)
+ max_relative_position = terminus.reset.relative_position
+ [[(MAX_POSITION - max_relative_position) / gaps, IDEAL_DISTANCE].min, max_relative_position]
+ else
+ terminus.move_sequence_after(true)
+ min_relative_position = terminus.reset.relative_position
+ [[(min_relative_position - MIN_POSITION) / gaps, IDEAL_DISTANCE].min, min_relative_position]
+ end
+ end
+
+ # @api private
+ # @param [Array<RelativePositioning>] objects The objects to give positions to. The relative
+ # order will be preserved (i.e. when this method returns,
+ # objects.first.relative_position < objects.last.relative_position)
+ # @param [Boolean] at_end: The placement.
+ # If `true`, then all objects with `null` positions are placed _after_
+ # all siblings with positions. If `false`, all objects with `null`
+ # positions are placed _before_ all siblings with positions.
+ def move_nulls(objects, at_end:)
+ objects = objects.reject(&:relative_position)
+ return if objects.empty?
+
+ representative = objects.first
+ number_of_gaps = objects.size + 1 # 1 at left, one between each, and one at right
+ position = if at_end
+ representative.max_relative_position
+ else
+ representative.min_relative_position
+ end
+
+ position ||= START_POSITION # If there are no positioned siblings, start from START_POSITION
+
+ gap, position = gap_size(representative, gaps: number_of_gaps, at_end: at_end, starting_from: position)
+
+ # Raise if we could not make enough space
+ raise NoSpaceLeft if gap < MIN_GAP
+
+ indexed = objects.each_with_index.to_a
+ starting_from = at_end ? position : position - (gap * number_of_gaps)
+
+ # Some classes are polymorphic, and not all siblings are in the same table.
+ by_model = indexed.group_by { |pair| pair.first.class }
+
+ by_model.each do |model, pairs|
+ model.transaction do
+ pairs.each_slice(100) do |batch|
+ # These are known to be integers, one from the DB, and the other
+ # calculated by us, and thus safe to interpolate
+ values = batch.map do |obj, i|
+ pos = starting_from + gap * (i + 1)
+ obj.relative_position = pos
+ "(#{obj.id}, #{pos})"
+ end.join(', ')
+
+ model.connection.exec_query(<<~SQL, "UPDATE #{model.table_name} positions")
+ WITH cte(cte_id, new_pos) AS (
+ SELECT *
+ FROM (VALUES #{values}) as t (id, pos)
+ )
+ UPDATE #{model.table_name}
+ SET relative_position = cte.new_pos
+ FROM cte
+ WHERE cte_id = id
+ SQL
+ end
end
end
end
@@ -97,11 +197,12 @@ def max_relative_position(&block)
calculate_relative_position('MAX', &block)
end
- def prev_relative_position
+ def prev_relative_position(ignoring: nil)
prev_pos = nil
if self.relative_position
prev_pos = max_relative_position do |relation|
+ relation = relation.id_not_in(ignoring.id) if ignoring.present?
relation.where('relative_position < ?', self.relative_position)
end
end
@@ -109,11 +210,12 @@ def prev_relative_position
prev_pos
end
- def next_relative_position
+ def next_relative_position(ignoring: nil)
next_pos = nil
if self.relative_position
next_pos = min_relative_position do |relation|
+ relation = relation.id_not_in(ignoring.id) if ignoring.present?
relation.where('relative_position > ?', self.relative_position)
end
end
@@ -125,24 +227,44 @@ def move_between(before, after)
return move_after(before) unless after
return move_before(after) unless before
- # If there is no place to insert an item we need to create one by moving the item
- # before this and all preceding items until there is a gap
before, after = after, before if after.relative_position < before.relative_position
- if (after.relative_position - before.relative_position) < 2
- after.move_sequence_before
- before.reset
+
+ pos_left = before.relative_position
+ pos_right = after.relative_position
+
+ if pos_right - pos_left < MIN_GAP
+ # Not enough room! Make space by shifting all previous elements to the left
+ # if there is enough space, else to the right
+ gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend
+
+ if gap.present?
+ after.move_sequence_before(next_gap: gap)
+ pos_left -= optimum_delta_for_gap(gap)
+ else
+ before.move_sequence_after
+ pos_right = after.reset.relative_position
+ end
end
- self.relative_position = self.class.position_between(before.relative_position, after.relative_position)
+ new_position = self.class.position_between(pos_left, pos_right)
+
+ self.relative_position = new_position
end
def move_after(before = self)
pos_before = before.relative_position
- pos_after = before.next_relative_position
+ pos_after = before.next_relative_position(ignoring: self)
+
+ if pos_before == MAX_POSITION || gap_too_small?(pos_after, pos_before)
+ gap = before.send(:find_next_gap_after) # rubocop:disable GitlabSecurity/PublicSend
- if pos_after && (pos_after - pos_before) < 2
- before.move_sequence_after
- pos_after = before.next_relative_position
+ if gap.nil?
+ before.move_sequence_before(true)
+ pos_before = before.reset.relative_position
+ else
+ before.move_sequence_after(next_gap: gap)
+ pos_after += optimum_delta_for_gap(gap)
+ end
end
self.relative_position = self.class.position_between(pos_before, pos_after)
@@ -150,80 +272,168 @@ def move_after(before = self)
def move_before(after = self)
pos_after = after.relative_position
- pos_before = after.prev_relative_position
+ pos_before = after.prev_relative_position(ignoring: self)
- if pos_before && (pos_after - pos_before) < 2
- after.move_sequence_before
- pos_before = after.prev_relative_position
+ if pos_after == MIN_POSITION || gap_too_small?(pos_before, pos_after)
+ gap = after.send(:find_next_gap_before) # rubocop:disable GitlabSecurity/PublicSend
+
+ if gap.nil?
+ after.move_sequence_after(true)
+ pos_after = after.reset.relative_position
+ else
+ after.move_sequence_before(next_gap: gap)
+ pos_before -= optimum_delta_for_gap(gap)
+ end
end
self.relative_position = self.class.position_between(pos_before, pos_after)
end
def move_to_end
- self.relative_position = self.class.position_between(max_relative_position || START_POSITION, MAX_POSITION)
+ max_pos = max_relative_position
+
+ self.relative_position = max_pos.nil? ? START_POSITION : self.class.position_between(max_pos, MAX_POSITION)
end
def move_to_start
- self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION)
+ min_pos = max_relative_position
+
+ self.relative_position = min_pos.nil? ? START_POSITION : self.class.position_between(MIN_POSITION, min_pos)
end
# Moves the sequence before the current item to the middle of the next gap
- # For example, we have 5 11 12 13 14 15 and the current item is 15
- # This moves the sequence 11 12 13 14 to 8 9 10 11
- def move_sequence_before
- next_gap = find_next_gap_before
+ # For example, we have
+ #
+ # 5 . . . . . 11 12 13 14 [15] 16 . 17
+ # -----------
+ #
+ # This moves the sequence [11 12 13 14] to [8 9 10 11], so we have:
+ #
+ # 5 . . 8 9 10 11 . . . [15] 16 . 17
+ # ---------
+ #
+ # Creating a gap to the left of the current item. We can understand this as
+ # dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3.
+ #
+ # If `include_self` is true, the current item will also be moved, creating a
+ # gap to the right of the current item:
+ #
+ # 5 . . 8 9 10 11 [14] . . . 16 . 17
+ # --------------
+ #
+ # As an optimization, the gap can be precalculated and passed to this method.
+ #
+ # @api private
+ # @raises NoSpaceLeft if the sequence cannot be moved
+ def move_sequence_before(include_self = false, next_gap: find_next_gap_before)
+ raise NoSpaceLeft unless next_gap.present?
+
delta = optimum_delta_for_gap(next_gap)
- move_sequence(next_gap[:start], relative_position, -delta)
+ move_sequence(next_gap[:start], relative_position, -delta, include_self)
end
# Moves the sequence after the current item to the middle of the next gap
- # For example, we have 11 12 13 14 15 21 and the current item is 11
- # This moves the sequence 12 13 14 15 to 15 16 17 18
- def move_sequence_after
- next_gap = find_next_gap_after
+ # For example, we have:
+ #
+ # 8 . 10 [11] 12 13 14 15 . . . . . 21
+ # -----------
+ #
+ # This moves the sequence [12 13 14 15] to [15 16 17 18], so we have:
+ #
+ # 8 . 10 [11] . . . 15 16 17 18 . . 21
+ # -----------
+ #
+ # Creating a gap to the right of the current item. We can understand this as
+ # dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2.
+ #
+ # If `include_self` is true, the current item will also be moved, creating a
+ # gap to the left of the current item:
+ #
+ # 8 . 10 . . . [14] 15 16 17 18 . . 21
+ # ----------------
+ #
+ # As an optimization, the gap can be precalculated and passed to this method.
+ #
+ # @api private
+ # @raises NoSpaceLeft if the sequence cannot be moved
+ def move_sequence_after(include_self = false, next_gap: find_next_gap_after)
+ raise NoSpaceLeft unless next_gap.present?
+
delta = optimum_delta_for_gap(next_gap)
- move_sequence(relative_position, next_gap[:start], delta)
+ move_sequence(relative_position, next_gap[:start], delta, include_self)
end
private
- # Supposing that we have a sequence of items: 1 5 11 12 13 and the current item is 13
- # This would return: `{ start: 11, end: 5 }`
+ def gap_too_small?(pos_a, pos_b)
+ return false unless pos_a && pos_b
+
+ (pos_a - pos_b).abs < MIN_GAP
+ end
+
+ # Find the first suitable gap to the left of the current position.
+ #
+ # Satisfies the relations:
+ # - gap[:start] <= relative_position
+ # - abs(gap[:start] - gap[:end]) >= MIN_GAP
+ # - MIN_POSITION <= gap[:start] <= MAX_POSITION
+ # - MIN_POSITION <= gap[:end] <= MAX_POSITION
+ #
+ # Supposing that the current item is 13, and we have a sequence of items:
+ #
+ # 1 . . . 5 . . . . 11 12 [13] 14 . . 17
+ # ^---------^
+ #
+ # Then we return: `{ start: 11, end: 5 }`
+ #
+ # Here start refers to the end of the gap closest to the current item.
def find_next_gap_before
items_with_next_pos = scoped_items
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos')
.where('relative_position <= ?', relative_position)
.order(relative_position: :desc)
- find_next_gap(items_with_next_pos).tap do |gap|
- gap[:end] ||= MIN_POSITION
- end
+ find_next_gap(items_with_next_pos, MIN_POSITION)
end
- # Supposing that we have a sequence of items: 13 14 15 20 24 and the current item is 13
- # This would return: `{ start: 15, end: 20 }`
+ # Find the first suitable gap to the right of the current position.
+ #
+ # Satisfies the relations:
+ # - gap[:start] >= relative_position
+ # - abs(gap[:start] - gap[:end]) >= MIN_GAP
+ # - MIN_POSITION <= gap[:start] <= MAX_POSITION
+ # - MIN_POSITION <= gap[:end] <= MAX_POSITION
+ #
+ # Supposing the current item is 13, and that we have a sequence of items:
+ #
+ # 9 . . . [13] 14 15 . . . . 20 . . . 24
+ # ^---------^
+ #
+ # Then we return: `{ start: 15, end: 20 }`
+ #
+ # Here start refers to the end of the gap closest to the current item.
def find_next_gap_after
items_with_next_pos = scoped_items
.select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos')
.where('relative_position >= ?', relative_position)
.order(:relative_position)
- find_next_gap(items_with_next_pos).tap do |gap|
- gap[:end] ||= MAX_POSITION
- end
+ find_next_gap(items_with_next_pos, MAX_POSITION)
end
- def find_next_gap(items_with_next_pos)
- gap = self.class.from(items_with_next_pos, :items_with_next_pos)
- .where('ABS(pos - next_pos) > 1 OR next_pos IS NULL')
- .limit(1)
- .pluck(:pos, :next_pos)
- .first
+ def find_next_gap(items_with_next_pos, end_is_nil)
+ gap = self.class
+ .from(items_with_next_pos, :items)
+ .where('next_pos IS NULL OR ABS(pos::bigint - next_pos::bigint) >= ?', MIN_GAP)
+ .limit(1)
+ .pluck(:pos, :next_pos)
+ .first
- { start: gap[0], end: gap[1] }
+ return if gap.nil? || gap.first == end_is_nil
+
+ { start: gap.first, end: gap.second || end_is_nil }
end
def optimum_delta_for_gap(gap)
@@ -232,9 +442,10 @@ def optimum_delta_for_gap(gap)
[delta, IDEAL_DISTANCE].min
end
- def move_sequence(start_pos, end_pos, delta)
- scoped_items
- .where.not(id: self.id)
+ def move_sequence(start_pos, end_pos, delta, include_self = false)
+ relation = include_self ? scoped_items : relative_siblings
+
+ relation
.where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
.update_all("relative_position = relative_position + #{delta}")
end
@@ -255,6 +466,10 @@ def calculate_relative_position(calculation)
.first&.last
end
+ def relative_siblings(relation = scoped_items)
+ relation.id_not_in(id)
+ end
+
def scoped_items
self.class.relative_positioning_query_base(self)
end
diff --git a/changelogs/unreleased/ajk-relative-positioning-improvements.yml b/changelogs/unreleased/ajk-relative-positioning-improvements.yml
new file mode 100644
index 0000000000000000000000000000000000000000..c9de05a0403a353390554486d26fa08457cd6ea5
--- /dev/null
+++ b/changelogs/unreleased/ajk-relative-positioning-improvements.yml
@@ -0,0 +1,5 @@
+---
+title: Performance and robustness improvements for relative positioning
+merge_request: 37724
+author:
+type: performance
diff --git a/ee/app/models/concerns/epic_tree_sorting.rb b/ee/app/models/concerns/epic_tree_sorting.rb
index ae56137381cfa35824ff2b5c5278008326273b5f..4763462803661bcf9a2772e2372577b94a7b5f9a 100644
--- a/ee/app/models/concerns/epic_tree_sorting.rb
+++ b/ee/app/models/concerns/epic_tree_sorting.rb
@@ -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 @@ def relative_positioning_parent_column
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)
diff --git a/ee/spec/models/epic_issue_spec.rb b/ee/spec/models/epic_issue_spec.rb
index fca7dfa2e630376221a347466e4fe7ebda5a9dd2..84aa50377ad8abd6b1c684616037bd84a9a1dad2 100644
--- a/ee/spec/models/epic_issue_spec.rb
+++ b/ee/spec/models/epic_issue_spec.rb
@@ -28,9 +28,49 @@
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
diff --git a/ee/spec/models/epic_spec.rb b/ee/spec/models/epic_spec.rb
index 508a70e4e39318d7f589e88712b3d81fa7ef4de6..b42b8f4abfc67f994acfb59dbb1921dcaf5ec025 100644
--- a/ee/spec/models/epic_spec.rb
+++ b/ee/spec/models/epic_spec.rb
@@ -616,9 +616,19 @@ def epics(order_by)
end
context "relative positioning" do
- it_behaves_like "a class that supports relative positioning" do
- let(:factory) { :epic }
- let(:default_params) { {} }
+ 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
diff --git a/ee/spec/models/issue_spec.rb b/ee/spec/models/issue_spec.rb
index bad3dfde7432a8a2b79066d2ecca9914fa46d5b1..583527d93251d91fdb5ed0012c79cd833e7c0d57 100644
--- a/ee/spec/models/issue_spec.rb
+++ b/ee/spec/models/issue_spec.rb
@@ -454,20 +454,14 @@
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 @@
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 @@
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 @@
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 @@
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)
diff --git a/ee/spec/services/epic_issues/create_service_spec.rb b/ee/spec/services/epic_issues/create_service_spec.rb
index f3d42bbc4082c6231086767af701813ee002c751..12b661cd4f72b0d46b7f8218550c1af2b9163367 100644
--- a/ee/spec/services/epic_issues/create_service_spec.rb
+++ b/ee/spec/services/epic_issues/create_service_spec.rb
@@ -169,7 +169,9 @@ def assign_issue(references)
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 @@ def assign_issue(references)
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
diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb
index 859b53b9887d8f1e64d84e4268c003e109180690..e7df9fd27f01d6b6ceb8dbe0ccf541bf001ffbc2 100644
--- a/lib/gitlab/database.rb
+++ b/lib/gitlab/database.rb
@@ -22,6 +22,7 @@ module Database
# 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
diff --git a/spec/features/boards/issue_ordering_spec.rb b/spec/features/boards/issue_ordering_spec.rb
index 03a76d9d3fd461e0cf69d66b6ff222291b153e57..87d29eed68d254470f143300e0b85342aee391b1 100644
--- a/spec/features/boards/issue_ordering_spec.rb
+++ b/spec/features/boards/issue_ordering_spec.rb
@@ -21,7 +21,7 @@
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)
diff --git a/spec/models/analytics/cycle_analytics/project_stage_spec.rb b/spec/models/analytics/cycle_analytics/project_stage_spec.rb
index 2e0240115533e0ef58e6d88f13413301b28ffa08..4675f0379575c661b823853d284074a685f07bd9 100644
--- a/spec/models/analytics/cycle_analytics/project_stage_spec.rb
+++ b/spec/models/analytics/cycle_analytics/project_stage_spec.rb
@@ -23,7 +23,7 @@
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
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 586ef9da98ed4dda72e8d3455cadc4ecd454dc18..59634524e746a3ed8ff8e1aed2ceb38dbce69dbc 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -6,6 +6,7 @@
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 @@
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 @@
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 @@
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
diff --git a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb
index ab1467145a0433656786e3fe2695c4d9ddc53558..d09eb1fa14dcb1299d6f44237352a14e5bb75f38 100644
--- a/spec/support/shared_examples/models/relative_positioning_shared_examples.rb
+++ b/spec/support/shared_examples/models/relative_positioning_shared_examples.rb
@@ -25,7 +25,6 @@ def create_items_with_positions(positions)
items = [item1, item2, item3]
described_class.move_nulls_to_end(items)
- items.map(&:reload)
expect(items.sort_by(&:relative_position)).to eq(items)
expect(item1.relative_position).to be(1000)
@@ -35,22 +34,57 @@ def create_items_with_positions(positions)
expect(item3.next_relative_position).to be_nil
end
+ it 'preserves relative position' do
+ item1.update!(relative_position: nil)
+ item2.update!(relative_position: nil)
+
+ described_class.move_nulls_to_end([item1, item2])
+
+ expect(item1.relative_position).to be < item2.relative_position
+ end
+
it 'moves the item near the start position when there are no existing positions' do
item1.update!(relative_position: nil)
described_class.move_nulls_to_end([item1])
- item1.reload
-
- expect(item1.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE)
+ expect(item1.reset.relative_position).to eq(described_class::START_POSITION + described_class::IDEAL_DISTANCE)
end
it 'does not perform any moves if all items have their relative_position set' do
item1.update!(relative_position: 1)
+ expect do
+ described_class.move_nulls_to_end([item1])
+ end.not_to change { item1.reset.relative_position }
+ end
+
+ it 'manages to move nulls to the end even if there is a sequence at the end' do
+ bunch = create_items_with_positions(run_at_end)
+ item1.update!(relative_position: nil)
+
described_class.move_nulls_to_end([item1])
- item1.reload
- expect(item1.relative_position).to be(1)
+ items = [*bunch, item1]
+ items.each(&:reset)
+
+ expect(items.map(&:relative_position)).to all(be_valid_position)
+ expect(items.sort_by(&:relative_position)).to eq(items)
+ end
+
+ it 'does not have an N+1 issue' do
+ create_items_with_positions(10..12)
+
+ a, b, c, d, e, f = create_items_with_positions([nil, nil, nil, nil, nil, nil])
+
+ baseline = ActiveRecord::QueryRecorder.new do
+ described_class.move_nulls_to_end([a, e])
+ end
+
+ expect { described_class.move_nulls_to_end([b, c, d]) }
+ .not_to exceed_query_limit(baseline)
+
+ expect { described_class.move_nulls_to_end([f]) }
+ .not_to exceed_query_limit(baseline.count)
end
end
@@ -78,11 +112,19 @@ def create_items_with_positions(positions)
item1.update!(relative_position: nil)
described_class.move_nulls_to_start([item1])
- item1.reload
expect(item1.relative_position).to eq(described_class::START_POSITION - described_class::IDEAL_DISTANCE)
end
+ it 'preserves relative position' do
+ item1.update!(relative_position: nil)
+ item2.update!(relative_position: nil)
+
+ described_class.move_nulls_to_end([item1, item2])
+
+ expect(item1.relative_position).to be < item2.relative_position
+ end
+
it 'does not perform any moves if all items have their relative_position set' do
item1.update!(relative_position: 1)
@@ -101,8 +143,8 @@ def create_items_with_positions(positions)
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)
+ item1.update!(relative_position: 5)
+ item2.update!(relative_position: 15)
expect(item2.prev_relative_position).to eq item1.relative_position
end
@@ -114,8 +156,8 @@ def create_items_with_positions(positions)
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)
+ item1.update!(relative_position: 5)
+ item2.update!(relative_position: 15)
expect(item1.next_relative_position).to eq item2.relative_position
end
@@ -125,9 +167,172 @@ def create_items_with_positions(positions)
end
end
+ describe '#find_next_gap_before' do
+ context 'there is no gap' do
+ let(:items) { create_items_with_positions(run_at_start) }
+
+ it 'returns nil' do
+ items.each do |item|
+ expect(item.send(:find_next_gap_before)).to be_nil
+ end
+ end
+ end
+
+ context 'there is a sequence ending at MAX_POSITION' do
+ let(:items) { create_items_with_positions(run_at_end) }
+
+ let(:gaps) do
+ items.map { |item| item.send(:find_next_gap_before) }
+ end
+
+ it 'can find the gap at the start for any item in the sequence' do
+ gap = { start: items.first.relative_position, end: RelativePositioning::MIN_POSITION }
+
+ expect(gaps).to all(eq(gap))
+ end
+
+ it 'respects lower bounds' do
+ gap = { start: items.first.relative_position, end: 10 }
+ new_item.update!(relative_position: 10)
+
+ expect(gaps).to all(eq(gap))
+ end
+ end
+
+ specify do
+ item1.update!(relative_position: 5)
+
+ (0..10).each do |pos|
+ item2.update!(relative_position: pos)
+
+ gap = item2.send(:find_next_gap_before)
+
+ expect(gap[:start]).to be <= item2.relative_position
+ expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP
+ expect(gap[:start]).to be_valid_position
+ expect(gap[:end]).to be_valid_position
+ end
+ end
+
+ it 'deals with there not being any items to the left' do
+ create_items_with_positions([1, 2, 3])
+ new_item.update!(relative_position: 0)
+
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 0, end: RelativePositioning::MIN_POSITION)
+ end
+
+ it 'finds the next gap to the left, skipping adjacent values' do
+ create_items_with_positions([1, 9, 10])
+ new_item.update!(relative_position: 11)
+
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 1)
+ end
+
+ it 'finds the next gap to the left' do
+ create_items_with_positions([2, 10])
+
+ new_item.update!(relative_position: 15)
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 15, end: 10)
+
+ new_item.update!(relative_position: 11)
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 10, end: 2)
+
+ new_item.update!(relative_position: 9)
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 9, end: 2)
+
+ new_item.update!(relative_position: 5)
+ expect(new_item.send(:find_next_gap_before)).to eq(start: 5, end: 2)
+ end
+ end
+
+ describe '#find_next_gap_after' do
+ context 'there is no gap' do
+ let(:items) { create_items_with_positions(run_at_end) }
+
+ it 'returns nil' do
+ items.each do |item|
+ expect(item.send(:find_next_gap_after)).to be_nil
+ end
+ end
+ end
+
+ context 'there is a sequence starting at MIN_POSITION' do
+ let(:items) { create_items_with_positions(run_at_start) }
+
+ let(:gaps) do
+ items.map { |item| item.send(:find_next_gap_after) }
+ end
+
+ it 'can find the gap at the end for any item in the sequence' do
+ gap = { start: items.last.relative_position, end: RelativePositioning::MAX_POSITION }
+
+ expect(gaps).to all(eq(gap))
+ end
+
+ it 'respects upper bounds' do
+ gap = { start: items.last.relative_position, end: 10 }
+ new_item.update!(relative_position: 10)
+
+ expect(gaps).to all(eq(gap))
+ end
+ end
+
+ specify do
+ item1.update!(relative_position: 5)
+
+ (0..10).each do |pos|
+ item2.update!(relative_position: pos)
+
+ gap = item2.send(:find_next_gap_after)
+
+ expect(gap[:start]).to be >= item2.relative_position
+ expect((gap[:end] - gap[:start]).abs).to be >= RelativePositioning::MIN_GAP
+ expect(gap[:start]).to be_valid_position
+ expect(gap[:end]).to be_valid_position
+ end
+ end
+
+ it 'deals with there not being any items to the right' do
+ create_items_with_positions([1, 2, 3])
+ new_item.update!(relative_position: 5)
+
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: RelativePositioning::MAX_POSITION)
+ end
+
+ it 'finds the next gap to the right, skipping adjacent values' do
+ create_items_with_positions([1, 2, 10])
+ new_item.update!(relative_position: 0)
+
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10)
+ end
+
+ it 'finds the next gap to the right' do
+ create_items_with_positions([2, 10])
+
+ new_item.update!(relative_position: 0)
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 0, end: 2)
+
+ new_item.update!(relative_position: 1)
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 2, end: 10)
+
+ new_item.update!(relative_position: 3)
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 3, end: 10)
+
+ new_item.update!(relative_position: 5)
+ expect(new_item.send(:find_next_gap_after)).to eq(start: 5, end: 10)
+ end
+ end
+
describe '#move_before' do
+ let(:item3) { create(factory, default_params) }
+
it 'moves item before' do
- [item2, item1].each(&:move_to_end)
+ [item2, item1].each do |item|
+ item.move_to_end
+ item.save!
+ end
+
+ expect(item1.relative_position).to be > item2.relative_position
item1.move_before(item2)
@@ -135,12 +340,10 @@ def create_items_with_positions(positions)
end
context 'when there is no space' do
- let(:item3) { create(factory, default_params) }
-
before do
- item1.update(relative_position: 1000)
- item2.update(relative_position: 1001)
- item3.update(relative_position: 1002)
+ item1.update!(relative_position: 1000)
+ item2.update!(relative_position: 1001)
+ item3.update!(relative_position: 1002)
end
it 'moves items correctly' do
@@ -149,6 +352,73 @@ def create_items_with_positions(positions)
expect(item3.relative_position).to be_between(item1.reload.relative_position, item2.reload.relative_position).exclusive
end
end
+
+ it 'can move the item before an item at the start' do
+ item1.update!(relative_position: RelativePositioning::START_POSITION)
+
+ new_item.move_before(item1)
+
+ expect(new_item.relative_position).to be_valid_position
+ expect(new_item.relative_position).to be < item1.reload.relative_position
+ end
+
+ it 'can move the item before an item at MIN_POSITION' do
+ item1.update!(relative_position: RelativePositioning::MIN_POSITION)
+
+ new_item.move_before(item1)
+
+ expect(new_item.relative_position).to be >= RelativePositioning::MIN_POSITION
+ expect(new_item.relative_position).to be < item1.reload.relative_position
+ end
+
+ it 'can move the item before an item bunched up at MIN_POSITION' do
+ item1, item2, item3 = create_items_with_positions(run_at_start)
+
+ new_item.move_before(item3)
+ new_item.save!
+
+ items = [item1, item2, new_item, item3]
+
+ items.each do |item|
+ expect(item.reset.relative_position).to be_valid_position
+ end
+
+ expect(items.sort_by(&:relative_position)).to eq(items)
+ end
+
+ context 'leap-frogging to the left' do
+ before do
+ start = RelativePositioning::START_POSITION
+ item1.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 0)
+ item2.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 1)
+ item3.update!(relative_position: start - RelativePositioning::IDEAL_DISTANCE * 2)
+ end
+
+ let(:item3) { create(factory, default_params) }
+
+ def leap_frog(steps)
+ a = item1
+ b = item2
+
+ steps.times do |i|
+ a.move_before(b)
+ a.save!
+ a, b = b, a
+ end
+ end
+
+ it 'can leap-frog STEPS - 1 times before needing to rebalance' do
+ # This is less efficient than going right, due to the flooring of
+ # integer division
+ expect { leap_frog(RelativePositioning::STEPS - 1) }
+ .not_to change { item3.reload.relative_position }
+ end
+
+ it 'rebalances after leap-frogging STEPS times' do
+ expect { leap_frog(RelativePositioning::STEPS) }
+ .to change { item3.reload.relative_position }
+ end
+ end
end
describe '#move_after' do
@@ -164,9 +434,17 @@ def create_items_with_positions(positions)
let(:item3) { create(factory, default_params) }
before do
- item1.update(relative_position: 1000)
- item2.update(relative_position: 1001)
- item3.update(relative_position: 1002)
+ item1.update!(relative_position: 1000)
+ item2.update!(relative_position: 1001)
+ item3.update!(relative_position: 1002)
+ end
+
+ it 'can move the item after an item at MAX_POSITION' do
+ item1.update!(relative_position: RelativePositioning::MAX_POSITION)
+
+ new_item.move_after(item1)
+ expect(new_item.relative_position).to be_valid_position
+ expect(new_item.relative_position).to be > item1.reset.relative_position
end
it 'moves items correctly' do
@@ -175,6 +453,53 @@ def create_items_with_positions(positions)
expect(item1.relative_position).to be_between(item2.reload.relative_position, item3.reload.relative_position).exclusive
end
end
+
+ it 'can move the item after an item bunched up at MAX_POSITION' do
+ item1, item2, item3 = create_items_with_positions(run_at_end)
+
+ new_item.move_after(item1)
+ new_item.save!
+
+ items = [item1, new_item, item2, item3]
+
+ items.each do |item|
+ expect(item.reset.relative_position).to be_valid_position
+ end
+
+ expect(items.sort_by(&:relative_position)).to eq(items)
+ end
+
+ context 'leap-frogging' do
+ before do
+ start = RelativePositioning::START_POSITION
+ item1.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 0)
+ item2.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 1)
+ item3.update!(relative_position: start + RelativePositioning::IDEAL_DISTANCE * 2)
+ end
+
+ let(:item3) { create(factory, default_params) }
+
+ def leap_frog(steps)
+ a = item1
+ b = item2
+
+ steps.times do |i|
+ a.move_after(b)
+ a.save!
+ a, b = b, a
+ end
+ end
+
+ it 'can leap-frog STEPS times before needing to rebalance' do
+ expect { leap_frog(RelativePositioning::STEPS) }
+ .not_to change { item3.reload.relative_position }
+ end
+
+ it 'rebalances after leap-frogging STEPS+1 times' do
+ expect { leap_frog(RelativePositioning::STEPS + 1) }
+ .to change { item3.reload.relative_position }
+ end
+ end
end
describe '#move_to_end' do
@@ -193,8 +518,17 @@ def create_items_with_positions(positions)
describe '#move_between' do
before do
- [item1, item2].each do |item1|
- item1.move_to_end && item1.save
+ [item1, item2].each do |item|
+ item.move_to_end && item.save!
+ end
+ end
+
+ shared_examples 'moves item between' do
+ it 'moves the middle item to between left and right' do
+ expect do
+ middle.move_between(left, right)
+ middle.save!
+ end.to change { between_exclusive?(left, middle, right) }.from(false).to(true)
end
end
@@ -218,26 +552,26 @@ def create_items_with_positions(positions)
end
it 'positions items even when after and before positions are the same' do
- item2.update relative_position: item1.relative_position
+ item2.update! relative_position: item1.relative_position
new_item.move_between(item1, item2)
+ [item1, item2].each(&:reset)
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
+ context 'the two items are next to each other' do
+ let(:left) { item1 }
+ let(:middle) { new_item }
+ let(:right) { create_item(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
+ it_behaves_like 'moves item between'
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
+ item1.update! relative_position: 6000
+ item2.update! relative_position: 10000
new_item.move_between(item1, item2)
@@ -245,7 +579,8 @@ def create_items_with_positions(positions)
end
it 'positions item closer to the middle if we are at the very top' do
- item2.update relative_position: 6000
+ item1.update!(relative_position: 6001)
+ item2.update!(relative_position: 6000)
new_item.move_between(nil, item2)
@@ -253,51 +588,53 @@ def create_items_with_positions(positions)
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.update!(relative_position: 1)
+ item1.update!(relative_position: 6000)
+ item2.update!(relative_position: 5999)
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
+ it 'positions item in the middle of other two' 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)
+ context 'there is no space' do
+ let(:middle) { new_item }
+ let(:left) { create_item(relative_position: 100) }
+ let(:right) { create_item(relative_position: 101) }
- expect(new_item.relative_position).to be_between(item1.relative_position, item2.relative_position).exclusive
+ it_behaves_like 'moves item between'
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
+ context 'there is a bunch of items' do
+ let(:items) { create_items_with_positions(100..104) }
+ let(:left) { items[1] }
+ let(:middle) { items[3] }
+ let(:right) { items[2] }
- new_item.move_between(item2, item3)
- new_item.save!
+ it_behaves_like 'moves item between'
+
+ it 'handles bunches correctly' do
+ middle.move_between(left, right)
+ middle.save!
- expect(new_item.relative_position).to be_between(item2.relative_position, item3.relative_position).exclusive
- expect(item1.reload.relative_position).not_to eq(100)
+ expect(items.first.reset.relative_position).to be < middle.relative_position
+ end
end
- it 'positions item right if we pass none-sequential parameters' do
- item1.update relative_position: 99
- item2.update relative_position: 101
+ it 'positions item right if we pass non-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.update! relative_position: 103
new_item.move_between(item1, item3)
new_item.save!
@@ -329,6 +666,12 @@ def create_items_with_positions(positions)
expect(positions).to eq([90, 95, 96, 102])
end
+ it 'raises an error if there is no space' do
+ items = create_items_with_positions(run_at_start)
+
+ expect { items.last.move_sequence_before }.to raise_error(RelativePositioning::NoSpaceLeft)
+ end
+
it 'finds a gap if there are unused positions' do
items = create_items_with_positions([100, 101, 102])
@@ -336,7 +679,8 @@ def create_items_with_positions(positions)
items.last.save!
positions = items.map { |item| item.reload.relative_position }
- expect(positions).to eq([50, 51, 102])
+
+ expect(positions.last - positions.second).to be > RelativePositioning::MIN_GAP
end
end
@@ -358,7 +702,33 @@ def create_items_with_positions(positions)
items.first.save!
positions = items.map { |item| item.reload.relative_position }
- expect(positions).to eq([100, 601, 602])
+ expect(positions.second - positions.first).to be > RelativePositioning::MIN_GAP
end
+
+ it 'raises an error if there is no space' do
+ items = create_items_with_positions(run_at_end)
+
+ expect { items.first.move_sequence_after }.to raise_error(RelativePositioning::NoSpaceLeft)
+ end
+ end
+
+ def be_valid_position
+ be_between(RelativePositioning::MIN_POSITION, RelativePositioning::MAX_POSITION)
+ end
+
+ def between_exclusive?(left, middle, right)
+ a, b, c = [left, middle, right].map { |item| item.reset.relative_position }
+ return false if a.nil? || b.nil?
+ return a < b if c.nil?
+
+ a < b && b < c
+ end
+
+ def run_at_end(size = 3)
+ (RelativePositioning::MAX_POSITION - size)..RelativePositioning::MAX_POSITION
+ end
+
+ def run_at_start(size = 3)
+ (RelativePositioning::MIN_POSITION..).take(size)
end
end