Fix thousand separator issue in validation

.gitignore

@@ -8,6 +8,7 @@ spec/dummy/db/schema.rb
 .rvmrc
 .rbenv-version
 .ruby-version
+.ruby-gemset
 
 # dummy dbs
 *.sqlite3*

lib/money-rails/active_model/validator.rb

@@ -9,23 +9,44 @@ def abs_raw_value
         def decimal_pieces
           @decimal_pieces ||= abs_raw_value.split(decimal_mark)
         end
+
+        def has_too_many_decimal_points?
+          decimal_pieces.length > 2
+        end
+
+        def thousand_separator_after_decimal_mark?
+          return false unless thousands_separator.present?
+
+          decimal_pieces.length == 2 && decimal_pieces[1].include?(thousands_separator)
+        end
+
+        def invalid_thousands_separation?
+          pieces_array = decimal_pieces[0].split(thousands_separator.presence)
+
+          return false if pieces_array.length <= 1
+          return true  if pieces_array[0].length > 3
+
+          pieces_array[1..-1].any? do |thousands_group|
+            thousands_group.length != 3
+          end
+        end
+
+        # Remove thousands separators, normalize decimal mark,
+        # remove whitespaces and _ (E.g. 99 999 999 or 12_300_200.20)
+        def normalize
+          raw_value.to_s
+                   .gsub(thousands_separator, '')
+                   .gsub(decimal_mark, '.')
+                   .gsub(/[\s_]/, '')
+        end
       end
 
       def validate_each(record, attr, _value)
-        subunit_attr = record.class.monetized_attributes[attr.to_s]
         currency = record.public_send("currency_for_#{attr}")
 
         # 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
+        raw_value = record.try(before_type_cast) || record.public_send(attr)
 
         return if options[:allow_nil] && raw_value.nil?
 
@@ -44,17 +65,16 @@ def validate_each(record, attr, _value)
         # Cache abs_raw_value before normalizing because it's used in
         # many places and relies on the original raw_value.
         details = generate_details(raw_value, currency)
-        normalized_raw_value = normalize(details)
+        normalized_raw_value = details.normalize
 
         super(record, attr, normalized_raw_value)
 
         return unless stringy
         return if record_already_has_error?(record, attr, normalized_raw_value)
 
-        add_error!(record, attr, details) if
-          value_has_too_many_decimal_points(details) ||
-          thousand_separator_after_decimal_mark(details) ||
-          invalid_thousands_separation(details)
+        add_error!(record, attr, details) if details.has_too_many_decimal_points? ||
+                                             details.thousand_separator_after_decimal_mark? ||
+                                             details.invalid_thousands_separation?
       end
 
       private
@@ -87,37 +107,6 @@ def add_error!(record, attr, details)
         )
       end
 
-      def value_has_too_many_decimal_points(details)
-        ![1, 2].include?(details.decimal_pieces.length)
-      end
-
-      def thousand_separator_after_decimal_mark(details)
-        details.thousands_separator.present? &&
-          details.decimal_pieces.length == 2 &&
-          details.decimal_pieces[1].include?(details.thousands_separator)
-      end
-
-      def invalid_thousands_separation(details)
-        pieces_array = details.decimal_pieces[0].split(details.thousands_separator.presence)
-
-        return false if pieces_array.length <= 1
-        return true  if pieces_array[0].length > 3
-
-        pieces_array[1..-1].any? do |thousands_group|
-          thousands_group.length != 3
-        end
-      end
-
-      # Remove thousands separators, normalize decimal mark,
-      # remove whitespaces and _ (E.g. 99 999 999 or 12_300_200.20)
-      def normalize(details)
-        details.raw_value
-          .to_s
-          .gsub(details.thousands_separator, '')
-          .gsub(details.decimal_mark, '.')
-          .gsub(/[\s_]/, '')
-      end
-
       def lookup(key, currency)
         if locale_backend
           locale_backend.lookup(key, currency) || DEFAULTS[key]

lib/money-rails/active_record/monetizable.rb

@@ -12,11 +12,9 @@ module ClassMethods
         def monetized_attributes
           monetized_attributes = @monetized_attributes || {}.with_indifferent_access
 
-          if superclass.respond_to?(:monetized_attributes)
-            monetized_attributes.merge(superclass.monetized_attributes)
-          else
-            monetized_attributes
-          end
+          return monetized_attributes unless superclass.respond_to?(:monetized_attributes)
+
+          monetized_attributes.merge(superclass.monetized_attributes)
         end
 
         def monetize(*fields)
@@ -32,7 +30,7 @@ def monetize(*fields)
                                               ":with_currency or :with_model_currency")
             end
 
-            name = options[:as] || options[:target_name] || nil
+            name = options[:as] || options[:target_name]
 
             # Form target name for the money backed ActiveModel field:
             # if a target name is provided then use it
