This will shorten the list of allowed currencies #19946

melaxon · 2018-12-22T22:14:10Z

This will shorten the list of allowed currencies showing only installed currencies

Description (*)

in Store->Configuration->General->Currency setup will be shown only those currencies selected in
Installed currencies list (Configuration->Advanced->System)

Fixed Issues (if relevant)

magento/magento2#<issue_number>: Issue title
...

Manual testing scenarios (*)

Currently if you select any currency in the list in Store->Configuration->General->Currency setup and this currency is not yet selected in Installed currency list (Configuration->Advanced->System) you will receive error message.
After the modification is applied there will be only those currencies in the list (Store->Configuration->General->Currency setup) that can be selected without any error message

Contribution checklist (*)

[ x] Pull request has a meaningful description of its purpose
[ x] All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

in Store->Configuration->General->Currency setup will be shown only those currencies selected in Installed currencies list (Configuration->Advanced->System)

magento-engcom-team · 2018-12-22T22:14:15Z

Hi @melaxon. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

bartoszkubicki · 2018-12-22T22:33:59Z

lib/internal/Magento/Framework/Locale/TranslatedLists.php

@@ -150,10 +157,20 @@ public function getOptionCurrencies()
        $currencies = (new CurrencyBundle())->get($this->localeResolver->getLocale())['Currencies'] ?: [];
        $options = [];
        $allowed = $this->_config->getAllowedCurrencies();
