MC-38592 Fixed load regions for scope #30755

bogutskyy · 2020-11-02T21:54:00Z

Description (*)

Currently, if different scopes have different allowed countries, then can be issue on address forms. One of issue described in #30577: new order can not be created from admin. Another issue, which I found, on customer address edit page, when for US doesn't appear states dropdown.

I fixed regions loading depending on current scope.

Related Pull Requests

No related PRs

Fixed Issues (if relevant)

Fixes Can't place order on admin: "regionId" is required #30577

Manual testing scenarios (*)

See #30577

Questions or comments

Looks like directory helper has been broken after fix in ticket MC-33168. Unfortunately I don't know what the problem was in MC-33168, but after code changes investigation I guess it was issue, when scope present in request params. Not sure the fix of MC-33168 was correct, maybe logic of current scope determination should be moved from app/code/Magento/Directory/Helper/Data.php to app/code/Magento/Store/Model/StoreResolver.php.

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 are green)

…directory data helper

…ons of different scopes

m2-assistant · 2020-11-02T21:54:03Z

Hi @bogutskyy. 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 give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

vveb-developer · 2020-11-03T11:59:47Z

@magento run all tests

bogutskyy · 2020-11-03T19:33:00Z

@magento run all tests

gabrieldagama

Hi @bogutskyy, thanks for contributing! Can you have a look at my comments below? Also, can you have a look at the failed tests and fix them?

Thank you!

gabrieldagama · 2020-11-05T11:04:33Z

app/code/Magento/Directory/Helper/Data.php

+            $scopeKey = $scope['value'] ? '_' . implode('_', $scope) : '';
+            $cacheKey = 'DIRECTORY_REGIONS_JSON' . $scopeKey;


Can you give more details on why this change here is relevant?

Doesn't make sense to have string _STORE in cache key template, because $scope['type'] already contains scope type. In result currently cache key for store takes value DIRECTORY_REGIONS_JSON_STORE_STORE_1.

gabrieldagama · 2020-11-05T11:07:21Z

app/code/Magento/Directory/Helper/Data.php

@@ -406,14 +406,21 @@ public function getWeightUnit()
     * Get current scope from request
     *
     * @return array
+     * @throws \Magento\Framework\Exception\NoSuchEntityException
     */
    private function getCurrentScope(): array


This method has as a goal (as described on the PHP doc block) to get the current scope from the request, I'm not sure that such change makes sense, maybe we should look at the problem from the request perspective?

Method getCurrentScope has been added in commit 2b4cfab, ticket MC-33168, and this change produced issue #30577. tbh, my change looks like quick fix. I am not sure that method getCurrentScope should be here. In this class we have store resolver. I think make sense to move logic of current scope determination into store resolver. But I don't know what issue MC-33168 was about, and afraid to broke something, what has been fixed previously. Therefore I decided change default value in method getCurrentScope. If somebody can explain me issue of MC-33168, then I'll be able to move on.

I agree that getCurrentScope probably shouldn't be on this helper, but by adding that condition it makes the function ambiguous. My proposal would be to look at the solution from the Create Order perspective, see from which request this function is being called, and make sure we have the correct scope on the request, it should be also a quick fix, what do you think?

bogutskyy · 2020-11-08T08:11:11Z

@magento run all tests

gabrieldagama · 2021-01-04T17:16:00Z

Hi @bogutskyy, sorry for the delay on this. Please follow some information from MC-33168:

Default State dropdown is not filled with correct data

ISSUE:
When configuring 'Default Tax Destination Calculation', the 'Default State' dropdown only fills with data based on allowed countries of the default website.

STEPS TO REPLICATE:
1. Configure a second website, e.g. 'Test Website'.
2. Go to Stores -> Configurations.
3. Change the scope to 'Main Website'.
4. Navigate to General -> General -> Country Options -> Allow Countries and select some countries, e.g. select France and Germany.
5. Clear the cache.
6. Go to Stores -> Configurations again and change the scope to 'Test Website'.
7. Navigate to Sales -> Tax -> Default Tax Destination Calculation.
8. Select Default Country from the dropdown list instead of Use Default.
9. Try to select Default State from the dropdown list instead of Use Default.

ACTUAL RESULTS:
Only when selecting countries that have been selected for the Main Website (Step 4), will the Default State dropdown display a full list. Otherwise, it does not display the full list of states:

gabrieldagama · 2021-01-04T17:24:12Z

app/code/Magento/Directory/Helper/Data.php

@@ -406,14 +406,21 @@ public function getWeightUnit()
     * Get current scope from request
     *
     * @return array
+     * @throws \Magento\Framework\Exception\NoSuchEntityException
     */
    private function getCurrentScope(): array


I agree that getCurrentScope probably shouldn't be on this helper, but by adding that condition it makes the function ambiguous. My proposal would be to look at the solution from the Create Order perspective, see from which request this function is being called, and make sure we have the correct scope on the request, it should be also a quick fix, what do you think?

engcom-Charlie · 2021-01-13T11:27:41Z

Hello @bogutskyy, could you please look at #30755 (comment)?
Thank you.

engcom-Charlie · 2021-01-15T11:30:10Z

Hello @bogutskyy, I'll try to continue with your PR.

bogutskyy · 2021-01-16T08:34:24Z

@engcom-Charlie sure. Unfortunately, I don't have time to finish PR in the coming weeks Let me know if you need anything from me.

gabrieldagama · 2021-02-19T12:48:01Z

@magento run all tests

magento-engcom-team · 2021-02-19T12:49:33Z

Hi @gabrieldagama, thank you for the review.
ENGCOM-8797 has been created to process this Pull Request
✳️ @gabrieldagama, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

engcom-Delta · 2021-02-22T12:53:55Z

✔️ QA passed
Was checked case described in issue #30577
Before:
❌ Error Please check the shipping address information. "regionId" is required. Enter and try again. is shown and the order is not placed.

After:
✔️ Order is placed successfully

engcom-Delta · 2021-02-22T13:07:02Z

Note: Functional Tests B2B are failed

m2-assistant · 2021-03-18T17:18:35Z

Hi @bogutskyy, 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.

hussainbaig34 · 2021-05-20T23:02:07Z

Hi Guys,

I am running Magento 2.4.2. I Just migrated from 1.9.3. Alot of my orders are declining due to this issue. Is there a way I can fix this on my website quick?

Regards,
Hussain

bogutskyy added 2 commits November 2, 2020 21:37

MC-38592 Used store manager for retrieve current store by default in …

e8507db

…directory data helper

MC-38592 Extended unit tests of directory helper for testing get regi…

228cb68

…ons of different scopes

magento-engcom-team added Component: Directory Release Line: 2.4 labels Nov 2, 2020

ghost added the Progress: pending review label Nov 2, 2020

ghost added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Nov 3, 2020

gabrieldagama suggested changes Nov 5, 2020

View reviewed changes

m2-community-project bot assigned gabrieldagama Nov 5, 2020

m2-community-project bot added Progress: needs update Progress: review and removed Progress: pending review Progress: needs update labels Nov 5, 2020

MC-38592 Fixed code styles of unit tests

4e48a7a

bogutskyy requested a review from gabrieldagama November 8, 2020 15:13

m2-community-project bot added Progress: pending review and removed Progress: review labels Dec 28, 2020

gabrieldagama suggested changes Jan 4, 2021

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Jan 4, 2021

engcom-Charlie self-assigned this Jan 13, 2021

Merge branch '2.4-develop' into MC-38592

165a1c1

gabrieldagama approved these changes Feb 19, 2021

View reviewed changes

m2-community-project bot added Progress: ready for testing and removed Progress: pending review labels Feb 19, 2021

magento-engcom-team added Component: Config Component: Customer labels Feb 19, 2021

engcom-Delta self-assigned this Feb 22, 2021

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Feb 22, 2021

engcom-Delta added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Feb 22, 2021

m2-community-project bot added Progress: ready for testing and removed Progress: testing in progress labels Feb 22, 2021

m2-community-project bot added Progress: testing in progress Progress: extended testing and removed Progress: ready for testing Progress: testing in progress labels Feb 22, 2021

engcom-Charlie added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Feb 23, 2021

m2-community-project bot added Progress: ready for testing Progress: accept and removed Progress: extended testing Progress: ready for testing labels Feb 23, 2021

gabrieldagama mentioned this pull request Mar 18, 2021

Can't place order on admin: "regionId" is required #30577

Closed

5 tasks

magento-engcom-team pushed a commit that referenced this pull request Mar 18, 2021

ENGCOM-8797: MC-38592 Fixed load regions for scope #30755

9362157

magento-engcom-team merged commit 19e0a8e into magento:2.4-develop Mar 18, 2021

gabrieldagama added the Type: Bug Fix label Mar 18, 2021

		$scopeKey = $scope['value'] ? '_' . implode('_', $scope) : '';
		$cacheKey = 'DIRECTORY_REGIONS_JSON' . $scopeKey;

MC-38592 Fixed load regions for scope #30755

MC-38592 Fixed load regions for scope #30755

Uh oh!

Conversation

bogutskyy commented Nov 2, 2020

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Nov 2, 2020

Uh oh!

vveb-developer commented Nov 3, 2020

Uh oh!

bogutskyy commented Nov 3, 2020

Uh oh!

gabrieldagama left a comment

Choose a reason for hiding this comment

Uh oh!

gabrieldagama Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

bogutskyy Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

gabrieldagama Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

bogutskyy Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

gabrieldagama Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

bogutskyy commented Nov 8, 2020

Uh oh!

gabrieldagama commented Jan 4, 2021

Uh oh!

gabrieldagama Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

engcom-Charlie commented Jan 13, 2021

Uh oh!

engcom-Charlie commented Jan 15, 2021

Uh oh!

bogutskyy commented Jan 16, 2021

Uh oh!

gabrieldagama commented Feb 19, 2021

Uh oh!

magento-engcom-team commented Feb 19, 2021

Uh oh!

engcom-Delta commented Feb 22, 2021

Uh oh!

engcom-Delta commented Feb 22, 2021

Uh oh!

m2-assistant bot commented Mar 18, 2021

Uh oh!

hussainbaig34 commented May 20, 2021

Uh oh!

Uh oh!