Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #13236 from jetthoughts/13230_type_cast_bug_in_agr…

…_functions

Fix type cast on group sum with custom expression
  • Loading branch information...
commit 821153525cefb1a809be06ae584f24f5c0c11323 2 parents 900191f + 2a7fe7a
@senny senny authored
View
16 activerecord/CHANGELOG.md
@@ -1,3 +1,19 @@
+* Use the right column to type cast grouped calculations with custom expressions.
+
+ Fixes: #13230
+
+ Example:
+
+ # Before
+ Account.group(:firm_name).sum('0.01 * credit_limit')
+ # => { '37signals' => '0.5' }
+
+ # After
+ Account.group(:firm_name).sum('0.01 * credit_limit')
+ # => { '37signals' => 0.5 }
+
+ *Paul Nikitochkin*
+
* Polymorphic `belongs_to` associations with the `touch: true` option set update the timestamps of
the old and new owner correctly when moved between owners of different types.
View
4 activerecord/lib/active_record/relation/calculations.rb
@@ -311,7 +311,9 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
}
key = key.first if key.size == 1
key = key_records[key] if associated
- [key, type_cast_calculated_value(row[aggregate_alias], column_for(column_name), operation)]
+
+ column_type = calculated_data.column_types.fetch(aggregate_alias) { column_for(column_name) }
+ [key, type_cast_calculated_value(row[aggregate_alias], column_type, operation)]
end]
end
View
4 activerecord/test/cases/calculations_test.rb
@@ -211,6 +211,10 @@ def test_sum_should_return_valid_values_for_decimals
assert_equal 19.83, NumericData.sum(:bank_balance)
end
+ def test_should_return_type_casted_values_with_group_and_expression
+ assert_equal 0.5, Account.group(:firm_name).sum('0.01 * credit_limit')['37signals']
+ end
+
def test_should_group_by_summed_field_with_conditions
c = Account.where('firm_id > 1').group(:firm_id).sum(:credit_limit)
assert_nil c[1]

8 comments on commit 8211535

@senny
Owner

@acapilleri they were failing before https://travis-ci.org/rails/rails/builds/15217030 did you notice a new error? If so, can you please post the error?

@pftg

@senny, @acapilleri investigating, for now have not found tests broken by my changes.

@senny
Owner

@pftg I can't find new errors and ran the tests locally without error. Please report back if you find something related but the current master is not stable.

@acapilleri

@senny as I say to @pftg, before other patch to calculation we need to fix column_for as I try here because currently the trivial implementation of this method gives strange side effects to many cases

@senny
Owner

@acapilleri I will take a look at that when I find the time. But regarding your first comment. You linked to Travis. Are there build errors related to this merge? If so please post them.

@pftg

@senny have not found connections with my changes, too.

@acapilleri

@senny I read the error message in #rails–contrib irc so I thought that it could to be interesting to you. I will take a look at errors when I find the time

Please sign in to comment.
Something went wrong with that request. Please try again.