Add unique database index to checklist items position (#5886)

* Add unique database index to checklist items position and update errors to render flash message when a checklist item validation fails [SCI-8841]

* Improve protocol checklist item reordering [SCI-8841]

* Add act as list to gemfile and use sequential updates for reodering with act_as_list [SCI-8841]

Gemfile

@@ -54,6 +54,7 @@ gem 'js_cookie_rails' # Simple JS API for cookies
 gem 'spinjs-rails'
 
 gem 'activerecord-import'
+gem 'acts_as_list'
 gem 'ajax-datatables-rails', '~> 0.3.1'
 gem 'aspector' # Aspect-oriented programming for Rails
 gem 'auto_strip_attributes', '~> 2.1' # Removes unnecessary whitespaces AR

Gemfile.lock

@@ -132,6 +132,8 @@ GEM
       i18n (>= 1.6, < 2)
       minitest (>= 5.1)
       tzinfo (~> 2.0)
+    acts_as_list (1.1.0)
+      activerecord (>= 4.2)
     addressable (2.8.4)
       public_suffix (>= 2.0.2, < 6.0)
     aes_key_wrap (1.1.0)
@@ -707,6 +709,7 @@ PLATFORMS
 DEPENDENCIES
   active_model_serializers (~> 0.10.7)
   activerecord-import
+  acts_as_list
   ajax-datatables-rails (~> 0.3.1)
   aspector
   auto_strip_attributes (~> 2.1)

app/controllers/step_elements/checklist_items_controller.rb

@@ -27,15 +27,13 @@ module StepElements
 
       render json: checklist_item, serializer: ChecklistItemSerializer, user: current_user
     rescue ActiveRecord::RecordInvalid
-      render json: checklist_item, serializer: ChecklistItemSerializer,
-                                   user: current_user,
-                                   status: :unprocessable_entity
+      render json: { errors: checklist_item.errors }, status: :unprocessable_entity
     end
 
     def update
       old_text = @checklist_item.text
       @checklist_item.assign_attributes(
-        checklist_item_params.merge(last_modified_by: current_user)
+        checklist_item_params.except(:position, :id).merge(last_modified_by: current_user)
       )
 
       if @checklist_item.save!
@@ -49,9 +47,7 @@ module StepElements
 
       render json: @checklist_item, serializer: ChecklistItemSerializer, user: current_user
     rescue ActiveRecord::RecordInvalid
-      render json: @checklist_item, serializer: ChecklistItemSerializer,
-                                    user: current_user,
-                                    status: :unprocessable_entity
+      render json: { errors: @checklist_item.errors }, status: :unprocessable_entity
     end
 
     def toggle
@@ -97,14 +93,14 @@ module StepElements
     end
 
     def reorder
-      @checklist.with_lock do
-        params[:checklist_item_positions].each do |id, position|
-          @checklist.checklist_items.find(id).update_column(:position, position)
-        end
+      checklist_item = @checklist.checklist_items.find(checklist_item_params[:id])
+      ActiveRecord::Base.transaction do
+        checklist_item.insert_at(checklist_item_params[:position])
         @checklist.touch
       end
-
       render json: params[:checklist_item_positions], status: :ok
+    rescue ActiveRecord::RecordInvalid
+      render json: { errors: checklist_item.errors }, status: :conflict
     end
 
     private
@@ -122,7 +118,7 @@ module StepElements
     end
 
     def checklist_item_params
-      params.require(:attributes).permit(:text, :position)
+      params.require(:attributes).permit(:text, :position, :id)
     end
 
     def checklist_toggle_item_params

app/javascript/vue/shared/content/checklist.vue

@@ -172,8 +172,8 @@
           );
 
           if(callback) callback();
-        }).error(() => {
-          HelperModule.flashAlertMsg(this.i18n.t('errors.general'), 'danger');
+        }).error((xhr) => {
+          this.setFlashErrors(xhr.responseJSON.errors)
         });
 
         this.update();
