From dea75553a3ebe2b94c0e3a84acb21498079dd099 Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Mon, 7 Sep 2020 10:44:00 +0200 Subject: [PATCH] Fix handling of duplicated step positions [SCI-4880] --- app/assets/javascripts/protocols/steps.js.erb | 27 ++------ app/controllers/steps_controller.rb | 26 +++----- app/models/step.rb | 66 ++++++++++++++++--- app/views/steps/_step.html.erb | 2 +- 4 files changed, 74 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/protocols/steps.js.erb b/app/assets/javascripts/protocols/steps.js.erb index b6d58275c..24263347d 100644 --- a/app/assets/javascripts/protocols/steps.js.erb +++ b/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(); }); diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index ddec42b89..b6fa6c8e5 100644 --- a/app/controllers/steps_controller.rb +++ b/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 diff --git a/app/models/step.rb b/app/models/step.rb index bf40e18c7..797cc30ba 100644 --- a/app/models/step.rb +++ b/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) diff --git a/app/views/steps/_step.html.erb b/app/views/steps/_step.html.erb index 6374a6824..0f3912868 100644 --- a/app/views/steps/_step.html.erb +++ b/app/views/steps/_step.html.erb @@ -1,6 +1,6 @@ <% preview = (defined?(preview) ? preview : false) %> <% import = (defined?(import) ? import : false) %> -
"> +
" data-id="<%= step.id %>">