From 52cc18096098f8a3b6ba3796be16d844c72e7223 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 5 Aug 2024 11:24:12 -0700 Subject: [PATCH 01/11] Add some tests for track_blank_changes option This is mostly a "get my feet wet" commit, but it adds some tests for the track_blank_changes option being present and having the right default. As I haven't added track_blank_changes as an option yet, these tests fail as expected. I did a rake with out-of-the-box code and saved the results in testresults/0-baseline.txt, after running them through this script: #!/usr/bin/env ruby until (line = gets).nil? line.gsub!(/_id: [[:xdigit:]]{24}/, '_id: /') line.gsub!(/BSON::ObjectId\('[[:xdigit:]]{24}'\)/, '') line.gsub!(/[\d.]+/, 'n') if line.start_with?('Finished in ') puts line end to get rid of _ids and the total run time, both of which change every run. I then ran rake after the changes in this commit and saved the output to testresults/1-track_blank_changes-default-tests.txt. I won't include the entire 197 line diff here, but the important part is: $ diff -u testresults/0-baseline.txt testresults/1-track_blank_changes-default-tests.txt | head --- testresults/0-baseline.txt 2024-08-05 11:17:55.000000000 -0700 +++ testresults/1-track_blank_changes-default-tests.txt 2024-08-05 11:21:01.000000000 -0700 [...] -432 examples, 0 failures, 2 pending +432 examples, 6 failures, 2 pending [...] Those 6 failures are all to be expected given that the track_blank_changes_option doesn't exist yet. There were no rubocop complaints. --- spec/unit/options_spec.rb | 2 ++ spec/unit/trackable_spec.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/spec/unit/options_spec.rb b/spec/unit/options_spec.rb index c579f63..036122d 100644 --- a/spec/unit/options_spec.rb +++ b/spec/unit/options_spec.rb @@ -103,6 +103,7 @@ class EmbFour track_create: true, track_update: true, track_destroy: true, + track_blank_changes: false, format: nil } end @@ -173,6 +174,7 @@ class EmbFour track_create: true, track_update: true, track_destroy: true, + track_blank_changes: false, fields: %w[foo b], dynamic: [], relations: { embeds_one: {}, embeds_many: {} }, diff --git a/spec/unit/trackable_spec.rb b/spec/unit/trackable_spec.rb index 82d09d2..7316131 100644 --- a/spec/unit/trackable_spec.rb +++ b/spec/unit/trackable_spec.rb @@ -90,6 +90,7 @@ class MyModelWithNoModifier track_create: true, track_update: true, track_destroy: true, + track_blank_changes: false, fields: %w[foo], relations: { embeds_one: {}, embeds_many: {} }, dynamic: [], From 8081c34fcf542b549a32f7f141dfe558808631c0 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 5 Aug 2024 13:35:38 -0700 Subject: [PATCH 02/11] Add some more all blank changes tests Originally there was a single test for all blank changes. This commit adds a number more. Now it tests: - Both [nil, ] and [, nil] changes. - was originally just an empty Array. Now it can be: - [] - false - '' - A non-empty String consisting of all whitespace characters - {} - It tests with the track_blank_changes option the default, explicitly false, and explicitly true. The tests with track_blank_changes the default or explicitly false succeed, accidentally for now, as there is no such option and the code acts as if track_blank_changes is false, so tests that rely on that succeed. The tests that set track_blank_changes to true fail, as there's no such option and setting it has no effect on the operation of the code. Here's a diff of the test results with just the important parts, not all 422 lines: $ diff -u testresults/1-track_blank_changes-default-tests.txt testresults/2-addition-all-blank-changes.txt --- testresults/1-track_blank_changes-default-tests.txt 2024-08-05 11:21:01.000000000 -0700 +++ testresults/2-addition-all-blank-changes.txt 2024-08-05 11:34:14.000000000 -0700 -432 examples, 6 failures, 2 pending +455 examples, 14 failures, 2 pending The 6 failures are those introduced by commit 52cc180. The 8 additional failures are introduced in this commit. And, just to verify, I commented out the unless clause on lib/mongoid/history/attributes/update.rb:29, which makes track_blank_changes effectively always true, and the tests that don't set it or set it to false fail (as it's always implicitly true with the commented out unless), and the tests that set it to true succeed (the option isn't being set, there is no option, but the code acts as if it's true), all as I'd expect things to work. --- spec/unit/attributes/update_spec.rb | 90 +++++++++++++++++++---------- 1 file changed, 61 insertions(+), 29 deletions(-) diff --git a/spec/unit/attributes/update_spec.rb b/spec/unit/attributes/update_spec.rb index 5a88cca..8202153 100644 --- a/spec/unit/attributes/update_spec.rb +++ b/spec/unit/attributes/update_spec.rb @@ -305,37 +305,69 @@ class EmbOne end end - context 'when original and modified values blank' do - before :each do - class DummyParent - include Mongoid::Document - include Mongoid::History::Trackable - store_in collection: :dummy_parents - has_and_belongs_to_many :other_dummy_parents - track_history on: :fields - end - - class OtherDummyParent - include Mongoid::Document - has_and_belongs_to_many :dummy_parents - end - end - - before :each do - allow(base).to receive(:changes) { changes } - DummyParent.track_history on: :other_dummy_parent_ids - end + [false, true].each do |original_nil| + context "when original value #{original_nil ? 'nil' : 'blank'} and modified value #{original_nil ? 'blank' : 'nil'}" do + [nil, false, true].each do |track_blank_changes| + context "when track_blank_changes #{track_blank_changes.nil? ? 'default' : track_blank_changes}" do + before :each do + class DummyParent + include Mongoid::Document + include Mongoid::History::Trackable + store_in collection: :dummy_parents + has_and_belongs_to_many :other_dummy_parents + field :boolean, type: Boolean + field :string, type: String + field :hash, type: Hash + end + + class OtherDummyParent + include Mongoid::Document + has_and_belongs_to_many :dummy_parents + end + + if track_blank_changes.nil? + DummyParent.track_history on: :fields + else + DummyParent.track_history \ + on: :fields, + track_blank_changes: track_blank_changes + end + + allow(base).to receive(:changes) { changes } + end - let(:base) { described_class.new(DummyParent.new) } - let(:changes) do - { 'other_dummy_parent_ids' => [nil, []] } - end - subject { base.attributes } - it { expect(subject.keys).to_not include 'other_dummy_parent_ids' } + after :each do + Object.send(:remove_const, :DummyParent) + Object.send(:remove_const, :OtherDummyParent) + end - after :each do - Object.send(:remove_const, :DummyParent) - Object.send(:remove_const, :OtherDummyParent) + let(:base) { described_class.new(DummyParent.new) } + subject { base.attributes.keys } + + # These can't be memoizing methods (i.e. lets) because of limits + # on where those can be used. + + cmp = track_blank_changes ? 'should' : 'should_not' + cmp_name = cmp.humanize capitalize: false + + [ + { n: 'many-to-many', f: 'other_dummy_parent_ids', v: [] }, + { n: 'boolean', f: 'boolean', v: false }, + { n: 'empty string', f: 'string', v: '' }, + { n: 'all whitespace string', f: 'string', v: " \t\n\r\f\v" } + # The second character in that string is an actual tab (0x9). + ].each do |d| + context "#{d[:n]} field" do + let(:changes) do + { d[:f] => original_nil ? [nil, d[:v]] : [d[:v], nil] } + end + it "changes #{cmp_name} include #{d[:f]}" do + send(cmp, include(d[:f])) + end + end + end + end + end end end end From 11179d45b1ab941a6e1f106550d764f27f5e6aa7 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 5 Aug 2024 15:46:58 -0700 Subject: [PATCH 03/11] Test setting track_blank_changes option There are a number of tests that setting an option value actually sets the option to that value, so this commit adds one for track_blank_changes. Unfortunately I added it before discovering that the whole section isn't testing anything other than that setting a value in a Hash and retrieving that value gives the same value back (and that two of tests even do that wrong). The two tests that are wrong are those for :track_create and :track_destroy. Because they're setting the options to the default value, it's not possible to tell if the tests pass because the values are actually being set or because they're not and just the default value is being used. More problematic is that adding a test like: describe ':any_name' do let(:options) { { any_name: 'any_value' } } it { expect(subject[:any_name]).to be 'any_value' } end passes, as long as any_name is something other than a few special option names (like on). None of the tests between lines 319 and 359 are really testing anything useful IMO. Nevertheless, I added the :track_blank_changes test. Here's the diff of the test output vs that generated after commit 8081c34: $ diff -u testresults/2-additional-all-blank-changes.txt testresults/3-set-track_blank_changes.txt --- testresults/2-additional-all-blank-changes.txt 2024-08-05 11:34:14.000000000 -0700 +++ testresults/3-set-track_blank_changes.txt 2024-08-05 15:46:06.000000000 -0700 @@ -591,6 +591,8 @@ is expected to equal false :track_destroy is expected to equal true + :track_blank_changes + is expected to equal true #remove_reserved_fields is expected to eq ["foo"] is expected to eq [] @@ -1076,7 +1078,7 @@ # /Users/blm/.rvm/gems/ruby-2.3.7/gems/rspec-core-3.13.0/exe/rspec:4:in `
' Finished in n minute n seconds (files took n seconds to load) -455 examples, 14 failures, 2 pending +456 examples, 14 failures, 2 pending Failed examples: --- spec/unit/options_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/unit/options_spec.rb b/spec/unit/options_spec.rb index 036122d..170cad9 100644 --- a/spec/unit/options_spec.rb +++ b/spec/unit/options_spec.rb @@ -356,6 +356,11 @@ class EmbFour it { expect(subject[:track_destroy]).to be true } end + describe ':track_blank_changes' do + let(:options) { { track_blank_changes: true } } + it { expect(subject[:track_blank_changes]).to be true } + end + describe '#remove_reserved_fields' do let(:options) { { on: %i[_id _type foo version modifier_id] } } it { expect(subject[:fields]).to eq %w[foo] } From d3275e65da7373613c06df724ae5fe55336dc8c5 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 5 Aug 2024 19:56:53 -0700 Subject: [PATCH 04/11] Add track_blank_changes as default option Time to start fixing some test failures instead of creating them. Add track_blank_changes as a default option, defaulting to false. $ diff -u testresults/3-set-track_blank_changes.txt testresults/4-add-track_blank_changes-option.txt --- testresults/3-set-track_blank_changes.txt 2024-08-05 15:46:06.000000000 -0700 +++ testresults/4-add-track_blank_changes-option.txt 2024-08-05 19:34:48.000000000 -0700 @@ -504,7 +504,7 @@ [...] -456 examples, 14 failures, 2 pending +456 examples, 8 failures, 2 pending [...] The failures fixed are exactly the 6 introduced in commit 52cc180. --- .rubocop_todo.yml | 2 +- lib/mongoid/history/options.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index babce8d..69ccd7e 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -36,7 +36,7 @@ Metrics/BlockLength: # Offense count: 1 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 120 + Max: 121 # Offense count: 6 Metrics/CyclomaticComplexity: diff --git a/lib/mongoid/history/options.rb b/lib/mongoid/history/options.rb index cf9b380..a34cbc5 100644 --- a/lib/mongoid/history/options.rb +++ b/lib/mongoid/history/options.rb @@ -34,6 +34,7 @@ def default_options track_create: true, track_update: true, track_destroy: true, + track_blank_changes: false, format: nil } end From 52352bd822e42d0bbcd0bad1bbaa61a444975024 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 5 Aug 2024 23:43:25 -0700 Subject: [PATCH 05/11] Use track_blank_changes Use the track_blank_changes option. If it's true, any change reported by the change method is tracked, even if both the original and new values are blank (i.e. respond with true to blank?). The tests now all run successfully (and there are no rubocopy complaints, although I did increase the maximum class size in commit d3275e6 as Mongoid::History::Options was exactly at the class size limit). Here are the complete differences between the initial test output and now: $ diff -u testresults/0-baseline.txt testresults/5-use-track_blank_changes-option.txt --- testresults/0-baseline.txt 2024-08-05 11:17:55.000000000 -0700 +++ testresults/5-use-track_blank_changes-option.txt 2024-08-05 20:00:33.000000000 -0700 @@ -320,8 +320,62 @@ should save audit history under relation alias when original and modified value same is expected not to include "emb_ones" - when original and modified values blank - is expected not to include "other_dummy_parent_ids" + when original value blank and modified value nil + when track_blank_changes default + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes false + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes true + many-to-many field + changes should include other_dummy_parent_ids + boolean field + changes should include boolean + empty string field + changes should include string + all whitespace string field + changes should include string + when original value nil and modified value blank + when track_blank_changes default + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes false + many-to-many field + changes should not include other_dummy_parent_ids + boolean field + changes should not include boolean + empty string field + changes should not include string + all whitespace string field + changes should not include string + when track_blank_changes true + many-to-many field + changes should include other_dummy_parent_ids + boolean field + changes should include boolean + empty string field + changes should include string + all whitespace string field + changes should include string Mongoid::History::Options :if @@ -537,6 +591,8 @@ is expected to equal false :track_destroy is expected to equal true + :track_blank_changes + is expected to equal true #remove_reserved_fields is expected to eq ["foo"] is expected to eq [] @@ -824,6 +880,6 @@ # ./spec/unit/attributes/create_spec.rb:340 Finished in n minute n seconds (files took n seconds to load) -432 examples, 0 failures, 2 pending +456 examples, 0 failures, 2 pending [Coveralls] Outside the CI environment, not sending data. 24 new tests, including testing when track_blank_changes is true, and still 0 failures. --- lib/mongoid/history/attributes/update.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/mongoid/history/attributes/update.rb b/lib/mongoid/history/attributes/update.rb index 3276c8d..4bd41d3 100644 --- a/lib/mongoid/history/attributes/update.rb +++ b/lib/mongoid/history/attributes/update.rb @@ -18,6 +18,7 @@ def attributes private def changes_from_parent + track_blank_changes = trackable_class.history_trackable_options[:track_blank_changes] parent_changes = {} changes.each do |k, v| change_value = begin @@ -26,7 +27,7 @@ def changes_from_parent elsif trackable_class.tracked_embeds_many?(k) embeds_many_changes_from_parent(k, v) elsif trackable_class.tracked?(k, :update) - { k => format_field(k, v) } unless v.all?(&:blank?) + { k => format_field(k, v) } unless !track_blank_changes && v.all?(&:blank?) end end parent_changes.merge!(change_value) if change_value.present? From 15a8f10263689643d265102015c704fccfa35caf Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Tue, 6 Aug 2024 00:15:39 -0700 Subject: [PATCH 06/11] Update CHANGLOG --- CHANGELOG.md | 6 +++++- README.md | 31 +++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b55441d..4ea5c64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,11 @@ -### 0.8.6 (Next) +### 0.8.7 (Next) * Your contribution here. +### 0.8.6 (2024/08/07) + +* [#257](https://github.com/mongoid/mongoid-history/pull/257): Add track_blank_changes option - [@BrianLMatthews](https://github.com/BrianLMatthews). + ### 0.8.5 (2021/09/18) * [#250](https://github.com/mongoid/mongoid-history/pull/250): Migrate to Github actions - [@johnnyshields](https://github.com/johnnyshields). diff --git a/README.md b/README.md index 8fca725..08892f5 100644 --- a/README.md +++ b/README.md @@ -75,7 +75,8 @@ class Post :version_field => :version, # adds "field :version, :type => Integer" to track current version, default is :version :track_create => true, # track document creation, default is true :track_update => true, # track document updates, default is true - :track_destroy => true # track document destruction, default is true + :track_destroy => true, # track document destruction, default is true + :track_blank_changes => false # track changes from blank? to blank?, default is false end class Comment @@ -283,6 +284,32 @@ end It will now track only `_id` (Mandatory), `title` and `content` attributes for `pages` relation. +### Track all blank changes + +Normally changes where both the original and modified values respond with `true` to `blank?` (for example `nil` to `false`) aren't tracked. However, there may be cases where it's important to track such changes, for example when a field isn't present (so appears to be `nil`) then is set to `false`. To track such changes, set the `track_blank_changes` option to `true` (it defaults to `false`) when turning on history tracking: + +```ruby +class Book + include Mongoid::Document + ... + field :summary + track_history # Use default of false for track_blank_changes +end + +# summary change not tracked if summary hasn't been set (or has been set to something that responds true to blank?) +Book.find(id).update_attributes(:summary => '') + +class Chapter + include Mongoid::Document + ... + field :title + track_history :track_blank_changes => true +end + +# title change tracked even if title hasn't been set +Chapter.find(id).update_attributes(:title => '') +``` + ### Retrieving the list of tracked static and dynamic fields ```ruby @@ -604,6 +631,6 @@ You're encouraged to contribute to Mongoid History. See [CONTRIBUTING.md](CONTRI ## Copyright -Copyright (c) 2011-2020 Aaron Qian and Contributors. +Copyright (c) 2011-2024 Aaron Qian and Contributors. MIT License. See [LICENSE.txt](LICENSE.txt) for further details. From b3f25e8f5dc6b20a043a0c1ff8bc106661f55044 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Tue, 6 Aug 2024 10:55:38 -0700 Subject: [PATCH 07/11] Update version to 0.9.0 per request --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ea5c64..521945d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,8 @@ -### 0.8.7 (Next) +### 0.9.1 (Next) * Your contribution here. -### 0.8.6 (2024/08/07) +### 0.9.0 (2024/08/07) * [#257](https://github.com/mongoid/mongoid-history/pull/257): Add track_blank_changes option - [@BrianLMatthews](https://github.com/BrianLMatthews). From 1f055bb9c86b89452f056ed9bc9dbad8c37f27e5 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Thu, 8 Aug 2024 12:27:17 -0700 Subject: [PATCH 08/11] Update the internal version number as well --- lib/mongoid/history/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mongoid/history/version.rb b/lib/mongoid/history/version.rb index 44856a0..f68147c 100644 --- a/lib/mongoid/history/version.rb +++ b/lib/mongoid/history/version.rb @@ -1,5 +1,5 @@ module Mongoid module History - VERSION = '0.8.5'.freeze + VERSION = '0.9.0'.freeze end end From 524a6dc352e447227fb4232fca69256f3d283357 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Thu, 8 Aug 2024 13:22:40 -0700 Subject: [PATCH 09/11] Attempt to work around CI failures After exploring the failures locally, it appears that term-ansicolor is the culprit. Version 1.10.0 introduced an undocumented (as far as I can tell, but its CHANGES file stops at 1.7.1 :-( ) dependency on ruby 2.5, by using Hash#slice. So use 1.9.0 you say! Except 1.9.0 doesn't work at all (it's trying to call start_with? on a Symbol). I tried all the commits between 1.9.0 and 1.10.0 and all had some problem or another. Luckily, the latest version needed is 1.3.x (by coveralls), so if I specify that here, that's the one that's installed and bundle rake exec runs fine, so hopefully this will fix the CI failures. We'll see... --- Gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile b/Gemfile index 1223ce6..5b89e4d 100644 --- a/Gemfile +++ b/Gemfile @@ -45,5 +45,6 @@ group :test do gem 'request_store' gem 'rspec', '~> 3.1' gem 'rubocop', '~> 0.49.0' + gem 'term-ansicolor', '~> 1.3.0' gem 'yard' end From 95e05366b28e514254d68865742b8eace876ab21 Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Mon, 19 Aug 2024 11:13:30 -0700 Subject: [PATCH 10/11] Update per request --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 521945d..a5ce98c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ * Your contribution here. -### 0.9.0 (2024/08/07) +### 0.9.0 (Unreleased) * [#257](https://github.com/mongoid/mongoid-history/pull/257): Add track_blank_changes option - [@BrianLMatthews](https://github.com/BrianLMatthews). From a1e113fceba61e935b86b71ab63e00e763b2406a Mon Sep 17 00:00:00 2001 From: "Brian L. Matthews" Date: Wed, 21 Aug 2024 09:50:58 -0700 Subject: [PATCH 11/11] Update per request --- CHANGELOG.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5ce98c..38ce44d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,7 @@ -### 0.9.1 (Next) - -* Your contribution here. - -### 0.9.0 (Unreleased) +### 0.9.0 (Next) * [#257](https://github.com/mongoid/mongoid-history/pull/257): Add track_blank_changes option - [@BrianLMatthews](https://github.com/BrianLMatthews). +* Your contribution here. ### 0.8.5 (2021/09/18)