Fix handling of duplicated step positions [SCI-4880]

app/assets/javascripts/protocols/steps.js.erb

@@ -166,27 +166,14 @@
       if ($.isEmptyObject(data)) return;
 
       let $step = $(this).closest('.step');
-      let stepUpPosition = data.step_up_position;
-      let stepDownPosition = data.step_down_position;
-      let $stepDown, $stepUp;
+      let steps = $('#steps').find('.step');
+      $('#steps').append($.map(data.steps_order, function(step_data) {
+        let step = $('#steps').find(`.step[data-id=${step_data.id}]`);
+        step.find('.step-number').html(`${step_data.position + 1}.`);
+        return step;
+      }));
 
-      switch ($(this).data('direction')) {
-        case 'up':
-          $stepDown = $step.prev('.step');
-          $stepUp = $step;
-          break;
-        case 'down':
-          $stepDown = $step;
-          $stepUp = $step.next('.step');
-      }
-
-      // Switch position of top and bottom steps
-      if (!_.isUndefined($stepDown) && !_.isUndefined($stepUp)) {
-        $stepDown.insertAfter($stepUp);
-        $stepDown.find('.step-number').html(`${stepDownPosition + 1}.`);
-        $stepUp.find('.step-number').html(`${stepUpPosition + 1}.`);
-        $('html, body').animate({ scrollTop: $step.offset().top - window.innerHeight / 2 });
-      }
+      $('html, body').animate({ scrollTop: $step.offset().top - window.innerHeight / 2 });
 
       if (typeof refreshProtocolStatusBar === 'function') refreshProtocolStatusBar();
     });

app/controllers/steps_controller.rb

@@ -354,16 +354,11 @@ class StepsController < ApplicationController
   def move_up
     respond_to do |format|
       format.json do
-        if @step.protocol.steps.minimum(:position) != @step.position
-          @step.update!(position: @step.position - 1)
+        @step.move_up
 
-          render json: {
-            step_up_position: @step.position,
-            step_down_position: @step.position + 1
-          }
-        else
-          render json: {}
-        end
+        render json: {
+          steps_order: @protocol.steps.order(:position).select(:id, :position)
+        }
       end
     end
   end
@@ -371,16 +366,11 @@ class StepsController < ApplicationController
   def move_down
     respond_to do |format|
       format.json do
-        if @step.protocol.steps.maximum(:position) != @step.position
-          @step.update!(position: @step.position + 1)
+        @step.move_down
 
-          render json: {
-            step_up_position: @step.position - 1,
-            step_down_position: @step.position
-          }
-        else
-          render json: {}
-        end
+        render json: {
+          steps_order: @protocol.steps.order(:position).select(:id, :position)
+        }
       end
     end
   end

app/models/step.rb

@@ -13,10 +13,10 @@ class Step < ApplicationRecord
   validates :completed, inclusion: { in: [true, false] }
   validates :user, :protocol, presence: true
   validates :completed_on, presence: true, if: proc { |s| s.completed? }
+  validates :position, uniqueness: { scope: :protocol }, if: :position_changed?
 
   before_validation :set_completed_on, if: :completed_changed?
   before_save :set_last_modified_by
-  around_save :adjust_positions_on_save, if: :position_changed?
   before_destroy :cascade_before_destroy
   after_destroy :adjust_positions_after_destroy
 
@@ -124,25 +124,75 @@ class Step < ApplicationRecord
     end
   end
 
+  def move_up
+    return if position.zero?
+
+    move_in_protocol(:up)
+  end
+
+  def move_down
+    return if position == protocol.steps.count - 1
+
+    move_in_protocol(:down)
+  end
+
   private
 
-  def adjust_positions_on_save
-    step_to_swap = protocol.steps.find_by(position: position)
+  def move_in_protocol(direction)
+    transaction do
+      adjust_duplicated_step_positions
 
-    return yield unless step_to_swap
+      case direction
+      when :up
+        new_position = position - 1
+      when :down
+        new_position = position + 1
+      else
+        return
+      end
 
-    position_to_swap = position_was
-    step_to_swap.position = -1
-    yield
-    step_to_swap.update!(position: position_to_swap)
+      step_to_swap = protocol.steps.find_by(position: new_position)
+      position_to_swap = position
+
+      if step_to_swap
+        step_to_swap.update!(position: -1)
+        update!(position: new_position)
+        step_to_swap.update!(position: position_to_swap)
+      else
+        update!(position: new_position)
+      end
+    end
   end
 
   def adjust_positions_after_destroy
+    adjust_duplicated_step_positions
     protocol.steps.where('position > ?', position).find_each do |step|
       step.update!(position: step.position - 1)
     end
   end
 
+  def adjust_duplicated_step_positions
+    duplicated_steps = protocol.steps.where(position: position)
+    return unless duplicated_steps.size > 1
+
+    # Shifting following steps
+    next_steps = protocol.steps.where(position: (position + 1)..).order(:position)
+    if next_steps.present?
+      shift_by = duplicated_steps.size - next_steps.first.position + position
+      if shift_by.positive?
+        next_steps.reverse_each do |step|
+          step.update!(position: step.position + shift_by)
+        end
+      end
+    end
+
+    # Re-index duplicated positions
+    duplicated_steps.each_with_index do |step, i|
+      step.update!(position: step.position + i)
+    end
+    reload
+  end
+
   def cascade_before_destroy
     assets.each(&:destroy)
     tables.each(&:destroy)

app/views/steps/_step.html.erb

@@ -1,6 +1,6 @@
 <% preview = (defined?(preview) ? preview : false) %>
 <% import = (defined?(import) ? import : false) %>
-<div class ="step <%= step.completed? ? "completed" : "not-completed" %>">
+<div class ="step <%= step.completed? ? "completed" : "not-completed" %>" data-id="<%= step.id %>">
   <div class="panel panel-default">
     <div class="panel-heading step-heading">
       <div class="left-floats">