From 13caadea7a123d1dc5f3475d360cd07f1aef4acb Mon Sep 17 00:00:00 2001
From: Valery Sizov <valery@gitlab.com>
Date: Mon, 6 Mar 2017 18:08:18 +0200
Subject: [PATCH] Addressing review comments

---
 app/models/concerns/relative_positioning.rb   | 99 ++-----------------
 .../concerns/relative_positioning_spec.rb     | 89 ++++-------------
 .../boards/issues/move_service_spec.rb        |  4 +-
 3 files changed, 32 insertions(+), 160 deletions(-)

diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index 4cb7048acda..2fb711dabc3 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -4,10 +4,6 @@ module RelativePositioning
   MIN_POSITION = 0
   MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
 
-  included do
-    after_save :save_positionable_neighbours
-  end
-
   def min_relative_position
     self.class.in_projects(project.id).minimum(:relative_position)
   end
@@ -16,92 +12,24 @@ module RelativePositioning
     self.class.in_projects(project.id).maximum(:relative_position)
   end
 
-  def prev_relative_position
-    prev_pos = nil
-
-    if self.relative_position
-      prev_pos = self.class.
-        in_projects(project.id).
-        where('relative_position < ?', self.relative_position).
-        maximum(:relative_position)
-    end
-
-    prev_pos || MIN_POSITION
-  end
-
-  def next_relative_position
-    next_pos = nil
-
-    if self.relative_position
-      next_pos = self.class.
-        in_projects(project.id).
-        where('relative_position > ?', self.relative_position).
-        minimum(:relative_position)
-    end
-
-    next_pos || MAX_POSITION
-  end
-
   def move_between(before, after)
-    return move_after(before) if before && !after
-    return move_before(after) if after && !before
+    return move_to_end unless after
+    return move_to_top unless before
 
     pos_before = before.relative_position
     pos_after = after.relative_position
 
-    if pos_before && pos_after
-      if pos_before == pos_after
-        self.relative_position = pos_before
-        before.move_before(self)
-        after.move_after(self)
-
-        @positionable_neighbours = [before, after]
-      else
-        self.relative_position = position_between(pos_before, pos_after)
-      end
-    elsif pos_before
-      self.move_after(before)
-      after.move_after(self)
-
-      @positionable_neighbours = [after]
-    elsif pos_after
-      self.move_before(after)
-      before.move_before(self)
-
-      @positionable_neighbours = [before]
-    else
-      move_to_end
-      before.move_before(self)
-      after.move_after(self)
-
-      @positionable_neighbours = [before, after]
-    end
-  end
-
-  def move_before(after)
-    pos_after = after.relative_position
-
-    if pos_after
-      self.relative_position = position_between(MIN_POSITION, pos_after)
+    if pos_after && (pos_before == pos_after)
+      self.relative_position = pos_before
+      before.decrement! :relative_position
+      after.increment! :relative_position
     else
-      move_to_end
-      after.move_after(self)
-
-      @positionable_neighbours = [after]
+      self.relative_position = position_between(pos_before, pos_after)
     end
   end
 
-  def move_after(before)
-    pos_before = before.relative_position
-
-    if pos_before
-      self.relative_position = position_between(pos_before, MAX_POSITION)
-    else
-      move_to_end
-      before.move_before(self)
-
-      @positionable_neighbours = [before]
-    end
+  def move_to_top
+    self.relative_position = position_between(MIN_POSITION, min_relative_position)
   end
 
   def move_to_end
@@ -129,13 +57,4 @@ module RelativePositioning
 
     rand(pos_before.next..pos_after.pred)
   end
-
-  def save_positionable_neighbours
-    return unless @positionable_neighbours
-
-    status = @positionable_neighbours.all?(&:save)
-    @positionable_neighbours = nil
-
-    status
-  end
 end
diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb
index 989f46f1442..69f295f725b 100644
--- a/spec/models/concerns/relative_positioning_spec.rb
+++ b/spec/models/concerns/relative_positioning_spec.rb
@@ -12,68 +12,27 @@ describe Issue, 'RelativePositioning' do
   end
 
   describe '#min_relative_position' do
-    it 'returns minimum position' do
-      expect(issue1.min_relative_position).to eq issue.relative_position
+    it 'returns maximum position' do
+      expect(issue.min_relative_position).to eq issue.relative_position
     end
   end
 
-  describe '#man_relative_position' do
+  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 minimum position if there is no issue above' do
-      expect(issue.prev_relative_position).to eq RelativePositioning::MIN_POSITION
-    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 next position if there is no issue below' do
-      expect(issue1.next_relative_position).to eq RelativePositioning::MAX_POSITION
-    end
-  end
-
-  describe '#move_before' do
-    it 'moves issue before' do
-      issue1.move_before(issue)
-
-      expect(issue1.relative_position).to be < issue.relative_position
-    end
-
-    it 'moves unpositioned issue before' do
-      issue.update_attribute(:relative_position, nil)
+  describe '#move_to_top' do
+    it 'moves issue to the end' do
+      new_issue = create :issue, project: project
 
-      issue1.move_before(issue)
+      new_issue.move_to_top
 
-      expect(issue1.relative_position).to be < issue.relative_position
+      expect(new_issue.relative_position).to be < issue.relative_position
     end
   end
 
-  describe '#move_after' do
-    it 'moves issue after' do
-      issue.move_before(issue1)
-
-      expect(issue.relative_position).to be < issue1.relative_position
-    end
-
-    it 'moves unpositioned issue after' do
-      issue1.update_attribute(:relative_position, nil)
-
-      issue.move_before(issue1)
-
-      expect(issue.relative_position).to be < issue1.relative_position
-    end
-  end
 
   describe '#move_to_end' do
     it 'moves issue to the end' do
@@ -95,38 +54,30 @@ describe Issue, 'RelativePositioning' do
       expect(new_issue.relative_position).to be < issue1.relative_position
     end
 
-    it 'positions issue between two other if position of last one is nil' do
+    it 'positions issue between on top' do
       new_issue = create :issue, project: project
-      issue1.relative_position = nil
-      issue1.save
 
-      new_issue.move_between(issue, issue1)
+      new_issue.move_between(nil, issue)
 
-      expect(new_issue.relative_position).to be > issue.relative_position
-      expect(new_issue.relative_position).to be < issue1.relative_position
+      expect(new_issue.relative_position).to be < issue.relative_position
     end
 
-    it 'positions issue between two other if position of first one is nil' do
+    it 'positions issue between to end' do
       new_issue = create :issue, project: project
-      issue.relative_position = nil
-      issue.save
 
-      new_issue.move_between(issue, issue1)
+      new_issue.move_between(issue1, nil)
 
-      expect(new_issue.relative_position).to be > issue.relative_position
-      expect(new_issue.relative_position).to be < issue1.relative_position
+      expect(new_issue.relative_position).to be > issue1.relative_position
     end
 
-    it 'calls move_after if after is nil' do
-      expect(issue).to receive(:move_after)
-
-      issue.move_between(issue1, nil)
-    end
+    it 'positions issues even when after and before positions are the same' do
+      new_issue = create :issue, project: project
+      issue1.update relative_position: issue.relative_position
 
-    it 'calls move_before if before is nil' do
-      expect(issue).to receive(:move_before)
+      new_issue.move_between(issue, issue1)
 
-      issue.move_between(nil, issue1)
+      expect(new_issue.relative_position).to be > issue.relative_position
+      expect(issue.relative_position).to be < issue1.relative_position
     end
   end
 end
diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb
index e0bbdba531b..afa86f68344 100644
--- a/spec/services/boards/issues/move_service_spec.rb
+++ b/spec/services/boards/issues/move_service_spec.rb
@@ -94,13 +94,15 @@ describe Boards::Issues::MoveService, services: true do
       end
 
       it 'sorts issues' do
+        [issue1, issue2].each(&:move_to_end)
+
         issue.move_between!(issue1, issue2)
 
         params.merge! move_after_iid: issue.iid, move_before_iid: issue2.iid
 
         described_class.new(project, user, params).execute(issue1)
 
-        expect(issue1.relative_position).to be_between(issue.relative_position, issue2.relative_position)
+        expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
       end
     end
   end
-- 
GitLab