-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Fix and enhance usergroup permissions handling and display #16469
Conversation
@muzzwood - Hey - wanted to see if you'd have time to review this fix, since it's related to one of the issues you posted. |
b611d88
to
e4dc4a2
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jim Graham.
|
71da7e1
to
4100673
Compare
@GulomovCreative @Ibochkarev @muzzwood -- If any of you guys have time to test/review this, that'd be great! |
@smg6511 great UX improvement … thanks for submitting the PR and looking forward to seeing this in 3.1. |
Can we change the dropdown for "minimum role" to "authority" and just make it display the integer value? There is no direct link between authority in a permission and the role from which it is set. |
@opengeek Hey Jason, hope all's well - I'd argue that even if there's not necessarily a direct correlation between role and authority, displaying the field and labelling as I have justs makes the permission easier to immediately comprehend. IMO, the design of the permissions overall is too complex, perhaps due in part to the use of abstract terminology/settings such as authority (especially given its value and effect being the reverse of what everyday CMS managers would expect). Of course (remembering to take your programmers hat off), if you feel the way I did this will ultimately lead to more confusion I'll make the proposed change. BTW (if I remember correctly?) in a somewhat recent discussion with you on Slack, I think we landed on possibly removing authority altogether in the future (simplifying to just roles). I know that may not pertinent right now, but may inform how much effort goes into tinkering with the current architecture around this permissions detail. |
Honestly, the problem is two roles could have the same authority, so the role itself is only used to set the authority when creating an ACL. There is no valid way to link back to the role. We should just find a way to make it clear that roles are only used to set the authority value when creating an access control entry. It cannot be removed without a breaking change, so that will not happen in 3.x. I won't argue that it is too complex. But it's what we have for 3.x and we just need to find a way to make it work properly and clarify the intention as it exists. |
Would making the authority field visible as a simple integer value and making it immutable work? Or would leaving it mutable and including a description explaining its purpose so that if someone wants to manipulate the integer value manually, they can without going into the database? To be honest, I'm not sure what the best course of action is to handle it. I'm just trying to ensure referential integrity until we can get past 3.x. |
I'll look at this more this evening. The other item I was working on (PR #16568), centered around the links to the various permissions facets in the ACL grids or grids containing ACL assignments (which sparked the Slack chat we had) would coordinate with this, and might address what you're concerned with (in terms of mutability). Also, making sure we're on the same page, this PR pertains to the assignment of an ACL, not the alteration of the ACL itself. |
Wait. It says "Edit Mediasource Access" in the screenshots! I'm so confused! 😅 |
Correct, that screenshot depicts when you're in ACLs -> User Group -> Permissions -> Sources and you use the grid's contextual menu (or gear menu) to edit a particular ACL assignment. |
Ok, I'm just commenting on this "problem" in the wrong PR. Sorry. I'll move it to #16568. |
I'd love to see this in 3.1. Anyone up for reviewing it ??? ;-) |
What does it do?
Among a number of refactoring moves, the solution submitted here provides a new and centralized method to update the “Permissions in Selected Policy” component found in the various Create and Update windows within the ACLs area. Also:
Special Note
In the editing windows, I changed the Context combo’s displayField to
name
instead ofkey
(context_key
). Even as a developer, I've always found showing the key cumbersome — to me it's much more natural to associate with the name of any object rather than its id/key. If reviewers and code owners agree, I'd also implement that in the grids (which currently still display the key).Why is it needed?
As mentioned in #16386, permissions lists (in windows) were not updating correctly. Also:
Figure 1 — Before fix, missing perms list
Figure 2 — Window before and after, showing css update
Figure 3 — Grid before and after, showing css update
Figure 4 — Video clip showing correct permissions display when opening and after closing and re-opening editing windows
https://github.com/modxcms/revolution/assets/689075/f008cd55-97fb-432a-aa7a-d2bf8c2b28df
How to test
Related issue(s)/PR(s)
Resolves #16386