Respect 'on' and 'except' options when used with 'comment_required' o…

…ption (#419)

fixes #418

CHANGELOG.md

@@ -19,7 +19,8 @@ Changed
 
 Fixed
 
-- None
+- Ensure that `on` and `except` options jive with `comment_required: true`
+  [#419](https://github.com/collectiveidea/audited/pull/419)
 
 
 ## 4.6.0 (2018-01-10)

lib/audited/auditor.rb

@@ -51,8 +51,8 @@ def audited(options = {})
         self.audit_associated_with = audited_options[:associated_with]
 
         if audited_options[:comment_required]
-          validates_presence_of :audit_comment, if: :auditing_enabled
-          before_destroy :require_comment
+          validate :presence_of_audit_comment
+          before_destroy :require_comment if audited_options[:on].include?(:destroy)
         end
 
         has_many :audits, -> { order(version: :asc) }, as: :auditable, class_name: Audited.audit_class.name, inverse_of: :auditable
@@ -219,6 +219,23 @@ def write_audit(attrs)
         run_callbacks(:audit)  { audits.create(attrs) } if auditing_enabled
       end
 
+      def presence_of_audit_comment
+        case
+        when !auditing_enabled
+          return true
+        when audit_comment.present?
+          return true
+        when audited_options[:on].exclude?(:create) && self.new_record?
+          return true
+        when audited_options[:on].exclude?(:update) && self.persisted?
+          return true
+        when audited_changes.empty? && self.persisted?
+          return true
+        else
+          errors.add(:audit_comment, "can't be blank.")
+        end
+      end
+
       def require_comment
         if auditing_enabled && audit_comment.blank?
           errors.add(:audit_comment, "Comment required before destruction")

spec/audited/auditor_spec.rb

@@ -552,6 +552,11 @@ def non_column_attr=(val)
         expect(Models::ActiveRecord::CommentRequiredUser.new( audit_comment: 'Create')).to be_valid
       end
 
+      it "should validate when audit_comment is not supplied, and creating is not being audited" do
+        expect(Models::ActiveRecord::OnUpdateCommentRequiredUser.new).to be_valid
+        expect(Models::ActiveRecord::OnDestroyCommentRequiredUser.new).to be_valid
+      end
+
       it "should validate when audit_comment is not supplied, and auditing is disabled" do
         Models::ActiveRecord::CommentRequiredUser.disable_auditing
         expect(Models::ActiveRecord::CommentRequiredUser.new).to be_valid
@@ -561,11 +566,18 @@ def non_column_attr=(val)
 
     describe "on update" do
       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 }
 
       it "should not validate when audit_comment is not supplied" do
         expect(user.update_attributes(name: 'Test')).to eq(false)
       end
 
+      it "should validate when audit_comment is not supplied, and updating is not being audited" do
+        expect(on_create_user.update_attributes(name: 'Test')).to eq(true)
+        expect(on_destroy_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
@@ -579,6 +591,8 @@ def non_column_attr=(val)
 
     describe "on destroy" do
       let( :user ) { Models::ActiveRecord::CommentRequiredUser.create!( audit_comment: 'Create' )}
+      let( :on_create_user ) { Models::ActiveRecord::OnCreateCommentRequiredUser.create!( audit_comment: 'Create' ) }
+      let( :on_update_user ) { Models::ActiveRecord::OnUpdateCommentRequiredUser.create }
 
       it "should not validate when audit_comment is not supplied" do
         expect(user.destroy).to eq(false)
@@ -589,6 +603,11 @@ def non_column_attr=(val)
         expect(user.destroy).to eq(user)
       end
 
+      it "should validate when audit_comment is not supplied, and destroying is not being audited" do
+        expect(on_create_user.destroy).to eq(on_create_user)
+        expect(on_update_user.destroy).to eq(on_update_user)
+      end
+
       it "should validate when audit_comment is not supplied, and auditing is disabled" do
         Models::ActiveRecord::CommentRequiredUser.disable_auditing
         expect(user.destroy).to eq(user)

spec/support/active_record/models.rb

@@ -24,6 +24,21 @@ class CommentRequiredUser < ::ActiveRecord::Base
       audited comment_required: true
     end
 
+    class OnCreateCommentRequiredUser < ::ActiveRecord::Base
+      self.table_name = :users
+      audited comment_required: true, on: :create
+    end
+
+    class OnUpdateCommentRequiredUser < ::ActiveRecord::Base
+      self.table_name = :users
+      audited comment_required: true, on: :update
+    end
+
+    class OnDestroyCommentRequiredUser < ::ActiveRecord::Base
+      self.table_name = :users
+      audited comment_required: true, on: :destroy
+    end
+
     class AccessibleAfterDeclarationUser < ::ActiveRecord::Base
       self.table_name = :users
       audited