From 15e899bf9729ff4af8a1bfb1be4386f146c20610 Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Thu, 9 Feb 2023 09:50:08 -0500 Subject: [PATCH 01/23] Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be51fcf0..af1bc87a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Allow using with Padrino and other non-Rails projects - @https://www.npr.org/2023/02/09/1155464337/rebecca-black-leaves-the-meme-in-the-rear-view + [#655](https://github.com/collectiveidea/audited/pull/655) - Testing updates - @jdufresne [#652](https://github.com/collectiveidea/audited/pull/652) [#653](https://github.com/collectiveidea/audited/pull/653) From 69810dce8bbefce2805cfc664582da82b26579b8 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 10:58:14 -0700 Subject: [PATCH 02/23] Audit touch events --- lib/audited/auditor.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index c5e2f44a..9167b47c 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -84,6 +84,7 @@ def audited(options = {}) after_create :audit_create if audited_options[:on].include?(:create) before_update :audit_update if audited_options[:on].include?(:update) + after_touch :audit_touch if audited_options[:on].include?(:touch) before_destroy :audit_destroy if audited_options[:on].include?(:destroy) # Define and set after_audit and around_audit callbacks. This might be useful if you want @@ -245,6 +246,21 @@ def audited_changes filtered_changes.to_hash end + def touch_changes + all_changes = previous_changes + filtered_changes = \ + if audited_options[:only].present? + all_changes.slice(*self.class.audited_columns) + else + all_changes.except(*self.class.non_audited_columns) + end + + filtered_changes = redact_values(filtered_changes) + filtered_changes = filter_encrypted_attrs(filtered_changes) + filtered_changes = normalize_enum_changes(filtered_changes) + filtered_changes.to_hash + end + def normalize_enum_changes(changes) return changes if Audited.store_synthesized_enums @@ -324,6 +340,13 @@ def audit_update end end + def audit_touch + unless (changes = touch_changes).empty? + write_audit(action: "update", audited_changes: changes, + comment: audit_comment) + end + end + def audit_destroy unless new_record? write_audit(action: "destroy", audited_changes: audited_attributes, From a808f8eef39ebf69047041b6a7e0399e8cfcc2c1 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 13:23:20 -0700 Subject: [PATCH 03/23] Simplify --- lib/audited/auditor.rb | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index 9167b47c..c1189cc4 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -231,23 +231,13 @@ def revision_with(attributes) private - def audited_changes - all_changes = respond_to?(:changes_to_save) ? changes_to_save : changes - filtered_changes = \ - if audited_options[:only].present? - all_changes.slice(*self.class.audited_columns) - else - all_changes.except(*self.class.non_audited_columns) - end - - filtered_changes = redact_values(filtered_changes) - filtered_changes = filter_encrypted_attrs(filtered_changes) - filtered_changes = normalize_enum_changes(filtered_changes) - filtered_changes.to_hash - end + def audited_changes(for_touch: false) + all_changes = if for_touch + previous_changes + else + respond_to?(:changes_to_save) ? changes_to_save : changes + end - def touch_changes - all_changes = previous_changes filtered_changes = \ if audited_options[:only].present? all_changes.slice(*self.class.audited_columns) @@ -341,7 +331,7 @@ def audit_update end def audit_touch - unless (changes = touch_changes).empty? + unless (changes = audited_changes(for_touch: true)).empty? write_audit(action: "update", audited_changes: changes, comment: audit_comment) end From 670b9473b31fe4f70b82ce65bd1ff6625f37cf51 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 13:27:12 -0700 Subject: [PATCH 04/23] Include as a default --- lib/audited/auditor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index c1189cc4..803570b2 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -487,7 +487,7 @@ def default_ignored_attributes def normalize_audited_options audited_options[:on] = Array.wrap(audited_options[:on]) - audited_options[:on] = [:create, :update, :destroy] if audited_options[:on].empty? + audited_options[:on] = [:create, :update, :touch, :destroy] if audited_options[:on].empty? audited_options[:only] = Array.wrap(audited_options[:only]).map(&:to_s) audited_options[:except] = Array.wrap(audited_options[:except]).map(&:to_s) max_audits = audited_options[:max_audits] || Audited.max_audits From 6789b9b5705eb61b41237b7e3150304443859b35 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 14:15:27 -0700 Subject: [PATCH 05/23] Add spec coverage --- spec/audited/auditor_spec.rb | 39 ++++++++++++++++++++++++++++ spec/spec_helper.rb | 1 + spec/support/active_record/models.rb | 5 ++++ 3 files changed, 45 insertions(+) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index da04972b..d6e65dc2 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -414,6 +414,45 @@ def non_column_attr=(val) end end + describe "on touch" do + before do + @user = create_user(name: "Brandon", status: :active, audit_comment: "Touch") + end + + around do |example| + freeze_time do + example.run + end + end + + it "should save an audit" do + expect { @user.touch(:suspended_at) }.to change(Audited::Audit, :count).by(1) + end + + it "should set the action to 'update'" do + @user.touch(:suspended_at) + expect(@user.audits.last.action).to eq("update") + expect(Audited::Audit.updates.order(:id).last).to eq(@user.audits.last) + expect(@user.audits.updates.last).to eq(@user.audits.last) + end + + it "should store the changed attributes" do + @user.touch(:suspended_at) + expect(@user.audits.last.audited_changes).to eq({"suspended_at" => [nil, Time.current]}) + end + + it "should store audit comment" do + expect(@user.audits.last.comment).to eq("Touch") + end + + it "should not save an audit if only specified on create/destroy" do + on_create_destroy = Models::ActiveRecord::OnCreateDestroyUser.create(name: "Bart") + expect { + on_create_destroy.touch(:suspended_at) + }.to_not change(Audited::Audit, :count) + end + end + describe "on destroy" do before do @user = create_user(status: :active) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 967b5b85..ae83931c 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -19,6 +19,7 @@ RSpec.configure do |config| config.include AuditedSpecHelpers + config.include ActiveSupport::Testing::TimeHelpers config.use_transactional_fixtures = false if Rails.version.start_with?("4.") config.use_transactional_tests = false if config.respond_to?(:use_transactional_tests=) end diff --git a/spec/support/active_record/models.rb b/spec/support/active_record/models.rb index 8680e2aa..82843fc2 100644 --- a/spec/support/active_record/models.rb +++ b/spec/support/active_record/models.rb @@ -146,6 +146,11 @@ class OnCreateDestroy < ::ActiveRecord::Base audited on: [:create, :destroy] end + class OnCreateDestroyUser < ::ActiveRecord::Base + self.table_name = "users" + audited on: [:create, :destroy] + end + class OnCreateDestroyExceptName < ::ActiveRecord::Base self.table_name = "companies" audited except: :name, on: [:create, :destroy] From 02c5b89bc62a31d55115158346f72923f49f1433 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 21:06:10 -0700 Subject: [PATCH 06/23] Suggested changes --- lib/audited/auditor.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index 803570b2..f5770c29 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -234,8 +234,10 @@ def revision_with(attributes) def audited_changes(for_touch: false) all_changes = if for_touch previous_changes + elsif respond_to?(:changes_to_save) + changes_to_save else - respond_to?(:changes_to_save) ? changes_to_save : changes + changes end filtered_changes = \ From b0bb2c38ed9d3d3042bbb13de43f50959335cb63 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Mon, 13 Feb 2023 21:25:03 -0700 Subject: [PATCH 07/23] An older rails friendly test --- spec/audited/auditor_spec.rb | 9 ++------- spec/spec_helper.rb | 1 - 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index d6e65dc2..c5b52636 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -419,12 +419,6 @@ def non_column_attr=(val) @user = create_user(name: "Brandon", status: :active, audit_comment: "Touch") end - around do |example| - freeze_time do - example.run - end - end - it "should save an audit" do expect { @user.touch(:suspended_at) }.to change(Audited::Audit, :count).by(1) end @@ -438,7 +432,8 @@ def non_column_attr=(val) it "should store the changed attributes" do @user.touch(:suspended_at) - expect(@user.audits.last.audited_changes).to eq({"suspended_at" => [nil, Time.current]}) + expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil + expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1).of(Time.current) end it "should store audit comment" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ae83931c..967b5b85 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -19,7 +19,6 @@ RSpec.configure do |config| config.include AuditedSpecHelpers - config.include ActiveSupport::Testing::TimeHelpers config.use_transactional_fixtures = false if Rails.version.start_with?("4.") config.use_transactional_tests = false if config.respond_to?(:use_transactional_tests=) end From 5d41025870ea6d748ebf35207cafdcdcf9386484 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 14 Feb 2023 09:57:49 -0700 Subject: [PATCH 08/23] Do touch auditing on 6+ Rails --- lib/audited/auditor.rb | 2 +- spec/audited/auditor_spec.rb | 54 +++++++++++++++++++----------------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index f5770c29..5c1bfdf4 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -84,7 +84,7 @@ def audited(options = {}) after_create :audit_create if audited_options[:on].include?(:create) before_update :audit_update if audited_options[:on].include?(:update) - after_touch :audit_touch if audited_options[:on].include?(:touch) + after_touch :audit_touch if audited_options[:on].include?(:touch) && ::ActiveRecord::VERSION::MAJOR >= 6 before_destroy :audit_destroy if audited_options[:on].include?(:destroy) # Define and set after_audit and around_audit callbacks. This might be useful if you want diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index c5b52636..212a4989 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -414,37 +414,39 @@ def non_column_attr=(val) end end - describe "on touch" do - before do - @user = create_user(name: "Brandon", status: :active, audit_comment: "Touch") - end + if ::ActiveRecord::VERSION::MAJOR >= 6 + describe "on touch" do + before do + @user = create_user(name: "Brandon", status: :active, audit_comment: "Touch") + end - it "should save an audit" do - expect { @user.touch(:suspended_at) }.to change(Audited::Audit, :count).by(1) - end + it "should save an audit" do + expect { @user.touch(:suspended_at) }.to change(Audited::Audit, :count).by(1) + end - it "should set the action to 'update'" do - @user.touch(:suspended_at) - expect(@user.audits.last.action).to eq("update") - expect(Audited::Audit.updates.order(:id).last).to eq(@user.audits.last) - expect(@user.audits.updates.last).to eq(@user.audits.last) - end + it "should set the action to 'update'" do + @user.touch(:suspended_at) + expect(@user.audits.last.action).to eq("update") + expect(Audited::Audit.updates.order(:id).last).to eq(@user.audits.last) + expect(@user.audits.updates.last).to eq(@user.audits.last) + end - it "should store the changed attributes" do - @user.touch(:suspended_at) - expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil - expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1).of(Time.current) - end + it "should store the changed attributes" do + @user.touch(:suspended_at) + expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil + expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1).of(Time.current) + end - it "should store audit comment" do - expect(@user.audits.last.comment).to eq("Touch") - end + it "should store audit comment" do + expect(@user.audits.last.comment).to eq("Touch") + end - it "should not save an audit if only specified on create/destroy" do - on_create_destroy = Models::ActiveRecord::OnCreateDestroyUser.create(name: "Bart") - expect { - on_create_destroy.touch(:suspended_at) - }.to_not change(Audited::Audit, :count) + it "should not save an audit if only specified on create/destroy" do + on_create_destroy = Models::ActiveRecord::OnCreateDestroyUser.create(name: "Bart") + expect { + on_create_destroy.touch(:suspended_at) + }.to_not change(Audited::Audit, :count) + end end end From 82feab7da07805c61ca03c99d55b84ef5e5d85ab Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 14 Feb 2023 10:02:05 -0700 Subject: [PATCH 09/23] Compare w/ second --- spec/audited/auditor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 212a4989..613d50ac 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -434,7 +434,7 @@ def non_column_attr=(val) it "should store the changed attributes" do @user.touch(:suspended_at) expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil - expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1).of(Time.current) + expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1.second).of(Time.current) end it "should store audit comment" do From 4962e204aef793e89d2e1a6b3cc7e547d8ad87bb Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 14 Feb 2023 13:16:39 -0700 Subject: [PATCH 10/23] Make time spec work --- spec/audited/auditor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 613d50ac..72e3cb8d 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -434,7 +434,7 @@ def non_column_attr=(val) it "should store the changed attributes" do @user.touch(:suspended_at) expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil - expect(@user.audits.last.audited_changes["suspended_at"][1]).to be_within(1.second).of(Time.current) + expect(Time.parse(@user.audits.last.audited_changes["suspended_at"][1].to_s)).to be_within(1.second).of(Time.current) end it "should store audit comment" do From c568864e19445a790b6a4a042a77fee09521c232 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 14 Feb 2023 15:11:01 -0700 Subject: [PATCH 11/23] Adjust coverage line count for < 6 ActiveRecord and touch. --- spec/audited/auditor_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 72e3cb8d..89fdb9f7 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -1,6 +1,9 @@ require "spec_helper" -SingleCov.covered! uncovered: 9 # not testing proxy_respond_to? hack / 2 methods / deprecation of `version` +# not testing proxy_respond_to? hack / 2 methods / deprecation of `version` +# also, an additional 3 around `after_touch` for Versions before 6. +uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 12 : 9 +SingleCov.covered! uncovered: uncovered class ConditionalPrivateCompany < ::ActiveRecord::Base self.table_name = "companies" From 908f3c7af1c9619e084412da7fd44352f85e544c Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Thu, 9 Feb 2023 09:50:08 -0500 Subject: [PATCH 12/23] Update CHANGELOG --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af1bc87a..90fa54c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,9 @@ ## Unreleased -- Allow using with Padrino and other non-Rails projects - @https://www.npr.org/2023/02/09/1155464337/rebecca-black-leaves-the-meme-in-the-rear-view +- Audit touch calls - @mcyoung + [#657](https://github.com/collectiveidea/audited/pull/657) +- Allow using with Padrino and other non-Rails projects - @nicduke38degrees [#655](https://github.com/collectiveidea/audited/pull/655) - Testing updates - @jdufresne [#652](https://github.com/collectiveidea/audited/pull/652) From e2d9ebce79702d0717092bd5d907d9a0c10399c7 Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Tue, 14 Feb 2023 18:11:01 -0500 Subject: [PATCH 13/23] Bump version --- CHANGELOG.md | 2 +- lib/audited/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90fa54c4..fd9c194d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Audited ChangeLog -## Unreleased +## 5.3.0 (2023-02-14) - Audit touch calls - @mcyoung [#657](https://github.com/collectiveidea/audited/pull/657) diff --git a/lib/audited/version.rb b/lib/audited/version.rb index 8fd79656..0ab22b92 100644 --- a/lib/audited/version.rb +++ b/lib/audited/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Audited - VERSION = "5.2.0" + VERSION = "5.3.0" end From 21827deeb31541c427b3d86aa53acc19dfc96496 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Wed, 15 Feb 2023 11:11:26 -0700 Subject: [PATCH 14/23] Ensure testing of touched audit --- spec/audited/auditor_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 89fdb9f7..a9bccd22 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -420,7 +420,7 @@ def non_column_attr=(val) if ::ActiveRecord::VERSION::MAJOR >= 6 describe "on touch" do before do - @user = create_user(name: "Brandon", status: :active, audit_comment: "Touch") + @user = create_user(name: "Brandon", status: :active) end it "should save an audit" do @@ -441,7 +441,10 @@ def non_column_attr=(val) end it "should store audit comment" do - expect(@user.audits.last.comment).to eq("Touch") + @user.audit_comment = "Here exists a touch comment" + @user.touch(:suspended_at) + expect(@user.audits.last.action).to eq("update") + expect(@user.audits.last.comment).to eq("Here exists a touch comment") end it "should not save an audit if only specified on create/destroy" do From 25560969b91c8fe6322c88f60e4c83aaefa41825 Mon Sep 17 00:00:00 2001 From: Matt Young Date: Wed, 15 Feb 2023 11:16:09 -0700 Subject: [PATCH 15/23] Give spec a little more time to reduce flakiness --- spec/audited/auditor_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index a9bccd22..cccfc640 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -437,7 +437,7 @@ def non_column_attr=(val) it "should store the changed attributes" do @user.touch(:suspended_at) expect(@user.audits.last.audited_changes["suspended_at"][0]).to be_nil - expect(Time.parse(@user.audits.last.audited_changes["suspended_at"][1].to_s)).to be_within(1.second).of(Time.current) + expect(Time.parse(@user.audits.last.audited_changes["suspended_at"][1].to_s)).to be_within(2.seconds).of(Time.current) end it "should store audit comment" do From 9a583baa4161c8afb6b79923ae26f621a246663a Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Wed, 15 Feb 2023 13:28:43 -0500 Subject: [PATCH 16/23] Update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd9c194d..87d22555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Audited ChangeLog +## Unreleased + +- Testing Improvements - @mcyoung + [#658](https://github.com/collectiveidea/audited/pull/658) + ## 5.3.0 (2023-02-14) - Audit touch calls - @mcyoung From 7dbbb664370682b1a30d0d66fdcec8975e5fcf81 Mon Sep 17 00:00:00 2001 From: vlad psh Date: Thu, 23 Jun 2022 17:38:29 +0900 Subject: [PATCH 17/23] Fix redacted attributes bug when no changes were made --- spec/audited/auditor_spec.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index cccfc640..401aa865 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -218,17 +218,25 @@ def non_column_attr=(val) redacted = Audited::Auditor::AuditedInstanceMethods::REDACTED user = Models::ActiveRecord::UserMultipleRedactedAttributes.create( - password: "password", - ssn: 123456789 + password: "password" ) user.save! expect(user.audits.last.audited_changes["password"]).to eq(redacted) + # Saving '[REDACTED]' value for 'ssn' even if value wasn't set explicitly when record was created expect(user.audits.last.audited_changes["ssn"]).to eq(redacted) + user.password = "new_password" user.ssn = 987654321 user.save! expect(user.audits.last.audited_changes["password"]).to eq([redacted, redacted]) expect(user.audits.last.audited_changes["ssn"]).to eq([redacted, redacted]) + + # If we haven't changed any attrs from 'redacted' list, audit should not contain these keys + user.name = "new name" + user.save! + expect(user.audits.last.audited_changes).to have_key('name') + expect(user.audits.last.audited_changes).not_to have_key('password') + expect(user.audits.last.audited_changes).not_to have_key('ssn') end it "should redact columns in 'redacted' column with custom option" do From a07561fa65b5a484f6fbcc7a09d7a92dcc57699c Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Wed, 15 Feb 2023 21:29:05 -0500 Subject: [PATCH 18/23] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87d22555..96ef2345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Testing Improvements - @vlad-psh + [#628](https://github.com/collectiveidea/audited/pull/628) - Testing Improvements - @mcyoung [#658](https://github.com/collectiveidea/audited/pull/658) From b773b4a62108e8a4cfffb89d63a88b918a0ad96f Mon Sep 17 00:00:00 2001 From: Matt Young Date: Sun, 19 Feb 2023 11:25:34 -0700 Subject: [PATCH 19/23] Don't double audit on touch Added specs Handle for update/create Remove unused --- lib/audited/auditor.rb | 7 +++++ spec/audited/auditor_spec.rb | 44 ++++++++++++++++++++++++++-- spec/support/active_record/models.rb | 3 +- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index 5c1bfdf4..398e50c3 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -247,6 +247,13 @@ def audited_changes(for_touch: false) all_changes.except(*self.class.non_audited_columns) end + if for_touch + filtered_changes.reject! do |k, v| + audits.last.audited_changes[k].to_json == v.to_json || + audits.last.audited_changes[k].to_json == v[1].to_json + end + end + filtered_changes = redact_values(filtered_changes) filtered_changes = filter_encrypted_attrs(filtered_changes) filtered_changes = normalize_enum_changes(filtered_changes) diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 401aa865..9cea788b 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -1,8 +1,8 @@ require "spec_helper" # not testing proxy_respond_to? hack / 2 methods / deprecation of `version` -# also, an additional 3 around `after_touch` for Versions before 6. -uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 12 : 9 +# also, an additional 5 around `after_touch` for Versions before 6. +uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 14 : 9 SingleCov.covered! uncovered: uncovered class ConditionalPrivateCompany < ::ActiveRecord::Base @@ -146,7 +146,7 @@ class Secret2 < ::ActiveRecord::Base end it "should be configurable which attributes are not audited via ignored_attributes" do - Audited.ignored_attributes = ["delta", "top_secret", "created_at"] + Audited.ignored_attributes = ["delta", "top_secret", "created_at", "updated_at"] expect(Secret.non_audited_columns).to include("delta", "top_secret", "created_at") end @@ -461,6 +461,44 @@ def non_column_attr=(val) on_create_destroy.touch(:suspended_at) }.to_not change(Audited::Audit, :count) end + + context "don't double audit" do + let(:user) { Models::ActiveRecord::Owner.create(name: "OwnerUser", suspended_at: 1.month.ago, companies_attributes: [{ name: "OwnedCompany" }]) } + let(:company) { user.companies.first } + + it "should only create 1 (create) audit for object" do + expect(user.audits.count).to eq(1) + expect(user.audits.first.action).to eq("create") + end + + it "should only create 1 (create) audit for nested resource" do + expect(company.audits.count).to eq(1) + expect(company.audits.first.action).to eq("create") + end + + context "after creating" do + it "updating / touching nested resource shouldn't save touch audit on parent object" do + expect { company.touch(:type) }.not_to change(user.audits, :count) + expect { company.update(type: "test") }.not_to change(user.audits, :count) + end + + it "updating / touching parent object shouldn't save previous data" do + expect { user.touch(:suspended_at) }.to change(user.audits, :count).from(1).to(2) + expect(user.audits.last.action).to eq("update") + expect(user.audits.last.audited_changes.keys).to eq(%w[suspended_at]) + end + end + + context "after updating" do + it "changing nested resource shouldn't audit owner" do + expect { user.update(username: "test") }.to change(user.audits, :count).from(1).to(2) + expect { company.update(type: "test") }.not_to change(user.audits, :count) + + expect { user.touch(:suspended_at) }.to change(user.audits, :count).from(2).to(3) + expect { company.update(type: "another_test") }.not_to change(user.audits, :count) + end + end + end end end diff --git a/spec/support/active_record/models.rb b/spec/support/active_record/models.rb index 82843fc2..60c5da72 100644 --- a/spec/support/active_record/models.rb +++ b/spec/support/active_record/models.rb @@ -124,11 +124,12 @@ class Owner < ::ActiveRecord::Base audited has_associated_audits has_many :companies, class_name: "OwnedCompany", dependent: :destroy + accepts_nested_attributes_for :companies end class OwnedCompany < ::ActiveRecord::Base self.table_name = "companies" - belongs_to :owner, class_name: "Owner" + belongs_to :owner, class_name: "Owner", touch: true attr_accessible :name, :owner if respond_to?(:attr_accessible) # declare attr_accessible before calling aaa audited associated_with: :owner end From 7315642ffca038ad8998d0b094b3f1a49e6bff18 Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Tue, 21 Feb 2023 13:56:01 -0500 Subject: [PATCH 20/23] Bump version --- CHANGELOG.md | 4 +++- lib/audited/version.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96ef2345..ea2f1dbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,9 @@ # Audited ChangeLog -## Unreleased +## 5.3.1 (2023-02-21) +- Ensure touch support doesn't cause double audits - @mcyoung + [#660](https://github.com/collectiveidea/audited/pull/660) - Testing Improvements - @vlad-psh [#628](https://github.com/collectiveidea/audited/pull/628) - Testing Improvements - @mcyoung diff --git a/lib/audited/version.rb b/lib/audited/version.rb index 0ab22b92..35ff298c 100644 --- a/lib/audited/version.rb +++ b/lib/audited/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Audited - VERSION = "5.3.0" + VERSION = "5.3.1" end From 829bdb8375a946d43b1ba74a1e3490c28039dafc Mon Sep 17 00:00:00 2001 From: Matt Young Date: Tue, 21 Feb 2023 14:08:00 -0700 Subject: [PATCH 21/23] Ensure audits Check for audits --- lib/audited/auditor.rb | 2 ++ spec/audited/auditor_spec.rb | 20 ++++++++++++++++++-- spec/support/active_record/models.rb | 5 +++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index 398e50c3..e28cfaf0 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -249,6 +249,8 @@ def audited_changes(for_touch: false) if for_touch filtered_changes.reject! do |k, v| + next unless audits.present? + audits.last.audited_changes[k].to_json == v.to_json || audits.last.audited_changes[k].to_json == v[1].to_json end diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 9cea788b..12e8e34b 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -1,8 +1,8 @@ require "spec_helper" # not testing proxy_respond_to? hack / 2 methods / deprecation of `version` -# also, an additional 5 around `after_touch` for Versions before 6. -uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 14 : 9 +# also, an additional 6 around `after_touch` for Versions before 6. +uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 15 : 9 SingleCov.covered! uncovered: uncovered class ConditionalPrivateCompany < ::ActiveRecord::Base @@ -462,6 +462,22 @@ def non_column_attr=(val) }.to_not change(Audited::Audit, :count) end + it "should store an audit if touch is the only audit" do + on_touch = Models::ActiveRecord::OnTouchOnly.create(name: "Bart") + expect { + on_touch.update(name: "NotBart") + }.to_not change(Audited::Audit, :count) + expect { + on_touch.touch(:suspended_at) + }.to change(on_touch.audits, :count).from(0).to(1) + + @user.audits.destroy_all + expect(@user.audits).to be_empty + expect { + @user.touch(:suspended_at) + }.to change(@user.audits, :count).from(0).to(1) + end + context "don't double audit" do let(:user) { Models::ActiveRecord::Owner.create(name: "OwnerUser", suspended_at: 1.month.ago, companies_attributes: [{ name: "OwnedCompany" }]) } let(:company) { user.companies.first } diff --git a/spec/support/active_record/models.rb b/spec/support/active_record/models.rb index 60c5da72..5c5def30 100644 --- a/spec/support/active_record/models.rb +++ b/spec/support/active_record/models.rb @@ -161,5 +161,10 @@ class OnCreateUpdate < ::ActiveRecord::Base self.table_name = "companies" audited on: [:create, :update] end + + class OnTouchOnly < ::ActiveRecord::Base + self.table_name = "users" + audited on: [:touch] + end end end From 818ad340510aaa03c341e4a40f9522b9394ba36d Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Wed, 22 Feb 2023 06:08:42 -0500 Subject: [PATCH 22/23] Run standardrb --fix --- lib/audited/auditor.rb | 14 +++++++------- spec/audited/auditor_spec.rb | 10 +++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index e28cfaf0..0ea2798b 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -233,12 +233,12 @@ def revision_with(attributes) def audited_changes(for_touch: false) all_changes = if for_touch - previous_changes - elsif respond_to?(:changes_to_save) - changes_to_save - else - changes - end + previous_changes + elsif respond_to?(:changes_to_save) + changes_to_save + else + changes + end filtered_changes = \ if audited_options[:only].present? @@ -252,7 +252,7 @@ def audited_changes(for_touch: false) next unless audits.present? audits.last.audited_changes[k].to_json == v.to_json || - audits.last.audited_changes[k].to_json == v[1].to_json + audits.last.audited_changes[k].to_json == v[1].to_json end end diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index 12e8e34b..265dfba3 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -2,7 +2,7 @@ # not testing proxy_respond_to? hack / 2 methods / deprecation of `version` # also, an additional 6 around `after_touch` for Versions before 6. -uncovered = ActiveRecord::VERSION::MAJOR < 6 ? 15 : 9 +uncovered = (ActiveRecord::VERSION::MAJOR < 6) ? 15 : 9 SingleCov.covered! uncovered: uncovered class ConditionalPrivateCompany < ::ActiveRecord::Base @@ -234,9 +234,9 @@ def non_column_attr=(val) # If we haven't changed any attrs from 'redacted' list, audit should not contain these keys user.name = "new name" user.save! - expect(user.audits.last.audited_changes).to have_key('name') - expect(user.audits.last.audited_changes).not_to have_key('password') - expect(user.audits.last.audited_changes).not_to have_key('ssn') + expect(user.audits.last.audited_changes).to have_key("name") + expect(user.audits.last.audited_changes).not_to have_key("password") + expect(user.audits.last.audited_changes).not_to have_key("ssn") end it "should redact columns in 'redacted' column with custom option" do @@ -479,7 +479,7 @@ def non_column_attr=(val) end context "don't double audit" do - let(:user) { Models::ActiveRecord::Owner.create(name: "OwnerUser", suspended_at: 1.month.ago, companies_attributes: [{ name: "OwnedCompany" }]) } + let(:user) { Models::ActiveRecord::Owner.create(name: "OwnerUser", suspended_at: 1.month.ago, companies_attributes: [{name: "OwnedCompany"}]) } let(:company) { user.companies.first } it "should only create 1 (create) audit for object" do From 548cbe98c1abe07114490c923c72c233170ee6b5 Mon Sep 17 00:00:00 2001 From: Daniel Morrison Date: Wed, 22 Feb 2023 06:08:54 -0500 Subject: [PATCH 23/23] Bump version --- CHANGELOG.md | 5 +++++ lib/audited/version.rb | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea2f1dbb..a122fc01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Audited ChangeLog +## 5.3.2 (2023-02-22) + +- Touch audit bug fixes - @mcyoung + [#662](https://github.com/collectiveidea/audited/pull/662) + ## 5.3.1 (2023-02-21) - Ensure touch support doesn't cause double audits - @mcyoung diff --git a/lib/audited/version.rb b/lib/audited/version.rb index 35ff298c..a0a01edf 100644 --- a/lib/audited/version.rb +++ b/lib/audited/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Audited - VERSION = "5.3.1" + VERSION = "5.3.2" end