Admin user auth controller refactor #16560

AnshuMishra17 · 2018-07-05T12:35:39Z

Description

Refactor the code by removing the direct use of ObjectManager and includes the dependencies using Constructor Dependency Injection

Manual testing scenarios

Created a new admin user
Login and logout using new admin user
Clicked on forgot password and provided new admin user email id.
Clicked on reset password link in email.
Created new password from reset password page.
Login using new password.

magento-engcom-team · 2018-07-05T12:35:43Z

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

arnoudhgz · 2018-07-05T22:53:43Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

+    /**
+     * @var \Magento\Backend\Helper\Data
+     */
+    protected $_backendHelper;


It is actually a backendDataHelper. The Backend\Helper namespace has more Helper classes.

arnoudhgz · 2018-07-05T22:55:21Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

     */
    public function __construct(
        \Magento\Backend\App\Action\Context $context,
-        \Magento\User\Model\UserFactory $userFactory
+        \Magento\User\Model\UserFactory $userFactory,
+        \Magento\Backend\Helper\Data $backendHelper = null


Maybe you could do the PR on 2.3-develop instead for the refactoring. Than you can remove the = null part and you don't need the if statement on line 42 and 43.

I can do a forwardport for 2.3-develop.

arnoudhgz · 2018-07-05T22:56:56Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

@@ -6,6 +6,7 @@
 namespace Magento\User\Controller\Adminhtml;

 use Magento\Framework\Encryption\Helper\Security;
+use Magento\Framework\App\ObjectManager;


Since you are refactoring, please add import statement for all used classes in this file.

Thanks! Please also do the ones on line 24 and 29.

arnoudhgz · 2018-07-05T22:57:27Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

@@ -7,26 +7,38 @@
 namespace Magento\User\Controller\Adminhtml\Auth;

 use Magento\Security\Model\SecurityManager;
+use Magento\Framework\App\ObjectManager;


Please import all used classes in this file.

arnoudhgz · 2018-07-05T22:58:31Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

     */
    public function __construct(
        \Magento\Backend\App\Action\Context $context,
        \Magento\User\Model\UserFactory $userFactory,
-        \Magento\Security\Model\SecurityManager $securityManager
+        \Magento\Security\Model\SecurityManager $securityManager,
+        \Magento\User\Model\ResourceModel\User\CollectionFactory $userCollectionFactory = null


Maybe you could do the PR on 2.3-develop instead for the refactoring. Than you can remove the = null part and you don't need the if statement on line 40 and 41.

I can do a forwardport for 2.3-develop.

arnoudhgz · 2018-07-05T23:00:36Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

@@ -54,7 +66,7 @@ public function execute()
                    $this->messageManager->addErrorMessage($exception->getMessage());
                    return $resultRedirect->setPath('admin');
                }
-                $collection = $this->_objectManager->get(\Magento\User\Model\ResourceModel\User\Collection::class);
+                $collection = $this->userCollectionFactory->create();
                /** @var $collection \Magento\User\Model\ResourceModel\User\Collection */


switch line 69 and 70

arnoudhgz · 2018-07-06T06:03:02Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

+use Magento\Framework\App\ObjectManager;
+use Magento\Backend\App\Action\Context;
+use Magento\User\Model\UserFactory;
+use Magento\Security\Model\SecurityManager;


This specific one is now imported twice. Please fix.

arnoudhgz · 2018-07-06T08:32:12Z

@AnshuMishra17 could you make the PR to 2.3-develop, than you could make it backwards-incompatible and remove the ObjectManager

arnoudhgz · 2018-07-06T09:42:28Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

-        \Magento\User\Model\UserFactory $userFactory
+        Context $context,
+        UserFactory $userFactory,
+        Data $backendDataHelper = null


Please do PR on 2.3-develop and remove the =null

This part was done correctly, no need to introduce backward incompatible changes if possible

arnoudhgz · 2018-07-06T09:43:09Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

    ) {
        parent::__construct($context);
        $this->_userFactory = $userFactory;
+        $this->_backendDataHelper = $backendDataHelper ?: ObjectManager::getInstance()->get(Data::class);


Please do PR on 2.3-develop and remove the ?: ObjectManager::getInstance()->get(Data::class); part

This part was done correctly, no need to introduce backward incompatible changes if possible

arnoudhgz · 2018-07-06T09:44:59Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

+        Context $context,
+        UserFactory $userFactory,
+        SecurityManager $securityManager,
+        CollectionFactory $userCollectionFactory = null


Please do PR on 2.3-develop and remove the =null

This part was done correctly, no need to introduce backward incompatible changes if possible

arnoudhgz · 2018-07-06T09:46:58Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

    ) {
        parent::__construct($context, $userFactory);
        $this->securityManager = $securityManager;
+        $this->userCollectionFactory = $userCollectionFactory ?:


Please do PR on 2.3-develop and remove the ?: ObjectManager::getInstance()->get(Data::class); part

This part was done correctly, no need to introduce backward incompatible changes if possible

AnshuMishra17 · 2018-07-06T09:55:42Z

How can I make backward incompatible?
Do I need to close this one and create a new PR for 2.3-develop?
I am not sure what are the correct steps for this.
Kindly share the steps.

arnoudhgz · 2018-07-06T10:52:32Z

@AnshuMishra17

First option (probably not working)
You can try to just update the target:

Second option
If that is not possible start a new branch from 2.3-develop, do a PR for the new one and close this indeed. Please add a comment to me (with @arnoudhgz mention) the new PR referring this old one.

AnshuMishra17 · 2018-07-06T11:08:09Z

@arnoudhgz Second option seems to be better. But I am not aware of how to fetch/pull 2.3-develop. I would be nice and helpful if you can share correct steps.

arnoudhgz · 2018-07-06T12:24:15Z

@AnshuMishra17 You created a local branch from 2.2-develop, right?

You should first update your fork: https://help.github.com/articles/syncing-a-fork/
Checkout the 2.3-develop branch
Create a new branch from that branch and name it, similar to you current branch (but keep it specific for 2.3 so you don't mixup)
Apply the changes manually in the same files
Do a PR again

ihor-sviziev · 2018-07-11T05:51:26Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

-        \Magento\User\Model\UserFactory $userFactory
+        Context $context,
+        UserFactory $userFactory,
+        Data $backendDataHelper = null


This part was done correctly, no need to introduce backward incompatible changes if possible

ihor-sviziev · 2018-07-11T05:51:46Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

    ) {
        parent::__construct($context);
        $this->_userFactory = $userFactory;
+        $this->_backendDataHelper = $backendDataHelper ?: ObjectManager::getInstance()->get(Data::class);


This part was done correctly, no need to introduce backward incompatible changes if possible

ihor-sviziev · 2018-07-11T05:51:59Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

+        Context $context,
+        UserFactory $userFactory,
+        SecurityManager $securityManager,
+        CollectionFactory $userCollectionFactory = null


This part was done correctly, no need to introduce backward incompatible changes if possible

ihor-sviziev · 2018-07-11T05:52:27Z

app/code/Magento/User/Controller/Adminhtml/Auth/Forgotpassword.php

    ) {
        parent::__construct($context, $userFactory);
        $this->securityManager = $securityManager;
+        $this->userCollectionFactory = $userCollectionFactory ?:


This part was done correctly, no need to introduce backward incompatible changes if possible

ihor-sviziev · 2018-07-11T05:52:56Z

app/code/Magento/User/Controller/Adminhtml/Auth.php

+    /**
+     * @var Data
+     */
+    protected $_backendDataHelper;


please make this variable private and remove underscore

arnoudhgz · 2018-07-11T06:19:15Z

@AnshuMishra17 my apologies, I thought you shouldn't use the ObjectManager anymore like this. Please let @ihor-sviziev help you out with this PR.

Thanks @ihor-sviziev I learned something today too.

magento-engcom-team · 2018-07-11T15:24:48Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2282 has been created to process this Pull Request

ihor-sviziev

Hi,
Intregration tests failed and DI compile also failed. Please try to fix them

magento-engcom-team · 2018-07-12T06:17:44Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2282 has been created to process this Pull Request

magento-engcom-team · 2018-07-23T14:04:05Z

Hi @AnshuMishra17. Thank you for your contribution.
We will aim to release these changes as part of 2.2.6.
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.

Admin user auth controller refactor

d5e2546

magento-engcom-team added the Component: User label Jul 5, 2018

arnoudhgz reviewed Jul 5, 2018

View reviewed changes

Changes as per comments on Github

caade16

arnoudhgz reviewed Jul 6, 2018

View reviewed changes

Changes as per comments on Github

b4f2e27

arnoudhgz reviewed Jul 6, 2018

View reviewed changes

AnshuMishra17 mentioned this pull request Jul 9, 2018

[forwardport] Admin user auth controller refactor #16650

Closed

ihor-sviziev self-assigned this Jul 11, 2018

ihor-sviziev requested changes Jul 11, 2018

View reviewed changes

AnshuMishra17 added 2 commits July 11, 2018 13:19

changes as per comments on GitHub

adec3da

missing parameter correction

fd0f48a

ihor-sviziev approved these changes Jul 11, 2018

View reviewed changes

magento-engcom-team added this to the Release: 2.2.6 milestone Jul 11, 2018

magento-engcom-team added the Release Line: 2.2 label Jul 11, 2018

ihor-sviziev added the Progress: needs update label Jul 11, 2018

ihor-sviziev requested changes Jul 11, 2018

View reviewed changes

magento-engcom-team removed the Progress: needs update label Jul 11, 2018

ihor-sviziev added the Progress: needs update label Jul 11, 2018

magento-engcom-team added Progress: needs update and removed Progress: needs update labels Jul 11, 2018

included missing context class

de9ddca

ihor-sviziev approved these changes Jul 12, 2018

View reviewed changes

ihor-sviziev removed the Progress: needs update label Jul 13, 2018

magento-engcom-team added the Progress: needs update label Jul 13, 2018

sidolov removed the Progress: needs update label Jul 16, 2018

magento-engcom-team merged commit de9ddca into magento:2.2-develop Jul 23, 2018

magento-engcom-team pushed a commit that referenced this pull request Jul 23, 2018

ENGCOM-2282: Admin user auth controller refactor #16560

cdb3342

This was referenced Jul 25, 2018

[Forwardport] Admin user auth controller refactor #17141

Closed

[Backport] Admin user auth controller refactor #17156

Closed

[Forwardport] Replacing Usage of Deprecated Methods for Message Manager #17140

Merged

Admin user auth controller refactor #16560

Admin user auth controller refactor #16560

Uh oh!

Conversation

AnshuMishra17 commented Jul 5, 2018

Description

Manual testing scenarios

Uh oh!

magento-engcom-team commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnoudhgz Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnoudhgz commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnshuMishra17 commented Jul 6, 2018

Uh oh!

arnoudhgz commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnshuMishra17 commented Jul 6, 2018

Uh oh!

arnoudhgz commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

arnoudhgz Jul 5, 2018 •

edited

Loading

arnoudhgz commented Jul 6, 2018 •

edited

Loading