Skip to content

FIPRO-37 Refactor AdminUser Class #13

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.

Sign up for GitHub

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

Open
Nuuttif wants to merge 1 commit into
base: master
Choose a base branch
from
Open

FIPRO-37 Refactor AdminUser Class #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions src/App/Action/Plugin/Authentication.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,9 @@ class Authentication
private $config;

/**
* @var \Vaimo\AdminAutoLogin\Model\Config\Source\AdminUser
* @var \Vaimo\AdminAutoLogin\Model\Config\Source\AdminUsernameList
*/
private $adminUserSource;
private $adminUsernameList;

/**
* @var AdminSessionsManager
Expand All @@ -86,19 +86,19 @@ class Authentication
* @param \Magento\Framework\Data\Collection\ModelFactory $modelFactory
* @param \Magento\Framework\Controller\Result\RedirectFactory $resultRedirectFactory
* @param \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig
* @param \Vaimo\AdminAutoLogin\Model\Config\Source\AdminUser $adminUserSource
* @param \Vaimo\AdminAutoLogin\Model\Config\Source\AdminUsernameList $adminUsernameList
* @param AdminSessionsManager $adminSessionsManager
*/
public function __construct(
\Magento\Backend\Model\Auth $auth,
\Magento\Backend\Model\UrlInterface $backendUrl,
\Magento\Framework\Event\ManagerInterface $eventManager,
\Magento\Framework\Message\ManagerInterface $messageManager,
\Magento\Framework\Data\Collection\ModelFactory $modelFactory,
\Magento\Framework\Controller\Result\RedirectFactory $resultRedirectFactory,
\Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
\Vaimo\AdminAutoLogin\Model\Config\Source\AdminUser $adminUserSource,
\Magento\Security\Model\AdminSessionsManager $adminSessionsManager
\Magento\Backend\Model\Auth $auth,
\Magento\Backend\Model\UrlInterface $backendUrl,
\Magento\Framework\Event\ManagerInterface $eventManager,
\Magento\Framework\Message\ManagerInterface $messageManager,
\Magento\Framework\Data\Collection\ModelFactory $modelFactory,
\Magento\Framework\Controller\Result\RedirectFactory $resultRedirectFactory,
\Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
\Vaimo\AdminAutoLogin\Model\Config\Source\AdminUsernameList $adminUsernameList,
\Magento\Security\Model\AdminSessionsManager $adminSessionsManager
Copy link
Contributor

@evktalo evktalo Jul 8, 2022

Choose a reason for hiding this comment

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

Not a fan of the fancy indentation, it's a pain to upkeep

Copy link
Contributor Author

@Nuuttif Nuuttif Jul 8, 2022

Choose a reason for hiding this comment

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

Hmmm.. maybe we should change the whole styling of the code anyway.

) {
$this->auth = $auth;
$this->backendUrl = $backendUrl;
Expand All @@ -107,7 +107,7 @@ public function __construct(
$this->modelFactory = $modelFactory;
$this->resultRedirectFactory = $resultRedirectFactory;
$this->config = $scopeConfig;
$this->adminUserSource = $adminUserSource;
$this->adminUsernameList = $adminUsernameList;
$this->adminSessionsManager = $adminSessionsManager;
}

Expand Down Expand Up @@ -182,7 +182,7 @@ private function getLoginUsername()
return $username;
}

$usernameList = array_keys($this->adminUserSource->toArray(false));
$usernameList = array_keys($this->adminUsernameList->get(false));

foreach (self::DEFAULT_USERNAMES as $username) {
if (in_array($username, $usernameList, true)) {
Expand Down
30 changes: 6 additions & 24 deletions src/Model/Config/Source/AdminUser.php → ...Model/Config/Source/AdminUsernameList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,8 @@
*/
namespace Vaimo\AdminAutoLogin\Model\Config\Source;

use function __;

class AdminUser implements \Magento\Framework\Option\ArrayInterface
class AdminUsernameList
Copy link
Contributor

@evktalo evktalo Jul 8, 2022

Choose a reason for hiding this comment

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

The ArrayInterface is actually needed, for the dropdown in module's admin config when selecting the admin user to automatically log in as.

So please refactor like this:

  • Move this model to Vaimo/AdminAutoLogin/Model namespace, it's good as this
  • Recreate Model/Config/Source/AdminUser, so that its toOptionArray implementation uses AdminUsernameList

Copy link
Contributor Author

@Nuuttif Nuuttif Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Wow totally missed that 😁. That's a smart idea, thank you! Regarding the indentation I'm not sure I agree.. But maybe we should change the whole styling of this module anyways. It's kind of weird / out dated.

Copy link
Contributor

@evktalo evktalo Jul 8, 2022

Choose a reason for hiding this comment

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

We should style according to Magento coding standard guidelines with phpcs!

{

/**
* @var \Magento\User\Model\ResourceModel\User\Collection
*/
Expand All @@ -21,12 +18,13 @@ class AdminUser implements \Magento\Framework\Option\ArrayInterface
private $users;

/**
* AdminUser constructor.
* AdminUsernameList constructor.
*
* @param \Magento\User\Model\ResourceModel\User\Collection $userCollection
*/
public function __construct(\Magento\User\Model\ResourceModel\User\Collection $userCollection)
{
public function __construct(
\Magento\User\Model\ResourceModel\User\Collection $userCollection
) {
$this->userCollection = $userCollection;
}

Expand All @@ -36,7 +34,7 @@ public function __construct(\Magento\User\Model\ResourceModel\User\Collection $u
* @param bool $includeEmptyChoice
* @return array
*/
public function toArray($includeEmptyChoice = true)
public function get($includeEmptyChoice = true)
{
if ($this->users === null) {
$this->users = [];
Expand All @@ -56,20 +54,4 @@ public function toArray($includeEmptyChoice = true)
return $this->users;
}

/**
* Options getter
*
* @return array
*/
public function toOptionArray()
{
$users = [];

foreach ($this->toArray() as $value => $label) {
$users[] = ['value' => $value, 'label' => $label];
}

return $users;
}

}