Merge pull request #2761 from okriuchykhin/ok_SCI_4880

Fix step position adjusting on move and destroy [SCI-4880]

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

@@ -162,29 +162,30 @@
 
   // Set callback for click on move step
   function applyMoveStepCallBack() {
-    $("[data-action='move-step']").off("ajax:success");
-    $("[data-action='move-step']")
-    .on("ajax:success", function(e, data) {
-      var $step = $(this).closest(".step");
-      var stepUpPosition = data.step_up_position;
-      var stepDownPosition = data.step_down_position;
-      var $stepDown, $stepUp;
-      switch (data.move_direction) {
-        case "up":
-          $stepDown = $step.prev(".step");
+    $('#steps').on('ajax:success', "[data-action='move-step']", function(e, data) {
+      if ($.isEmptyObject(data)) return;
+
+      let $step = $(this).closest('.step');
+      let stepUpPosition = data.step_up_position;
+      let stepDownPosition = data.step_down_position;
+      let $stepDown, $stepUp;
+
+      switch ($(this).data('direction')) {
+        case 'up':
+          $stepDown = $step.prev('.step');
           $stepUp = $step;
           break;
-        case "down":
+        case 'down':
           $stepDown = $step;
-          $stepUp = $step.next(".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 });
+        $stepDown.find('.step-number').html(`${stepDownPosition + 1}.`);
+        $stepUp.find('.step-number').html(`${stepUpPosition + 1}.`);
+        $('html, body').animate({ scrollTop: $step.offset().top - window.innerHeight / 2 });
       }
 
       if (typeof refreshProtocolStatusBar === 'function') refreshProtocolStatusBar();

app/controllers/steps_controller.rb

@@ -80,9 +80,6 @@ class StepsController < ApplicationController
       else
         log_activity(:add_step_to_protocol_repository, nil, protocol: @protocol.id)
       end
-
-      # Update protocol timestamp
-      update_protocol_ts(@step)
     end
 
     respond_to do |format|
@@ -194,9 +191,6 @@ class StepsController < ApplicationController
           log_activity(:edit_step_in_protocol_repository, nil, protocol: @protocol.id)
         end
 
-        # Update protocol timestamp
-        update_protocol_ts(@step)
-
         format.json {
           render json: {
             html: render_to_string({
@@ -226,11 +220,6 @@ class StepsController < ApplicationController
 
   def destroy
     if @step.can_destroy?
-      # Update position on other steps of this module
-      @protocol.steps.where('position > ?', @step.position).each do |step|
-        step.position = step.position - 1
-        step.save
-      end
 
       # Calculate space taken by this step
       team = @protocol.team
@@ -250,9 +239,6 @@ class StepsController < ApplicationController
       team.release_space(previous_size)
       team.save
 
-      # Update protocol timestamp
-      update_protocol_ts(@step)
-
       flash[:success] = t(
         'protocols.steps.destroy.success_flash',
         step: (@step.position_plus_one).to_s
@@ -367,57 +353,34 @@ class StepsController < ApplicationController
 
   def move_up
     respond_to do |format|
-      if @step.position.positive?
-        step_down = @step.protocol.steps.find_by(position: @step.position - 1)
-        @step.position -= 1
-        @step.save
+      format.json do
+        if @step.protocol.steps.minimum(:position) != @step.position
+          @step.update!(position: @step.position - 1)
 
-        if step_down
-          step_down.position += 1
-          step_down.save
-
-          # Update protocol timestamp
-          update_protocol_ts(@step)
-
-          format.json do
-            render json: { move_direction: 'up',
-                           step_up_position: @step.position,
-                           step_down_position: step_down.position },
-            status: :ok
-          end
+          render json: {
+            step_up_position: @step.position,
+            step_down_position: @step.position + 1
+          }
         else
-          format.json { render json: {}, status: :forbidden }
+          render json: {}
         end
-      else
-        format.json { render json: {}, status: :forbidden }
       end
     end
   end
 
   def move_down
     respond_to do |format|
-      if @step.position < @step.protocol.steps.count - 1
-        step_up = @step.protocol.steps.find_by(position: @step.position + 1)
-        @step.position += 1
-        @step.save
+      format.json do
+        if @step.protocol.steps.maximum(:position) != @step.position
+          @step.update!(position: @step.position + 1)
 
-        if step_up
-          step_up.position -= 1
-          step_up.save
-
-          # Update protocol timestamp
-          update_protocol_ts(@step)
-
-          format.json do
-            render json: { move_direction: 'down',
-                           step_up_position: step_up.position,
-                           step_down_position: @step.position }
-          end
+          render json: {
+            step_up_position: @step.position - 1,
+            step_down_position: @step.position
+          }
         else
-          format.json { render json: {}, status: :forbidden }
+          render json: {}
         end
-      else
-        format.json { render json: {}, status: :forbidden }
       end
     end
   end
@@ -517,12 +480,6 @@ class StepsController < ApplicationController
     end
   end
 
-  def update_protocol_ts(step)
-    if step.present? && step.protocol.present?
-      step.protocol.update(updated_at: Time.now)
-    end
-  end
-
   def update_checklist_items_without_callback(params)
     params.dig('checklists_attributes')&.values&.each do |cl|
       ck = @step.checklists.find_by(id: cl[:id])

app/models/step.rb

@@ -16,12 +16,13 @@ class Step < ApplicationRecord
 
   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
-  before_destroy :adjust_positions_on_destroy
+  after_destroy :adjust_positions_after_destroy
 
   belongs_to :user, inverse_of: :steps
   belongs_to :last_modified_by, foreign_key: 'last_modified_by_id', class_name: 'User', optional: true
-  belongs_to :protocol, inverse_of: :steps
+  belongs_to :protocol, inverse_of: :steps, touch: true
   has_many :checklists, inverse_of: :step, dependent: :destroy
   has_many :step_comments, foreign_key: :associated_id, dependent: :destroy
   has_many :step_assets, inverse_of: :step, dependent: :destroy
@@ -125,9 +126,20 @@ class Step < ApplicationRecord
 
   private
 
-  def adjust_positions_on_destroy
+  def adjust_positions_on_save
+    step_to_swap = protocol.steps.find_by(position: position)
+
+    return yield unless step_to_swap
+
+    position_to_swap = position_was
+    step_to_swap.position = -1
+    yield
+    step_to_swap.update!(position: position_to_swap)
+  end
+
+  def adjust_positions_after_destroy
     protocol.steps.where('position > ?', position).find_each do |step|
-      step.update(position: step.position - 1)
+      step.update!(position: step.position - 1)
     end
   end
 

app/views/steps/_step.html.erb

@@ -49,29 +49,28 @@
           <% end %>
         <% end %>
 
-        <% if (can_manage_protocol_in_module?(@protocol) ||
-                      can_manage_protocol_in_repository?(@protocol)) && !(preview) %>
-          <a data-action="move-step"
-             class="btn btn-light icon-btn"
-             href="<%= move_up_step_path(step, format: :json) %>"
-             title="<%= t("protocols.steps.options.up_arrow_title") %>"
-             data-remote="true">
-              <span class="fas fa-arrow-up"></span></a>
-          <a data-action="move-step"
-             class="btn btn-light icon-btn"
-             href="<%= move_down_step_path(step, format: :json) %>"
-             title="<%= t("protocols.steps.options.down_arrow_title") %>"
-             data-remote="true">
-            <span class="fas fa-arrow-down"></span></a>
-          <a data-action="edit-step"
-             class="btn btn-light icon-btn"
-             title="<%= t("protocols.steps.options.edit_title") %>"
-             href="<%= edit_step_path(step, format: :json) %>"
-             data-remote="true" >
-            <span class="fas fa-pencil-alt"></span>
-          </a>
-          <%= link_to(step_path(step), title: t("protocols.steps.options.delete_title"), method: "delete", class: "btn btn-light icon-btn",
-            data: {action: "delete-step", confirm: t("protocols.steps.destroy.confirm", step: step.name)}) do %>
+        <% if (can_manage_protocol_in_module?(@protocol) || can_manage_protocol_in_repository?(@protocol)) && !(preview) %>
+          <%= link_to(move_up_step_path(step),
+                      class: 'btn btn-light icon-btn',
+                      title: t('protocols.steps.options.up_arrow_title'),
+                      remote: true,
+                      method: :put,
+                      data: { action: 'move-step', direction: :up }) do %>
+            <span class="fas fa-arrow-up"></span>
+          <% end %>
+          <%= link_to(move_down_step_path(step),
+                      class: 'btn btn-light icon-btn',
+                      title: t('protocols.steps.options.down_arrow_title'),
+                      remote: true,
+                      method: :put,
+                      data: { action: 'move-step', direction: :down }) do %>
+            <span class="fas fa-arrow-down"></span>
+          <% end %>
+          <%= link_to(step_path(step),
+                      title: t('protocols.steps.options.delete_title'),
+                      method: :delete,
+                      class: 'btn btn-light icon-btn',
+                      data: { action: 'delete-step', confirm: t('protocols.steps.destroy.confirm', step: step.name) }) do %>
             <span class="fas fa-trash-alt"></span>
           <% end %>
         <% end %>

config/routes.rb

@@ -448,8 +448,8 @@ Rails.application.routes.draw do
       member do
         post 'checklistitem_state'
         post 'toggle_step_state'
-        get 'move_down'
-        get 'move_up'
+        put 'move_down'
+        put 'move_up'
         post 'update_view_state'
       end
     end

spec/factories/steps.rb

@@ -3,7 +3,7 @@
 FactoryBot.define do
   factory :step do
     name { Faker::Name.unique.name }
-    position { Faker::Number.between(from: 1, to: 10) }
+    position { protocol ? protocol.steps.count : Faker::Number.between(from: 1, to: 10) }
     completed { true }
     user
     protocol