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

Fixed category names in product edit for multi language stores. #22156

Closed

Conversation

ekalinak
Copy link
Contributor

@ekalinak ekalinak commented Apr 4, 2019

This pull request fixes problem reported in #22155

Description

In Multi store configuration, category names for different scopes are cached incorrectly on product edit form

Fixed Issues

  1. Category scope Name cached incorrectly on product edit #22155: Category scope Name cached incorrectly on product edit

Manual testing scenarios (*)

  1. Open "product edit" form
  2. Change "Store View"

Category attribute displays correct category names for particular Store View

@m2-assistant
Copy link

m2-assistant bot commented Apr 4, 2019

Hi @ekalinak. 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

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Apr 4, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ ekalinak
❌ Erik Kalinak


Erik Kalinak seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@ghost ghost assigned orlangur Apr 4, 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.

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

@ekalinak ekalinak force-pushed the fix-multistore-cateogry-names branch from 98c6bc2 to 2778acb Compare April 8, 2019 19:14
@ekalinak
Copy link
Contributor Author

Everything done but tests weren't successful. I can't see what can be possibly wrong. @orlangur : Can you ?

@orlangur
Copy link
Contributor

@ekalinak, error is pretty straightforward

There was 1 failure:
1) Magento\Catalog\Test\Unit\Ui\DataProvider\Product\Form\Modifier\CategoriesTest::testModifyMetaWithCaching
Expectation failed for method name is equal to 'load' when invoked 1 time(s)
Parameter 0 for invocation Magento\Framework\App\CacheInterface::load('CATALOG_PRODUCT_CATEGORY_TREE__') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'CATALOG_PRODUCT_CATEGORY_TREE_'
+'CATALOG_PRODUCT_CATEGORY_TREE__'
/home/travis/build/magento/magento2/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/Categories.php:316
/home/travis/build/magento/magento2/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/Categories.php:248
/home/travis/build/magento/magento2/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/Categories.php:127
/home/travis/build/magento/magento2/app/code/Magento/Catalog/Test/Unit/Ui/DataProvider/Product/Form/Modifier/CategoriesTest.php:189

@ekalinak
Copy link
Contributor Author

As I can see, it has no sense to continue here

  1. Original code ( before suggestion ) would pass the tests
  2. Meanwhile was added and merged commit 8d11456 which is fixing this bug
  • btw, logic of the code is not better from provided in this pull request, just author removed unit test from source code, so he can pass tests ( ??? )

@orlangur : I think you can close this pull request. ( It looks like whole time spend on creating issue and pull request worth for nothing .. :/ )

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.

@ekalinak your fix is much simpler than the one currently merged.

Please resolve conflicts and replace those changes with your ones, also, restoring a corresponding unit test.

Your PR now becomes not a bugfix but refactoring but it's still pretty cool.

@ekalinak
Copy link
Contributor Author

@orlangur : Everything seems to be OK, right ?

@orlangur
Copy link
Contributor

@engcom-dev-Foxtrot go over https://github.com/magento/magento2/pulls?q=is%3Aopen+is%3Apr+no%3Aassignee maybe? ;)

@ekalinak will check shortly.

@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

1 similar comment
@orlangur
Copy link
Contributor

@magento run all tests

@kmvickers628
Copy link

kmvickers628 commented Oct 21, 2019

@magento-admin @ekalinak hi there, i have been reviewing this thread here: #22155 and was curious if #22156 is the correct fix for issue 22155?

please confirm.

@ekalinak
Copy link
Contributor Author

Hi @kmvickers628 . I've created issue #22155 and also fix for it in this pull request. I've also applied this fix on one of installations I'm working on, and it's working.

Can you please explain, where is the problem exactly ?

@alexblueacorn
Copy link

Why this issue linked to this PR ?
Screen Shot 2019-10-21 at 3 13 31 PM
Is this a mistake ?

@kmvickers628
Copy link

@ekalinak , alexblueacorn provided a question above.

@ekalinak
Copy link
Contributor Author

@alexblueacorn : I don't know from where you have the screen, but from my point of view it is a mistake. This PR is fixing category names in product edit for multi language stores.

@alexblueacorn
Copy link

From this page.
https://devdocs.magento.com/guides/v2.3/release-notes/release-notes-2-3-3-commerce.html

Looks like they included wrong PR. So I'm trying find right one for reindexing issue.

@kmvickers628
Copy link

@ekalinak are you able to help Alex find the correct PR? thanks!

@ekalinak
Copy link
Contributor Author

@kmvickers628 : All I know that issue #22155 is fixed by this pull reqeust. I have no idea which PR is fixing indexing problem :/ How should I even find out ? By studying code from pull requests ?

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:21
@mightyknite
Copy link

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Mar 3, 2020

Hi @orlangur @ekalinak,
Could you info what's the status of this PR and what's left?

Update: I already understood the status

@ihor-sviziev ihor-sviziev self-assigned this Mar 3, 2020
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @ekalinak,

Looks like method getCategoriesTree was split to few smaller methods in 2.4-develop branch, while your PR currently reverting it, that looks incorrect. Could you sign Adobe CLA and update your PR to not combining all changes back into single method?

@ekalinak
Copy link
Contributor Author

ekalinak commented Mar 3, 2020

@ihor-sviziev : Omg, this is already one year old PR. Originally I submitted code to 2.3-develop branch. I understand that there are changes in 2.4-develop, but at the moment I have no time to investigate changes and again change my code. I'm closing this PR. After almost a year, this probably won't help anybody .. 😕

@ekalinak ekalinak closed this Mar 3, 2020
@m2-assistant
Copy link

m2-assistant bot commented Mar 3, 2020

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

@ihor-sviziev
Copy link
Contributor

@ekalinak
I’m so sorry that it took so long period to review and I’m fully understand your point. Now I’m trying to finalize all pull requests that weren’t processed due to different reasons.
Thank you for contribution and hope your next contribution will be processed much quicker

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.

10 participants