256 track all blank changes (#257)

* 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: <id>/') line.gsub!(/BSON::ObjectId$'[[:xdigit:]]{24}'$/, '<id>') 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. * 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, <blank thing>] and [<blank thing>, nil] changes. - <blank thing> 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. * 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 `<main>' 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: * 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. * 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. * Update CHANGLOG * Update version to 0.9.0 per request * Update the internal version number as well * 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... * Update per request * Update per request
mongoid · Aug 23, 2024 · c8c4de1 · c8c4de1
1 parent f01173b
commit c8c4de1
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 35 deletions.
diff --git 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/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,6 @@
-### 0.8.6 (Next)
+### 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)

diff --git 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
diff --git 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.
diff --git 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?

diff --git 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
 

diff --git 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
diff --git 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

diff --git 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: {} },
@@ -354,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] }

diff --git 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: [],