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

Allow CLDR to display deleted/inactive currencies #15643

Merged
merged 6 commits into from Oct 15, 2019

Conversation

@jolelievre
Copy link
Contributor

jolelievre commented Sep 23, 2019

Questions Answers
Branch? 1.7.6.x
Description? Allow CLDR to display delete currencies by fetching any currency in database regardless of its deleted or active status, and add a few behat tests for CLDR and currencies This required to add a few methods to existing interfaces thus creating a BC break But since these interfaces should mainly be used by the core for now it is relatively safe
Type? bug fix
Category? BO
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes #15376
Fixes #15486
How to test? See issues' contents for details, but mainly create orders and suppliers with prices in EUR the change the default currency and delete the EUR currency Then you should have an exception in Orders page and Suppliers page

This change is Reviewable

@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Sep 23, 2019
@jolelievre jolelievre added this to the 1.7.6.2 milestone Sep 23, 2019
@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch from 024bebf to 9386371 Sep 23, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Sep 23, 2019

@eternoendless unlike what we discussed I ended up adding new methods in the interfaces for few reasons:

  • function names were confusing as they already were named *AvailableCurrencies even becoming contradictory with isAvailableCurrency
  • even though the CLDR needs all currencies, these interfaces might be used by other services later which will really need only available currencies
  • the modified interfaces are very recent and should only be used by internal core services for now (probably)
  • although we wanted to avoid BC break by adding methods I didn't have a choice for the CurrencyDataProviderInterface anyway but to add a new parameter in the prototype which is a BC break already, so I figured if we're forced to add a BC we might as well add it for clean valid reasons
classes/Currency.php Outdated Show resolved Hide resolved
*
* @return array[] Installed currencies
*/
public function findAll($currentShopOnly = true);
public function findAll($currentShopOnly = true, $filterDeleted = true);

This comment has been minimized.

Copy link
@sarjon

sarjon Sep 23, 2019

Contributor

I don't think it's a good idea to add yet another flag argument. :/

This comment has been minimized.

Copy link
@jolelievre

jolelievre Sep 24, 2019

Author Contributor

I agree with you, but initially @eternoendless asked me to add an additional parameter in order to limite the BC break. The first one was introduced in this PR #15173

In this PR I though that since this is a BC break anyway on our interfaces we might as well "break it properly", that's why I decided to add new clearer functions in the interfaces
But I need @eternoendless approval to validate this change, and if we take upon ourselves to make this change should we clean this interface a bit as well?

src/Core/Localization/Currency/CurrencyDataSource.php Outdated Show resolved Hide resolved
@jolelievre jolelievre changed the title Allow CLDR to display delete currencies, and add a few behat tests fo… Allow CLDR to display deleted/inactive currencies Sep 23, 2019
@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch 4 times, most recently from 2883cbe to 8e58b7c Oct 3, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 7, 2019

@jolelievre I see you have used several classes from develop into this PR. So when we merge 1.7.6.x into develop, we can expect some nice conflicts 😭 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 7, 2019

Code is for me . If @eternoendless can confirm once more than the BC break is acceptable, then I think we can move on the QA stage.

