[Fix] Do not modify current list of countries with require states during setup upgrade #16885

jalogut · 2018-07-17T14:40:09Z

Upgrading from any version <2.2.0 to any version >=2.2.0 results in modifications of general/region/state_required value.

How to reproduce

Having a Magento project in any version <2.2.0
Set custom values in core_config_data for general/region/state_required
Upgrade Magento project to any version >=2.2.0

Expected result

general/region/state_required is not modified

Actual result

general/region/state_required is overridden with all enabled countries in general/country/allow

Fixed Issues

Checkout not working after upgrade for existing users that do no have a state in their address
State is a mandatory for all allowed countries which might break other functionalities depending on that (sign up, reset password, customer import...)

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)

…ing setup upgrade. Also mark getCountriesWithRequiredStates() as deprecated because is no longer needed

magento-engcom-team · 2018-07-17T14:40:14Z

Hi @jalogut. 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 {$VERSION} instance - deploy vanilla Magento instance

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

fooman · 2018-07-18T05:49:11Z

app/code/Magento/Directory/Setup/UpgradeData.php

@@ -41,10 +41,10 @@ public function __construct(Data $directoryData)
    public function upgrade(ModuleDataSetupInterface $setup, ModuleContextInterface $context)
    {
        if (version_compare($context->getVersion(), '2.0.1', '<')) {
-            $this->addCountryRegions($setup, $this->getDataForCroatia());
+            $this->addCountryRegions($setup, 'IN', $this->getDataForCroatia());


I believe IN and HR should be swapped here

dmanners

I have 1 question on this review and there is still one outstanding from FooMan.

dmanners · 2018-09-05T06:38:42Z

app/code/Magento/Directory/etc/di.xml

@@ -34,10 +34,6 @@
    <type name="Magento\Directory\Model\ResourceModel\Country\Collection" shared="false">
        <arguments>
            <argument name="helperData" xsi:type="object">DirectoryHelperDataProxy</argument>
-            <argument name="countriesWithNotRequiredStates" xsi:type="array">
-                <item name="FR" xsi:type="string">FR</item>


What was the reasoning behind making this change?

dmanners · 2018-09-05T06:39:59Z

Hi @jalogut do you have time to look into the feedback on this pull request?

Thanks

…s functionality

jalogut · 2018-09-06T14:13:13Z

Hi, sorry that was a mistake. I updated the PR with some changes. It should be ok now. Thanks for checking.

dmanners · 2018-09-19T10:56:27Z

During testing we had the following cases fail

I tried to upgrade from 2.2-develop to PR, - "general/region/state_required" is not modified.

When I upgraded from 2.2.3 to PR, - in addition to our value, IN is added to "general/region/state_required".

When upgrading from 2.1-develop to PR, - in addition to our value, HR and IN are added to "general/region/state_required"

@jalogut could you please look at these failures for us.

Thank you.

jalogut · 2018-09-19T12:39:26Z

What do you mean with failing? That is right behaviour. This setup scripts are depending on the current version of Magento_Directory module.

HR is added on 2.0.1 and IN in 2.0.2

dmanners · 2018-09-20T09:23:12Z

@jalogut are IN and HR added to "general/region/state_required" in earlier versions? Just going from your description:

general/region/state_required is not modified

If it was reported as a failure as in two cases it is modified.

jalogut · 2018-09-20T10:02:49Z

Well I meant “do not overwrite current value”. Before this PR, the current value is overwritten with the default magento one. This PR adds the new countries (IN, HR) while keeping previous value. Should I update the PR title?

dmanners · 2018-09-26T08:14:54Z

Ah I see @jalogut thanks for the clarification.

…re states during setup upgrade #16885

magento-engcom-team · 2018-09-28T12:18:05Z

Hi @jalogut. Thank you for your contribution.
We will aim to release these changes as part of 2.2.8.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

[Fix] Do not modify current list of countries with require states dur…

80384fc

…ing setup upgrade. Also mark getCountriesWithRequiredStates() as deprecated because is no longer needed

magento-engcom-team added the Component: Directory label Jul 17, 2018

fooman reviewed Jul 18, 2018

View reviewed changes

dmanners suggested changes Sep 5, 2018

View reviewed changes

dmanners self-assigned this Sep 5, 2018

jalogut added 2 commits September 6, 2018 16:03

[Fix] Set country codes in right places

80001a1

[Fix] Add required countries only for IN and HR while keeping previou…

875c394

…s functionality

jalogut force-pushed the fix-states-required branch from 0c29018 to 875c394 Compare September 6, 2018 14:11

sidolov added the Release Line: 2.2 label Sep 7, 2018

magento-engcom-team added this to the Release: 2.2.7 milestone Sep 17, 2018

magento-engcom-team added the Progress: accept label Sep 27, 2018

magento-engcom-team merged commit 875c394 into magento:2.2-develop Sep 28, 2018

magento-engcom-team pushed a commit that referenced this pull request Sep 28, 2018

ENGCOM-2988: [Fix] Do not modify current list of countries with requi…

89e5c60

…re states during setup upgrade #16885

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Do not modify current list of countries with require states during setup upgrade #16885

[Fix] Do not modify current list of countries with require states during setup upgrade #16885

jalogut commented Jul 17, 2018 •

edited

magento-engcom-team commented Jul 17, 2018

fooman Jul 18, 2018

dmanners left a comment

dmanners Sep 5, 2018

dmanners commented Sep 5, 2018

jalogut commented Sep 6, 2018

dmanners commented Sep 19, 2018

jalogut commented Sep 19, 2018

dmanners commented Sep 20, 2018

jalogut commented Sep 20, 2018 •

edited

dmanners commented Sep 26, 2018

magento-engcom-team commented Sep 28, 2018

[Fix] Do not modify current list of countries with require states during setup upgrade #16885

[Fix] Do not modify current list of countries with require states during setup upgrade #16885

Conversation

jalogut commented Jul 17, 2018 • edited

How to reproduce

Expected result

Actual result

Fixed Issues

Contribution checklist

magento-engcom-team commented Jul 17, 2018

fooman Jul 18, 2018

Choose a reason for hiding this comment

dmanners left a comment

Choose a reason for hiding this comment

dmanners Sep 5, 2018

Choose a reason for hiding this comment

dmanners commented Sep 5, 2018

jalogut commented Sep 6, 2018

dmanners commented Sep 19, 2018

jalogut commented Sep 19, 2018

dmanners commented Sep 20, 2018

jalogut commented Sep 20, 2018 • edited

dmanners commented Sep 26, 2018

magento-engcom-team commented Sep 28, 2018

jalogut commented Jul 17, 2018 •

edited

jalogut commented Sep 20, 2018 •

edited