Merge branch 'develop' into features/pwa

VERSION

@@ -1 +1 @@
-1.19.6
+1.19.7

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/protocol.rb

@@ -659,7 +659,7 @@ class Protocol < ApplicationRecord
   def destroy_contents
     # Calculate total space taken by the protocol
     st = space_taken
-    steps.destroy_all
+    steps.order(position: :desc).destroy_all
 
     # Release space taken by the step
     team.release_space(st)

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,66 @@ 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
+      re_index_following_steps
 
-    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
-    protocol.steps.where('position > ?', position).find_each do |step|
+    re_index_following_steps
+    protocol.steps.where('position > ?', position).order(:position).each do |step|
       step.update!(position: step.position - 1)
     end
   end
 
+  def re_index_following_steps
+    steps = protocol.steps.where(position: position..).order(:position).where.not(id: id)
+    i = position
+    steps.each do |step|
+      i += 1
+      step.position = i
+    end
+
+    steps.reverse_each do |step|
+      step.save! if step.position_changed?
+    end
+  end
+
   def cascade_before_destroy
     assets.each(&:destroy)
     tables.each(&:destroy)

app/views/canvas/edit/_my_module.html.erb

@@ -36,7 +36,7 @@
             <a class="move-module" href="" data-module-id="<%= my_module.id %>"><%= t('experiments.canvas.edit.move_module') %></a>
           </li>
         <% end %>
-        <% if module_group.my_modules.all? { |my_module| can_move_module?(my_module) } %>
+        <% if module_group&.my_modules&.all? { |my_module| can_move_module?(my_module) } %>
           <li>
             <a class="move-module-group" href="" data-module-id="<%= my_module.id %>"><%= t('experiments.canvas.edit.move_module_group') %></a>
           </li>
@@ -46,7 +46,7 @@
             <a class="delete-module" href="" data-module-id="<%= my_module.id %>"><%= t('experiments.canvas.edit.delete_module') %></a>
           </li>
         <% end %>
-        <% if module_group.my_modules.all? { |my_module| can_archive_module?(my_module) } %>
+        <% if module_group&.my_modules&.all? { |my_module| can_archive_module?(my_module) } %>
           <li data-hook="archive-module-group">
             <a class ="delete-module-group" href="" data-module-id="<%= my_module.id %>"><%= t('experiments.canvas.edit.delete_module_group') %></a>
           </li>

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">
@@ -66,6 +66,13 @@
                       data: { action: 'move-step', direction: :down }) do %>
             <span class="fas fa-arrow-down"></span>
           <% end %>
+          <%= link_to(edit_step_path(step),
+                      title: t('protocols.steps.options.edit_title'),
+                      class: 'btn btn-light icon-btn',
+                      remote: true,
+                      data: { action: 'edit-step' }) do %>
+            <span class="fas fa-pencil-alt"></span>
+          <% end %>
           <%= link_to(step_path(step),
                       title: t('protocols.steps.options.delete_title'),
                       method: :delete,