-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Issue-25968 - Added additional checking for returning needed variable… #26313
Issue-25968 - Added additional checking for returning needed variable… #26313
Conversation
Hi @AndreyChorniy. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AndreyChorniy, thanks for your contribution, could you please check the comments that I left?
Hi @AndreyChorniy. Please, take a look at the failing tests as well. Thank you |
Covering all the use cases in single Test method is considered to be a bad practice. You should introduce another Test method - for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your fixes, could you please check the other minor recommendations?
Hello @rogyar, I have fixed failed tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me.
Thanks.
Hi @rogyar, thank you for the review. |
✔️ QA passed |
Hi @AndreyChorniy, thank you for your contribution! |
Description (*)
This PR add fix for returned type for getPrice() method. Also added unit tests coverage for method getPrice.
Fixed Issues (if relevant)
getPrice()
returns a string when setting custom price in admin order #25968:getPrice()
returns a string when setting custom price in admin orderManual testing scenarios (*)
We have not easy steps to manual test these changes.
I can recommend use sandboxes to get test it more easily