Fix: Preserve position when creating new records

Co-authored-by: Karina <[email protected]>

CHANGELOG.md

@@ -1,5 +1,10 @@
-### 6.0.5 (Next)
+### 6.0.6 (Next)
 
+* Your change here.
+
+### 6.0.5 (2023/11/08)
+
+* [#75](https://github.com/mongoid/mongoid_orderable/pull/75): Fix: Preserve position when creating new records.
 * [#84](https://github.com/mongoid/mongoid_orderable/pull/84): Feature: Use Mongo::QueryCache if available.
 
 ### 6.0.4 (2023/02/01)

lib/mongoid/orderable/handlers/base.rb

@@ -8,6 +8,7 @@ class Base
 
     def initialize(doc)
       @doc = doc
+      reset_new_record
     end
 
     protected
@@ -22,12 +23,15 @@ def initialize(doc)
              :orderable_top,
              :orderable_bottom,
              :_id,
-             :new_record?,
              :persisted?,
              :embedded?,
              :collection_name,
              to: :doc
 
+    def new_record?
+      use_transactions ? @new_record : doc.new_record?
+    end
+
     def use_transactions
       false
     end
@@ -36,8 +40,18 @@ def any_field_changed?
       orderable_keys.any? {|field| changed?(field) }
     end
 
+    def set_new_record_positions
+      return unless new_record?
+
+      orderable_keys.each do |field|
+        next unless (position = doc.send(field))
+
+        move_all[field] ||= position
+      end
+    end
+
     def apply_all_positions
-      orderable_keys.map {|field| apply_one_position(field, move_all[field]) }
+      orderable_keys.each {|field| apply_one_position(field, move_all[field]) }
     end
 
     def apply_one_position(field, target_position)
@@ -56,7 +70,7 @@ def apply_one_position(field, target_position)
       end
 
       # Get the current position as exists in the database
-      current = if !persisted? || scope_changed
+      current = if new_record? || scope_changed
                   nil
                 elsif persisted? && !embedded?
                   scope.where(_id: _id).pluck(f).first
@@ -74,8 +88,9 @@ def apply_one_position(field, target_position)
       in_list = persisted? && current
       return if in_list && !target_position
 
-      # Use $inc operator to shift the position of the other documents
       target = resolve_target_position(field, target_position, in_list)
+
+      # Use $inc operator to shift the position of the other documents
       if !in_list
         scope.gte(f => target).inc(f => 1)
       elsif target < current
@@ -107,10 +122,21 @@ def move_all
       doc.send(:move_all)
     end
 
+    def reset
+      reset_new_record
+      doc.send(:clear_move_all!)
+    end
+
+    # Required for transactions, which perform some actions
+    # in the after_create callback.
+    def reset_new_record
+      @new_record = doc.new_record?
+    end
+
     def resolve_target_position(field, target_position, in_list)
       target_position ||= 'bottom'
 
-      unless target_position.is_a? Numeric
+      unless target_position.is_a?(Numeric)
         target_position = case target_position.to_s
                           when 'top' then (min ||= orderable_top(field))
                           when 'bottom' then (max ||= orderable_bottom(field, in_list))

lib/mongoid/orderable/handlers/document.rb

@@ -5,18 +5,26 @@ module Orderable
 module Handlers
   class Document < Base
     def before_create
+      set_new_record_positions
       apply_all_positions
     end
 
-    def after_create; end
+    def after_create
+      reset
+    end
 
     def before_update
       return unless any_field_changed?
       apply_all_positions
     end
 
+    def after_update
+      reset
+    end
+
     def after_destroy
       remove_all_positions
+      reset
     end
   end
 end

lib/mongoid/orderable/handlers/document_transactional.rb

@@ -5,11 +5,12 @@ module Orderable
 module Handlers
   class DocumentTransactional < Document
     def before_create
-      clear_all_positions
+      set_new_record_positions
     end
 
     def after_create
       apply_all_positions
+      super
     end
 
     protected
@@ -18,10 +19,6 @@ def apply_all_positions
       with_transaction { super }
     end
 
-    def clear_all_positions
-      orderable_keys.each {|field| doc.send("orderable_#{field}_position=", nil) }
-    end
-
     def use_transactions
       true
     end

lib/mongoid/orderable/mixins/callbacks.rb

@@ -12,11 +12,13 @@ module Callbacks
       before_create :orderable_before_create
       after_create :orderable_after_create, prepend: true
       before_update :orderable_before_update
+      after_update :orderable_after_update, prepend: true
       after_destroy :orderable_after_destroy, prepend: true
 
       delegate :before_create,
                :after_create,
                :before_update,
+               :after_update,
                :after_destroy,
                to: :orderable_handler,
                prefix: :orderable

lib/mongoid/orderable/mixins/movable.rb

@@ -47,12 +47,16 @@ def move_#{symbol}!(options = {})
     protected
 
     def move_all
-      @move_all || {}
+      @move_all ||= {}
     end
 
     def move_field_to(position, options)
       field = options[:field] || default_orderable_field
-      @move_all = move_all.merge(field => position)
+      move_all[field] = position
+    end
+
+    def clear_move_all!
+      @move_all = {}
     end
   end
 end

lib/mongoid/orderable/version.rb

@@ -2,6 +2,6 @@
 
 module Mongoid
   module Orderable
-    VERSION = '6.0.4'
+    VERSION = '6.0.5'
   end
 end

lib/mongoid_orderable.rb

@@ -7,9 +7,8 @@
 
 require 'mongoid'
 
-require 'mongoid/orderable/version'
-
 require 'mongoid/orderable'
+require 'mongoid/orderable/version'
 require 'mongoid/orderable/configs/global_config'
 require 'mongoid/orderable/configs/field_config'
 require 'mongoid/orderable/errors/invalid_target_position'

spec/integration/multiple_fields_spec.rb

@@ -428,8 +428,8 @@
     context 'group_count orderable' do
       before :each do
         MultipleFieldsOrderable.delete_all
-        2.times { MultipleFieldsOrderable.create! group_id: 1 }
-        3.times { MultipleFieldsOrderable.create! group_id: 2 }
+        2.times { MultipleFieldsOrderable.create!(group_id: 1) }
+        3.times { MultipleFieldsOrderable.create!(group_id: 2) }
       end
 
       let(:all_groups) { MultipleFieldsOrderable.order_by([:group_id, :asc], [:groups, :asc]).map(&:groups) }
@@ -457,27 +457,53 @@
 
       describe 'inserting' do
         it 'top' do
-          newbie = MultipleFieldsOrderable.create! group_id: 1
+          newbie = MultipleFieldsOrderable.create!(group_id: 1)
           newbie.move_groups_to! :top
           expect(all_groups).to eq([1, 2, 3, 1, 2, 3])
           expect(newbie.groups).to eq(1)
         end
 
         it 'bottom' do
-          newbie = MultipleFieldsOrderable.create! group_id: 2
+          newbie = MultipleFieldsOrderable.create!(group_id: 2)
           newbie.move_groups_to! :bottom
           expect(all_groups).to eq([1, 2, 1, 2, 3, 4])
           expect(newbie.groups).to eq(4)
         end
 
         it 'middle' do
-          newbie = MultipleFieldsOrderable.create! group_id: 2
+          newbie = MultipleFieldsOrderable.create!(group_id: 2)
           newbie.move_groups_to! 2
           expect(all_groups).to eq([1, 2, 1, 2, 3, 4])
           expect(newbie.groups).to eq(2)
         end
       end
 
+      describe 'moving existing document' do
+        it 'top' do
+          newbie = MultipleFieldsOrderable.create!(group_id: 1)
+          record = MultipleFieldsOrderable.find(newbie._id)
+          record.move_groups_to! :top
+          expect(all_groups).to eq([1, 2, 3, 1, 2, 3])
+          expect(record.groups).to eq(1)
+        end
+
+        it 'bottom' do
+          newbie = MultipleFieldsOrderable.create!(group_id: 2)
+          record = MultipleFieldsOrderable.find(newbie._id)
+          record.move_groups_to! :bottom
+          expect(all_groups).to eq([1, 2, 1, 2, 3, 4])
+          expect(record.groups).to eq(4)
+        end
+
+        it 'middle' do
+          newbie = MultipleFieldsOrderable.create!(group_id: 2)
+          record = MultipleFieldsOrderable.find(newbie._id)
+          record.move_groups_to! 2
+          expect(all_groups).to eq([1, 2, 1, 2, 3, 4])
+          expect(record.groups).to eq(2)
+        end
+      end
+
       describe 'scope movement' do
         let(:record) { MultipleFieldsOrderable.where(group_id: 2, groups: 2).first }
 

spec/integration/simple_spec.rb

@@ -85,8 +85,10 @@ def positions
         expect(another.position).to eq(6)
         newbie.save!
         expect(positions).to eq([1, 2, 3, 4, 5, 6, 7])
-        expect(newbie.position).to eq(7)
+        expect(newbie.position).to eq(6)
+        expect(newbie.reload.position).to eq(6)
         expect(another.position).to eq(6)
+        expect(another.reload.position).to eq(7)
       end
 
       it 'parallel updates' do
@@ -95,8 +97,41 @@ def positions
         another = SimpleOrderable.create!
         newbie.save!
         expect(positions).to eq([1, 2, 3, 4, 5, 6, 7])
-        expect(newbie.position).to eq(7)
+        expect(newbie.position).to eq(6)
+        expect(newbie.reload.position).to eq(6)
         expect(another.position).to eq(6)
+        expect(another.reload.position).to eq(7)
+      end
+
+      it 'with correct specific position as a number' do
+        record = SimpleOrderable.create!(position: 3)
+        expect(record.position).to eq(3)
+        expect(record.reload.position).to eq(3)
+      end
+
+      it 'with incorrect specific position as a number' do
+        record = SimpleOrderable.create!(position: -4)
+        expect(record.position).to eq(1)
+        expect(record.reload.position).to eq(1)
+      end
+
+      it 'with correct specific position as a string' do
+        record = SimpleOrderable.create!(position: '4')
+        expect(record.position).to eq(4)
+        expect(record.reload.position).to eq(4)
+      end
+
+      it 'with incorrect specific position as a string' do
+        record = SimpleOrderable.create!(position: '-4')
+        expect(record.position).to eq(1)
+        expect(record.reload.position).to eq(1)
+      end
+
+      it 'should offset the positions of all the next elements' do
+        records = SimpleOrderable.all
+        expect(records.pluck(:position)).to eq([1, 2, 3, 4, 5])
+        SimpleOrderable.create!(position: 3)
+        expect(records.pluck(:position)).to eq([1, 2, 4, 5, 6, 3])
       end
     end