diff --git a/CHANGELOG.md b/CHANGELOG.md index 44b1cf29..83ee9ebd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * [#239](https://github.com/mongoid/mongoid-history/pull/239): Optimize `modified_attributes_for_create` 6-7x - [@getaroom](https://github.com/getaroom). * [#240](https://github.com/mongoid/mongoid-history/pull/240): `Mongoid::History.disable` and `disable_tracking` now restore the original state - [@getaroom](https://github.com/getaroom). * [#240](https://github.com/mongoid/mongoid-history/pull/240): Added `Mongoid::History.enable`, `Mongoid::History.enable!`, `Mongoid::History.disable!`, `enable_tracking`, `enable_tracking!`, and `disable_tracking!` - [@getaroom](https://github.com/getaroom). +* [#248](https://github.com/mongoid/mongoid-history/pull/248): Don't update version on embedded documents if an ancestor is being destroyed in the same operation - [@getaroom](https://github.com/getaroom). ### 0.8.2 (2019/12/02) diff --git a/lib/mongoid/history/trackable.rb b/lib/mongoid/history/trackable.rb index 89610863..b735ffc6 100644 --- a/lib/mongoid/history/trackable.rb +++ b/lib/mongoid/history/trackable.rb @@ -139,6 +139,10 @@ def _create_relation(name, value) private + def ancestor_flagged_for_destroy?(doc) + doc && (doc.flagged_for_destroy? || ancestor_flagged_for_destroy?(doc._parent)) + end + def get_versions_criteria(options_or_version) if options_or_version.is_a? Hash options = options_or_version @@ -183,9 +187,7 @@ def related_scope end def traverse_association_chain(node = self) - list = node._parent ? traverse_association_chain(node._parent) : [] - list << association_hash(node) - list + (node._parent ? traverse_association_chain(node._parent) : []).tap { |list| list << association_hash(node) } end def association_hash(node = self) @@ -269,10 +271,7 @@ def track_destroy(&block) end def clear_trackable_memoization - @history_tracker_attributes = nil - @modified_attributes_for_create = nil - @modified_attributes_for_update = nil - @history_tracks = nil + @history_tracker_attributes = @modified_attributes_for_create = @modified_attributes_for_update = @history_tracks = nil end # Transform hash of pair of changes into an `original` and `modified` hash @@ -312,10 +311,12 @@ def expand_nested_document_key_value(document_key, value) expanded_key end + def next_version + (send(history_trackable_options[:version_field]) || 0) + 1 + end + def increment_current_version - current_version = (send(history_trackable_options[:version_field]) || 0) + 1 - send("#{history_trackable_options[:version_field]}=", current_version) - current_version + next_version.tap { |version| send("#{history_trackable_options[:version_field]}=", version) } end protected @@ -326,7 +327,7 @@ def track_history_for_action?(action) def track_history_for_action(action) if track_history_for_action?(action) - current_version = increment_current_version + current_version = ancestor_flagged_for_destroy?(_parent) ? next_version : increment_current_version last_track = self.class.tracker_class.create!( history_tracker_attributes(action.to_sym) .merge(version: current_version, action: action.to_s, trackable: self) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9b670a15..935728f0 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -27,4 +27,6 @@ config.before :each do Mongoid::History.reset! end + + config.include ErrorHelpers end diff --git a/spec/support/error_helpers.rb b/spec/support/error_helpers.rb new file mode 100644 index 00000000..9538701c --- /dev/null +++ b/spec/support/error_helpers.rb @@ -0,0 +1,7 @@ +module ErrorHelpers + def ignore_errors + yield + rescue StandardError => e + Mongoid.logger.debug "ignored error #{e}" + end +end diff --git a/spec/unit/trackable_spec.rb b/spec/unit/trackable_spec.rb index d85acb68..bcaf4e00 100644 --- a/spec/unit/trackable_spec.rb +++ b/spec/unit/trackable_spec.rb @@ -15,6 +15,26 @@ class MyDynamicModel include Mongoid::Attributes::Dynamic unless Mongoid::Compatibility::Version.mongoid3? end + class MyDeeplyNestedModel + include Mongoid::Document + include Mongoid::History::Trackable + + embeds_many :children, class_name: 'MyNestableModel', cascade_callbacks: true # The problem only occurs if callbacks are cascaded + accepts_nested_attributes_for :children, allow_destroy: true + track_history modifier_field: nil + end + + class MyNestableModel + include Mongoid::Document + include Mongoid::History::Trackable + + embedded_in :parent, class_name: 'MyDeeplyNestedModel' + embeds_many :children, class_name: 'MyNestableModel', cascade_callbacks: true + accepts_nested_attributes_for :children, allow_destroy: true + field :name, type: String + track_history modifier_field: nil + end + class HistoryTracker include Mongoid::History::Tracker end @@ -29,6 +49,8 @@ class User Object.send(:remove_const, :MyDynamicModel) Object.send(:remove_const, :HistoryTracker) Object.send(:remove_const, :User) + Object.send(:remove_const, :MyDeeplyNestedModel) + Object.send(:remove_const, :MyNestableModel) end let(:user) { User.create! } @@ -322,24 +344,14 @@ class MySubModel < MyModel end it 'should be rescued if an exception occurs in disable_tracking' do - begin - MyModel.disable_tracking do - raise 'exception' - end - rescue - end + ignore_errors { MyModel.disable_tracking { raise 'exception' } } expect(Mongoid::History.enabled?).to eq(true) expect(MyModel.new.track_history?).to eq(true) end it 'should be rescued if an exception occurs in enable_tracking' do MyModel.disable_tracking do - begin - MyModel.enable_tracking do - raise 'exception' - end - rescue - end + ignore_errors { MyModel.enable_tracking { raise 'exception' } } expect(Mongoid::History.enabled?).to eq(true) expect(MyModel.new.track_history?).to eq(false) end @@ -436,12 +448,7 @@ class MyModel2 it 'should be rescued if an exception occurs in disable' do Mongoid::History.disable do - begin - MyModel.disable_tracking do - raise 'exception' - end - rescue - end + ignore_errors { MyModel.disable_tracking { raise 'exception' } } expect(Mongoid::History.enabled?).to eq(false) expect(MyModel.new.track_history?).to eq(false) end @@ -449,12 +456,7 @@ class MyModel2 it 'should be rescued if an exception occurs in enable' do Mongoid::History.disable do - begin - Mongoid::History.enable do - raise 'exception' - end - rescue - end + ignore_errors { Mongoid::History.enable { raise 'exception' } } expect(Mongoid::History.enabled?).to eq(false) expect(MyModel.new.track_history?).to eq(false) end @@ -500,14 +502,7 @@ class MyModel2 end it 'should rescue errors through both local and global tracking scopes' do - begin - Mongoid::History.disable do - MyModel.disable_tracking do - raise 'exception' - end - end - rescue - end + ignore_errors { Mongoid::History.disable { MyModel.disable_tracking { raise 'exception' } } } expect(Mongoid::History.enabled?).to eq(true) expect(MyModel.new.track_history?).to eq(true) end @@ -698,13 +693,9 @@ class ModelTwo end end - after :each do - Object.send(:remove_const, :ModelTwo) - end + after(:each) { Object.send(:remove_const, :ModelTwo) } - before :each do - ModelTwo.history_settings paranoia_field: :neglected_at - end + before(:each) { ModelTwo.history_settings paranoia_field: :neglected_at } it { expect(Mongoid::History.trackable_settings[:ModelTwo]).to eq(paranoia_field: 'nglt') } end @@ -716,9 +707,7 @@ class MyTrackerClass end end - after :each do - Object.send(:remove_const, :MyTrackerClass) - end + after(:each) { Object.send(:remove_const, :MyTrackerClass) } context 'when options contain tracker_class_name' do context 'when underscored' do @@ -767,9 +756,7 @@ class EmbOne Object.send(:remove_const, :EmbOne) end - before :each do - model_one.save! - end + before(:each) { model_one.save! } let(:model_one) { ModelOne.new(foo: 'Foo') } let(:changes) { {} } @@ -832,9 +819,7 @@ class EmbOne end describe '#track_update' do - before :each do - MyModel.track_history(on: :foo, track_update: true) - end + before(:each) { MyModel.track_history(on: :foo, track_update: true) } let!(:m) { MyModel.create!(foo: 'bar', modifier: user) } @@ -851,9 +836,7 @@ class EmbOne end describe '#track_destroy' do - before :each do - MyModel.track_history(on: :foo, track_destroy: true) - end + before(:each) { MyModel.track_history(on: :foo, track_destroy: true) } let!(:m) { MyModel.create!(foo: 'bar', modifier: user) } @@ -867,6 +850,58 @@ class EmbOne expect { m.destroy }.to raise_error(StandardError) end.to change(Tracker, :count).by(0) end + + context 'with a deeply nested model' do + let(:m) do + MyDeeplyNestedModel.create!( + children: [ + MyNestableModel.new( + name: 'grandparent', + children: [ + MyNestableModel.new(name: 'parent 1', children: [MyNestableModel.new(name: 'child 1')]), + MyNestableModel.new(name: 'parent 2', children: [MyNestableModel.new(name: 'child 2')]) + ] + ) + ] + ) + end + let(:attributes) do + { + 'children_attributes' => [ + { + 'id' => m.children[0].id, + 'children_attributes' => [ + { 'id' => m.children[0].children[0].id, '_destroy' => '0' }, + { 'id' => m.children[0].children[1].id, '_destroy' => '1' } + ] + } + ] + } + end + + subject(:updated) do + m.update_attributes attributes + m.reload + end + + let(:names_of_destroyed) do + MyDeeplyNestedModel.tracker_class + .where('association_chain.id' => updated.id, 'action' => 'destroy') + .map { |track| track.original['name'] } + end + + it 'does not corrupt embedded models' do + expect(updated.children[0].children.count).to eq 1 # When the problem occurs, the 2nd child will continue to be present, but will only contain the version attribute + end + + it 'creates a history track for the doc explicitly destroyed' do + expect(names_of_destroyed).to include 'parent 2' + end + + it 'creates a history track for the doc implicitly destroyed' do + expect(names_of_destroyed).to include 'child 2' + end + end end describe '#track_create' do @@ -879,9 +914,7 @@ class MyModelWithNoModifier end end - after :each do - Object.send(:remove_const, :MyModelWithNoModifier) - end + after(:each) { Object.send(:remove_const, :MyModelWithNoModifier) } before :each do MyModel.track_history(on: :foo, track_create: true) @@ -919,9 +952,7 @@ class Fish end end - after :each do - Object.send(:remove_const, :Fish) - end + after(:each) { Object.send(:remove_const, :Fish) } it 'should track history' do expect do @@ -947,9 +978,7 @@ def my_changes MyModel.history_trackable_options end - after :each do - Object.send(:remove_const, :CustomTracker) - end + after(:each) { Object.send(:remove_const, :CustomTracker) } it 'should not override in parent class' do expect(MyModel.history_trackable_options[:changes_method]).to eq :changes