Fix relative position calculation

2 files changed, 39 insertions, 8 deletions

diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index 603f2dd7e5d..ec23c8a08fb 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -3,6 +3,7 @@ module RelativePositioning
 
   MIN_POSITION = 0
   MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
+  DISTANCE = 500
 
   included do
     after_save :save_positionable_neighbours
@@ -49,7 +50,9 @@ module RelativePositioning
     pos_before = before.relative_position
     pos_after = after.relative_position
 
-    if pos_after && (pos_before == pos_after)
+    # We can't insert an issue between two other if the distance is 1 or 0
+    # so we need to handle this collision properly
+    if pos_after && (pos_after - pos_before).abs <= 1
       self.relative_position = pos_before
       before.move_before(self)
       after.move_after(self)
@@ -75,19 +78,20 @@ module RelativePositioning
   private
 
   # This method takes two integer values (positions) and
-  # calculates some random position between them. The range is huge as
-  # the maximum integer value is 2147483647. Ideally, the calculated value would be
-  # exactly between those terminating values, but this will introduce possibility of a race condition
-  # so two or more issues can get the same value, we want to avoid that and we also want to avoid
-  # using a lock here. If we have two issues with distance more than one thousand, we are OK.
-  # Given the huge range of possible values that integer can fit we shoud never face a problem.
+  # calculates the position between them. The range is huge as
+  # the maximum integer value is 2147483647. We are incrementing position by 1000 every time
+  # when we have enough space. If distance is less then 500 we are calculating an average number
   def position_between(pos_before, pos_after)
     pos_before ||= MIN_POSITION
     pos_after ||= MAX_POSITION
 
     pos_before, pos_after = [pos_before, pos_after].sort
 
-    rand(pos_before.next..pos_after.pred)
+    if pos_after - pos_before > DISTANCE * 2
+      pos_before + DISTANCE
+    else
+      pos_before + (pos_after - pos_before) / 2
+    end
   end
 
   def save_positionable_neighbours
diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb
index 69906382545..12f44bfe0a5 100644
--- a/spec/models/concerns/relative_positioning_spec.rb
+++ b/spec/models/concerns/relative_positioning_spec.rb
@@ -100,5 +100,32 @@ describe Issue, 'RelativePositioning' do
       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 closer to before-issue if distance is big enough' do
+      issue.update relative_position: 100
+      issue1.update relative_position: 6000
+
+      new_issue.move_between(issue, issue1)
+
+      expect(new_issue.relative_position).to eq(100 + RelativePositioning::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
   end
 end