From 9fee5b2679275bc755e44ed818c5b14ad8cc0e8c Mon Sep 17 00:00:00 2001 From: Chris Griego Date: Wed, 1 Apr 2020 11:55:48 -0500 Subject: [PATCH 1/2] Fix tracking subclasses with additional fields --- CHANGELOG.md | 1 + lib/mongoid/history/options.rb | 78 ++++++++++++++--------------- lib/mongoid/history/trackable.rb | 6 ++- spec/integration/subclasses_spec.rb | 10 +++- spec/unit/options_spec.rb | 6 +++ spec/unit/trackable_spec.rb | 21 ++++++++ 6 files changed, 80 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c63e62d..1995c27c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ * Your contribution here. * [#236](https://github.com/mongoid/mongoid-history/pull/236): Fix Ruby 2.7 keyword argument warnings - [@vasilysn](https://github.com/vasilysn). +* [#237](https://github.com/mongoid/mongoid-history/pull/237): Fix tracking subclasses with additional fields - [@getaroom](https://github.com/getaroom). ### 0.8.2 (2019/12/02) diff --git a/lib/mongoid/history/options.rb b/lib/mongoid/history/options.rb index 8a382348..32cb1faf 100644 --- a/lib/mongoid/history/options.rb +++ b/lib/mongoid/history/options.rb @@ -14,28 +14,28 @@ def scope def prepared @prepared ||= begin + @prepared = options.dup prepare_skipped_fields prepare_formatted_fields parse_tracked_fields_and_relations - options + @prepared end end private def default_options - @default_options ||= - { on: :all, - except: %i[created_at updated_at], - tracker_class_name: nil, - modifier_field: :modifier, - version_field: :version, - changes_method: :changes, - scope: scope, - track_create: true, - track_update: true, - track_destroy: true, - format: nil } + { on: :all, + except: %i[created_at updated_at], + tracker_class_name: nil, + modifier_field: :modifier, + version_field: :version, + changes_method: :changes, + scope: scope, + track_create: true, + track_update: true, + track_destroy: true, + format: nil } end # Sets the :except attributes and relations in `options` to be an [ Array ] @@ -43,15 +43,15 @@ def default_options # Removes the `nil` and duplicate entries from skipped attributes/relations list def prepare_skipped_fields # normalize :except fields to an array of database field strings - @options[:except] = Array(options[:except]) - @options[:except] = options[:except].map { |field| trackable.database_field_name(field) }.compact.uniq + @prepared[:except] = Array(@prepared[:except]) + @prepared[:except] = @prepared[:except].map { |field| trackable.database_field_name(field) }.compact.uniq end def prepare_formatted_fields formats = {} - if options[:format].class == Hash - options[:format].each do |field, format| + if @prepared[:format].class == Hash + @prepared[:format].each do |field, format| next if field.nil? field = trackable.database_field_name(field) @@ -68,7 +68,7 @@ def prepare_formatted_fields end end - options[:format] = formats + @prepared[:format] = formats end def parse_tracked_fields_and_relations @@ -76,25 +76,25 @@ def parse_tracked_fields_and_relations # when `posts: [:id, :title]`, then it will convert it to `[[:posts, [:id, :title]]]` # when `:foo`, then `[:foo]` # when `[:foo, { posts: [:id, :title] }]`, then return as is - @options[:on] = Array(options[:on]) + @prepared[:on] = Array(@prepared[:on]) - @options[:on] = options[:on].map { |opt| opt == :all ? :fields : opt } + @prepared[:on] = @prepared[:on].map { |opt| opt == :all ? :fields : opt } - if options[:on].include?(:fields) - @options[:on] = options[:on].reject { |opt| opt == :fields } - @options[:on] = options[:on] | trackable.fields.keys.map(&:to_sym) - reserved_fields.map(&:to_sym) + if @prepared[:on].include?(:fields) + @prepared[:on] = @prepared[:on].reject { |opt| opt == :fields } + @prepared[:on] = @prepared[:on] | trackable.fields.keys.map(&:to_sym) - reserved_fields.map(&:to_sym) end - if options[:on].include?(:embedded_relations) - @options[:on] = options[:on].reject { |opt| opt == :embedded_relations } - @options[:on] = options[:on] | trackable.embedded_relations.keys + if @prepared[:on].include?(:embedded_relations) + @prepared[:on] = @prepared[:on].reject { |opt| opt == :embedded_relations } + @prepared[:on] = @prepared[:on] | trackable.embedded_relations.keys end - @options[:fields] = [] - @options[:dynamic] = [] - @options[:relations] = { embeds_one: {}, embeds_many: {} } + @prepared[:fields] = [] + @prepared[:dynamic] = [] + @prepared[:relations] = { embeds_one: {}, embeds_many: {} } - options[:on].each do |option| + @prepared[:on].each do |option| if option.is_a?(Hash) option.each { |k, v| split_and_categorize(k => v) } else @@ -145,7 +145,7 @@ def get_field_options(option) # @param [ String ] field The database field name of field or relation to track # @param [ nil | Array ] field_options The tracked fields for embedded relations def categorize_tracked_option(field, field_options = nil) - return if options[:except].include?(field) + return if @prepared[:except].include?(field) return if reserved_fields.include?(field) field_options = Array(field_options) @@ -155,23 +155,23 @@ def categorize_tracked_option(field, field_options = nil) elsif trackable.embeds_many?(field) track_relation(field, :embeds_many, field_options) elsif trackable.fields.keys.include?(field) - @options[:fields] << field + @prepared[:fields] << field else - @options[:dynamic] << field + @prepared[:dynamic] << field end end def track_relation(field, kind, field_options) relation_class = trackable.relation_class_of(field) - @options[:relations][kind][field] = if field_options.blank? - relation_class.fields.keys - else - %w[_id] | field_options.map { |opt| relation_class.database_field_name(opt) } - end + @prepared[:relations][kind][field] = if field_options.blank? + relation_class.fields.keys + else + %w[_id] | field_options.map { |opt| relation_class.database_field_name(opt) } + end end def reserved_fields - @reserved_fields ||= ['_id', '_type', options[:version_field].to_s, "#{options[:modifier_field]}_id"] + @reserved_fields ||= ['_id', '_type', @prepared[:version_field].to_s, "#{@prepared[:modifier_field]}_id"] end end end diff --git a/lib/mongoid/history/trackable.rb b/lib/mongoid/history/trackable.rb index 6a8ab9d2..f9753995 100644 --- a/lib/mongoid/history/trackable.rb +++ b/lib/mongoid/history/trackable.rb @@ -559,7 +559,11 @@ def clear_trackable_memoization @tracked_fields = nil @tracked_embeds_one = nil @tracked_embeds_many = nil - @obfuscated_fields = nil + end + + def inherited(subclass) + super + subclass.mongoid_history_options = Mongoid::History::Options.new(subclass, mongoid_history_options.options) end end end diff --git a/spec/integration/subclasses_spec.rb b/spec/integration/subclasses_spec.rb index fe092b24..2171690d 100644 --- a/spec/integration/subclasses_spec.rb +++ b/spec/integration/subclasses_spec.rb @@ -7,12 +7,16 @@ class Element include Mongoid::Timestamps include Mongoid::History::Trackable + track_history + field :body - track_history on: [:body] + # force preparation of options + history_trackable_options end class Prompt < Element + field :head end class User @@ -31,11 +35,13 @@ class User it 'tracks subclass create and update' do prompt = Prompt.new(modifier: user) expect { prompt.save! }.to change(Tracker, :count).by(1) - expect { prompt.update_attributes!(body: 'one') }.to change(Tracker, :count).by(1) + expect { prompt.update_attributes!(body: 'one', head: 'two') }.to change(Tracker, :count).by(1) prompt.undo! user expect(prompt.body).to be_blank + expect(prompt.head).to be_blank prompt.redo! user, 2 expect(prompt.body).to eq('one') + expect(prompt.head).to eq('two') expect { prompt.destroy }.to change(Tracker, :count).by(1) end end diff --git a/spec/unit/options_spec.rb b/spec/unit/options_spec.rb index 4fd7b5f4..c579f634 100644 --- a/spec/unit/options_spec.rb +++ b/spec/unit/options_spec.rb @@ -84,6 +84,12 @@ class EmbFour end describe '#parse' do + it 'does not mutate the original options' do + original_options = service.options.dup + service.prepared + expect(service.options).to eq original_options + end + describe '#default_options' do let(:expected_options) do { diff --git a/spec/unit/trackable_spec.rb b/spec/unit/trackable_spec.rb index 0a443741..8f79a4fa 100644 --- a/spec/unit/trackable_spec.rb +++ b/spec/unit/trackable_spec.rb @@ -836,4 +836,25 @@ def my_changes m.save! end end + + context "subclassing a #{described_class}" do + before :each do + MyModel.track_history(track_destroy: false) + + class MySubclassModel < MyModel + end + end + + after :each do + Object.send(:remove_const, :MySubclassModel) + end + + describe '.inherited' do + it 'creates new history options for the subclass' do + options = MySubclassModel.mongoid_history_options + expect(options.trackable).to eq MySubclassModel + expect(options.options).to eq MyModel.mongoid_history_options.options + end + end + end end From c00e6c73b40485cd19363d46e51fe33a107bf9c3 Mon Sep 17 00:00:00 2001 From: Chris Griego Date: Wed, 1 Apr 2020 15:25:21 -0500 Subject: [PATCH 2/2] Simplify by using return instead of ||= begin/end --- lib/mongoid/history/options.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/mongoid/history/options.rb b/lib/mongoid/history/options.rb index 32cb1faf..cf9b3808 100644 --- a/lib/mongoid/history/options.rb +++ b/lib/mongoid/history/options.rb @@ -13,13 +13,12 @@ def scope end def prepared - @prepared ||= begin - @prepared = options.dup - prepare_skipped_fields - prepare_formatted_fields - parse_tracked_fields_and_relations - @prepared - end + return @prepared if @prepared + @prepared = options.dup + prepare_skipped_fields + prepare_formatted_fields + parse_tracked_fields_and_relations + @prepared end private