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

[3.x] Refine grids code to correct actions access and visibility #15915

Closed
wants to merge 6 commits into from

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Nov 23, 2021

NOTE: This is really a draft PR, although I'd like one of the code owners to do a review for (hopefully) initial approval to proceed further (which is why I've not tagged it as a draft yet).

What does it do?

This draft PR initially set out to fix issues raised in #14929, which uncovered other problems in the grids UI. There are quite a few loose ends there in terms of reflecting what a user can do according to their group permissions and object-level access policies (when applied [to contexts, media sources, etc.]). Additionally there's a lack of full validation within some grids, including their create forms. Also, as @Ruslan-Aleev points out in #15281, validation response is inconsistent because the proper specification is missing on the js side of things. I've arrived at the solution you'll see by creating a number of methods and properties in the main grid class (modx.grid.js), as well as an updated way to identify/apply permissions in the child grid classes and their respective GetList processors.

Fixes

  1. First and foremost, the visibility of the actions (gear) menu and its contents matched to permissions
  2. Ensures certain core entries are protected from deletion and that certain core entry values can not be duplicated in other user-entered entries (e.g., Filesystem [in sources], mgr and web keys and Manager name [in contexts], Super User and Member name [in roles])
  3. Provides in-grid validation where it was missing and more visible feedback in some instances
  4. General code consistency and clarity in the associated js and php files
  5. Match front end validation messages with back end ones (mostly via adding the blankText config to required fields)
  6. For checkbox model grids (e.g., the media sources one shown in this PR), adds a new checkbox style for rows that can not be selected for batch processing (protected from deletion in this case). Further, the batch delete toolbar menu item will only be active when one or more deletable grid items is selected.

Proposed Enhancements Included

  1. Created a new arbitrary field (creator) to distinguish core entries from user entries. This enables better control the over view (i.e., you can sort on this field) and a mechanism to hide one group or the other via a toolbar control (not implemented here). IMO the ability to hide, for example, the 12 core policy entries when working on your own set of policies would be nice (especially if you're working on a relatively large set of policies).
  2. Also set apart core entries by italicizing them and setting a slightly different color ($colorSplash)
  3. Changed the anchor/link style to a dotted bottom border instead of solid. There many solid rules/borders in the UI, which is fine, but the solid text decoration on links seems over the top to me. IMO the dotted rule is still plenty visible but tones it down a bit.

How to test

Note that while I have applied some of my changes to a couple other grids, their implementation may be incomplete. The three grids to test for this draft are:

  • The Roles grid (ACLs/Roles [tab])
  • The Contexts grid
  • The Media Sources grid

Process

  1. Run grunt build to ensure css is updated.
  2. Create a secondary user that you will apply lower-level permissions to; then log in as this user from a different browser)
  3. Create a new [Test User Group] and ACL administrator policy to apply to that group; then add your new secondary user to the test group.
  4. Experiment with a variety or permissions in this new policy and verify their effect on each grid. The relevant permissions for each grid are:
    • Roles: save_role, edit_role, new_role, and delete_role
    • Contexts: save_context, edit_context, new_context, and delete_context
    • Sources: source_save, source_edit, and source_delete
  5. Create a new context policy using the ContextTemplate template and a new media source policy using the MediaSourceTemplate template that will be used to further limit permissions for specific test contexts and sources you've created.
  6. For contexts and sources, create new user-group specific access rules in ACLs/[Test User Group]/Access Permissions. Experiment with these permissions and verify your secondary user's access is reflected accurately in each grid.
  7. In each of the three grids, attempt to rename one of your entries with the same name as one of the core entries (where creator is 'modx'). You should see an alert prompting you to enter another valid value.

As those of you who actually coded these areas of MODX or are very familiar with their inner workings, this is a necessarily highly tedious process. I'm hoping at least a couple of decision makers have the time to dive in to playing with this draft, as there are quite a few grids to go through to fully implement these changes (assuming they get the green light). I'd envision doing this across multiple PRs so small logical batches could be tested and put into place over time (as short a time as possible).

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Nov 23, 2021
@Ruslan-Aleev
Copy link
Collaborator

Thank you for your work, but I have to criticize you, do not be offended :)

  1. Such global fixes are almost impossible to test, there are not enough people. If I were you, I would do separate PRs, in stages, without trying to cover a lot.
    For example, you did PR for TV, but there were UX questions (I have at least), maybe it is worth solving them, than throwing headlong into new global fixes? :)
  2. Commits are not useful because include everything at once, in fact.
  3. Add screenshots, if it's a UX PR, to immediately indicate the fixes, and it's easier to understand what has changed.
  4. You are returning corrections that were made by other members. For example, in Simplified lexicons for grids #15420 I removed unnecessary lexicons for grids, simplified them. And you returned these lexicons :) And this is extra code and translations, sooooo many translations, which is annoying.
    There was such a problem with PR on TV too, be more attentive in the future.

In any case, thanks for your participation!
I hope I haven't offended you. It's great that you are actively involved in the process, but I wish that participation was productive for everyone: you and the testers. Together we can make MODX better!

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 23, 2021

@Ruslan-Aleev - No offense taken. Maybe additional explanation is needed as well. In response to your comments:

  1. I know this is looking big, and there are more in the commits than I might submit as a first real PR. That's why I pointed out that this is meant as a draft and focused on the three grids in particular. As I mentioned this really began as a fix for the actions menu, but I found so many issues that were intertwined along the way. I wasn't expecting that, I was just expecting to create and submit a pretty simple fix for the issue at hand ;-) But to properly make the fix and make the methodology more universally usable (and be able to address the other issues found), I had to make a decent amount of changes/additions. I know there are a handful of files which ideally wouldn't be in this draft, but I'm a little paranoid about losing work when putting so much thought into it. That said, I get what you're saying and the initial impression you may have gotten — I just saw this issue as a bit more critical than the fine-tuning what needs to continue to be done on my TVs PR you referenced.
  2. Sorry for the un-ideal commits — as I said, while working this out I just get uneasy about losing things.
  3. Ok, I can add some screenshots. At the same time, with this being on the security/ACL side of things it's still going to take some review via experimentation rather than replicating what I might show in screenshots. But I can see how they'd be helpful to get started.
  4. I based this on the current 3.x branch, so I'm not sure how I would have lexicons that were removed by others. Let me know a few that you're specifically seeing. I'll look at the PR you referenced as well.

Look forward to continuing the dialog...

@Ruslan-Aleev
Copy link
Collaborator

  1. No, I confused the lexicon of the name of the policy with the lexicon of the grid, sorry.

Anyway, I would recommend that you simplify your PR. If in the process of work you find an error, then it is better to create an issue and come back to it later. Otherwise, be prepared for the fact that your PRs will not be tested for a long time and will not be merged, due to the fact that there are not enough people for tests.

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 25, 2021

I've re-submitted a cleaner version of this PR (see #15919).

@smg6511 smg6511 closed this Nov 25, 2021
@smg6511 smg6511 deleted the 3.x-issue-14929 branch July 19, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants