leikind / wice_grid

A Rails grid plugin to create grids with sorting, pagination, and (automatically generated) filters
MIT License
545 stars 214 forks source link

Support localized decimal values in ColumnInteger and ColumnRange filters #339

Closed kreintjes closed 7 years ago

kreintjes commented 7 years ago

Also do not crash or give invalid text representations of numbers to PostgreSQL. Fixes https://github.com/leikind/wice_grid/issues/338. Any suggestions are welcome!

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 37.452% when pulling b304f95ab30d43cbb95e79970d2c0197df2d1fc3 on kreintjes:fix/localized-number-parsing into 2fcb600bb7f72e0c6de1b4df83babdffd9671b61 on leikind:rails3.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 37.452% when pulling cea1f7a00dd3bc1e36f59a6c54df21008c7c494f on kreintjes:fix/localized-number-parsing into 2fcb600bb7f72e0c6de1b4df83babdffd9671b61 on leikind:rails3.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 37.452% when pulling cea1f7a00dd3bc1e36f59a6c54df21008c7c494f on kreintjes:fix/localized-number-parsing into 2fcb600bb7f72e0c6de1b4df83babdffd9671b61 on leikind:rails3.

leikind commented 7 years ago

relying on I18n and presence of number.format.separator doesn't seem right to me.

Isn't to_i just enough?

irb(main):003:0> '4,4222'.to_i
4
irb(main):004:0> '4.4222'.to_i
4
kreintjes commented 7 years ago

@leikind It would for parsing to an integer, but I am trying to parse to a float/decimal. to_f and to_d aren't working, they simply ignore the comma and round the values down:

[10] pry(main)> '1,234'.to_f
=> 1.0
[11] pry(main)> '1,234'.to_d
=> 0.1e1

Relying on I18n does not seem a problem to me. Wice_grid has activerecord as dependency, which has activesupport as dependency, which has i18n as dependency. So I18n should be present in any environment in which wice_grid is used. The same goes for number.format.separator since it is a default Rails key. However, if you would like, I could build in a check for I18n and number.format.separator being present.

Another solution would be to add a configuration variable to wice grid to set the decimal separator. Although that would lead to additional configuration, while this works out of the box. Let me know which solution you prefer.

leikind commented 7 years ago

let's have some fallback behavior / default separator(s) if number.format.separator is missing and I'll merge it

kreintjes commented 7 years ago

@leikind Done. When I18n or separator is not present, there is no replacing necessary (dot will already be the likely separator). So when looking up number.format.separator fails, either because I18n or the translation is missing, it simply skips the replacing.

Ps. Your gem is great when having an application which needs many tables with basic sorting, filtering and exporting mechanisms. Thanks!

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.06%) to 37.415% when pulling 471be6bebf7203cd370cea88cbccc0d392aed580 on kreintjes:fix/localized-number-parsing into 5d31d46b842f3f9b5b4a28d0e83fd6132cb76a25 on leikind:rails3.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.06%) to 37.415% when pulling 471be6bebf7203cd370cea88cbccc0d392aed580 on kreintjes:fix/localized-number-parsing into 5d31d46b842f3f9b5b4a28d0e83fd6132cb76a25 on leikind:rails3.