Re-fix honoring of 'except' when used with 'comment_required'

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)

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

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

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

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

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