From cd96089a56b97cd11f7502826636895253eca27d Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Sun, 28 Jan 2018 23:53:24 -0600 Subject: [PATCH] belongs_to: Change default req/opt to match global default In Rails 5, belongs_to associations default to `required: true`. This is configurable by setting `ActiveRecord::Base.belongs_to_required_by_default` to true. In new Rails 5 apps, this is set to true, because the Rails generator will add an initializer with the following line in it: config.active_record.belongs_to_required_by_default = true However, for Rails apps that have been upgraded from 4 to 5, this initializer may not be present, and in that case, that setting will not be set, and `belong_to` associations will not default to `required: true`. This means that under Rails 5, our `belong_to` matcher cannot always default to applying the `required` qualifier; it must abide by the `belongs_to_required_by_default` setting in doing so. --- .../active_record/association_matcher.rb | 18 +- .../association_matchers/optional_matcher.rb | 21 +- .../association_matchers/required_matcher.rb | 19 +- spec/support/unit/helpers/message_helpers.rb | 10 +- .../active_record/association_matcher_spec.rb | 291 +++++++++++++++--- 5 files changed, 289 insertions(+), 70 deletions(-) diff --git a/lib/shoulda/matchers/active_record/association_matcher.rb b/lib/shoulda/matchers/active_record/association_matcher.rb index c45017e0..5b2b3faa 100644 --- a/lib/shoulda/matchers/active_record/association_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matcher.rb @@ -253,8 +253,6 @@ module Shoulda # should belong_to(:organization).inverse_of(:employees) # end # - # @return [AssociationMatcher] - # # ##### required # # Use `required` to assert that the association is not allowed to be nil. @@ -274,8 +272,6 @@ module Shoulda # should belong_to(:organization).required # end # - # @return [AssociationMatcher] - # # ##### optional # # Use `optional` to assert that the association is allowed to be nil. @@ -954,7 +950,7 @@ module Shoulda @missing = '' if macro == :belongs_to - if RailsShim.active_record_gte_5? + if RailsShim.active_record_gte_5? && belongs_to_required_by_default? required else optional @@ -1051,12 +1047,12 @@ module Shoulda self end - def optional(optional = true) + def optional remove_submatcher(AssociationMatchers::RequiredMatcher) add_submatcher( AssociationMatchers::OptionalMatcher, name, - optional, + true, ) self end @@ -1156,8 +1152,8 @@ module Shoulda end def failing_submatchers - @failing_submatchers ||= submatchers.reject do |matcher| - matcher.matches?(subject) + @failing_submatchers ||= submatchers.select do |matcher| + !matcher.matches?(subject) end end @@ -1339,6 +1335,10 @@ module Shoulda def submatchers_match? failing_submatchers.empty? end + + def belongs_to_required_by_default? + ::ActiveRecord::Base.belongs_to_required_by_default + end end end end diff --git a/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb index 83bef57f..531a0fff 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/optional_matcher.rb @@ -7,37 +7,36 @@ module Shoulda attr_reader :missing_option def initialize(attribute_name, optional) - @attribute_name = attribute_name + @optional = optional + @submatcher = ActiveModel::AllowValueMatcher.new(nil). + for(attribute_name) @missing_option = '' - @submatcher = submatcher_class_for(optional).new(nil). - for(attribute_name). - with_message(:required) end def description - 'required: true' + "optional: #{optional}" end def matches?(subject) - if submatcher.matches?(subject) + if submatcher_passes?(subject) true else @missing_option = 'the association should have been defined ' + - 'with `optional: true`, but was not' + "with `optional: #{optional}`, but was not" false end end private - attr_reader :subject, :submatcher + attr_reader :optional, :submatcher - def submatcher_class_for(optional) + def submatcher_passes?(subject) if optional - ActiveModel::AllowValueMatcher + submatcher.matches?(subject) else - ActiveModel::DisallowValueMatcher + submatcher.does_not_match?(subject) end end end diff --git a/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb index 54e29645..9e4be0eb 100644 --- a/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb +++ b/lib/shoulda/matchers/active_record/association_matchers/required_matcher.rb @@ -7,36 +7,37 @@ module Shoulda attr_reader :missing_option def initialize(attribute_name, required) - @missing_option = '' - @submatcher = submatcher_class_for(required).new(nil). + @required = required + @submatcher = ActiveModel::DisallowValueMatcher.new(nil). for(attribute_name). with_message(validation_message_key) + @missing_option = '' end def description - 'required: true' + "required: #{required}" end def matches?(subject) - if submatcher.matches?(subject) + if submatcher_passes?(subject) true else @missing_option = 'the association should have been defined ' + - 'with `required: true`, but was not' + "with `required: #{required}`, but was not" false end end private - attr_reader :subject, :submatcher + attr_reader :required, :submatcher - def submatcher_class_for(required) + def submatcher_passes?(subject) if required - ActiveModel::DisallowValueMatcher + submatcher.matches?(subject) else - ActiveModel::AllowValueMatcher + submatcher.does_not_match?(subject) end end diff --git a/spec/support/unit/helpers/message_helpers.rb b/spec/support/unit/helpers/message_helpers.rb index c0e7d3ff..67203387 100644 --- a/spec/support/unit/helpers/message_helpers.rb +++ b/spec/support/unit/helpers/message_helpers.rb @@ -6,8 +6,14 @@ module UnitTests example_group.include(self) end - def format_message(message) - word_wrap(message.strip_heredoc.strip) + def format_message(message, one_line: false) + stripped_message = message.strip_heredoc.strip + + if one_line + stripped_message.tr("\n", " ").squeeze(" ") + else + word_wrap(stripped_message) + end end end end diff --git a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb index 1a9d23f1..3db2c28b 100644 --- a/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb @@ -279,62 +279,258 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do end end - context 'given an association with neither :required nor :optional specified' 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? + context 'when belongs_to is configured to be required by default' do + it 'passes' do + configuring_default_belongs_to_requiredness(true) do + expect(belonging_to_parent).to belong_to(:parent).required(true) + end + end + end + + context 'when belongs_to is not configured to be required by default' do + it 'fails with an appropriate message' do + configuring_default_belongs_to_requiredness(false) do + assertion = lambda do + 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 association should have been defined with `required: + true`, but was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end + end + end + else + it 'fails with an appropriate message' do + assertion = lambda do + 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 + association should have been defined with `required: true`, but + was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end + end + end + + context 'when qualified with required(false)' do + if active_record_supports_required_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 + assertion = lambda do + expect(belonging_to_parent). + to belong_to(:parent).required(false) + 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 `required: + false`, but was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end + end + end + + context 'when belongs_to is not configured to be required by default' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + expect(belonging_to_parent).to belong_to(:parent).required(false) + end + end + end + else + it 'passes' do + expect(belonging_to_parent).to belong_to(:parent).required(false) + end + end + end + + context 'when qualified with optional' do + if active_record_supports_required_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 + assertion = lambda do + expect(belonging_to_parent). + to belong_to(:parent).optional + 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 + + context 'when belongs_to is not configured to be required by default' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + expect(belonging_to_parent).to belong_to(:parent).optional + end + end + end + else + it 'passes' do + expect(belonging_to_parent).to belong_to(:parent).optional + end + end + end + if active_record_supports_required_for_associations? - it 'assumes it is required' do - expect(belonging_to_parent).to belong_to(:parent).required - end - else - it 'assumes it is optional' do - expect(belonging_to_parent).to belong_to(:parent).optional + context 'when qualified with nothing' do + context 'when belongs_to is configured to be required by default' do + it 'passes' do + configuring_default_belongs_to_requiredness(true) do + expect(belonging_to_parent).to belong_to(:parent) + end + end + end + + context 'when belongs_to is not configured to be required by default' do + it 'passes' do + configuring_default_belongs_to_requiredness(false) do + expect(belonging_to_parent).to belong_to(:parent) + end + end + end end end end - context 'given an association with a matching :required option' do - it 'passes' do - expect(belonging_to_parent(required: true)). - to belong_to(:parent).required - end - end - - context 'given an association with a non-matching :required option' do - it 'fails with an appropriate message' do - assertion = lambda do - expect(belonging_to_parent(required: false)). - to belong_to(:parent).required + context 'given the association is configured with required: true' do + context 'when qualified with required(true)' do + it 'passes' do + expect(belonging_to_parent(required: true)). + to belong_to(:parent).required(true) end + end - message = - 'Expected Child to have a belongs_to association called parent ' + - '(the association should have been defined with `required: true`, ' + - 'but was not)' + context 'when qualified with required(false)' do + it 'passes' do + assertion = lambda do + expect(belonging_to_parent(required: true)). + to belong_to(:parent).required(false) + end - expect(&assertion).to fail_with_message(message) + message = format_message(<<-MESSAGE, one_line: true) + Expected Child to have a belongs_to association called parent (the + association should have been defined with `required: false`, but + was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end + end + + context 'when qualified with optional' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(belonging_to_parent(required: true)). + to belong_to(:parent).optional + 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 + + context 'when qualified with nothing' do + if active_record_supports_required_for_associations? + it 'passes' do + 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? - context 'given an association with a matching :optional option' do - it 'passes' do - expect(belonging_to_parent(optional: true)). - to belong_to(:parent).optional - end - end + context 'given the association is configured as optional: true' do + context 'when qualified with required(true)' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(belonging_to_parent(optional: true)). + to belong_to(:parent).required(true) + end - context 'given an association with a non-matching :optional option' do - it 'fails with an appropriate message' do - assertion = lambda do - expect(belonging_to_parent(optional: false)). + message = format_message(<<-MESSAGE, one_line: true) + Expected Child to have a belongs_to association called parent (the + association should have been defined with `required: true`, but + was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end + end + + context 'when qualified with required(false)' do + it 'passes' do + expect(belonging_to_parent(optional: true)). + to belong_to(:parent).required(false) + end + end + + context 'when qualified with optional' do + it 'passes' do + expect(belonging_to_parent(optional: true)). to belong_to(:parent).optional end + end - message = - 'Expected Child to have a belongs_to association called parent ' + - '(the association should have been defined with `optional: ' + - 'true`, but was not)' + context 'when qualified with nothing' do + it 'fails with an appropriate message' do + assertion = lambda do + expect(belonging_to_parent(optional: true)). + to belong_to(:parent) + end - expect(&assertion).to fail_with_message(message) + message = format_message(<<-MESSAGE, one_line: true) + Expected Child to have a belongs_to association called parent (the + association should have been defined with `required: true`, but + was not) + MESSAGE + + expect(&assertion).to fail_with_message(message) + end end end end @@ -1342,4 +1538,21 @@ describe Shoulda::Matchers::ActiveRecord::AssociationMatcher, type: :model do [:destroy, :delete, :nullify, :restrict] end end + + def configuring_default_belongs_to_requiredness(value, &block) + configuring_application( + ActiveRecord::Base, + :belongs_to_required_by_default, + value, + &block + ) + end + + def configuring_application(config, name, value) + previous_value = config.send(name) + config.send("#{name}=", value) + yield + ensure + config.send("#{name}=", previous_value) + end end