public static function prepare(BeforeSuiteScope $scope)
{
// Disable legacy object model cache to prevent conflicts between scenarios.
ObjectModel::disableCache();

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 7, 2019

Contributor

You fear nothing!

This comment has been minimized.

Copy link
@matks

matks Oct 8, 2019

Contributor

It actually make sense to disable cache capabilities for such tests.
The funny thing is, I'm sure that it will not disable 100% of the cache 😛 I bet there is some cannot-be-disabled cache behaviors

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 8, 2019

Author Contributor

I actually copied this code from the develop branch without even reading it 😅
But I can guarantee you @matks that it is indeed not enough.. for example I had to call Currency::getIdByIsoCode with a noCache parameter to avoid unwanted behaviour Although in that case the Cache class was used

That said although disabling the cache like this is handy, it may give us false positive tests, because everything will work fine in the test but when cache is enabled in real context then unexpected behaviour will occur I don't really know what the ObjectModel cache is supposed to do so I can't imagine the consequences, but maybe it would be safer to disable it case by case when we really need it than disabling it altogether?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 8, 2019

Contributor

Disabling the cache means it's not the same behavior as the front 🤔
Hope everything's going well!

Copy link
Member

eternoendless left a comment

Reviewed 2 of 22 files at r1, 11 of 16 files at r2.
Dismissed @PierreRambaud and @sarjon from 12 discussions.
Reviewable status: 13 of 23 files reviewed, 12 unresolved discussions (waiting on @jolelievre, @PierreRambaud, and @sarjon)


classes/Currency.php, line 433 at r2 (raw file):

    public static function findAllInDatabase()
    {
        $currencies = Db::getInstance()->executeS(

Please use Db::getInstance(_PS_USE_SQL_SLAVE_) for reads


classes/Currency.php, line 593 at r2 (raw file):

     * @param string $isoCode ISO code
     * @param int $idShop Shop ID
     * @param bool $noCache

Consider changing this parameter to $forceRefreshCache

     * @param bool $forceRefreshCache [default=false] Se to TRUE to forcefully refresh any currently cached results

src/Adapter/Currency/CommandHandler/AddCurrencyHandler.php, line 89 at r1 (raw file):

Previously, jolelievre wrote…

What he said 😅

This value is a string, not an int, should DEFINITELY NOT be casted to int


src/Adapter/Currency/CommandHandler/EditCurrencyHandler.php, line 271 at r1 (raw file):

Previously, jolelievre wrote…

Nice catch!

It shouldn't be casted to int


src/Core/Currency/CurrencyDataProviderInterface.php, line 59 at r2 (raw file):

regardless of their active or deleted status

"regardless of their active or soft deleted status"


src/Core/Currency/CurrencyDataProviderInterface.php, line 63 at r2 (raw file):

Previously, PierreRambaud (GoT) wrote…

isn't a BC if the interface has already been implemented?

Unfortunately it's necessary. It's unlikely it's been reimplemented, but it has to be duly noted in the release notes.


src/Core/Localization/Currency/DataSourceInterface.php, line 78 at r1 (raw file):

Previously, jolelievre wrote…

Totally it is, however the alternative we thought of at first with Pablo was to change the behaviour of the getAvailableCurrencies* methods and make them return all the currencies in database (wether they are active or deleted) But it bothers me because it is contradictory to the function name and can lead to misunderstanding and unexpected behaviour later

I'm uncomfortable with the InDatabase part here as this interface is used for any kind of resource. Maybe getAllCurrenciesDataIncludingDeleted($localeCode)?

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Oct 8, 2019

@jolelievre I see you have used several classes from develop into this PR. So when we merge 1.7.6.x into develop, we can expect some nice conflicts 😭 😄

Indeed I needed them to add behat tests in this branch Although some of them are exact copy so there won't be much conflict But for the other one fear not! I will take care of the merge and conflict fixing 😉
I need these modifications to start a pending issue anyway so as son as this one is merged I will merge 176x back to develop

Copy link
Contributor Author

jolelievre left a comment

Reviewable status: 13 of 23 files reviewed, 14 unresolved discussions (waiting on @eternoendless, @PierreRambaud, and @sarjon)


classes/Currency.php, line 433 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Please use Db::getInstance(_PS_USE_SQL_SLAVE_) for reads

I didn't know about this. But I don't understand it does really make sense.. I mean the const _PS_USE_SQL_SLAVE_ implies that we use slave instead of master, but the Db::getInstance has $master = true as a parameter So logically to use the slave server accordingly with the const we should call Db::getInstance(!_PS_USE_SQL_SLAVE_) or am I missing something? Because this call is present everywhere in the codebase


classes/Currency.php, line 593 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Consider changing this parameter to $forceRefreshCache

     * @param bool $forceRefreshCache [default=false] Se to TRUE to forcefully refresh any currently cached results

Done.


src/Adapter/Currency/CommandHandler/AddCurrencyHandler.php, line 89 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This value is a string, not an int, should DEFINITELY NOT be casted to int

My bad you're right! Done


src/Adapter/Currency/CommandHandler/EditCurrencyHandler.php, line 271 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

It shouldn't be casted to int

Done.


src/Core/Currency/CurrencyDataProviderInterface.php, line 59 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…
regardless of their active or deleted status

"regardless of their active or soft deleted status"

Done.


src/Core/Localization/Currency/DataSourceInterface.php, line 78 at r1 (raw file):

getAllCurrenciesDataIncludingDeleted
I get your point of view, but this starts to be a really long method name.. I wasn't fan of getAllCurrenciesDataInDatabase either I'd like to call it getAllCurrenciesData but as you said this could be interpreted as all the existing currencies in the world..
So maybe getInstalledCurrenciesData or getAllInstalledCurrenciesData could be nice? With the right PHPDoc on top of it I think it leaves out the confusion but remains relatively short


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 17 at r1 (raw file):

Previously, matks (Mathieu Ferment) wrote…

CRUD operations are meaningful in a BO, because when we click on this "submit" button there is a modification of the system and the domain. Even if it's a basic modification (it sets the data) it's worth being tested if in the past it has proven to be unstable 👍

I actually ended up siding with @sarjon proposal in order to have cleaner tests in this cldr part But in the coming issues for 177 linked to currency and their customization I intend to add a few CLDR display tests inside the currency management scenarii So this will validate the full chain nicely

@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch from 6307e32 to af963fc Oct 8, 2019
Copy link
Member

eternoendless left a comment

Reviewed 1 of 16 files at r2, 6 of 8 files at r3.
Reviewable status: 15 of 24 files reviewed, 10 unresolved discussions (waiting on @jolelievre, @PierreRambaud, and @sarjon)


classes/Currency.php, line 433 at r2 (raw file):

Previously, jolelievre wrote…

I didn't know about this. But I don't understand it does really make sense.. I mean the const _PS_USE_SQL_SLAVE_ implies that we use slave instead of master, but the Db::getInstance has $master = true as a parameter So logically to use the slave server accordingly with the const we should call Db::getInstance(!_PS_USE_SQL_SLAVE_) or am I missing something? Because this call is present everywhere in the codebase

I agree this is counterintuitive, but it looks like this constant does the opposite of what its name would indicate: https://github.com/PrestaShop/PrestaShop/blob/develop/config/defines.inc.php#L179


src/Core/Localization/Currency/DataSourceInterface.php, line 78 at r1 (raw file):

Previously, jolelievre wrote…

getAllCurrenciesDataIncludingDeleted
I get your point of view, but this starts to be a really long method name.. I wasn't fan of getAllCurrenciesDataInDatabase either I'd like to call it getAllCurrenciesData but as you said this could be interpreted as all the existing currencies in the world..
So maybe getInstalledCurrenciesData or getAllInstalledCurrenciesData could be nice? With the right PHPDoc on top of it I think it leaves out the confusion but remains relatively short

Okay that sounds good

Copy link
Contributor Author

jolelievre left a comment

Reviewable status: 15 of 24 files reviewed, 10 unresolved discussions (waiting on @eternoendless, @PierreRambaud, and @sarjon)


classes/Currency.php, line 433 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I agree this is counterintuitive, but it looks like this constant does the opposite of what its name would indicate: https://github.com/PrestaShop/PrestaShop/blob/develop/config/defines.inc.php#L179

Yeah I saw the define but it's still counter-intuitive ^^ So, considering this default definition, actually the slave connection is used by default? Maybe a better comment would be useful here?


src/Core/Localization/Currency/DataSourceInterface.php, line 78 at r1 (raw file):

getAllInstalledCurrenciesData

Alright I'll go with getAllInstalledCurrenciesData then

@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch from 16ad99c to dc6023d Oct 8, 2019
@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch 2 times, most recently from bfbeff5 to 617b844 Oct 10, 2019
@jolelievre jolelievre closed this Oct 10, 2019
@jolelievre jolelievre reopened this Oct 10, 2019
Copy link
Member

eternoendless left a comment

Reviewed 1 of 22 files at r1, 1 of 16 files at r2, 9 of 9 files at r4, 1 of 1 files at r6.
Dismissed @PierreRambaud and @sarjon from 3 discussions.
Reviewable status: 17 of 24 files reviewed, 10 unresolved discussions (waiting on @jolelievre, @matks, @PierreRambaud, and @sarjon)


classes/Currency.php, line 433 at r2 (raw file):

Previously, jolelievre wrote…

Yeah I saw the define but it's still counter-intuitive ^^ So, considering this default definition, actually the slave connection is used by default? Maybe a better comment would be useful here?

If you pass the setting, then yes, by default it will use the slave server (as long as it's configured).


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 11 at r6 (raw file):

    Given language "language1" with locale "en-US" exists
    Given language "language2" with locale "fr-FR" exists
    Given currency "currency1" with isoCode "USD" exists

Be consistent: "ISO code" and not "isoCode"


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 14 at r6 (raw file):

    Given currency "currency2" with isoCode "EUR" exists
    Given currency "currency3" with isoCode "AUD" exists
    And I delete currency "currency2"

Any action should be a When

If it's a state then it should be a Given:Given the currency "currency2" has been deleted

Also, it's confusing that currencies are sometimes identified using an identifier like "currency2" and sometimes using ISO code.


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 18 at r6 (raw file):

  Scenario: Display USD
    Then display a price of 14789.5426 "USD" with locale "en-US" should look like "$14,789.54"

I think this wording can be improved:

    Then a price of 14789.5426 using "USD" in locale "en-US" should look like "$14,789.54"

tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 22 at r6 (raw file):

  Scenario: Display a deleted EUR currency
    And there should be 1 currencies of "EUR"

What is this, a given? a when? -> Not a good idea to start a scenario with "and" because it's ambiguous.

Also, I think the wording can be clarified.

  • Where is that instance? (database/installed/active?)
  • How could we have more than one instance of the same ISO currency at the same time?
Copy link
Contributor Author

jolelievre left a comment

Reviewable status: 16 of 24 files reviewed, 10 unresolved discussions (waiting on @eternoendless, @matks, @PierreRambaud, and @sarjon)


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 11 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Be consistent: "ISO code" and not "isoCode"

Done.


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 14 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Any action should be a When

If it's a state then it should be a Given:Given the currency "currency2" has been deleted

Also, it's confusing that currencies are sometimes identified using an identifier like "currency2" and sometimes using ISO code.

There is a "has been deleted" statement but it is used to check that the currency is indeed deleted, whereas this "I delete" statement is used as an action to delete the currency
And the identifier are used to access the objects save in the SharedStorage whereas the iso code used further is just a parameter of the Locale::formatPrice function, or it is used to check the data present in the database
Which is why both notations exist and are useful for different things
(I just understood what you meant so I'll use a When instead ^^)


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 18 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I think this wording can be improved:

    Then a price of 14789.5426 using "USD" in locale "en-US" should look like "$14,789.54"

Done.


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 22 at r6 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What is this, a given? a when? -> Not a good idea to start a scenario with "and" because it's ambiguous.

Also, I think the wording can be clarified.

  • Where is that instance? (database/installed/active?)
  • How could we have more than one instance of the same ISO currency at the same time?

It's a test on the database content, so it's a state I replaced it with a Given here, and used a new sentence Given database contains 1 rows of currency "EUR"

About your question, it is actually possible to have several rows with the same ISO code, if you remove a currency and then install it again you will have two rows (one deleted the other not) Although it's not a great behaviour that's how PrestaShop works for now (this could be improved in another issue)

That said there was a weird bug I talked to you about, when you delete a currency but then just try to display it, with this new fix that allows to display deleted currencies, then the CLDR service automatically reinserted a new currency in the database This was because of the CurrencyDatabase layer which performed write operations when he shouldn't have I removed this behaviour and this test is here to assess that we never (hopefully) do the same mistake

@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch from 83d124c to 97581dd Oct 11, 2019
@jolelievre jolelievre force-pushed the jolelievre:cldr-deleted-currency branch from a7d869b to 4c8b930 Oct 11, 2019
Copy link
Contributor

PierreRambaud left a comment

Reviewed 2 of 5 files at r7.
Reviewable status: 17 of 25 files reviewed, 6 unresolved discussions (waiting on @eternoendless, @matks, and @sarjon)

Copy link
Contributor

PierreRambaud left a comment

Reviewed 3 of 22 files at r1, 1 of 16 files at r2, 1 of 8 files at r3, 1 of 5 files at r7.
Reviewable status: 23 of 25 files reviewed, 5 unresolved discussions (waiting on @eternoendless and @sarjon)

Copy link
Member

eternoendless left a comment

Reviewed 2 of 5 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sarjon)


tests/Integration/Behaviour/Features/Scenario/CLDR/cldr.feature, line 22 at r6 (raw file):

if you remove a currency and then install it again you will have two rows (one deleted the other not) Although it's not a great behaviour that's how PrestaShop works for now (this could be improved in another issue)

I agree, this should be fixed in another PR on develop. Could you please create the issue?

Copy link
Member

eternoendless left a comment

Dismissed @sarjon from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sarahdib sarahdib self-assigned this Oct 14, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Oct 14, 2019

@eternoendless I created the issue about duplicated rows: #15934

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Oct 15, 2019
@PierreRambaud PierreRambaud merged commit ef0387a into PrestaShop:1.7.6.x Oct 15, 2019
3 checks passed
3 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
code-review/reviewable 25 files reviewed (sarahdib)
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Oct 15, 2019

Thanks @jolelievre

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 15, 2019

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.