Skip to content

Conversation

cezary-zeglen
Copy link
Contributor

@cezary-zeglen cezary-zeglen commented Aug 29, 2018

Description

Currently if you enable custom design theme for category the translations of selected theme are not used. This commit fixes the problem.

Fixed Issues (if relevant)

#17625

Manual testing scenarios

(Copied from #17625 )

  1. Enable Magento_Blank for the website
  2. Create a category and set Magento_Luma as it's theme
  3. Add a translation to the /i18n/ directory of Magento_Luma
  4. Flush caches
  5. The translation is now not applied.
    Commit changes point 5. so the translation is being applied

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 29, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team magento-engcom-team added Component: Catalog Component: Framework/Translate USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to translation in Catalog use just Catalog labels Aug 29, 2018
@magento-engcom-team
Copy link
Contributor

Hi @cezary-zeglen. 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

@cezary-zeglen
Copy link
Contributor Author

@phoenix128 Thanks for the code review, I thought I should stick to the old coding conventions when modifying old classes. I'll commit fixes sometime this week.

@phoenix128
Copy link
Contributor

Hello @cezary-zeglen ,
thank you for your time. It is really appreciated. Please also refer to the static tests errors.

@phoenix128 phoenix128 removed their assignment Sep 17, 2018
@sidolov
Copy link
Contributor

sidolov commented Sep 18, 2018

Hi @cezary-zeglen , looks like that you made a commit with a different email than in the GitHub profile. Please, add email from commit to your profile or make a new commit with the email from profile and force push the branch. Also, please, sign CLA, otherwise we can't proceed with your PR.
Thank you!

@cezary-zeglen
Copy link
Contributor Author

Hi @cezary-zeglen , looks like that you made a commit with a different email than in the GitHub profile. Please, add email from commit to your profile or make a new commit with the email from profile and force push the branch. Also, please, sign CLA, otherwise we can't proceed with your PR.
Thank you!

@sidolov Yeah, sorry about that. When I'll find a bit of time I'll fix issues with the code and just squash all the commits. BTW, I have a question regarding the workflow here. I'm used to pushing commits, checking reports for test results, fixing and then setting commit in gerrit to special status to announce to my reviewers that the commit awaits code review. This way noone is bothered with the commit untill I set its status to the correct one. How does it work here? I don't want to bother people with my work in progress commits but I also don't see that much need for setting up all tests locally.

@sidolov
Copy link
Contributor

sidolov commented Oct 2, 2018

Hi @cezary-zeglen , we are expecting that newly created PR us ready to review (all tests passed, solution well tested, etc). I understand that in some cases you don't know about scenarios that can be affected by fix, you need to submit the PR and wait for a Travis. In that case, we can wait for the test results and drop you a message in something went wrong.

@slavvka
Copy link
Member

slavvka commented Oct 3, 2018

Hey @cezary-zeglen. Thank you for your contribution. Test Magento\Catalog\Model\DesignTest::testApplyCustomDesign seems to be failing due to your changes. Please fix it before we can proceed.

@slavvka slavvka self-assigned this Oct 3, 2018
@cezary-zeglen
Copy link
Contributor Author

@slavvka I know, sorry it takes so much time for me to commit a fix but I'll try to find time this week to take care of this.

@slavvka
Copy link
Member

slavvka commented Oct 10, 2018

@cezary-zeglen okay, thank you for the efforts!

@cezary-zeglen cezary-zeglen force-pushed the fix/categorytranslation branch 3 times, most recently from fbf3fd0 to 40c2a9c Compare October 15, 2018 12:42
Copy link
Member

@slavvka slavvka left a comment

Choose a reason for hiding this comment

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

@cezary-zeglen thank you the changes. But I have comments so please fix it.

@cezary-zeglen
Copy link
Contributor Author

@slavvka Sorry but I'm not sure if I understand correctly, so you want me just to improve the unit tests or also to remove the setter and reimplement the whole bugfix?

@slavvka
Copy link
Member

slavvka commented Oct 29, 2018

@slavvka Sorry but I'm not sure if I understand correctly, so you want me just to improve the unit tests or also to remove the setter and reimplement the whole bugfix?

@cezary-zeglen Sorry, I really missed that you use the added method in the solution. So you are right, you don't need to update Translate classa and its test. But could you please consider covering the change in Design class with integration test since it is serious changes that may affect whole application?

@cezary-zeglen
Copy link
Contributor Author

cezary-zeglen commented Oct 31, 2018

@slavvka Ok, now this is embarassing - I realized that force reloading translator is enough to fix this bug. Do you want me to squash all of these commits or leave it as is?

@cezary-zeglen cezary-zeglen force-pushed the fix/categorytranslation branch 2 times, most recently from 72005d7 to 57bb789 Compare October 31, 2018 15:56
@slavvka
Copy link
Member

slavvka commented Oct 31, 2018

@slavvka Ok, now this is embarrassing - I realized that force reloading translator is enough to fix this bug. Do you want me to squash all of these commits or leave it as is?

@cezary-zeglen Yep, but that's wonderful since the time could give you the possibility to dramatically improve your solution!

Yes, please squash it to make this PR even more ideal :) Also after having all tests passed could you please to up-port it to 2.3 as we want to all fixes to be delivered to 2.3 firstly. Then we could deliver this PR as well.

@magento-engcom-team magento-engcom-team added this to the Release: 2.2.8 milestone Nov 8, 2018
@magento-engcom-team magento-engcom-team removed the Component: Framework/Translate USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to translation in Catalog use just Catalog label Nov 8, 2018
@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @cezary-zeglen. 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.

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.

6 participants