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

CLDR has access to all currencies regardless of the current shop #15173

Merged
merged 3 commits into from Aug 23, 2019

Conversation

@jolelievre
Copy link
Contributor

commented Aug 20, 2019

Questions Answers
Branch? 1.7.6.x
Description? The CLDR used to get the currencies list related to the current shop, which caused errors in some specific contexts where orders are shared between shops, the CLDR could not display an unknown currency So from now on it can display any currency installed regardless of the shop context BUT this required to add a new method in the CurrencyDataProviderInterface which is a BC break as we change the interface
Type? bug fix
Category? BO
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes #14595 and #15144
How to test? See the issues which describes precisely at the end how to reproduce the bug (two shops with their own currencies), then try to access an order from Shop B in Shop A context, the order will be correctly displayed When you try to export orders it will also work BUT we need to check that in the front office we only display the expected currencies (one per shop)

This change is Reviewable

@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 20, 2019

@matthieu-rolland
Copy link
Contributor

left a comment

LGTM

$id_lang = Configuration::get('PS_LANG_DEFAULT');
}
return Tools::ucfirst($this->name[$id_lang]);

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Aug 20, 2019

Contributor

any reason to use ucfirst here and not above?

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Aug 20, 2019

Contributor

It seems like $this->name has already been set in the constructor using Tools:Ucfirst(). I'm not sure if applying the transformation again in a getter context is overkill or not though... It seems fine to me as it is.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 21, 2019

Author Contributor

if $this->name is a string then it means it already has been formatted in the constructor, so no need for an additional ucfirst
However in EditCurrencyHandler the $currency->name field is filled with an array, which is logical to allow updating the translated fields, but it caused an error when DefaultCurrencyInMultiShopException was thrown because the expected string field was then an array, hence this new getter

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member

This method is useful but not a good idea because of the hidden context (aka the current language being inferred magically).

The proper getter would be getName($idLang) where $idLang cannot be null.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 21, 2019

Author Contributor

Yes except that if $this->name doesn't contain the array the method may return a string that doesn't match the asked language, which can be confusing..
Unless we store the localized names in another field $this->names in the constructor, similar to what I had to do in another PR:

if (is_array($this->name)) {

This would allow to get the name in any language regardless of the context

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member

Argh, you're right. This sucks.

unresolved review comments

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

LGTM

Hmmm nope, as I said privately in multistore Store A and B can share orders: this is a feature we don't want to break.

Did someone checked what was the behavior in 1.7.5 in the same conditions?

@jolelievre jolelievre force-pushed the jolelievre:multi-shop-currency branch from 9f9b1e2 to 5beb6f8 Aug 21, 2019

@jolelievre jolelievre changed the title Prevent order from being displayed in a shop it is not associated with CLDR has access to all currencies regardless of the current shop Aug 21, 2019

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

LGTM

Hmmm nope, as I said privately in multistore Store A and B can share orders: this is a feature we don't want to break.

Did someone checked what was the behavior in 1.7.5 in the same conditions?

@mickaelandrieu The behavior in 175 is that you can see an order from another shop if you have the url, although when you list them you don't see it in the list which doesn't seem logical If you switch to a specific shop you should only see this shop's orders
Even though it worked in previous versions it doesn't mean it's the right behavior, this is the kind of bug that became a feature 😅

But the weird thing is that when you export invoices they are shared between the shops... So the two behaviors are contradictory

So we decided not to change the behavior for now, and CLDR is now able to display any installed currency regardless of the shop context But the orders behavior will be discussed later with the multi shop refacto

@jolelievre jolelievre force-pushed the jolelievre:multi-shop-currency branch from 5beb6f8 to f6123cd Aug 21, 2019

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Hey @jolelievre, no more wording to check here?

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

Hey @jolelievre, no more wording to check here?

Nope no wording in this PR

@@ -380,12 +401,12 @@ public static function getCurrencies($object = false, $active = true, $groupBy =
*
* @return array Currency data from database
*/
public static function findAll($active = true, $groupBy = false)
public static function findAll($active = true, $groupBy = false, $filterShop = true)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member

I think it one of these would be easier to understand:

  • $allShops = false
  • $currentShopOnly = true

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member

Also, missing Phpdoc

/**
* Return raw currencies data from the database regardless of the current shop.
*
* @return array

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member
Suggested change
* @return array
* @return array[]
@@ -47,12 +47,19 @@ interface CurrencyDataProviderInterface
public function getCurrencies($object = false, $active = true, $group_by = false);
/**
* Return raw currencies data from the database reated to the current shop.
* Return raw currencies data from the database related to the current shop.
*
* @return array Installed currencies

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member
Suggested change
* @return array Installed currencies
* @return array[] Installed currencies
*
* @return array
*/
public function findAllAvailable();

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Member

This could be misunderstood as "all languages available to install", which we use in other places. I think it would be better just to add an optional parameter to findAll() instead.

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@eternoendless thanks, all your feedbacks have been addressed

@eternoendless eternoendless added this to the 1.7.6.1 milestone Aug 21, 2019

@sarahdib sarahdib self-assigned this Aug 22, 2019

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Aug 22, 2019

@eternoendless

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Thank you @jolelievre

@eternoendless eternoendless merged commit b37502d into PrestaShop:1.7.6.x Aug 23, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI 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.