Skip to content
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

FIPRO-37 Refactor AdminUser Class #13

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

Conversation

Nuuttif
Copy link
Contributor

@Nuuttif Nuuttif commented Jul 7, 2022

Refactored Vaimo\AdminAutoLogin\Model\Config\Source\AdminUser.php

Changes:

  • Renamed the class and toArray() methods
  • Refactored to not implement \Magento\Framework\Option\ArrayInterface
  • Removed toOptionArray() method
  • Cleaned code

Why:

  • The Class is only used to get a list of admin usernames. New naming makes the single purpose of the class more clear
  • Inheritance of \Magento\Framework\Option\ArrayInterface is redundant
  • toOptionArray() is not used anywhere
  • Easier to test

\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

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

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.

use function __;

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

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

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

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants