Address review comments

3 files changed, 31 insertions, 28 deletions

diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb
index 75d814b045c..e4c3426b0ca 100644
--- a/app/models/concerns/relative_positioning.rb
+++ b/app/models/concerns/relative_positioning.rb
@@ -43,7 +43,6 @@ module RelativePositioning
   end
 
   def move_between(before, after)
-    return move_nowhere unless before || after
     return move_after(before) if before && !after
     return move_before(after) if after && !before
 
@@ -52,11 +51,10 @@ module RelativePositioning
 
     if pos_before && pos_after
       if pos_before == pos_after
-        pos = pos_before
-
-        self.relative_position = pos
+        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)
@@ -64,47 +62,50 @@ module RelativePositioning
     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)
     else
       move_to_end
       after.move_after(self)
+
       @positionable_neighbours = [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
   end
 
-  def move_nowhere
-    self.relative_position = nil
-  end
-
   def move_to_end
-    self.relative_position = position_between(max_relative_position || MIN_POSITION, MAX_POSITION)
+    self.relative_position = position_between(max_relative_position, MAX_POSITION)
   end
 
   def move_between!(*args)
@@ -114,6 +115,9 @@ module RelativePositioning
   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
 
     rand(pos_before.next..pos_after.pred)
@@ -122,9 +126,9 @@ module RelativePositioning
   def save_positionable_neighbours
     return unless @positionable_neighbours
 
-    @positionable_neighbours.each(&:save)
+    status = @positionable_neighbours.all?(&:save)
     @positionable_neighbours = nil
 
-    true
+    status
   end
 end
diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb
index 7c0df55e9b6..a946a42a116 100644
--- a/app/services/boards/issues/move_service.rb
+++ b/app/services/boards/issues/move_service.rb
@@ -71,6 +71,7 @@ module Boards
         move_after_iid = params[:move_after_iid]
         move_before_iid = params[:move_before_iid]
         return unless move_after_iid || move_before_iid
+
         [move_after_iid, move_before_iid]
       end
     end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index b043fc2a5b6..2158c8ada08 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -37,11 +37,13 @@ module Issues
       end
 
       added_labels = issue.labels - old_labels
+
       if added_labels.present?
         notification_service.relabeled_issue(issue, added_labels, current_user)
       end
 
       added_mentions = issue.mentioned_users - old_mentioned_users
+
       if added_mentions.present?
         notification_service.new_mentions_in_issue(issue, added_mentions, current_user)
       end
@@ -56,27 +58,23 @@ module Issues
     end
 
     def handle_move_between_iids(issue)
-      if move_between_iids = params.delete(:move_between_iids)
-        after_iid, before_iid = move_between_iids
-
-        issue_before = nil
-        if before_iid
-          issue_before = issue.project.issues.find_by(iid: before_iid)
-          issue_before = nil unless can?(current_user, :update_issue, issue_before)
-        end
-
-        issue_after = nil
-        if after_iid
-          issue_after = issue.project.issues.find_by(iid: after_iid)
-          issue_after = nil unless can?(current_user, :update_issue, issue_after)
-        end
-
-        issue.move_between(issue_before, issue_after)
-      end
+      return unless move_between_iids = params.delete(:move_between_iids)
+
+      after_iid, before_iid = move_between_iids
+
+      issue_before = get_issue_if_allowed(issue.project, before_iid) if before_iid
+      issue_after = get_issue_if_allowed(issue.project, after_iid) if after_iid
+
+      issue.move_between(issue_before, issue_after)
     end
 
     private
 
+    def get_issue_if_allowed(project, iid)
+      issue = project.issues.find_by(iid: iid)
+      issue if can?(current_user, :update_issue, issue)
+    end
+
     def create_confidentiality_note(issue)
       SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)
     end