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

Fix and enhance usergroup permissions handling and display #16469

Merged
merged 5 commits into from
Jul 5, 2024

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Sep 7, 2023

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:

  1. Created a new parent class for the 5 User Group permissions grids
  2. Created a new parent class for the 5 Create windows
  3. Refactors implemented trim the code overall by ~450 lines
  4. Because the changes were so significant, code style issues were fixed at the same time (instead of in a separate commit)
  5. Updated style of the permissions display, both in windows and in the row expander (within the grids), making them significantly more readable
  6. Added a handful of Lexicons for select windows to make all window headings consistent. Additionally, a couple of descriptions were missing or needed updating.
  7. Altered how the window combos are set to avoid issue where certain combos would initially show the raw value instead of the displayField value (name in most cases)

Special Note

In the editing windows, I changed the Context combo’s displayField to name instead of key (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:

  1. Permissions were not initially showing up in the edit window (see Figure 1)
  2. A number of UI display issues needed solving (items 5–7 above) (see Figures 2 & 3)
  3. Lastly, this are had a large amount of duplicate/very similar code, making this area far from DRY.

Figure 1 — Before fix, missing perms list
fig-1

Figure 2 — Window before and after, showing css update
fig-1

Figure 3 — Grid before and after, showing css update
fig-3

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

  1. Run grunt build to ensure css is updated.
  2. Ensure you have a number of ACLs created for each of the Permissions areas in a User Group (Context, Resource Group, Category, Namespace, and Source).
  3. Test creating and editing various entries to verify the editing windows always display the correct information.

Related issue(s)/PR(s)

Resolves #16386

@smg6511
Copy link
Collaborator Author

smg6511 commented Sep 23, 2023

@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.

@cla-bot
Copy link

cla-bot bot commented Oct 25, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jim Graham.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@smg6511 smg6511 added the pr/review-needed Pull request requires review and testing. label Oct 30, 2023
@Mark-H Mark-H added this to the v3.1.0 milestone Feb 13, 2024
@smg6511
Copy link
Collaborator Author

smg6511 commented Mar 22, 2024

@GulomovCreative @Ibochkarev @muzzwood -- If any of you guys have time to test/review this, that'd be great!

@rthrash
Copy link
Member

rthrash commented Apr 16, 2024

@smg6511 great UX improvement … thanks for submitting the PR and looking forward to seeing this in 3.1.

@opengeek
Copy link
Member

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.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jun 13, 2024

@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.

@opengeek
Copy link
Member

@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.

@opengeek
Copy link
Member

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.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jun 13, 2024

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.

@opengeek
Copy link
Member

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! 😅

@smg6511
Copy link
Collaborator Author

smg6511 commented Jun 13, 2024

Wait. It says "Edit Mediasource Access"

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.

@opengeek
Copy link
Member

Ok, I'm just commenting on this "problem" in the wrong PR. Sorry. I'll move it to #16568.

@smg6511
Copy link
Collaborator Author

smg6511 commented Jun 28, 2024

I'd love to see this in 3.1. Anyone up for reviewing it ??? ;-)

@opengeek opengeek merged commit 7d90842 into modxcms:3.x Jul 5, 2024
6 checks passed
@smg6511 smg6511 deleted the 3.x-issue-16386 branch August 27, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"User Group Access to Context" window displaying incorrect permissions when reopened
4 participants