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

Add/Edit a currency with the CLDR implementation #14972

Merged
merged 77 commits into from Nov 12, 2019

Conversation

@jolelievre
Copy link
Contributor

jolelievre commented Aug 2, 2019

Questions Answers
Branch? develop
Description? Allow edition of Currency, and creation of custom currencies
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15752
How to test? You can look in the issue the expected behavior. For test scenarii you can also base them on the behat file https://github.com/PrestaShop/PrestaShop/blob/ced3b4fa384c1017b360c3f131cbd99325692cdd/tests/Integration/Behaviour/Features/Scenario/Currency/currency_management.feature which explain every use cases (some of them might not be testable in the back office because some fields will be forced to read only)

This change is Reviewable

@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 2, 2019
@jolelievre jolelievre changed the title Add/Edit a currency with the CLDR implementation [WIP] Add/Edit a currency with the CLDR implementation Aug 5, 2019
@jolelievre jolelievre force-pushed the jolelievre:currency-edition branch from 6a14bc4 to cd3248a Aug 12, 2019
@jolelievre jolelievre changed the title [WIP] Add/Edit a currency with the CLDR implementation Add/Edit a currency with the CLDR implementation Aug 12, 2019
Copy link
Contributor Author

jolelievre left a comment

I lacked a few informations about business logic (mainly what fields are allowed to modify? is a custom currency custom forever?) So I try to deduce them, they can all be found in the human readable file from behat:
https://github.com/PrestaShop/PrestaShop/blob/ced3b4fa384c1017b360c3f131cbd99325692cdd/tests/Integration/Behaviour/Features/Scenario/Currency/currency_management.feature

I will need a validation from the @MatShir @colinegin @samuel-pires @marionf ?

Also @eternoendless I found in the codebase different places where the legacy object classes are used (in install process, language installation). I think it could be a good idea to use the command/handler everywhere to centralize every currency manipulation (validated via behat scenarii). Could it be possible to create an issue to make a refacto and use these commands everywhere in the codebase?

@MatShir

This comment has been minimized.

Copy link

MatShir commented Aug 14, 2019

is a custom currency custom forever?

What do you mean? If in a certain point it becomes an official one on his own CLDR database?

So I try to deduce them, they can all be found in the human readable file from behat:
https://github.com/PrestaShop/PrestaShop/blob/ced3b4fa384c1017b360c3f131cbd99325692cdd/tests/Integration/Behaviour/Features/Scenario/Currency/currency_management.feature

The behat tests seem good. One question, the specs and design included the numeric iso code but is it a requirement to create a currency?
I think the iso code (alpha) currency should be enough.
Wdyt? @jolelievre

jolelievre added 25 commits Oct 14, 2019
…r to use it, also fix cache subscriber service definition
…ormatted, remove toArray from ReferenceCurrency
@jolelievre jolelievre force-pushed the jolelievre:currency-edition branch from f12b371 to 5e27391 Nov 12, 2019
@Progi1984 Progi1984 merged commit a1aa963 into PrestaShop:develop Nov 12, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 4 files, 11 discussions left (colinegin, eternoendless, PierreRambaud, PrestaShop, sarahdib, TristanLDD)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.