Fix RSpec matchers along with a few other things. (#420)

This commit fixes issues with the RSpec matchers, notably:

- For the `except` matcher code path, `model_class.default_ignored_attributes` was being called, but that method was made protected in a previous change as discussed [here](8f8495d#commitcomment-28001384). This commit makes that method public again.

- Both the `on` and `except` matcher options have undergone a pretty substantial refactor. As I was fixing the original issue with `on` and `except`, I started to notice other things and addressed them. I will try to make explanatory comments on the PR to give some insight.

- The matchers as a whole have undergone pretty significant changes, and I found that many of them were not working as intended, or, for example, in the case of `on`, it was a no-op. I made the changes that I thought reflected what the matchers ought to do and I also included tests (which actually ended up being extremely useful in discovering some of the bugs!) to make sure that future changes don't break anything.

- The `requires_comment` matcher's behavior has changed to look for callbacks instead. I also changed the way that comment-required state validity is checked in the `Auditor`, but I will discuss that below. My reasoning for the two changes are related though - given an audited model where audit comments are required, we only care about the existence of the comment when an object is being created, if it has been changed (and subsequently updated), or if it is about to be destroyed (and of course we can include & exclude any combination of these three states with the `on` option). Previously, the presence of the comment was related to the overall state of the object itself, and I found this confusing since the comment only matters when we are _doing_ something to the object.

- I have decoupled the codepaths for each option so that they could be run independently of one another (and all together, of course). I was getting some interesting behavior before, but I believe I have nailed down the correct behavior.

- I will discuss anything not discussed above for the matchers in PR comments.

The `Auditor` itself:

- Changed the way non-audited columns were calculated. Will discuss in-line.

- Revised and (hopefully) simplified the way that audit comments are required & validated. Will discuss in-line.

Other comments:

- As I mentioned above, I noticed some behavioral issues that I tried to address. I will talk about them more in-line and if there are any questions about particular things I did please feel free to ask.

- Another thing I would like to do is a complete rework of the test suite itself (or if anyone else wants to take this up, feel free :P) In writing the tests for the matchers, I noticed many things such as audited columns for the `User` model being inconsistent, the `Audited.ignored_attributes` changing, and more. This is directly caused by the behavior of some of the tests, and I think it would be beneficial to figure out a way to rework the behavior such that what happens in one test doesn't have an impact on all the others. I also have a slight fear that some tests may in fact be returning false-positives, but I haven't looked too far into that yet.

- I was wondering if it would be better to not memoize `Auditor#audited_columns` and `Auditor#non_audited_columns` since the calculations are not really intensive enough to warrant memoization, but when I did that tests were breaking left and right and so I think this really ties into my point above about the test suite. Something to keep in mind / look into. I didn't do it in this PR because there is already quite a bit going on.

lib/audited/auditor.rb

@@ -251,22 +251,17 @@ def write_audit(attrs)
       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.")
+        if comment_required_state?
+          errors.add(:audit_comment, "Comment can't be blank!") unless audit_comment.present?
         end
       end
 
+      def comment_required_state?
+        auditing_enabled &&
+          ((audited_options[:on].include?(:create) && self.new_record?) ||
+          (audited_options[:on].include?(:update) && self.persisted? && self.changed?))
+      end
+
       def combine_audits_if_needed
         max_audits = audited_options[:max_audits]
         if max_audits && (extra_count = audits.count - max_audits) > 0
@@ -277,9 +272,9 @@ def combine_audits_if_needed
 
       def require_comment
         if auditing_enabled && audit_comment.blank?
-          errors.add(:audit_comment, "Comment required before destruction")
+          errors.add(:audit_comment, "Comment can't be blank!")
           return false if Rails.version.start_with?('4.')
-          throw :abort
+          throw(:abort)
         end
       end
 
@@ -321,9 +316,7 @@ def audited_columns
 
       # We have to calculate this here since column_names may not be available when `audited` is called
       def non_audited_columns
-        @non_audited_columns ||= audited_options[:only].present? ?
-                                 column_names - audited_options[:only] :
-                                 default_ignored_attributes | audited_options[:except]
+        @non_audited_columns ||= calculate_non_audited_columns
       end
 
       def non_audited_columns=(columns)
@@ -369,11 +362,12 @@ def auditing_enabled=(val)
         Audited.store["#{table_name}_auditing_enabled"] = val
       end
 
-      protected
       def default_ignored_attributes
-        [primary_key, inheritance_column] + Audited.ignored_attributes
+        [primary_key, inheritance_column] | Audited.ignored_attributes
       end
 
+      protected
+
       def normalize_audited_options
         audited_options[:on] = Array.wrap(audited_options[:on])
         audited_options[:on] = [:create, :update, :destroy] if audited_options[:on].empty?
@@ -382,6 +376,16 @@ def normalize_audited_options
         max_audits = audited_options[:max_audits] || Audited.max_audits
         audited_options[:max_audits] = Integer(max_audits).abs if max_audits
       end
+
+      def calculate_non_audited_columns
+        if audited_options[:only].present?
+          (column_names | default_ignored_attributes) - audited_options[:only]
+        elsif audited_options[:except].present?
+          default_ignored_attributes | audited_options[:except]
+        else
+          default_ignored_attributes
+        end
+      end
     end
   end
 end

lib/audited/rspec_matchers.rb

@@ -41,12 +41,12 @@ def associated_with(model)
       end
 
       def only(*fields)
-        @options[:only] = fields.flatten
+        @options[:only] = fields.flatten.map(&:to_s)
         self
       end
 
       def except(*fields)
-        @options[:except] = fields.flatten
+        @options[:except] = fields.flatten.map(&:to_s)
         self
       end
 
@@ -56,16 +56,13 @@ def requires_comment
       end
 
       def on(*actions)
-        @options[:on] = actions.flatten
+        @options[:on] = actions.flatten.map(&:to_sym)
         self
       end
 
       def matches?(subject)
         @subject = subject
-        auditing_enabled? &&
-          associated_with_model? &&
-          records_changes_to_specified_fields? &&
-          comment_required_valid?
+        auditing_enabled? && required_checks_for_options_satisfied?
       end
 
       def failure_message
@@ -109,31 +106,83 @@ def associated_with_model?
       end
 
       def records_changes_to_specified_fields?
-        if @options[:only] || @options[:except]
-          if @options[:only]
-            except = model_class.column_names - @options[:only].map(&:to_s)
-          else
-            except = model_class.default_ignored_attributes + Audited.ignored_attributes
-            except |= @options[:except].collect(&:to_s) if @options[:except]
-          end
+        ignored_fields = build_ignored_fields_from_options
+
+        expects "non audited columns (#{model_class.non_audited_columns.inspect}) to match (#{ignored_fields})"
+        model_class.non_audited_columns.to_set == ignored_fields.to_set
+      end
+
+      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?
+      end
 
-          expects "non audited columns (#{model_class.non_audited_columns.inspect}) to match (#{except})"
-          model_class.non_audited_columns =~ except
+      def only_audit_on_designated_callbacks?
+        {
+          create: [:after, :audit_create],
+          update: [:before, :audit_update],
+          destroy: [:before, :audit_destroy]
+        }.map do |(action, kind_callback)|
+          kind, callback = kind_callback
+          callbacks_for(action, kind: kind).include?(callback) if @options[:on].include?(action)
+        end.compact.all?
+      end
+
+      def validate_callbacks_include_presence_of_comment?
+        if @options[:comment_required] && audited_on_create_or_update?
+          callbacks_for(:validate).include?(:presence_of_audit_comment)
         else
           true
         end
       end
 
-      def comment_required_valid?
-        if @options[:comment_required]
-          @subject.audit_comment = nil
+      def audited_on_create_or_update?
+        model_class.audited_options[:on].include?(:create) || model_class.audited_options[:on].include?(:update)
+      end
 
-          expects "to be invalid when audit_comment is not specified"
-          @subject.valid? == false && @subject.errors.key?(:audit_comment)
+      def destroy_callbacks_include_comment_required?
+        if @options[:comment_required] && model_class.audited_options[:on].include?(:destroy)
+          callbacks_for(:destroy).include?(:require_comment)
         else
           true
         end
       end
+
+      def requires_comment_before_callbacks?
+        [:create, :update, :destroy].map do |action|
+          if @options[:comment_required] && model_class.audited_options[:on].include?(action)
+            callbacks_for(action).include?(:require_comment)
+          end
+        end.compact.all?
+      end
+
+      def callbacks_for(action, kind: :before)
+        model_class.send("_#{action}_callbacks").select { |cb| cb.kind == kind }.map(&:filter)
+      end
+
+      def build_ignored_fields_from_options
+        default_ignored_attributes = model_class.default_ignored_attributes
+
+        if @options[:only].present?
+          (default_ignored_attributes | model_class.column_names) - @options[:only]
+        elsif @options[:except].present?
+          default_ignored_attributes | @options[:except]
+        else
+          default_ignored_attributes
+        end
+      end
+
+      def required_checks_for_options_satisfied?
+        {
+          only: :records_changes_to_specified_fields?,
+          except: :records_changes_to_specified_fields?,
+          comment_required: :comment_required_valid?,
+          associated_with: :associated_with_model?,
+          on: :only_audit_on_designated_callbacks?
+        }.map do |(option, check)|
+          send(check) if @options[option].present?
+        end.compact.all?
+      end
     end
 
     class AssociatedAuditMatcher # :nodoc:

spec/audited/auditor_spec.rb

@@ -728,22 +728,26 @@ def stub_global_max_audits(max_audits)
   describe "comment required" do
 
     describe "on create" do
-      it "should not validate when audit_comment is not supplied" do
-        expect(Models::ActiveRecord::CommentRequiredUser.new).not_to be_valid
+      it "should not validate when audit_comment is not supplied when initialized" do
+        expect(Models::ActiveRecord::CommentRequiredUser.new(name: 'Foo')).not_to be_valid
+      end
+
+      it "should not validate when audit_comment is not supplied trying to create" do
+        expect(Models::ActiveRecord::CommentRequiredUser.create(name: 'Foo')).not_to be_valid
       end
 
       it "should validate when audit_comment is supplied" do
-        expect(Models::ActiveRecord::CommentRequiredUser.new( audit_comment: 'Create')).to be_valid
+        expect(Models::ActiveRecord::CommentRequiredUser.create(name: 'Foo', 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
+        expect(Models::ActiveRecord::OnUpdateCommentRequiredUser.create(name: 'Foo')).to be_valid
+        expect(Models::ActiveRecord::OnDestroyCommentRequiredUser.create(name: 'Foo')).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
+        expect(Models::ActiveRecord::CommentRequiredUser.create(name: 'Foo')).to be_valid
         Models::ActiveRecord::CommentRequiredUser.enable_auditing
       end
     end

spec/audited/rspec_matchers_spec.rb

@@ -0,0 +1,69 @@
+require "spec_helper"
+
+describe Models::ActiveRecord::UserExceptPassword do
+  let(:non_audited_columns) { subject.class.non_audited_columns }
+
+  it { should_not be_audited.only(non_audited_columns) }
+  it { should be_audited.except(:password) }
+  it { should_not be_audited.requires_comment }
+  it { should be_audited.on(:create, :update, :destroy) }
+  # test chaining
+  it { should be_audited.except(:password).on(:create, :update, :destroy) }
+end
+
+describe Models::ActiveRecord::UserOnlyPassword do
+  let(:audited_columns) { subject.class.audited_columns }
+
+  it { should be_audited.only(:password) }
+  it { should_not be_audited.except(audited_columns) }
+  it { should_not be_audited.requires_comment }
+  it { should be_audited.on(:create, :update, :destroy) }
+  it { should be_audited.only(:password).on(:create, :update, :destroy) }
+end
+
+describe Models::ActiveRecord::CommentRequiredUser do
+  let(:audited_columns) { subject.class.audited_columns }
+  let(:non_audited_columns) { subject.class.non_audited_columns }
+
+  it { should_not be_audited.only(non_audited_columns) }
+  it { should_not be_audited.except(audited_columns) }
+  it { should be_audited.requires_comment }
+  it { should be_audited.on(:create, :update, :destroy) }
+  it { should be_audited.requires_comment.on(:create, :update, :destroy) }
+end
+
+describe Models::ActiveRecord::OnCreateCommentRequiredUser do
+  let(:audited_columns) { subject.class.audited_columns }
+  let(:non_audited_columns) { subject.class.non_audited_columns }
+
+  it { should_not be_audited.only(non_audited_columns) }
+  it { should_not be_audited.except(audited_columns) }
+  it { should be_audited.requires_comment }
+  it { should be_audited.on(:create) }
+  it { should_not be_audited.on(:update, :destroy) }
+  it { should be_audited.requires_comment.on(:create) }
+end
+
+describe Models::ActiveRecord::OnUpdateCommentRequiredUser do
+  let(:audited_columns) { subject.class.audited_columns }
+  let(:non_audited_columns) { subject.class.non_audited_columns }
+
+  it { should_not be_audited.only(non_audited_columns) }
+  it { should_not be_audited.except(audited_columns) }
+  it { should be_audited.requires_comment }
+  it { should be_audited.on(:update) }
+  it { should_not be_audited.on(:create, :destroy) }
+  it { should be_audited.requires_comment.on(:update) }
+end
+
+describe Models::ActiveRecord::OnDestroyCommentRequiredUser do
+  let(:audited_columns) { subject.class.audited_columns }
+  let(:non_audited_columns) { subject.class.non_audited_columns }
+
+  it { should_not be_audited.only(non_audited_columns) }
+  it { should_not be_audited.except(audited_columns) }
+  it { should be_audited.requires_comment }
+  it { should be_audited.on(:destroy) }
+  it { should_not be_audited.on(:create, :update) }
+  it { should be_audited.requires_comment.on(:destroy) }
+end

spec/spec_helper.rb

@@ -7,6 +7,7 @@
 require 'rails_app/config/environment'
 require 'rspec/rails'
 require 'audited'
+require 'audited-rspec'
 require 'audited_spec_helpers'
 require 'support/active_record/models'
 

spec/support/active_record/models.rb

@@ -13,6 +13,11 @@ def name=(val)
       end
     end
 
+    class UserExceptPassword < ::ActiveRecord::Base
+      self.table_name = :users
+      audited except: :password
+    end
+
     class UserOnlyPassword < ::ActiveRecord::Base
       self.table_name = :users
       attribute :non_column_attr if Rails.version >= '5.1'