From dd27fadd986c1d582a6afd61586e0cb7e2d45fe7 Mon Sep 17 00:00:00 2001
From: artoscinote <85488244+artoscinote@users.noreply.github.com>
Date: Tue, 12 Jul 2022 10:13:47 +0200
Subject: [PATCH] Add step reordering and step element reordering service
 endpoints to API [SCI-6891][SCI-6892] (#4179)

* Add step reordering service endpoint to API [SCI-6891]

* Generalize reorder validation [SCI-6891]

* Add endpoint for reordering step elements, fix issues [SCI-6892]

* Add appropriate serializers [SCI-6891][SCI-6892]

* Add step elements to step serializer [SCI-6891]

* Simplify routes, add locking [SCI-6891]
---
 .../api/service/protocols_controller.rb       |  44 +++++++
 .../api/service/steps_controller.rb           |  48 ++++++++
 .../api/service/reorder_validation.rb         |  16 +++
 .../v1/step_orderable_element_serializer.rb   |  20 ++++
 app/serializers/api/v1/step_serializer.rb     |   1 +
 .../api/v1/step_text_serializer.rb            |  16 +++
 config/locales/en.yml                         |   8 ++
 config/routes.rb                              |  10 +-
 spec/factories/step_reorderable_elements.rb   |   9 ++
 .../api/service/protocols_controller_spec.rb  | 109 +++++++++++++++++
 .../api/service/steps_controller_spec.rb      | 110 ++++++++++++++++++
 11 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 app/controllers/api/service/protocols_controller.rb
 create mode 100644 app/controllers/api/service/steps_controller.rb
 create mode 100644 app/controllers/concerns/api/service/reorder_validation.rb
 create mode 100644 app/serializers/api/v1/step_orderable_element_serializer.rb
 create mode 100644 app/serializers/api/v1/step_text_serializer.rb
 create mode 100644 spec/factories/step_reorderable_elements.rb
 create mode 100644 spec/requests/api/service/protocols_controller_spec.rb
 create mode 100644 spec/requests/api/service/steps_controller_spec.rb

diff --git a/app/controllers/api/service/protocols_controller.rb b/app/controllers/api/service/protocols_controller.rb
new file mode 100644
index 000000000..9fd6c463f
--- /dev/null
+++ b/app/controllers/api/service/protocols_controller.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module Api
+  module Service
+    class ProtocolsController < BaseController
+      include Api::Service::ReorderValidation
+
+      before_action :load_protocol
+      before_action :validate_step_order, only: :reorder_steps
+
+      def reorder_steps
+        @protocol.with_lock do
+          step_reorder_params.each do |order|
+            # rubocop:disable Rails/SkipsModelValidations
+            @protocol.steps.find(order['id']).update_column(:position, order['position'])
+            # rubocop:enable Rails/SkipsModelValidations
+          end
+        rescue StandardError
+          head :bad_request
+        end
+
+        render json: @protocol.steps, each_serializer: Api::V1::StepSerializer
+      end
+
+      private
+
+      def load_protocol
+        @protocol = Protocol.find(params.require(:protocol_id))
+        raise PermissionError.new(Protocol, :manage) unless can_manage_protocol_in_module?(@protocol)
+      end
+
+      def step_reorder_params
+        params.require(:step_order).map { |o| o.permit(:id, :position).to_h }
+      end
+
+      def validate_step_order
+        unless reorder_params_valid?(@protocol.steps, step_reorder_params)
+          render json: { error: I18n.t('activerecord.errors.models.protocol.attributes.step_order.invalid') },
+                 status: :bad_request
+        end
+      end
+    end
+  end
+end
diff --git a/app/controllers/api/service/steps_controller.rb b/app/controllers/api/service/steps_controller.rb
new file mode 100644
index 000000000..da99ab12e
--- /dev/null
+++ b/app/controllers/api/service/steps_controller.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+module Api
+  module Service
+    class StepsController < BaseController
+      include Api::Service::ReorderValidation
+      before_action :load_step
+      before_action :validate_element_order, only: :reorder_elements
+
+      def reorder_elements
+        @step.with_lock do
+          step_element_reorder_params.each do |order|
+            # rubocop:disable Rails/SkipsModelValidations
+            @step.step_orderable_elements.find(order['id']).update_column(:position, order['position'])
+            # rubocop:enable Rails/SkipsModelValidations
+          end
+        rescue StandardError
+          head :bad_request
+        end
+
+        render json: @step.step_orderable_elements, each_serializer: Api::V1::StepOrderableElementSerializer
+      end
+
+      private
+
+      def load_step
+        @step = Step.find(params.require(:step_id))
+        raise PermissionError.new(Protocol, :manage) unless can_manage_protocol_in_module?(@step.protocol)
+      end
+
+      def step_element_reorder_params
+        params.require(:step_element_order).map { |o| o.permit(:id, :position).to_h }
+      end
+
+      def validate_element_order
+        unless reorder_params_valid?(@step.step_orderable_elements, step_element_reorder_params)
+          render(
+            json:
+              {
+                error: I18n.t('activerecord.errors.models.step.attributes.step_orderable_elements_order.invalid')
+              },
+            status: :bad_request
+          )
+        end
+      end
+    end
+  end
+end
diff --git a/app/controllers/concerns/api/service/reorder_validation.rb b/app/controllers/concerns/api/service/reorder_validation.rb
new file mode 100644
index 000000000..f85ff1ef9
--- /dev/null
+++ b/app/controllers/concerns/api/service/reorder_validation.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Api
+  module Service
+    module ReorderValidation
+      extend ActiveSupport::Concern
+
+      def reorder_params_valid?(collection, reorder_params)
+        # contains all collection ids, positions have values from 0 to number of items in collection - 1
+
+        collection.order(:id).pluck(:id) == reorder_params.pluck(:id).sort &&
+          reorder_params.pluck(:position).sort == (0...reorder_params.length).to_a
+      end
+    end
+  end
+end
diff --git a/app/serializers/api/v1/step_orderable_element_serializer.rb b/app/serializers/api/v1/step_orderable_element_serializer.rb
new file mode 100644
index 000000000..f458273c7
--- /dev/null
+++ b/app/serializers/api/v1/step_orderable_element_serializer.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module Api
+  module V1
+    class StepOrderableElementSerializer < ActiveModel::Serializer
+      attributes :position, :element
+
+      def element
+        case object.orderable_type
+        when 'Checklist'
+          ChecklistSerializer.new(object.orderable).as_json
+        when 'StepTable'
+          TableSerializer.new(object.orderable.table).as_json
+        when 'StepText'
+          StepTextSerializer.new(object.orderable).as_json
+        end
+      end
+    end
+  end
+end
diff --git a/app/serializers/api/v1/step_serializer.rb b/app/serializers/api/v1/step_serializer.rb
index ed8837975..3896e3e5d 100644
--- a/app/serializers/api/v1/step_serializer.rb
+++ b/app/serializers/api/v1/step_serializer.rb
@@ -17,6 +17,7 @@ module Api
       has_many :tables, serializer: TableSerializer
       has_many :step_texts, serializer: StepTextSerializer
       has_many :step_comments, key: :comments, serializer: CommentSerializer
+      has_many :step_orderable_elements, key: :step_elements, serializer: StepOrderableElementSerializer
 
       include TimestampableModel
 
diff --git a/app/serializers/api/v1/step_text_serializer.rb b/app/serializers/api/v1/step_text_serializer.rb
new file mode 100644
index 000000000..b57deb150
--- /dev/null
+++ b/app/serializers/api/v1/step_text_serializer.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Api
+  module V1
+    class StepTextSerializer < ActiveModel::Serializer
+      type :tables
+      attributes :id, :text
+
+      include TimestampableModel
+
+      def contents
+        object.text&.force_encoding(Encoding::UTF_8)
+      end
+    end
+  end
+end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index f154155ab..32d57498e 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -129,6 +129,14 @@ en:
           attributes:
             team_id:
               same_team: "Inventory can't be shared to the same team as it belongs to"
+        protocol:
+          attributes:
+            step_order:
+              invalid: "Invalid step order."
+        step:
+          attributes:
+            step_orderable_element_order:
+              invalid: "Invalid step element order"
         my_module:
           attributes:
             my_module_status_id:
diff --git a/config/routes.rb b/config/routes.rb
index 2c76badd5..0297f27e8 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -715,8 +715,16 @@ Rails.application.routes.draw do
       get 'status', to: 'api#status'
       namespace :service do
         post 'projects_json_export', to: 'projects_json_export#projects_json_export'
-        resources :teams, except: %i(index new create show edit update destroy) do
+        resources :teams, only: [] do
           post 'clone_experiment' => 'experiments#clone'
+
+          resources :protocols, only: [] do
+            post 'reorder_steps' => 'protocols#reorder_steps'
+          end
+
+          resources :steps, only: [] do
+            post 'reorder_elements' => 'steps#reorder_elements'
+          end
         end
       end
       if Rails.configuration.x.core_api_v1_enabled
diff --git a/spec/factories/step_reorderable_elements.rb b/spec/factories/step_reorderable_elements.rb
new file mode 100644
index 000000000..a9c1cfd09
--- /dev/null
+++ b/spec/factories/step_reorderable_elements.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :step_orderable_element do
+    orderable { create :step_text, step: step }
+    step
+    position { step ? step.step_orderable_elements.count : Faker::Number.between(from: 1, to: 10) }
+  end
+end
diff --git a/spec/requests/api/service/protocols_controller_spec.rb b/spec/requests/api/service/protocols_controller_spec.rb
new file mode 100644
index 000000000..a9bd8cd37
--- /dev/null
+++ b/spec/requests/api/service/protocols_controller_spec.rb
@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe "Api::Service::ProtocolsController", type: :request do
+  before :all do
+    @user = create(:user)
+    @team = create(:team, created_by: @user)
+    create(:user_team, user: @user, team: @team, role: 2)
+
+    @project = create(:project, name: Faker::Name.unique.name, created_by: @user, team: @team)
+    @experiment = create(:experiment, created_by: @user, last_modified_by: @user, project: @project, created_by: @user)
+    @my_module = create(
+      :my_module,
+      :with_due_date,
+      created_by: @user,
+      last_modified_by: @user,
+      experiment: @experiment
+    )
+
+    @protocol = create(:protocol, team: @team, my_module: @my_module, name: "Test protocol")
+
+    create_list(:step, 3, protocol: @protocol)
+
+    @valid_headers =
+      {
+        'Authorization'=> 'Bearer ' + generate_token(@user.id),
+        'Content-Type' => 'application/json'
+      }
+  end
+
+  describe 'POST reorder steps, #reorder_steps' do
+    let(:action) do
+      post(
+        api_service_team_protocol_reorder_steps_path(
+          team_id: @team.id,
+          protocol_id: @protocol.id
+        ),
+        params: request_body.to_json,
+        headers: @valid_headers
+      )
+    end
+
+    context 'when has valid params' do
+      let(:request_body) do
+        { step_order:
+            @protocol.steps.pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: @protocol.steps.length - 1 - i }
+            end
+        }
+      end
+
+      it 'returns status 200 and reorderes steps' do
+        action
+        expect(response).to have_http_status 200
+
+        new_step_order = @protocol.steps.order(position: :asc).pluck(:id, :position)
+
+        expect(new_step_order).to(
+          eq(
+            request_body[:step_order].map(&:values).sort { |a, b| a[1] <=> b[1] }
+          )
+        )
+      end
+    end
+
+    context "when step order doesn't include all step ids" do
+      let(:request_body) do
+        { step_order:
+            @protocol.steps.last(2).pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: @protocol.steps.length - 1 - i }
+            end
+        }
+      end
+
+      it 'returns status 400' do
+        action
+        expect(response).to have_http_status 400
+      end
+    end
+
+    context "when step order doesn't have the correct positions" do
+      let(:request_body) do
+        { step_order:
+            @protocol.steps.last(2).pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: i + 1 }
+            end
+        }
+      end
+
+      it 'returns status 400' do
+        action
+        expect(response).to have_http_status 400
+      end
+    end
+
+    context 'when has missing param' do
+      let(:request_body) do
+        {}
+      end
+
+      it 'renders 400' do
+        action
+
+        expect(response).to have_http_status(400)
+      end
+    end
+  end
+end
diff --git a/spec/requests/api/service/steps_controller_spec.rb b/spec/requests/api/service/steps_controller_spec.rb
new file mode 100644
index 000000000..112866962
--- /dev/null
+++ b/spec/requests/api/service/steps_controller_spec.rb
@@ -0,0 +1,110 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe "Api::Service::StepsController", type: :request do
+  before :all do
+    @user = create(:user)
+    @team = create(:team, created_by: @user)
+    create(:user_team, user: @user, team: @team, role: 2)
+
+    @project = create(:project, name: Faker::Name.unique.name, created_by: @user, team: @team)
+    @experiment = create(:experiment, created_by: @user, last_modified_by: @user, project: @project, created_by: @user)
+    @my_module = create(
+      :my_module,
+      :with_due_date,
+      created_by: @user,
+      last_modified_by: @user,
+      experiment: @experiment
+    )
+
+    @protocol = create(:protocol, team: @team, my_module: @my_module, name: "Test protocol")
+    @step = create(:step, protocol: @protocol)
+
+    create_list(:step_orderable_element, 3, step: @step)
+
+    @valid_headers =
+      {
+        'Authorization'=> 'Bearer ' + generate_token(@user.id),
+        'Content-Type' => 'application/json'
+      }
+  end
+
+  describe 'POST reorder steps, #reorder_steps' do
+    let(:action) do
+      post(
+        api_service_team_step_reorder_elements_path(
+          team_id: @team.id,
+          step_id: @step.id
+        ),
+        params: request_body.to_json,
+        headers: @valid_headers
+      )
+    end
+
+    context 'when has valid params' do
+      let(:request_body) do
+        { step_element_order:
+            @step.step_orderable_elements.pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: @step.step_orderable_elements.length - 1 - i }
+            end
+        }
+      end
+
+      it 'returns status 200 and reorderes step elements' do
+        action
+
+        expect(response).to have_http_status 200
+        new_step_element_order = @step.step_orderable_elements.order(position: :asc).pluck(:id, :position)
+
+        expect(new_step_element_order).to(
+          eq(
+            request_body[:step_element_order].map(&:values).sort { |a, b| a[1] <=> b[1] }
+          )
+        )
+      end
+    end
+
+    context "when step order doesn't include all step ids" do
+      let(:request_body) do
+        { step_element_order:
+          @step.step_orderable_elements.last(2).pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: @step.step_orderable_elements.length - 1 - i }
+            end
+        }
+      end
+
+      it 'returns status 400' do
+        action
+        expect(response).to have_http_status 400
+      end
+    end
+
+    context "when step order doesn't have the correct positions" do
+      let(:request_body) do
+        { step_element_order:
+            @step.step_orderable_elements.last(2).pluck(:id).each_with_index.map do |id, i|
+              { id: id, position: i + 1 }
+            end
+        }
+      end
+
+      it 'returns status 400' do
+        action
+        expect(response).to have_http_status 400
+      end
+    end
+
+    context 'when has missing param' do
+      let(:request_body) do
+        {}
+      end
+
+      it 'renders 400' do
+        action
+
+        expect(response).to have_http_status(400)
+      end
+    end
+  end
+end