Fix default behavior of belong_to under Rails 4.2

Rails 5 introduced a change where `belongs_to` would default to adding a
presence validation along with the association. However, it also
introduced a configuration option, `belongs_to_required_by_default`, to
emulate the old behavior prior to Rails 5. For Rails 4.2 projects as
well as Rails 5 which were migrated from 4, this setting is false, so
that existing apps do not break.

To mimic this, a change was made to the `belong_to` matcher to check for
the presence of the presence validator if
`belongs_to_required_by_default` is true and check for the absence of
the presence validator if it is false. However, this last bit of the
logic actually causes problems. Take this case, for example:

    ActiveRecord::Base.belongs_to_required_by_default = false

    class Post < ActiveRecord::Base
      belongs_to :user
      validates :user, presence: true
    end

    RSpec.describe Post, type: :model do
      it { is_expected.to belong_to(:user) }
    end

In this example, the developer has chosen to place a presence validation
on the association manually. `belong_to` doesn't know this, however, and
will check to make sure that `user` can be nil, which of course it
can't. Therefore, this test will fail. In addition, the failure message
that `belong_to` generates is confusing:

    Expected Post to have a belongs_to association called user (the
    association should have been defined with `optional: true`, but was
    not)

The reason why the test fails is that when
`belongs_to_required_by_default` is false, belong_to` will place an
implicit `optional` qualifier on itself. In other words, these two tests
are equivalent:

    it { is_expected.to belong_to(:user) }
    it { is_expected.to belong_to(:user).optional }

However, this is not only wrong, but the `belongs_to` macro in Rails 4.2
doesn't have an `optional` option (it has `required` instead), so the
failure message that `belong_to` generates is confusing.

This commit fixes this by modifying `belong_to` so that under Rails 4.2,
the matcher will have not have any qualifiers on it by default.

lib/shoulda/matchers/active_record/association_matcher.rb

@@ -968,12 +968,8 @@ module Shoulda
           @submatchers = []
           @missing = ''
 
-          if macro == :belongs_to
+          if macro == :belongs_to && RailsShim.active_record_gte_5?
-            if RailsShim.active_record_gte_5? && belongs_to_required_by_default?
+            required(belongs_to_required_by_default?)
-              required
-            else
-              optional
-            end
           end
         end
 

spec/support/unit/helpers/active_record_versions.rb

@@ -37,7 +37,7 @@ module UnitTests
       active_record_version < 5
     end
 
-    def active_record_supports_required_for_associations?
+    def active_record_supports_optional_for_associations?
       active_record_version >= 5
     end
   end

spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb

@@ -281,7 +281,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
 
     context 'given the association is neither configured to be required nor optional' do
       context 'when qualified with required(true)' do
-        if active_record_supports_required_for_associations?
+        if active_record_supports_optional_for_associations?
           context 'when belongs_to is configured to be required by default' do
             it 'passes' do
               configuring_default_belongs_to_requiredness(true) do
@@ -311,13 +311,14 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
         else
           it 'fails with an appropriate message' do
             assertion = lambda do
-              expect(belonging_to_parent).to belong_to(:parent).required(true)
+              expect(belonging_to_parent).
+                to belong_to(:parent).required(true)
             end
 
             message = format_message(<<-MESSAGE, one_line: true)
-              Expected Child to have a belongs_to association called parent (the
+              Expected Child to have a belongs_to association called parent
-              association should have been defined with `required: true`, but
+              (the association should have been defined with `required:
-              was not)
+              true`, but was not)
             MESSAGE
 
             expect(&assertion).to fail_with_message(message)
@@ -326,7 +327,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
       end
 
       context 'when qualified with required(false)' do
-        if active_record_supports_required_for_associations?
+        if active_record_supports_optional_for_associations?
           context 'when belongs_to is configured to be required by default' do
             it 'fails with an appropriate message' do
               configuring_default_belongs_to_requiredness(true) do
@@ -361,7 +362,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
       end
 
       context 'when qualified with optional' do
-        if active_record_supports_required_for_associations?
+        if active_record_supports_optional_for_associations?
           context 'when belongs_to is configured to be required by default' do
             it 'fails with an appropriate message' do
               configuring_default_belongs_to_requiredness(true) do
@@ -395,8 +396,8 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
         end
       end
 
-      if active_record_supports_required_for_associations?
+      context 'when qualified with nothing' do
-        context 'when qualified with nothing' do
+        if active_record_supports_optional_for_associations?
           context 'when belongs_to is configured to be required by default' do
             it 'passes' do
               configuring_default_belongs_to_requiredness(true) do
@@ -411,6 +412,92 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
                 expect(belonging_to_parent).to belong_to(:parent)
               end
             end
+
+            context 'and a presence validation is on the attribute instead of using required: true' do
+              it 'passes' do
+                configuring_default_belongs_to_requiredness(false) do
+                  record = belonging_to_parent do
+                    validates_presence_of :parent
+                  end
+
+                  expect(record).to belong_to(:parent)
+                end
+              end
+            end
+
+            context 'and a presence validation is on the attribute with a condition' do
+              context 'and the condition is true' do
+                it 'passes' do
+                  configuring_default_belongs_to_requiredness(false) do
+                    child_model = create_child_model_belonging_to_parent do
+                      attr_accessor :condition
+                      validates_presence_of :parent, if: :condition
+                    end
+
+                    record = child_model.new(condition: true)
+
+                    expect(record).to belong_to(:parent)
+                  end
+                end
+              end
+
+              context 'and the condition is false' do
+                it 'passes' do
+                  configuring_default_belongs_to_requiredness(false) do
+                    child_model = create_child_model_belonging_to_parent do
+                      attr_accessor :condition
+                      validates_presence_of :parent, if: :condition
+                    end
+
+                    record = child_model.new(condition: false)
+
+                    expect(record).to belong_to(:parent)
+                  end
+                end
+              end
+            end
+          end
+        else
+          it 'passes' do
+            expect(belonging_to_parent).to belong_to(:parent)
+          end
+
+          context 'and a presence validation is on the attribute instead of using required: true' do
+            it 'passes' do
+              record = belonging_to_parent do
+                validates_presence_of :parent
+              end
+
+              expect(record).to belong_to(:parent)
+            end
+          end
+
+          context 'and a presence validation is on the attribute with a condition' do
+            context 'and the condition is true' do
+              it 'passes' do
+                child_model = create_child_model_belonging_to_parent do
+                  attr_accessor :condition
+                  validates_presence_of :parent, if: :condition
+                end
+
+                record = child_model.new(condition: true)
+
+                expect(record).to belong_to(:parent)
+              end
+            end
+
+            context 'and the condition is false' do
+              it 'passes' do
+                child_model = create_child_model_belonging_to_parent do
+                  attr_accessor :condition
+                  validates_presence_of :parent, if: :condition
+                end
+
+                record = child_model.new(condition: false)
+
+                expect(record).to belong_to(:parent)
+              end
+            end
           end
         end
       end
@@ -459,31 +546,13 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
       end
 
       context 'when qualified with nothing' do
-        if active_record_supports_required_for_associations?
+        it 'passes' do
-          it 'passes' do
+          expect(belonging_to_parent(required: true)).to belong_to(:parent)
-            expect(belonging_to_parent(required: true)).
-              to belong_to(:parent)
-          end
-        else
-          it 'fails with an appropriate message' do
-            assertion = lambda do
-              expect(belonging_to_parent(required: true)).
-                to belong_to(:parent)
-            end
-
-            message = format_message(<<-MESSAGE, one_line: true)
-              Expected Child to have a belongs_to association called parent (the
-              association should have been defined with `optional: true`, but
-              was not)
-            MESSAGE
-
-            expect(&assertion).to fail_with_message(message)
-          end
         end
       end
     end
 
-    if active_record_supports_required_for_associations?
+    if active_record_supports_optional_for_associations?
       context 'given the association is configured as optional: true' do
         context 'when qualified with required(true)' do
           it 'fails with an appropriate message' do
@@ -535,11 +604,29 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
       end
     end
 
-    def belonging_to_parent(options = {}, parent_options = {})
+    def belonging_to_parent(options = {}, parent_options = {}, &block)
-      define_model :parent, parent_options
+      child_model = create_child_model_belonging_to_parent(
+        options,
+        parent_options,
+        &block
+      )
+      child_model.new
+    end
+
+    def create_child_model_belonging_to_parent(
+      options = {},
+      parent_options = {},
+      &block
+    )
+      define_model(:parent, parent_options)
+
       define_model :child, parent_id: :integer do
         belongs_to :parent, options
-      end.new
+
+        if block
+          class_eval(&block)
+        end
+      end
     end
 
     def belonging_to_with_inverse(association, inverse_association)
@@ -1128,7 +1215,7 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do
       end
     end
 
-    if active_record_supports_required_for_associations?
+    if active_record_supports_optional_for_associations?
       context 'given an association with a matching :required option' do
         it 'passes' do
           expect(having_one_detail(required: true)).