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

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

Merged
merged 1 commit into from
Apr 29, 2019

Conversation

melaxon
Copy link
Member

@melaxon melaxon commented Apr 8, 2019

Description (*)

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

This filters the list of allowed currencies to show only selected ones

This PR replaces closed one #19946

Fixed Issues (if relevant)

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

Manual testing scenarios (*)

  1. The list of currencies will look like in screenshot
    screenshot 2019-01-18 14 26 30

  2. These changes can be applied to 2.2.x as well

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • 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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 8, 2019

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

@ghost
Copy link

ghost commented Apr 8, 2019

@melaxon unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

$options = array_filter(
$this->_options,
function ($option) use ($selected) {
return in_array($option['value'], $selected);
Copy link
Contributor

@orlangur orlangur Apr 9, 2019

Choose a reason for hiding this comment

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

Please flip array so that we can check with isset.

Copy link
Member Author

@melaxon melaxon Apr 9, 2019

Choose a reason for hiding this comment

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

Please array so that we can check with isset.

Hi orlangur!
Please take a look if this is what you mean:
de3d22e

That's okay if all currency codes are 3 latin characters. If one day some exotic currency will be used this checking may fail. Taking into account that this operation is not really time critical (it works only for admin) I would leave it as it was

Copy link
Contributor

Choose a reason for hiding this comment

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

@melaxon,

If one day some exotic currency will be used this checking may fail

I don't see how it can fail, also, 3 letters is an ISO standard.

No need in filling keys, just use array_flip.

Copy link
Member Author

Choose a reason for hiding this comment

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

..., just use array_flip.

Done: 2d9465c

@ghost ghost assigned orlangur Apr 9, 2019
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

Please squash changes into a single commit so that we have perfectly clean history 😉

@melaxon
Copy link
Member Author

melaxon commented Apr 10, 2019

Please squash changes into a single commit so that we have perfectly clean history 😉

Done (I think)

@magento-engcom-team
Copy link
Contributor

Hi @aleron75, thank you for the review.
ENGCOM-4736 has been created to process this Pull Request

@VasylShvorak
Copy link
Contributor

✔️ QA passed

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

@melaxon nope, I still see 5 commits here.

@melaxon
Copy link
Member Author

melaxon commented Apr 20, 2019

@melaxon nope, I still see 5 commits here.

Sorry for the delay. I squashed the commits

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-4736 has been created to process this Pull Request

@VasylShvorak
Copy link
Contributor

✔️ QA passed

@sivaschenko
Copy link
Member

@paliarush can you please take a look at this pull request - a new optional constructor argument is added to API class

@paliarush
Copy link
Contributor

SVC failure due to optional argument in constructor approved.

@magento-engcom-team magento-engcom-team merged commit 0c1ffcd into magento:2.3-develop Apr 29, 2019
@m2-assistant
Copy link

m2-assistant bot commented Apr 29, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants