Fix thousand separator issue in validation

[RubenIgnacio]

When validating a model with a monetized field retrieved from the database, a validation error is raised if the field is not set before the model is validated when a validation with a limit is applied, regardless of whether the monetized field already has a value. For example:

class Product < ActiveRecord::Base
  monetize :price_cents, numericality: { less_than_or_equal_to: 1000 }
end

This issue occurs specifically with currencies that use a dot (.) as a thousand separator, such as EUR. Consider a scenario where a product with a price of 1,200 EUR is retrieved from the database. When attempting to validate it, a validation error occurs due to the numericality limit:

product = Product.find(1) # <Product id: 1, price_cents: 120000, price_currency: "EUR">
product.valid? # => false

# However, if the price is set before validation, the operation succeeds:
product.price = 1200
product.valid? # => true

During the validation process, the MoneyValidator#validate_each method is invoked to verify whether raw_value.nil? && record.public_send(subunit_attr) is true. When this condition is true, raw_value is calculated as subunit_value.to_f / currency.subunit_to_unit, resulting in a float value.

Subsequently, when the generate_details method is called, it determines the currency's thousand separator in order to instantiate the Details object. For currencies that use a dot (.) as a thousand separator, the Details object, when passed to the normalize method, converts the raw_value to a string and removes the thousand separator.

This conversion process results in the original float value of 120.0 for raw_value being transformed into the string '120.0'. However, after the thousand separator is removed, this string is misinterpreted as '12000', ultimately resulting in a validation failure when a numericality limit is enforced.

Proposed Solution

Replace the following code:

# WARNING: Currently this is only defined in ActiveRecord extension!
before_type_cast = :"#{attr}_money_before_type_cast"
raw_value = record.try(before_type_cast)

# If raw value is nil and changed subunit is nil, then
# nil is a assigned value, else we should treat the
# subunit value as the one assigned.
if raw_value.nil? && record.public_send(subunit_attr)
  subunit_value = record.public_send(subunit_attr)
  raw_value = subunit_value.to_f / currency.subunit_to_unit
end

with this code:

# WARNING: Currently this is only defined in ActiveRecord extension!
before_type_cast = :"#{attr}_money_before_type_cast"
raw_value = record.try(before_type_cast) || record.public_send(attr)

This change ensures that if the before_type_cast attribute is nil, the method will call the attribute directly to retrieve the current value. Since the retrieved value is a Money object, it is correctly formatted when converted to a string, avoiding the issue caused by misinterpretation of the thousand separator.

Changes included in this PR:

• Organize monetizable specs to improve readability.
• Add tests to validate a monetizable field that uses a currency with a dot as a thousand separator.
• Fix the issue when validating a monetizable field that uses a currency with a dot as a thousand separator.

[sunny]

Hi @RubenIgnacio this is a bit hard to review as it mixes a fix as well as multiple linting changes. Ideally we would split the lint changes into their own PRs, see also RubyMoney/money#1134 🙏🏻

[RubenIgnacio]

Hi @RubenIgnacio this is a bit hard to review as it mixes a fix as well as multiple linting changes. Ideally we would split the lint changes into their own PRs, see also RubyMoney/money#1134 🙏🏻

You're right, I made some extra changes in the middle. I can create a PR first to organize the RSpec file, and then another one for the fix. I did the reorganization because I had trouble finding where to put my changes at first, but let me know if you'd prefer I only focus on the fix.

[sunny]

Ideally I’d say to go for the fix first as we will soon do a new release and it would be nice to include this if we can.

As for the lint changes, the best would be to split it into several PRs if possible.

[RubenIgnacio]

ok, got it, I'll start from scratch to keep the history clean and take the chance to improve the fix, should I force-push the fix to this same branch or close this PR and open a new one? I'll handle the linting changes in separate PRs afterwards.

[sunny]

Thank you 🙏🏻

should I force-push the fix to this same branch or close this PR and open a new one?

As you wish!

[RubenIgnacio]

I've been reviewing my fix and realized that the issue was specific to my local setup. There are existing configurations that solve this without needing these changes, so I'll close this PR. Sorry for the trouble!

I also noticed that those methods could be moved to the Details class to improve clarity.