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

Translate currency names #13068

Merged
merged 6 commits into from Jul 27, 2018
Merged

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Jun 16, 2018

The currency in the sites manager is not localised but shows all currency names in English.

This PR uses localised currency from CLDR, similar to what was done for timezones in #12901.

I have updated the existing list of currencies in core/Intl/Data/Resources/currencies.php to contain existing currencies based on the list in CLDR (those without a _to date). Several changes were existing currencies being replaced by another currency with the same name due to inflation.

I moved Bitcoin to global.inc.php. Bitcoin is only one of many cryptocurrencies. Currently no cryptocurrencies have official ISO codes, and it is still unclear which of them will survive in the long run, and whether they will be used as actual transaction currencies. So for the time being I don't think they belong in core Piwik (though I kept Bitcoin for backwards compatibilty). When moved to the settings, users have the ability to add the cryptocurrency du jour.

The code supports currencies in use on a site being removed in future Matomo versions. Both currency name and symbol will be displayed as the 3-letter currency code, but the site cannot be saved without updating to a valid currency code or adding the legacy currency to config.inc.php.

@c960657
Copy link
Contributor Author

c960657 commented Jun 16, 2018

AFAICT the failing tests are false positives.

@Findus23 Findus23 added the c: i18n For issues around internationalisation and localisation. label Jun 17, 2018
@c960657 c960657 force-pushed the translate-currencies branch 3 times, most recently from fe18ced to 109dd82 Compare June 17, 2018 21:06
@mattab mattab added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Jun 26, 2018
@mattab mattab added this to the 3.6.0 milestone Jun 26, 2018
@@ -179,5 +176,4 @@
'XOF' => array('Fr', 'West African CFA franc'),
'YER' => array('﷼', 'Yemeni rial'),
'ZMW' => array('ZK', 'Zambian kwacha'),
'ZWL' => array('$', 'Zimbabwean dollar'),
Copy link
Member

Choose a reason for hiding this comment

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

Did you check if removing/changing such currencies might break Matomo if a site had one of those chosen beofre? If so, we maybe should provide an update script to update all sites using one of the changed/removed currencies...

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. If a site is configured to use a removed currency, the 3-letter ISO code will be used as both the full and short name.

When editing a site with a removed currency, the user will get an error message instructing her to select another code: The currency "XXX" is not valid. Please enter a valid currency symbol (eg. USD, EUR, etc.). Alternatively, the admin can add the legacy currency to config.inc.php.

As always, changing the currency of a site will not convert the historic values, so data before/after the change will not be comparable. Fixing this is a separate issue.

@diosmosis
Copy link
Member

Thanks for the PR @c960657! Reviewed & tested locally, everything looks good to me except the behavior when a currency in use is one that is removed. Generally everything works, but the user is not notified they need to update their currency, which could result in some bug reports/support requests. Maybe a notification if the site's currency isn't recognized would be helpful (checked in Plugin\Controller). What do you think @sgiehl / @mattab / @tsteur?

@mattab
Copy link
Member

mattab commented Jul 26, 2018

Maybe a notification if the site's currency isn't recognized would be helpful (checked in Plugin\Controller). What do you think @sgiehl / @mattab / @tsteur?

It would be good to have such notification when editing the site, but as described editing a site would result in an error explaining the currency is outdated. so I don't think it is needed for this edge case. the deprecated currency's 3 letter code is still displayed so all reports should look fine in theory.

Copy link
Member

@mattab mattab left a comment

Choose a reason for hiding this comment

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

Could we restore the old currency symbols as they were in USD, EUR to use $ and € ?

Moving to 3.7.0 but we could merge it before if none of the APIs change output?

"revenue_tax": "$ ",
"revenue": "USD 0",
"revenue_discount": "USD ",
"revenue_shipping": "USD ",
Copy link
Member

Choose a reason for hiding this comment

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

this is in my opinion a regression, as users would expect to see $ 25 rather than the wordy USD 25. Could we restore the old currency symbols as they were?

Copy link
Member

Choose a reason for hiding this comment

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

It's just that the translations aren't loaded in that test. It works when they are explicitly loaded.

@mattab mattab modified the milestones: 3.6.0, 3.7.0 Jul 26, 2018
@diosmosis
Copy link
Member

@mattab since the test change doesn't represent a regression, moving back into 3.6 & will merge soon

@diosmosis diosmosis modified the milestones: 3.7.0, 3.6.0 Jul 26, 2018
@diosmosis diosmosis merged commit 09c4fcc into matomo-org:3.x-dev Jul 27, 2018
@c960657 c960657 deleted the translate-currencies branch July 27, 2018 08:56
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Translate currency names

* Update tests

* Fix more tests

* Use plural form in config key name

* Update screenshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: i18n For issues around internationalisation and localisation. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants