diff --git a/CHANGELOG.md b/CHANGELOG.md index 246dc5e..a806e68 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/mongoid/orderable/handlers/base.rb b/lib/mongoid/orderable/handlers/base.rb index ce73fdf..67d8fc8 100644 --- a/lib/mongoid/orderable/handlers/base.rb +++ b/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)) diff --git a/lib/mongoid/orderable/handlers/document.rb b/lib/mongoid/orderable/handlers/document.rb index f7886c2..550a46b 100644 --- a/lib/mongoid/orderable/handlers/document.rb +++ b/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 diff --git a/lib/mongoid/orderable/handlers/document_transactional.rb b/lib/mongoid/orderable/handlers/document_transactional.rb index abdaac5..5264a18 100644 --- a/lib/mongoid/orderable/handlers/document_transactional.rb +++ b/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 diff --git a/lib/mongoid/orderable/mixins/callbacks.rb b/lib/mongoid/orderable/mixins/callbacks.rb index 9d86e14..305bc62 100644 --- a/lib/mongoid/orderable/mixins/callbacks.rb +++ b/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 diff --git a/lib/mongoid/orderable/mixins/movable.rb b/lib/mongoid/orderable/mixins/movable.rb index 222fe25..24ec3c2 100644 --- a/lib/mongoid/orderable/mixins/movable.rb +++ b/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 diff --git a/lib/mongoid/orderable/version.rb b/lib/mongoid/orderable/version.rb index 7f90781..4b6132e 100644 --- a/lib/mongoid/orderable/version.rb +++ b/lib/mongoid/orderable/version.rb @@ -2,6 +2,6 @@ module Mongoid module Orderable - VERSION = '6.0.4' + VERSION = '6.0.5' end end diff --git a/lib/mongoid_orderable.rb b/lib/mongoid_orderable.rb index 2c0caf8..6e14589 100644 --- a/lib/mongoid_orderable.rb +++ b/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' diff --git a/spec/integration/multiple_fields_spec.rb b/spec/integration/multiple_fields_spec.rb index 942753a..3daa685 100644 --- a/spec/integration/multiple_fields_spec.rb +++ b/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 } diff --git a/spec/integration/simple_spec.rb b/spec/integration/simple_spec.rb index 1b2afa8..74a00cb 100644 --- a/spec/integration/simple_spec.rb +++ b/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