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

Open
wants to merge 4 commits into
base: 3.x
Choose a base branch
from

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.

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
3 participants