diff options
Diffstat (limited to 'activerecord')
6 files changed, 25 insertions, 19 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5f5c9e4915..186c7bf244 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,10 @@ ## Rails 4.0.0 (unreleased) ## +* Fix bug with presence validation of associations. Would incorrectly add duplicated errors + when the association was blank. Bug introduced in 1fab518c6a75dac5773654646eb724a59741bc13. + + *Scott Willson* + * Fix bug where sum(expression) returns string '0' for no matching records Fixes #7439 diff --git a/activerecord/lib/active_record/attribute_assignment.rb b/activerecord/lib/active_record/attribute_assignment.rb index af13b75a9d..6c5e2ac05d 100644 --- a/activerecord/lib/active_record/attribute_assignment.rb +++ b/activerecord/lib/active_record/attribute_assignment.rb @@ -57,9 +57,8 @@ module ActiveRecord # by calling new on the column type or aggregation type (through composed_of) object with these parameters. # So having the pairs written_on(1) = "2004", written_on(2) = "6", written_on(3) = "24", will instantiate # written_on (a date type) with Date.new("2004", "6", "24"). You can also specify a typecast character in the - # parentheses to have the parameters typecasted before they're used in the constructor. Use i for Fixnum, - # f for Float, s for String, and a for Array. If all the values for a given attribute are empty, the - # attribute will be set to +nil+. + # parentheses to have the parameters typecasted before they're used in the constructor. Use i for Fixnum and + # f for Float. If all the values for a given attribute are empty, the attribute will be set to +nil+. def assign_multiparameter_attributes(pairs) execute_callstack_for_multiparameter_attributes( extract_callstack_for_multiparameter_attributes(pairs) diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 6317004631..a7d2f4bd24 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -343,7 +343,7 @@ module ActiveRecord def column_for(field) field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split('.').last - @klass.columns.detect { |c| c.name.to_s == field_name } + @klass.columns_hash[field_name] end def type_cast_calculated_value(value, column, operation = nil) diff --git a/activerecord/lib/active_record/validations/presence.rb b/activerecord/lib/active_record/validations/presence.rb index 81a3521d24..6b14c39686 100644 --- a/activerecord/lib/active_record/validations/presence.rb +++ b/activerecord/lib/active_record/validations/presence.rb @@ -5,8 +5,10 @@ module ActiveRecord super attributes.each do |attribute| next unless record.class.reflect_on_association(attribute) - value = record.send(attribute) - if Array(value).all? { |r| r.marked_for_destruction? } + associated_records = Array(record.send(attribute)) + + # Superclass validates presence. Ensure present records aren't about to be destroyed. + if associated_records.present? && associated_records.all? { |r| r.marked_for_destruction? } record.errors.add(attribute, :blank, options) end end diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index de4902f89a..abbf2a765e 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -6,6 +6,8 @@ require 'models/edge' require 'models/organization' require 'models/possession' require 'models/topic' +require 'models/minivan' +require 'models/speedometer' Company.has_many :accounts @@ -239,21 +241,12 @@ class CalculationsTest < ActiveRecord::TestCase end def test_should_group_by_association_with_non_numeric_foreign_key - ActiveRecord::Base.connection.expects(:select_all).returns([{"count_all" => 1, "firm_id" => "ABC"}]) + Speedometer.create! id: 'ABC' + Minivan.create! id: 'OMG', speedometer_id: 'ABC' - firm = mock() - firm.expects(:id).returns("ABC") - firm.expects(:class).returns(Firm) - Company.expects(:find).with(["ABC"]).returns([firm]) - - column = mock() - column.expects(:name).at_least_once.returns(:firm_id) - column.expects(:type_cast).with("ABC").returns("ABC") - Account.expects(:columns).at_least_once.returns([column]) - - c = Account.group(:firm).count(:all) + c = Minivan.group(:speedometer).count(:all) first_key = c.keys.first - assert_equal Firm, first_key.class + assert_equal Speedometer, first_key.class assert_equal 1, c[first_key] end diff --git a/activerecord/test/cases/validations/presence_validation_test.rb b/activerecord/test/cases/validations/presence_validation_test.rb index cd9175f454..1de8934406 100644 --- a/activerecord/test/cases/validations/presence_validation_test.rb +++ b/activerecord/test/cases/validations/presence_validation_test.rb @@ -18,6 +18,13 @@ class PresenceValidationTest < ActiveRecord::TestCase assert b.valid? end + def test_validates_presence_of_has_one + Boy.validates_presence_of(:face) + b = Boy.new + assert b.invalid?, "should not be valid if has_one association missing" + assert_equal 1, b.errors[:face].size, "validates_presence_of should only add one error" + end + def test_validates_presence_of_has_one_marked_for_destruction Boy.validates_presence_of(:face) b = Boy.new |