+        $selectedCurrencies = explode(
+            ',', $this->scopeConfig->getValue(
+                'system/currency/installed',


I would put inside private const inside class

Not needed until it is used only once.

bartoszkubicki · 2018-12-22T22:35:14Z

lib/internal/Magento/Framework/Locale/TranslatedLists.php

+        $selectedCurrencies = explode(
+            ',', $this->scopeConfig->getValue(
+                'system/currency/installed',
+                \Magento\Store\Model\ScopeInterface::SCOPE_STORE


Maybe it could be move to imports and here left in short version

bartoszkubicki · 2018-12-22T22:35:53Z

lib/internal/Magento/Framework/Locale/TranslatedLists.php

        foreach ($currencies as $code => $data) {
            if (!in_array($code, $allowed)) {
                continue;
            }
+            if (!in_array($code, $selectedCurrencies)) {


I think condition could be moved to separate protected function, in order to encapsulate it

bartoszkubicki · 2018-12-22T22:36:41Z

lib/internal/Magento/Framework/Locale/TranslatedLists.php

@@ -150,10 +157,20 @@ public function getOptionCurrencies()
        $currencies = (new CurrencyBundle())->get($this->localeResolver->getLocale())['Currencies'] ?: [];
        $options = [];
        $allowed = $this->_config->getAllowedCurrencies();
+        $selectedCurrencies = explode(


Maybe getting selecting countries could be done in separate protected method?

This function will be called only once. Does it deserve it?

sivaschenko · 2019-01-10T13:15:09Z

Hi @melaxon Thanks for the contribution! Please see the code review notes and consider covering the introduced improvement with a test. Also failed unit tests need to be fixed.

melaxon · 2019-01-11T10:56:43Z

@magento-engcom-team give me test instance

magento-engcom-team · 2019-01-11T10:57:02Z

Hi @melaxon. Thank you for your request. I'm working on Magento instance for you

magento-engcom-team · 2019-01-11T10:57:34Z

Hi @melaxon, here is your new Magento instance.
Admin access: https://pr-19946.instances.magento-community.engineering/admin
Login: admin Password: 123123q

melaxon · 2019-01-11T11:08:18Z

It works in test instance.
I allowed UDS and EUR in Configuration->Advanced->System
Then I see only allowed currencies in General->Currency Setup:
https://gyazo.com/c6a970fd6d34d1459ee7e9ed47828822

melaxon · 2019-01-11T14:06:36Z

New commit will try to solve this:
TypeError: Argument 1 passed to Magento\Framework\Locale\TranslatedLists::__construct() must implement interface Magento\Framework\App\Config\ScopeConfigInterface, instance of Mock_ConfigInterface_33ac175b given, called in /home/travis/build/magento/magento2/lib/internal/Magento/Framework/Locale/Test/Unit/TranslatedListsTest.php on line 40

Trying to solve this: TypeError: Argument 1 passed to Magento\Framework\Locale\TranslatedLists::__construct() must implement interface Magento\Framework\App\Config\ScopeConfigInterface, instance of Mock_ConfigInterface_33ac175b given, called in /home/travis/build/magento/magento2/lib/internal/Magento/Framework/Locale/Test/Unit/TranslatedListsTest.php on line 40

fix test unit

melaxon · 2019-01-13T08:59:54Z

done

sivaschenko

Hi @melaxon please see the code review notes

sivaschenko · 2019-01-14T14:45:25Z

lib/internal/Magento/Framework/Locale/TranslatedLists.php

+    /**
+     * @var TranslatedLists
+     */
+    public $selectedCurrencies;


Please change the access level of new properties to private

sivaschenko · 2019-01-14T14:46:45Z

lib/internal/Magento/Framework/Locale/Test/Unit/TranslatedListsTest.php

        $this->mockConfig->expects($this->once())
            ->method('getAllowedCurrencies')
            ->will($this->returnValue($allowedCurrencies));

-        $expectedResults = ['USD', 'EUR', 'GBP', 'UAH'];
+        $this->listsModel->selectedCurrencies = $selectedCurrencies;


Please mock the return of selected currencies from the scopeConfig.

ok. Along with the above comment

sivaschenko · 2019-01-14T14:47:03Z

lib/internal/Magento/Framework/Locale/Test/Unit/TranslatedListsTest.php

@@ -64,12 +73,15 @@ public function testGetOptionAllCurrencies()
    public function testGetOptionCurrencies()
    {
        $allowedCurrencies = ['USD', 'EUR', 'GBP', 'UAH'];
-
+		$selectedCurrencies = ['USD', 'EUR'];


Suggested change

$selectedCurrencies = ['USD', 'EUR'];

$selectedCurrencies = ['USD', 'EUR'];

ghost · 2019-01-18T11:08:28Z

Hi @melaxon, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

melaxon · 2019-01-18T11:18:46Z

I understood that the Framework is not the place where such modification can be applied because it is based on user input. New PR will be opened now

This will shorten the list of allowed currencies

11fb7e7

in Store->Configuration->General->Currency setup will be shown only those currencies selected in Installed currencies list (Configuration->Advanced->System)

magento-engcom-team added Component: Framework/Locale Release Line: 2.3 labels Dec 22, 2018

bartoszkubicki self-requested a review December 22, 2018 22:32

bartoszkubicki reviewed Dec 22, 2018

View reviewed changes

sivaschenko self-assigned this Jan 10, 2019

sivaschenko added the Progress: needs update label Jan 10, 2019

melaxon added 3 commits January 11, 2019 17:07

fix test unit

1e8c76b

fix test unit

2822fa1

fix test unit

sivaschenko requested changes Jan 14, 2019

View reviewed changes

melaxon closed this Jan 18, 2019

melaxon deleted the patch-2 branch January 18, 2019 11:08

melaxon mentioned this pull request Jan 18, 2019

Shortening currency list in Configuration->General (this replaces PR #19946) #20397

Closed

4 tasks

melaxon mentioned this pull request Apr 8, 2019

Shortening currency list in Configuration->General (replace PR #20397) #22230

Merged

4 tasks

amol2jcommerce mentioned this pull request Apr 30, 2019

[Backport] Shortening currency list in Configuration->General (replace PR #20397) #22551

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This will shorten the list of allowed currencies #19946

This will shorten the list of allowed currencies #19946

melaxon commented Dec 22, 2018 •

edited

Loading

magento-engcom-team commented Dec 22, 2018

bartoszkubicki Dec 22, 2018

orlangur Dec 28, 2018

bartoszkubicki Dec 22, 2018

bartoszkubicki Dec 22, 2018

bartoszkubicki Dec 22, 2018

melaxon Jan 11, 2019

sivaschenko commented Jan 10, 2019

melaxon commented Jan 11, 2019

magento-engcom-team commented Jan 11, 2019

magento-engcom-team commented Jan 11, 2019

melaxon commented Jan 11, 2019

melaxon commented Jan 11, 2019 •

edited

Loading

melaxon commented Jan 13, 2019

sivaschenko left a comment

sivaschenko Jan 14, 2019

sivaschenko Jan 14, 2019

melaxon Jan 14, 2019

sivaschenko Jan 14, 2019

ghost commented Jan 18, 2019

melaxon commented Jan 18, 2019

	$selectedCurrencies = ['USD', 'EUR'];
	$selectedCurrencies = ['USD', 'EUR'];

This will shorten the list of allowed currencies #19946

This will shorten the list of allowed currencies #19946

Conversation

melaxon commented Dec 22, 2018 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Dec 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivaschenko commented Jan 10, 2019

melaxon commented Jan 11, 2019

magento-engcom-team commented Jan 11, 2019

magento-engcom-team commented Jan 11, 2019

melaxon commented Jan 11, 2019

melaxon commented Jan 11, 2019 • edited Loading

melaxon commented Jan 13, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 18, 2019

melaxon commented Jan 18, 2019

melaxon commented Dec 22, 2018 •

edited

Loading

melaxon commented Jan 11, 2019 •

edited

Loading