From a206890a8c62b72058f6d2568c0d3035651d194f Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Mon, 27 Jul 2020 16:14:14 +0200 Subject: [PATCH 1/2] Fix step position adjusting on move and destroy [SCI-4880] --- app/assets/javascripts/protocols/steps.js.erb | 23 +++--- app/controllers/steps_controller.rb | 75 +++---------------- app/models/step.rb | 40 ++++++++-- app/views/steps/_step.html.erb | 45 ++++++----- config/routes.rb | 4 +- 5 files changed, 82 insertions(+), 105 deletions(-) diff --git a/app/assets/javascripts/protocols/steps.js.erb b/app/assets/javascripts/protocols/steps.js.erb index 8f96b177b..952ff3e86 100644 --- a/app/assets/javascripts/protocols/steps.js.erb +++ b/app/assets/javascripts/protocols/steps.js.erb @@ -162,29 +162,28 @@ // 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"); + $('#steps').on('ajax:success', "[data-action='move-step']", 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"); + + 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(); diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index 58077aabf..c159c0ad3 100644 --- a/app/controllers/steps_controller.rb +++ b/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,26 @@ 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 + @step.move_up! - 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 - else - format.json { render json: {}, status: :forbidden } - end - else - format.json { render json: {}, status: :forbidden } + render json: { + step_up_position: @step.position, + step_down_position: @step.position + 1 + } 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 + @step.move_down! - 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 - else - format.json { render json: {}, status: :forbidden } - end - else - format.json { render json: {}, status: :forbidden } + render json: { + step_up_position: @step.position - 1, + step_down_position: @step.position + } end end end @@ -517,12 +472,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]) diff --git a/app/models/step.rb b/app/models/step.rb index 86fa7002c..bd28277a7 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -9,7 +9,7 @@ class Step < ApplicationRecord presence: true, length: { maximum: Constants::NAME_MAX_LENGTH } validates :description, length: { maximum: Constants::RICH_TEXT_MAX_LENGTH } - validates :position, presence: true + validates :position, presence: true, uniqueness: { scope: :protocol } validates :completed, inclusion: { in: [true, false] } validates :user, :protocol, presence: true validates :completed_on, presence: true, if: proc { |s| s.completed? } @@ -17,11 +17,11 @@ class Step < ApplicationRecord before_validation :set_completed_on, if: :completed_changed? before_save :set_last_modified_by 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 @@ -90,6 +90,36 @@ class Step < ApplicationRecord protocol.present? ? protocol.my_module : nil end + def move_up! + return unless position.positive? + + step_above = protocol.steps.find_by(position: position - 1) + + return unless step_above + + transaction do + step_above.position = position + update!(position: -1) + step_above.save! + update!(position: step_above.position - 1) + end + end + + def move_down! + return unless position < protocol.steps.count - 1 + + step_below = protocol.steps.find_by(position: position + 1) + + return unless step_below + + transaction do + step_below.position = position + update!(position: -1) + step_below.save! + update!(position: step_below.position + 1) + end + end + def position_plus_one position + 1 end @@ -125,9 +155,9 @@ class Step < ApplicationRecord private - def adjust_positions_on_destroy + 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 diff --git a/app/views/steps/_step.html.erb b/app/views/steps/_step.html.erb index bfbfadd56..6374a6824 100644 --- a/app/views/steps/_step.html.erb +++ b/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) %> - " - data-remote="true"> - - " - data-remote="true"> - - " - href="<%= edit_step_path(step, format: :json) %>" - data-remote="true" > - - - <%= 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 %> + + <% 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 %> + + <% 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 %> <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 298b2ead2..8e603000e 100644 --- a/config/routes.rb +++ b/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 From ed2eb81c68a312c65d9fa40adddcfbe1eac1d67d Mon Sep 17 00:00:00 2001 From: Oleksii Kriuchykhin Date: Tue, 28 Jul 2020 15:27:05 +0200 Subject: [PATCH 2/2] Switch to position swapping when updating step position [SCI-4880] --- app/assets/javascripts/protocols/steps.js.erb | 10 +++-- app/controllers/steps_controller.rb | 28 +++++++----- app/models/step.rb | 44 ++++++------------- spec/factories/steps.rb | 2 +- 4 files changed, 38 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/protocols/steps.js.erb b/app/assets/javascripts/protocols/steps.js.erb index 952ff3e86..b6d58275c 100644 --- a/app/assets/javascripts/protocols/steps.js.erb +++ b/app/assets/javascripts/protocols/steps.js.erb @@ -163,10 +163,12 @@ // Set callback for click on move step function applyMoveStepCallBack() { $('#steps').on('ajax:success', "[data-action='move-step']", function(e, data) { - var $step = $(this).closest('.step'); - var stepUpPosition = data.step_up_position; - var stepDownPosition = data.step_down_position; - var $stepDown, $stepUp; + 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': diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index c159c0ad3..ddec42b89 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -354,12 +354,16 @@ class StepsController < ApplicationController def move_up respond_to do |format| format.json do - @step.move_up! + if @step.protocol.steps.minimum(:position) != @step.position + @step.update!(position: @step.position - 1) - render json: { - step_up_position: @step.position, - step_down_position: @step.position + 1 - } + render json: { + step_up_position: @step.position, + step_down_position: @step.position + 1 + } + else + render json: {} + end end end end @@ -367,12 +371,16 @@ class StepsController < ApplicationController def move_down respond_to do |format| format.json do - @step.move_down! + if @step.protocol.steps.maximum(:position) != @step.position + @step.update!(position: @step.position + 1) - render json: { - step_up_position: @step.position - 1, - step_down_position: @step.position - } + render json: { + step_up_position: @step.position - 1, + step_down_position: @step.position + } + else + render json: {} + end end end end diff --git a/app/models/step.rb b/app/models/step.rb index bd28277a7..bf40e18c7 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -9,13 +9,14 @@ class Step < ApplicationRecord presence: true, length: { maximum: Constants::NAME_MAX_LENGTH } validates :description, length: { maximum: Constants::RICH_TEXT_MAX_LENGTH } - validates :position, presence: true, uniqueness: { scope: :protocol } + validates :position, presence: true validates :completed, inclusion: { in: [true, false] } validates :user, :protocol, presence: true validates :completed_on, presence: true, if: proc { |s| s.completed? } 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 @@ -90,36 +91,6 @@ class Step < ApplicationRecord protocol.present? ? protocol.my_module : nil end - def move_up! - return unless position.positive? - - step_above = protocol.steps.find_by(position: position - 1) - - return unless step_above - - transaction do - step_above.position = position - update!(position: -1) - step_above.save! - update!(position: step_above.position - 1) - end - end - - def move_down! - return unless position < protocol.steps.count - 1 - - step_below = protocol.steps.find_by(position: position + 1) - - return unless step_below - - transaction do - step_below.position = position - update!(position: -1) - step_below.save! - update!(position: step_below.position + 1) - end - end - def position_plus_one position + 1 end @@ -155,6 +126,17 @@ class Step < ApplicationRecord private + 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) diff --git a/spec/factories/steps.rb b/spec/factories/steps.rb index 94886a6b8..8c6677963 100644 --- a/spec/factories/steps.rb +++ b/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