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 js currency #1886

Merged
merged 4 commits into from Oct 13, 2017
Merged

Conversation

pdelacroix
Copy link
Contributor

@pdelacroix pdelacroix commented Sep 27, 2017

What? Why?

This PR fixes both problems raised in #1623. The issue was that serializers sent amounts of money as localized strings (to_money.to_s) so if the locale used a comma as decimal separator, parsing them in JS to perform operations (in the /account page) would fail leading either to a wrong amount (parsing stopped at the comma stripping decimals) or a NaN (not a number) error.
Here I made serializers send the amounts as unlocalized strings, and improved the localizeCurrency filter to use i18n-js toCurrency method, which as a nice "side effect" also localizes the decimal separator and spacer character.
I18n-js's toCurrency method uses translation values in number.currency.format, which are set in the rails-i18n gem.

What should we test?

The localizeCurrency filter is used in many places but the most notable page is the page at /account (see images in related issue) which now presents no error when using a locale with comma as decimal separator. In other places, amounts should also be nicely localized.

def total
object.total.to_money.to_s
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't use it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we do but it would still appear as it's declared in the attributes method. What we want is that it's like def total; object.total; end and there's no need to have this as it's like not writing the method (customizing the field) at all.

@daniellemoorhead daniellemoorhead added this to the October dot point release milestone Oct 5, 2017
Copy link
Contributor

@oeoeaio oeoeaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for testing.

@sstead
Copy link

sstead commented Oct 13, 2017

Testing Notes

  • The /Account/Transactions shop 'balance due' numbers are correct, and the tally is calculating correctly (see below)
    image

Some currency display errors we saw...
image

@RohanM RohanM merged commit 508dfa4 into openfoodfoundation:master Oct 13, 2017
@myriamboure
Copy link
Contributor

Are the currency display issues connected to that PR ? Any idea @ltrls ? It seems it is being merged so I suspect it is not related, or should we open another issue ? @sstead this is not clear to me :-o

@daniellemoorhead
Copy link
Contributor

@oeoeaio see @myriamboure's comment above. Were the currency display errors fixed by you before you merged it, or where they put into another issue to be done separately?

@oeoeaio
Copy link
Contributor

oeoeaio commented Oct 16, 2017

Sorry, I thought I responded to this on Friday. Currency display errors were not fixed, but I've just realised that the reason was probably our user error. The currency components affected by this PR were displaying correctly, but the components being rendered by Rails were not displaying correctly. I now realise that this was probably because we didn't change the configuration of currency_symbol_position in the backend. I didn't really think about it (since we don't use it) and just assumed that the locale would change this automatically.

So, in short: @myriamboure, can you confirm that this looks ok for you in France the next time you deploy? There problem is probably just a fairly non-intuitive configuration process, rather than an issue with the logic.

@myriamboure
Copy link
Contributor

@oeoeaio I can confirm we just deployed the master version and it's all good on the currency display, the € symbol is after the price on all locations, shopfront, basket, payment, order management, etc. 👍

@oeoeaio
Copy link
Contributor

oeoeaio commented Oct 16, 2017

Awesome, thanks!

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.

None yet

7 participants