@@ -190,7 +190,7 @@
               updatedItem.attributes.id = item.attributes.id
               this.$set(this.checklistItems, item.attributes.position, updatedItem)
             },
-            error: () => HelperModule.flashAlertMsg(this.i18n.t('errors.general'), 'danger')
+            error: (xhr) => setFlashErrors(xhr.responseJSON.errors)
           });
         } else {
           // create item, then append next one
@@ -231,26 +231,26 @@
       startReorder() {
         this.reordering = true;
       },
-      endReorder() {
+      endReorder(event) {
         this.reordering = false;
-        this.saveItemOrder();
+        if(
+          Number.isInteger(event.newIndex)
+          && Number.isInteger(event.newIndex)
+          && event.newIndex !== event.oldIndex
+        ){
+          const { id, position } = this.orderedChecklistItems[event.newIndex]?.attributes
+          this.saveItemOrder(id, position);
+        }
       },
-      saveItemOrder() {
-        let checklistItemPositions =
-          {
-            checklist_item_positions: this.orderedChecklistItems.map(
-              (i) => [i.attributes.id, i.attributes.position]
-            )
-          };
-
+      saveItemOrder(id, position) {
         $.ajax({
           type: "POST",
           url: this.element.attributes.orderable.urls.reorder_url,
-          data: JSON.stringify(checklistItemPositions),
+          data: JSON.stringify({ attributes: { id, position } }),
           contentType: "application/json",
           dataType: "json",
-          error: (() => HelperModule.flashAlertMsg(this.i18n.t('errors.general'), 'danger')),
-          success: (() => this.update())
+          error: (xhr) => this.setFlashErrors(xhr.responseJSON.errors),
+          success: () => this.update()
         });
       },
       handleMultilinePaste(data) {
@@ -274,6 +274,14 @@
         };
 
         synchronousPost(0);
+      },
+      setFlashErrors(errors) {
+        for(const key in errors){
+          HelperModule.flashAlertMsg(
+            this.i18n.t(`activerecord.errors.models.checklist_item.attributes.${key}`),
+            'danger'
+          )
+        }
       }
     }
   }

app/models/checklist_item.rb

@@ -5,10 +5,11 @@ class ChecklistItem < ApplicationRecord
             length: { maximum: Constants::TEXT_MAX_LENGTH }
   validates :checklist, presence: true
   validates :checked, inclusion: { in: [true, false] }
-  validates :position, uniqueness: { scope: :checklist }, unless: -> { position.nil? }
+  validates :position, uniqueness: { scope: :checklist }
 
   belongs_to :checklist,
              inverse_of: :checklist_items
+  acts_as_list scope: :checklist, top_of_list: 0, sequential_updates: true
   belongs_to :created_by,
              foreign_key: 'created_by_id',
              class_name: 'User',
@@ -18,8 +19,6 @@ class ChecklistItem < ApplicationRecord
              class_name: 'User',
              optional: true
 
-  after_destroy :update_positions
-
   # conditional touch excluding checked updates
   after_destroy :touch_checklist
   after_save :touch_checklist
@@ -35,12 +34,4 @@ class ChecklistItem < ApplicationRecord
     checklist.touch
     # rubocop:enable Rails/SkipsModelValidations
   end
-
-  def update_positions
-    transaction do
-      checklist.checklist_items.order(position: :asc).each_with_index do |checklist_item, i|
-        checklist_item.update!(position: i)
-      end
-    end
-  end
 end

config/locales/en.yml

@@ -232,6 +232,10 @@ en:
           attributes:
             output_id:
               creates_cycle: "mustn't create cycle"
+        checklist_item:
+          attributes:
+            text: Text is too long
+            position: "Position has already been taken by another item in the checklist"
   errors:
     general: "Something went wrong."
     general_text_too_long: 'Text is too long'

db/migrate/20230728104825_add_index_to_checklist_items.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class
+ AddIndexToChecklistItems < ActiveRecord::Migration[7.0]
+  def change
+    add_index :checklist_items, %i(checklist_id position), unique: true
+  end
+end