Allow decimal numbers in transaction collections

[kidhab]

If fixes one of the issues of #722.
Tested with point and comma as delimeter.

[kidhab]

@paroga's comment is still valid. Tested with these browsers:

Browser	Locales	accepted delimeters	note
Chromium	English	point	not possible to enter a comma at all
Chromium	German	comma, point
Firefox	English	point	notification when inserting a comma
Firefox	German	comma, point
AOSP default browser	German	comma, point

Despite these browser specific issues I'm in favor of merging this commit: With the current field the Foodsoft silently changes comma seperated numbers to integer values. That's a big problem for Foodcoops that use this feature a lot and are not aware of this behaviour.

[wvengen]

Thanks for doing these tests! Entering numbers looks fine.

I think a problem could be that when a validation fails, and Rails renders the number input, it may render the number in a locale-specific way, breaking the recognition of the number input when the page with error is shown. Not sure if this actually happens, but would be good to check.

[kidhab]

With <input type="number"> the validation runs on client side/browser. In my understanding a browser prevents from entering anything else than numbers (+delimeter). In what cases could a validation fail?

[wvengen]

When a rails validation would fail, e.g.when the amount is >100000 or <-100000.

[kidhab]

Would a rage prevent this failure? Like:

number_field_tag 'financial_transactions[][amount]', nil, class: 'input-small', step: 0.01, in: -100000..100000

[wvengen]

It would prevent this particular failure. But if anyone changes the financial transactions model (e.g. even with a plugin), the issue may still appear - and be hard to debug. I guess we could add it with a BIG WARNING in the model about this situation.

But I'm not sure the issue I suspected could happen actually happens at all - would need to test!

[paroga]

one problem with using <input type="number"> is when editing existing values: in e.g. german rails prints numbers with , as decimal delimiter, but the <input> only understands ., so the browser removes the decimal points. you should see that problem in your code if you enter a decimal value and then a trigger a validation (e.g. missing note), so that the same page gets rendered with an existing decimal value
i personally don't see a good solution for this other than finally replacing the problematic localize_input gem. (see also #613)
i'm strongly object in having different input methods (number vs text field) on different pages, since the already hard to maintain code gets significant worse with such a change IMO. adding an additional hack/workaround on top of broken localize_input code does not sound good to me.

[kidhab]

You know the internals of the Foodsoft + Rails better than me - that is out of question. Though I want address some of your points:
The commit changes the input form of one specific page. I can't see a situation where this form is used to edit existing values. If a validation is triggered - which I think is only possible if the note field is left blank - the pages doesn't get rendered again (as far as I understand it).
It also seams to me that a number_field is basically the same as a text_field with the only difference that a validation in the browser happens (by using special HTML syntax for this field) or is there some internal Ruby/Rails logic connected with this type of field?

It think silently removing the decimal numbers (that's the case now) is much worse from a Foodcoop's point of view as it could mean to lost money in a significant amount (by not charging the decimal numbers for a lot of orders groups over a long period). Actually this issue is based on feedback from users of the global Foodsoft hosting that came across this behavoiur of the Foodsoft.

Anyway: Could you explain what steps are necessary to replace the localize_input gem? Thanks a lot.

[paroga]

please don't get me wrong. i really like your effort in improving it. i got a lot of feedback to for this issue too and it's very annoying.

if you just want to fix the problem on exactly this page, then i'd suggest to do it in a similar ugly way, as we do in the other places. "just" add a call to the localize_input code before converting the input to a float. i'd prefer that, since it would work in a similar way as all the other existing code, compared to introducing an additional variant (which nobody will usually test with all different kind of browsers, which very likely will lead to some bugs, when somebody will change the code there).
if you want to go that path, i'd suggest to move the code of the gem into the foodsoft code and remove the dependency first, since it would require some changes anyway later and depending on an unmaintained gem with only a few lines of code does not make a lot of sense too IMHO

Anyway: Could you explain what steps are necessary to replace the localize_input gem? Thanks a lot.

i started to remove localize_input in my deloc branch, but it did not work everywhere as it should (can't remember which browser version was the main problem, since it's already 2 years old). finishing that would of course be the better and cleaner solution, but would require significant more work. but maybe there is already a better way to do that integrated into rails itself

[kidhab]

I think a problem could be that when a validation fails, and Rails renders the number input, it may render the number in a locale-specific way, [..]

I just tested this: it is not an issue here, since the form is not validated by the backend at all (i.e. non-numeric strings are silently accepted and treated as zero). This is a separate issue, which I will not address in this proposal.
I'm not sure if your point that a validation can lead to problems is still valid if there is no existing validation at the backend.

if you just want to fix the problem on exactly this page, then i'd suggest to do it in a similar ugly way, as we do in the other places. "just" add a call to the localize_input code before converting the input to a float.

Correct me if I'm wrong: The financial_transaction model already localizes the amount. The specific form isn't part of the model, is it?

if you want to go that path, i'd suggest to move the code of the gem into the foodsoft code and remove the dependency first, [..]

Thank you for the suggestion, unfortunately that goes beyond my knowledge of Rails at the moment.

i started to remove localize_input in my deloc branch, but it did not work everywhere as it should (can't remember which browser version was the main problem, since it's already 2 years old).

While you are looking for a sustainable solution my approach is to address this particular issue.
I guess, you are aware of the mental traps, that such a situation may cause even to innocent and well-meaning brains? :)
(i.e. dead-lock due to "Perfect is the enemy of good" and "Sunk Cost")

[paroga]

since the form is not validated by the backend at all

ok, sorry for the confusion. my bad.

The financial_transaction model already localizes the amount. The specific form isn't part of the model, is it?

yes, that's the problem here (as far as i understand it now)

Thank you for the suggestion, unfortunately that goes beyond my knowledge of Rails at the moment.

I've done it for you in this commit. Can you do the rest?

I guess, you are aware of the mental traps, that such a situation may cause even to innocent and well-meaning brains? :)
(i.e. dead-lock due to "Perfect is the enemy of good" and "Sunk Cost")

100% agree and that's the reason why i proposed the "ugly" version. i think it's far better to have the same ugliness for parsing amounts everywhere, than to have to deal with different behavior on different pages.

[paroga]

i don't think that my PR is a clean implementation, but it keeps the current behavior and should not introduce new compatibility problems, while still allowing localized input for transaction collections