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

Remove deprecated Admin user multirole functionality #2876

Draft
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

elidrissidev
Copy link
Member

Description (*)

This PR aims to get rid of all the remnant code of multi-role functionality for Admin user. This functionality was deprecated ages ago and it's just tech debt at this point. Currently, only one Role can be assigned to an Admin user.

This is not meant to introduce any major breaking changes, although I did a change in a template file in adminhtml, but that's because I doubt someone will have that file overridden.

Todo

  • Consolidate Mage_Admin_Model_Resource_User::saveRelations() and Mage_Admin_Model_Resource_User::add() methods, both used to add a role to a user.
  • Deprecate methods that operate on array of roles (e.g. Mage_Admin_Model_Resource_User::getRoles), and replace them with methods that work with a single role.

Manual testing scenarios (*)

  1. Assigning a Role to an Admin user from user edit page should work as usual.
  2. Assigning a Role to an Admin user from role edit page (Role Users tab) should work as usual.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin Component: Install Relates to Mage_Install labels Jan 1, 2023
@elidrissidev elidrissidev added Cleanup backwards compatibility Might affect backwards compatibility for some users labels Jan 1, 2023
@Flyingmana
Copy link
Contributor

does it need to be in the 19.x releases?

@elidrissidev
Copy link
Member Author

does it need to be in the 19.x releases?

Not necessarily, I can change the target branch when I'm done.

@elidrissidev elidrissidev changed the base branch from 1.9.4.x to 20.0 January 3, 2023 11:26
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Backup Relates to Mage_Backup Component: Bundle Relates to Mage_Bundle Component: Captcha Relates to Mage_Captcha Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogRule Relates to Mage_CatalogRule Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogSearch Relates to Mage_CatalogSearch Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cm/RedisSession Relates to Cm_RedisSession Component: Cms Relates to Mage_Cms Component: ConfigurableSwatches Relates to Mage_ConfigurableSwatches Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: CurrencySymbol Relates to Mage_CurrencySymbol labels Jan 3, 2023
@github-actions github-actions bot removed Component: Centinel Relates to Mage_Centinel htaccess phpcs Component: CatalogInventory Relates to Mage_CatalogInventory Component: Cron Relates to Mage_Cron Component: CurrencySymbol Relates to Mage_CurrencySymbol ddev Component: CatalogSearch Relates to Mage_CatalogSearch Component: Cms Relates to Mage_Cms phpstan Component: Cm/RedisSession Relates to Cm_RedisSession Component: Authorizenet Relates to Mage_Authorizenet labels Jan 3, 2023
@elidrissidev
Copy link
Member Author

Created a bit of a mess, but this refactor will now target v20.

@sreichel
Copy link
Contributor

sreichel commented Jan 4, 2023

does it need to be in the 19.x releases?

Why not remove dead code at all?

@fballiano fballiano changed the base branch from 20.0 to main April 4, 2023 17:35
@fballiano
Copy link
Contributor

@elidrissidev what do we need to finish this?

@fballiano
Copy link
Contributor

@elidrissidev do you remember what's needed in order to finish this PR?

@elidrissidev
Copy link
Member Author

The Admin code is kinda messy and all over the place, the current state of the PR should be working fine, but there's still some classes that seem useless and could be removed (e.g. Mage_Admin_Model_Roles could be merged with Mage_Admin_Model_Role model since they represent the same entity) but I don't remember if refactoring will break BC.

@fballiano
Copy link
Contributor

I kinda think that this should be in next but, @elidrissidev do you think you'll restart your work on this or what should we do on this PR?

@fballiano fballiano changed the base branch from main to next April 20, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards compatibility Might affect backwards compatibility for some users Cleanup Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: Install Relates to Mage_Install documentation environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants