[Issue Board Sorting] More accurate move through the listorderable-issues

3 files changed, 95 insertions, 22 deletions

diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index 2fb711dabc3..9cb3d6dabe2 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -4,6 +4,10 @@ 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
@@ -12,32 +16,61 @@ 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_to_end unless after
-    return move_to_top unless before
+    return move_after(before) unless after
+    return move_before(after) unless before
 
     pos_before = before.relative_position
     pos_after = after.relative_position
 
     if pos_after && (pos_before == pos_after)
       self.relative_position = pos_before
-      before.decrement! :relative_position
-      after.increment! :relative_position
+      before.move_before(self)
+      after.move_after(self)
+
+      @positionable_neighbours = [before, after]
     else
       self.relative_position = position_between(pos_before, pos_after)
     end
   end
 
-  def move_to_top
-    self.relative_position = position_between(MIN_POSITION, min_relative_position)
+  def move_before(after)
+    self.relative_position = position_between(after.prev_relative_position, after.relative_position)
   end
 
-  def move_to_end
-    self.relative_position = position_between(max_relative_position, MAX_POSITION)
+  def move_after(before)
+    self.relative_position = position_between(before.relative_position, before.next_relative_position)
   end
 
-  def move_between!(*args)
-    move_between(*args) && save!
+  def move_to_end
+    self.relative_position = position_between(max_relative_position, MAX_POSITION)
   end
 
   private
@@ -57,4 +90,13 @@ 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 0c777c04669..901bde8e8dd 100644
--- a/spec/models/concerns/relative_positioning_spec.rb
+++ b/spec/models/concerns/relative_positioning_spec.rb
@@ -24,13 +24,44 @@ describe Issue, 'RelativePositioning' do
     end
   end
 
-  describe '#move_to_top' do
-    it 'moves issue to the end' do
-      new_issue = create :issue, project: project
+  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
 
-      new_issue.move_to_top
+    it 'returns minimum position if there is no issue above' do
+      expect(issue.prev_relative_position).to eq RelativePositioning::MIN_POSITION
+    end
+  end
 
-      expect(new_issue.relative_position).to be < issue.relative_position
+  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, 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
 
diff --git a/spec/services/boards/issues/move_service_spec.rb b/spec/services/boards/issues/move_service_spec.rb
index 6c9709c808e..727ea04ea5c 100644
--- a/spec/services/boards/issues/move_service_spec.rb
+++ b/spec/services/boards/issues/move_service_spec.rb
@@ -78,10 +78,10 @@ describe Boards::Issues::MoveService, services: true do
     end
 
     context 'when moving to same list' do
-      let(:issue)  { create(:labeled_issue, project: project, labels: [bug, development]) }
+      let(:issue)   { create(:labeled_issue, project: project, labels: [bug, development]) }
       let(:issue1)  { create(:labeled_issue, project: project, labels: [bug, development]) }
       let(:issue2)  { create(:labeled_issue, project: project, labels: [bug, development]) }
-      let(:params) { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
+      let(:params)  { { board_id: board1.id, from_list_id: list1.id, to_list_id: list1.id } }
 
       it 'returns false' do
         expect(described_class.new(project, user, params).execute(issue)).to eq false
@@ -94,13 +94,13 @@ describe Boards::Issues::MoveService, services: true do
       end
 
       it 'sorts issues' do
-        [issue1, issue2].each(&:move_to_end)
-
-        issue.move_between!(issue1, issue2)
+        [issue, issue1, issue2].each do |issue|
+          issue.move_to_end && issue.save!
+        end
 
-        params.merge!(move_after_iid: issue.iid, move_before_iid: issue2.iid)
+        params.merge!(move_after_iid: issue1.iid, move_before_iid: issue2.iid)
 
-        described_class.new(project, user, params).execute(issue1)
+        described_class.new(project, user, params).execute(issue)
 
         expect(issue.relative_position).to be_between(issue1.relative_position, issue2.relative_position)
       end