-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
base: next
Are you sure you want to change the base?
Remove deprecated Admin user multirole functionality #2876
Conversation
does it need to be in the 19.x releases? |
Not necessarily, I can change the target branch when I'm done. |
e6e5b9b
to
d0d3b1f
Compare
Created a bit of a mess, but this refactor will now target v20. |
Why not remove dead code at all? |
d0d3b1f
to
bf591d6
Compare
@elidrissidev what do we need to finish this? |
@elidrissidev do you remember what's needed in order to finish this PR? |
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. |
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? |
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
Mage_Admin_Model_Resource_User::saveRelations()
andMage_Admin_Model_Resource_User::add()
methods, both used to add a role to a user.Mage_Admin_Model_Resource_User::getRoles
), and replace them with methods that work with a single role.Manual testing scenarios (*)
Contribution checklist (*)