Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix thousand separator issue in validation #712

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

RubenIgnacio
Copy link

@RubenIgnacio RubenIgnacio commented Jan 24, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant