diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b51535d4..cbd691406 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,8 @@ Changed Fixed +- Ensure that `except` option jibes with `comment_required: true` + [#425](https://github.com/collectiveidea/audited/pull/425) - Ensure enum changes are stored consistently [#429](https://github.com/collectiveidea/audited/pull/429) @@ -81,7 +83,7 @@ Changed Fixed -- Ensure that `on` and `except` options jive with `comment_required: true` +- Ensure that `on` and `except` options jibe with `comment_required: true` [#419](https://github.com/collectiveidea/audited/pull/419) - Fix RSpec matchers [#420](https://github.com/collectiveidea/audited/pull/420) diff --git a/lib/audited/audit.rb b/lib/audited/audit.rb index f3fc630ec..268d8c1d1 100644 --- a/lib/audited/audit.rb +++ b/lib/audited/audit.rb @@ -131,7 +131,7 @@ def self.audited_classes # by +user+. This method is hopefully threadsafe, making it ideal # for background operations that require audit information. def self.as_user(user) - last_audited_user = ::Audited.store[:audited_user] + last_audited_user = ::Audited.store[:audited_user] ::Audited.store[:audited_user] = user yield ensure @@ -143,7 +143,7 @@ def self.reconstruct_attributes(audits) audits.each_with_object({}) do |audit, all| all.merge!(audit.new_attributes) all[:audit_version] = audit.version - end + end end # @private diff --git a/lib/audited/auditor.rb b/lib/audited/auditor.rb index d01949649..7614f8597 100644 --- a/lib/audited/auditor.rb +++ b/lib/audited/auditor.rb @@ -63,7 +63,7 @@ def audited(options = {}) self.audit_associated_with = audited_options[:associated_with] if audited_options[:comment_required] - validate :presence_of_audit_comment + validates_presence_of :audit_comment, if: :eligible_for_comment_validation? before_destroy :require_comment if audited_options[:on].include?(:destroy) end @@ -280,16 +280,11 @@ def write_audit(attrs) end end - def presence_of_audit_comment - if comment_required_state? - errors.add(:audit_comment, "Comment can't be blank!") unless audit_comment.present? - end - end - - def comment_required_state? + def eligible_for_comment_validation? auditing_enabled && - ((audited_options[:on].include?(:create) && self.new_record?) || - (audited_options[:on].include?(:update) && self.persisted? && self.changed?)) + ((audited_options[:on].include?(:create) && self.new_record?) || + (audited_options[:on].include?(:update) && self.persisted?)) && + !(audited_changes.empty? && self.persisted?) end def combine_audits_if_needed diff --git a/lib/audited/rspec_matchers.rb b/lib/audited/rspec_matchers.rb index fbb728298..296793a0a 100644 --- a/lib/audited/rspec_matchers.rb +++ b/lib/audited/rspec_matchers.rb @@ -114,7 +114,7 @@ def records_changes_to_specified_fields? def comment_required_valid? expects "to require audit_comment before #{model_class.audited_options[:on]} when comment required" - validate_callbacks_include_presence_of_comment? && destroy_callbacks_include_comment_required? + validations_include_presence_of_comment? && destroy_callbacks_include_comment_required? end def only_audit_on_designated_callbacks? @@ -128,9 +128,9 @@ def only_audit_on_designated_callbacks? end.compact.all? end - def validate_callbacks_include_presence_of_comment? + def validations_include_presence_of_comment? if @options[:comment_required] && audited_on_create_or_update? - callbacks_for(:validate).include?(:presence_of_audit_comment) + model_class.validators_on(:audit_comment).present? else true end diff --git a/spec/audited/auditor_spec.rb b/spec/audited/auditor_spec.rb index ac45a84a6..254700824 100644 --- a/spec/audited/auditor_spec.rb +++ b/spec/audited/auditor_spec.rb @@ -840,6 +840,7 @@ def stub_global_max_audits(max_audits) let( :user ) { Models::ActiveRecord::CommentRequiredUser.create!( audit_comment: 'Create' ) } let( :on_create_user ) { Models::ActiveRecord::OnDestroyCommentRequiredUser.create } let( :on_destroy_user ) { Models::ActiveRecord::OnDestroyCommentRequiredUser.create } + let( :on_update_except_name_user ) { Models::ActiveRecord::OnUpdateCommentRequiredExceptNameUser.create } it "should not validate when audit_comment is not supplied" do expect(user.update_attributes(name: 'Test')).to eq(false) @@ -850,6 +851,10 @@ def stub_global_max_audits(max_audits) expect(on_destroy_user.update_attributes(name: 'Test')).to eq(true) end + it "should validate when audit_comment is not supplied, and only excepted attributes have been updated" do + expect(on_update_except_name_user.update_attributes(name: 'Test')).to eq(true) + end + it "should validate when audit_comment is supplied" do expect(user.update_attributes(name: 'Test', audit_comment: 'Update')).to eq(true) end diff --git a/spec/support/active_record/models.rb b/spec/support/active_record/models.rb index 12c851496..bc91b19c9 100644 --- a/spec/support/active_record/models.rb +++ b/spec/support/active_record/models.rb @@ -45,6 +45,11 @@ class OnDestroyCommentRequiredUser < ::ActiveRecord::Base audited comment_required: true, on: :destroy end + class OnUpdateCommentRequiredExceptNameUser < ::ActiveRecord::Base + self.table_name = :users + audited comment_required: true, except: :name, on: :update + end + class AccessibleAfterDeclarationUser < ::ActiveRecord::Base self.table_name = :users audited