-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Enabled 'Shopping Cart' tab for customer edit interface in admin #20918
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
Enabled 'Shopping Cart' tab for customer edit interface in admin #20918
Conversation
Hi @rav-redchamps. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@magento-engcom-team give me test instance |
Hi @rav-redchamps. Thank you for your request. I'm working on Magento instance for you |
Hi @rav-redchamps, here is your new Magento instance. |
@magento-engcom-team seems like the Travis automated tests stucked. It is running for more than 24 hours. Kindly check. |
Hi @ihor-sviziev, thank you for the review. |
@rav-redchamps thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository. |
@magento-engcom-team give me test instance |
Hi @sivaschenko. Thank you for your request. I'm working on Magento instance for you |
Hi @sivaschenko, here is your new Magento instance. |
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 @rav-redchamps , thanks for the pull request!
Public methods should not be added to classes marked as API without approval by the architect because of backward compatibility concerns.
Please consider introducing a new block class implementing TabInterface
to avoid adding new public methods to an API class?
@sivaschenko make sense. Will do it by tomorrow via introducing a new block class implementing TabInterface |
… methods from API block class
…/magento2 into customer-shopping-cart-tab
@sivaschenko just pushed the changes. Please have a look |
Hi @sivaschenko, |
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 @rav-redchamps , thanks for your patience! Unfortunately, we couldn't approve the newly introduced usage of deprecated Registry. Can you please replace it with i.e. service class storing the current customer id.
*/ | ||
public function canShowTab() | ||
{ | ||
return $this->coreRegistry->registry(RegistryConstants::CURRENT_CUSTOMER_ID); |
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.
The registry is a deprecated class, please use another way to retrieve the current customer id. I.e. passing through an introduced service class.
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.
For example, introduce a class that will init customer id from the request and provide a method to retrieve this customer id.
@ihor-sviziev the pull request was put on hold as I needed to discuss and decide on whether the registry usage, in this case, can be accepted |
@sivaschenko PR updated, please review. |
Hi @sivaschenko, thank you for the review. |
✔️ QA Passed |
hi, |
Hi @rav-redchamps, thank you for your contribution! |
Description (*)
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)