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

FIX for issue #14849 - In Sales Emails no translation using order.getStatusLabel() #14914

Conversation

phoenix128
Copy link
Contributor

@phoenix128 phoenix128 commented Apr 30, 2018

Description

\Magento\Sales\Model\Order\Config::getStatusLabel(), used by template email was forcing the usage of getLabel() when in adminhtml area.
I added a new method getFrontendStatusLabel() in \Magento\Sales\Model\Order and modified email template files to use it instead of the original implementation.
I did not modify the original implementation because still in use by backend and frontend.

Fixed Issues (if relevant)

  1. In Sales Emails no translation using order.getStatusLabel()  #14849: In Sales Emails no translation using order.getStatusLabel()

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)

…der.getStatusLabel()

\Magento\Sales\Model\Order\Config::getStatusLabel(), used by template email was forcing the usage of getLabel() when in adminhtml area.
I added a new method getFrontendStatusLabel() in \Magento\Sales\Model\Order and modified email template files to use it instead of the original implementation.
I did not modify the original implementation because still in use by backend and frontend.
@rogyar rogyar self-assigned this May 1, 2018
@magento-engcom-team magento-engcom-team added this to the May 2018 milestone May 2, 2018
* Retrieve frontend label of order status
*
* @return string
* @throws \Magento\Framework\Exception\LocalizedException
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, import the class to the 'use' section instead of specifying the full class path inline

*/
public function getFrontendStatusLabel()
{
return $this->getConfig()->getStatusLabel($this->getStatus(), \Magento\Framework\App\Area::AREA_FRONTEND);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, import the class to the 'use' section instead of specifying the full class path inline

/**
* Retrieve label of order status
*
* @return string
* @throws \Magento\Framework\Exception\LocalizedException
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, import the class to the 'use' section instead of specifying the full class path inline

* @return string
* @throws \Magento\Framework\Exception\LocalizedException
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, import the class to the 'use' section instead of specifying the full class path inline

@@ -118,11 +118,13 @@ public function getStateDefaultStatus($state)
* Retrieve status label
*
* @param string $code
* @param null $forceArea
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we have string|null here. Please, fix the DocBlock

@rogyar
Copy link
Contributor

rogyar commented May 2, 2018

Hello @phoenix128. Thank you for the collaboration. Please, consider fixing minor issues mentioned in the review.

@phoenix128
Copy link
Contributor Author

@rogyar , just fixed.

*/
public function getStatusLabel($code)
public function getStatusLabel($code, $forceArea = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

@phoenix128 , adding new parameters to public method is not allowed because this is breaking changes. Please, refer to Backward compatible development guide section Adding parameters in public methods for more information

Copy link
Contributor Author

@phoenix128 phoenix128 Aug 29, 2018

Choose a reason for hiding this comment

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

@sidolov , does the rule applies for optional parameters?

Copy link
Contributor

Choose a reason for hiding this comment

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

@phoenix128 , yes, this changes may affect 3d party code with the same changes but with different logic

Copy link
Contributor

@sidolov sidolov left a comment

Choose a reason for hiding this comment

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

Small refactoring is needed

@rogyar
Copy link
Contributor

rogyar commented Aug 9, 2018

Hi @phoenix128. Are you going to proceed with this PR?

@phoenix128
Copy link
Contributor Author

@rogyar ,
yes, sorry for the delay. I am back to work.

@sidolov
Copy link
Contributor

sidolov commented Oct 23, 2018

Hi @phoenix128 , will you continue work on this PR or we can close it for now?
Thank you!

@phoenix128
Copy link
Contributor Author

@sidolov , now should be fine.

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

5 participants