-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#: Replace deprecated methods in back-end controllers of Magento/Customer #24285
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
Conversation
Hi @atwixfirster. 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.
@atwixfirster could yo please check the failing unit 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.
Hi @atwixfirster,
Could you review and fix integration tests?
@magento run all tests |
done Thanks, guys! |
Hi @ihor-sviziev, thank you for the review.
|
During testing we faced the issue Problem: "404 Error" message occurs when trying to "Subscribe to Newsletter" via mass action Manual testing scenario:
Actual Result: @atwixfirster Could you take a look, please? Thanks! |
Thank you, @engcom-Alfa ! I will check |
@dmytro-ch : Should they combine to one?? |
@edenduong, these PRs will be combined after approval. |
It's really easier to test each PR separately |
I agree, it depends on PRs. |
Hi @ihor-sviziev, thank you for the review. |
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.
@atwixfirster please, take a look at the issue reported by @engcom-Alfa
@sidolov, of course . I will check that issue in 3-4 hours. Thanks |
887c9cc
to
d1e9194
Compare
fixed Thank you, @engcom-Alfa 👍 |
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 @atwixfirster,
Unfortunately integration tests started failing after your latest change.
Please review & fix them.
Thank you!
… of Magento/Customer - app/code/Magento/Customer/Controller/Adminhtml/Index/MassAssignGroup.php - app/code/Magento/Customer/Controller/Adminhtml/Index/MassDelete.php - app/code/Magento/Customer/Controller/Adminhtml/Index/MassSubscribe.php - app/code/Magento/Customer/Controller/Adminhtml/Index/ResetPassword.php - app/code/Magento/Customer/Controller/Adminhtml/Index/Save.php - app/code/Magento/Customer/Controller/Adminhtml/Index.php - app/code/Magento/Customer/Controller/Adminhtml/Locks/Unlock.php
d1e9194
to
5842160
Compare
Good morning, @ihor-sviziev! fixed |
Hi @dmytro-ch, thank you for the review. |
✔️ QA Passed |
…d controllers of Magento/Customer #24285
Hi @atwixfirster, thank you for your contribution! |
Description (*)
Pull request replaces deprecated
addError
,addSuccess
,addException
methodsin the next controllers:
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)
Thank you!