@@ -46,34 +44,38 @@ def monetize(*fields)
               if name == subunit_name
                 raise ArgumentError, "monetizable attribute name cannot be the same as options[:as] parameter"
               end
-
-            elsif subunit_name =~ /#{MoneyRails::Configuration.amount_column[:postfix]}$/
-              name = subunit_name.sub(/#{MoneyRails::Configuration.amount_column[:postfix]}$/, "")
             else
-              raise ArgumentError, "Unable to infer the name of the monetizable attribute for '#{subunit_name}'. " \
-                                   "Expected amount column postfix is '#{MoneyRails::Configuration.amount_column[:postfix]}'. " \
-                                   "Use :as option to explicitly specify the name or change the amount column postfix in the initializer."
+              amount_column_postfix = MoneyRails::Configuration.amount_column[:postfix]
+
+              if subunit_name =~ /#{amount_column_postfix}$/
+                name = subunit_name.sub(/#{amount_column_postfix}$/, "")
+              else
+                raise ArgumentError, "Unable to infer the name of the monetizable attribute for '#{subunit_name}'. " \
+                                    "Expected amount column postfix is '#{amount_column_postfix}'. " \
+                                    "Use :as option to explicitly specify the name or change the amount column postfix in the initializer."
+              end
             end
 
             # Optional accessor to be run on an instance to detect currency
             instance_currency_name = options[:with_model_currency] ||
-              options[:model_currency] ||
-              MoneyRails::Configuration.currency_column[:column_name]
+                                     options[:model_currency] ||
+                                     MoneyRails::Configuration.currency_column[:column_name]
 
             # Infer currency column from name and postfix
-            if !instance_currency_name && MoneyRails::Configuration.currency_column[:postfix].present?
-              instance_currency_name = "#{name}#{MoneyRails::Configuration.currency_column[:postfix]}"
-            end
+            currency_column_postfix = MoneyRails::Configuration.currency_column[:postfix]
 
-            instance_currency_name = instance_currency_name && instance_currency_name.to_s
+            if instance_currency_name
+              instance_currency_name = instance_currency_name.to_s
+            elsif currency_column_postfix.present?
+              instance_currency_name = "#{name}#{currency_column_postfix}"
+            end
 
             # This attribute allows per column currency values
             # Overrides row and default currency
-            field_currency_name = options[:with_currency] ||
-              options[:field_currency] || nil
+            field_currency_name = options[:with_currency] || options[:field_currency]
 
             # Create a reverse mapping of the monetized attributes
-            track_monetized_attribute name, subunit_name
+            track_monetized_attribute(name, subunit_name)
 
             # Include numericality validations if needed.
             # There are two validation options:
@@ -97,56 +99,54 @@ def monetize(*fields)
             #
             # To disable validation entirely, use :disable_validation, E.g:
             #   monetize :price_in_a_range_cents, disable_validation: true
-            if (validation_enabled = MoneyRails.include_validations && !options[:disable_validation])
-
-              # This is a validation for the subunit
-              if (subunit_numericality = options.fetch(:subunit_numericality, true))
-                validates subunit_name, {
-                  allow_nil: options[:allow_nil],
-                  numericality: subunit_numericality
-                }
+            validation_enabled = MoneyRails.include_validations && !options[:disable_validation]
+
+            if validation_enabled
+              validate_subunit_numericality = options.fetch(:subunit_numericality, true)
+              validate_numericality = options.fetch(:numericality, true)
+
+              if validate_subunit_numericality
+                validates subunit_name, allow_nil: options[:allow_nil],
+                                        numericality: validate_subunit_numericality
               end
 
               # Allow only Money objects or Numeric values!
-              if (numericality = options.fetch(:numericality, true))
-                validates name.to_sym, {
-                  allow_nil: options[:allow_nil],
-                  'money_rails/active_model/money' => numericality
-                }
+              if validate_numericality
+                validates name.to_sym, allow_nil: options[:allow_nil],
+                                       'money_rails/active_model/money' => validate_numericality
               end
             end
 
-
             # Getter for monetized attribute
             define_method name do |*args, **kwargs|
-              read_monetized name, subunit_name, options, *args, **kwargs
+              read_monetized(name, subunit_name, options, *args, **kwargs)
             end
 
             # Setter for monetized attribute
             define_method "#{name}=" do |value|
-              write_monetized name, subunit_name, value, validation_enabled, instance_currency_name, options
+              write_monetized(name, subunit_name, value, validation_enabled, instance_currency_name, options)
             end
 
             if validation_enabled
               # Ensure that the before_type_cast value is cleared when setting
               # the subunit value directly
               define_method "#{subunit_name}=" do |value|
-                instance_variable_set "@#{name}_money_before_type_cast", nil
+                instance_variable_set("@#{name}_money_before_type_cast", nil)
                 write_attribute(subunit_name, value)
               end
             end
 
             # Currency getter
             define_method "currency_for_#{name}" do
-              currency_for name, instance_currency_name, field_currency_name
+              currency_for(name, instance_currency_name, field_currency_name)
             end
 
             attr_reader "#{name}_money_before_type_cast"
 
             # Hook to ensure the reset of before_type_cast attr
             # TODO: think of a better way to avoid this
             after_save do
-              instance_variable_set "@#{name}_money_before_type_cast", nil
+              instance_variable_set("@#{name}_money_before_type_cast", nil)
             end
           end
         end
@@ -185,12 +185,12 @@ def read_monetized(name, subunit_name, options = nil, *args, **kwargs)
           kwargs = {}
         end
 
-        if kwargs.any?
-          amount = public_send(subunit_name, *args, **kwargs)
-        else
-          # Ruby 2.x does not allow empty kwargs
-          amount = public_send(subunit_name, *args)
-        end
+        amount = if kwargs.any?
+                   public_send(subunit_name, *args, **kwargs)
+                 else
+                   # Ruby 2.x does not allow empty kwargs
+                   public_send(subunit_name, *args)
+                 end
 
         return if amount.nil? && options[:allow_nil]
         # Get the currency object
@@ -218,7 +218,8 @@ def read_monetized(name, subunit_name, options = nil, *args, **kwargs)
         end
 
         if MoneyRails::Configuration.preserve_user_input
-          value_before_type_cast = instance_variable_get "@#{name}_money_before_type_cast"
+          value_before_type_cast = instance_variable_get("@#{name}_money_before_type_cast")
+
           if errors.has_key?(name.to_sym)
             result.define_singleton_method(:to_s) { value_before_type_cast }
             result.define_singleton_method(:format) { |_| value_before_type_cast }
@@ -230,23 +231,21 @@ def read_monetized(name, subunit_name, options = nil, *args, **kwargs)
 
       def write_monetized(name, subunit_name, value, validation_enabled, instance_currency_name, options)
         # Keep before_type_cast value as a reference to original input
-        instance_variable_set "@#{name}_money_before_type_cast", value
+        instance_variable_set("@#{name}_money_before_type_cast", value)
 
         # Use nil or get a Money object
         if options[:allow_nil] && value.blank?
           money = nil
+        elsif value.is_a?(Money)
+          money = value
         else
-          if value.is_a?(Money)
-            money = value
-          else
-            begin
-              money = value.to_money(public_send("currency_for_#{name}"))
-            rescue NoMethodError
-              return nil
-            rescue Money::Currency::UnknownCurrency, Monetize::ParseError => e
-              raise MoneyRails::Error, e.message if MoneyRails.raise_error_on_money_parsing
-              return nil
-            end
+          begin
+            money = value.to_money(public_send("currency_for_#{name}"))
+          rescue NoMethodError
+            return nil
+          rescue Money::Currency::UnknownCurrency, Monetize::ParseError => e
+            raise MoneyRails::Error, e.message if MoneyRails.raise_error_on_money_parsing
+            return nil
           end
         end
 
@@ -273,26 +272,38 @@ def write_monetized(name, subunit_name, value, validation_enabled, instance_curr
             public_send("#{instance_currency_name}=", money_currency.iso_code)
           else
             current_currency = public_send("currency_for_#{name}")
+
             if current_currency != money_currency.id
-              raise ReadOnlyCurrencyException.new("Can't change readonly currency '#{current_currency}' to '#{money_currency}' for field '#{name}'") if MoneyRails.raise_error_on_money_parsing
+              if MoneyRails.raise_error_on_money_parsing
+                raise ReadOnlyCurrencyException,
+                      "Can't change readonly currency '#{current_currency}' to '#{money_currency}' for field '#{name}'"
+              end
+
               return nil
             end
           end
         end
 
         # Save and return the new Money object
-        instance_variable_set "@#{name}", money
+        instance_variable_set("@#{name}", money)
       end
 
       def currency_for(name, instance_currency_name, field_currency_name)
-        if instance_currency_name.present? && respond_to?(instance_currency_name) &&
-            Money::Currency.find(public_send(instance_currency_name))
-
-          Money::Currency.find(public_send(instance_currency_name))
-        elsif field_currency_name.respond_to?(:call)
-          Money::Currency.find(field_currency_name.call(self))
-        elsif field_currency_name
-          Money::Currency.find(field_currency_name)
+        if instance_currency_name.present? && respond_to?(instance_currency_name)
+          currency_name = public_send(instance_currency_name)
+          currency = Money::Currency.find(currency_name)
+
+          return currency if currency
+        end
+
+        if field_currency_name
+          currency_name = if field_currency_name.respond_to?(:call)
+                            field_currency_name.call(self)
+                          else
+                            field_currency_name
+                          end
+
+          Money::Currency.find(currency_name)
         elsif self.class.respond_to?(:currency)
           self.class.currency
         else