From 3faefef599455632abd743e09bb6efb8d24aca0e Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Wed, 8 Nov 2023 02:01:39 +0900 Subject: [PATCH 1/2] Failing specs for embedded doc position updating --- spec/integration/embedded_spec.rb | 190 ++++++++++++++++++++++++++++++ 1 file changed, 190 insertions(+) diff --git a/spec/integration/embedded_spec.rb b/spec/integration/embedded_spec.rb index 5c00d62..ef1b9eb 100644 --- a/spec/integration/embedded_spec.rb +++ b/spec/integration/embedded_spec.rb @@ -187,6 +187,196 @@ def positions expect(child2.reload.position).to eq(1) end end + + context '#save! on parent' do + let!(:parent) { EmbedsOrderable.last } + let!(:child1) { parent.embedded_orderables.first } + let!(:child2) { parent.embedded_orderables.second } + let!(:child3) { parent.embedded_orderables.third } + + it 'sets existing item to top' do + child3.position = 1 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 1] + end + + it 'sets existing item above top' do + child3.position = 0 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 0] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 1] + end + + it 'sets existing item to middle' do + child3.position = 2 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 2] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 2] + end + + it 'sets existing item to bottom' do + child1.position = 3 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [3, 2, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [3, 1, 2] + end + + it 'sets existing item below bottom' do + child1.position = 4 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [4, 2, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [3, 1, 2] + end + + it 'sets existing item with nil position' do + child2.position = nil + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 4, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 2] + end + + it 'saves new item with position set to top' do + child4 = parent.embedded_orderables.build + child4.position = 1 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + end + + it 'saves new item with position set above top' do + child4 = parent.embedded_orderables.build + child4.position = 0 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + end + + it 'saves new item with position set to middle' do + child4 = parent.embedded_orderables.build + child4.position = 2 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 3, 4, 2] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 4, 2] + end + + it 'saves new item with position set to bottom' do + child4 = parent.embedded_orderables.build + child4.position = 4 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 3, 5] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + + it 'saves new item with position set below bottom' do + child4 = parent.embedded_orderables.build + child4.position = 5 + parent.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + + it 'saves new item with nil position' do + parent.embedded_orderables.build + parent.save! + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + end + + context '#save! on child' do + let!(:parent) { EmbedsOrderable.last } + let!(:child1) { parent.embedded_orderables.first } + let!(:child2) { parent.embedded_orderables.second } + let!(:child3) { parent.embedded_orderables.third } + + it 'sets existing item to top' do + child3.position = 1 + child3.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 1] + end + + it 'sets existing item above top' do + child3.position = 0 + child3.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 0] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 1] + end + + it 'sets existing item to middle' do + child3.position = 2 + child3.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 2] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 2] + end + + it 'sets existing item to bottom' do + child1.position = 3 + child1.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [3, 2, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [3, 1, 2] + end + + it 'sets existing item below bottom' do + child1.position = 4 + child1.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [4, 2, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [3, 1, 2] + end + + it 'sets existing item with nil position' do + child2.position = nil + child2.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 4, 3] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 2] + end + + it 'saves new item with position set to top' do + child4 = parent.embedded_orderables.build + child4.position = 1 + child4.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + end + + it 'saves new item with position set above top' do + child4 = parent.embedded_orderables.build + child4.position = 0 + child4.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [2, 3, 4, 1] + end + + it 'saves new item with position set to middle' do + child4 = parent.embedded_orderables.build + child4.position = 2 + child4.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 3, 4, 2] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 3, 4, 2] + end + + it 'saves new item with position set to bottom' do + child4 = parent.embedded_orderables.build + child4.position = 4 + child4.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 3, 5] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + + it 'saves new item with position set below bottom' do + child4 = parent.embedded_orderables.build + child4.position = 5 + child4.save! + # expect(parent.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + + it 'saves new item with nil position' do + child4 = parent.embedded_orderables.build + child4.save! + expect(parent.reload.embedded_orderables.map(&:position)).to eq [1, 2, 3, 4] + end + end end context 'with transactions' do From b289cbbb608ac44b883fe3c2c8bbcba51add74b4 Mon Sep 17 00:00:00 2001 From: johnnyshields <27655+johnnyshields@users.noreply.github.com> Date: Wed, 8 Nov 2023 03:03:22 +0900 Subject: [PATCH 2/2] This branch demonstrates an experimental approach whereby move_to can be replaced with the position field directly. There are a few edge case specs to fix but overall it has potential to be much cleaner than current. --- lib/mongoid/orderable/generators/position.rb | 9 +++++++++ lib/mongoid/orderable/handlers/base.rb | 16 ++++++++++++---- lib/mongoid/orderable/handlers/document.rb | 3 ++- .../orderable/handlers/document_transactional.rb | 2 +- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/lib/mongoid/orderable/generators/position.rb b/lib/mongoid/orderable/generators/position.rb index f58bd1e..e68d2d3 100644 --- a/lib/mongoid/orderable/generators/position.rb +++ b/lib/mongoid/orderable/generators/position.rb @@ -10,12 +10,21 @@ def orderable_position(field = nil) field ||= default_orderable_field send("orderable_\#{field}_position") end + + def orderable_position_was(field = nil) + field ||= default_orderable_field + send("orderable_\#{field}_position_was") + end KLASS generate_method("orderable_#{field_name}_position") do send(field_name) end + generate_method("orderable_#{field_name}_position_was") do + send("#{field_name}_was") + end + generate_method("orderable_#{field_name}_position=") do |value| send("#{field_name}=", value) end diff --git a/lib/mongoid/orderable/handlers/base.rb b/lib/mongoid/orderable/handlers/base.rb index dbb7a39..f497780 100644 --- a/lib/mongoid/orderable/handlers/base.rb +++ b/lib/mongoid/orderable/handlers/base.rb @@ -16,6 +16,7 @@ def initialize(doc) delegate :orderable_keys, :orderable_field, :orderable_position, + :orderable_position_was, :orderable_if, :orderable_unless, :orderable_scope, @@ -37,12 +38,10 @@ def use_transactions end def any_field_changed? - orderable_keys.any? {|field| changed?(field) } + orderable_keys.any? {|field| changed?(field) } || move_all.any? end - def set_new_record_positions - return unless new_record? - + def set_target_positions orderable_keys.each do |field| next unless (position = doc.send(field)) @@ -74,6 +73,8 @@ def apply_one_position(field, target_position) nil elsif persisted? && !embedded? scope.where(_id: _id).pluck(f).first + elsif persisted? && embedded? + orderable_position_was(field) else orderable_position(field) end @@ -86,16 +87,23 @@ def apply_one_position(field, target_position) # Return if there is no instruction to change the position in_list = persisted? && current + puts 'uuu' + puts persisted?.inspect + puts current.inspect + puts target_position.inspect return if in_list && !target_position target = resolve_target_position(field, target_position, in_list) # Use $inc operator to shift the position of the other documents if !in_list + puts '111' scope.gte(f => target).inc(f => 1) elsif target < current + puts '222' scope.where(f => { '$gte' => target, '$lt' => current }).inc(f => 1) elsif target > current + puts '333' scope.where(f => { '$gt' => current, '$lte' => target }).inc(f => -1) end diff --git a/lib/mongoid/orderable/handlers/document.rb b/lib/mongoid/orderable/handlers/document.rb index 550a46b..6ac6709 100644 --- a/lib/mongoid/orderable/handlers/document.rb +++ b/lib/mongoid/orderable/handlers/document.rb @@ -5,7 +5,7 @@ module Orderable module Handlers class Document < Base def before_create - set_new_record_positions + set_target_positions apply_all_positions end @@ -15,6 +15,7 @@ def after_create def before_update return unless any_field_changed? + set_target_positions apply_all_positions end diff --git a/lib/mongoid/orderable/handlers/document_transactional.rb b/lib/mongoid/orderable/handlers/document_transactional.rb index 40b4ecc..71df949 100644 --- a/lib/mongoid/orderable/handlers/document_transactional.rb +++ b/lib/mongoid/orderable/handlers/document_transactional.rb @@ -5,7 +5,7 @@ module Orderable module Handlers class DocumentTransactional < Document def before_create - set_new_record_positions + set_target_positions end def after_create