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 (revised) #15919

Draft
wants to merge 39 commits into
base: 3.x
Choose a base branch
from

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Nov 25, 2021

NOTE: This is a simplified re-submission of a PR submitted on 11/23.

What does it do?

This changes in this 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'd envision doing this across multiple PRs so small logical batches could be tested and put into place over time (in as short a time as possible).

Screenshots Highlighting Changes Made

Using the Sources grid as an example (since it highlights all of the fixes/proposed features implemented), below are a few annotated visuals to get a quick idea of what to expect.

Screen Shot 2021-11-23 at 11 17 13 PM

Screen Shot 2021-11-23 at 11 38 07 PM

Screen Shot 2021-11-23 at 11 34 44 PM

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Nov 25, 2021
@modxcms modxcms deleted a comment from mishanthrop Nov 25, 2021
@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 26, 2021

@mishantrop - First of all I'd like to express that I appreciate you taking the time to review this PR. It would, however, be much more useful for you to review the functionality and the usability of what is being implemented rather than getting into the weeds with super fine-grained code suggestions and what are, in some instances, personal coding/code-style preferences.

Does the PR solve the problem it's trying to solve? Does it do it in a way that improves the UI? Does it bring a useful new feature? I'm certainly not looking to ignore your suggestions, and if I and others feel some of them would provide an improvement I'll make the change(s). I'm just saying I think your initial focus is in the wrong place.

@modxcms modxcms locked and limited conversation to collaborators Nov 26, 2021
@modxcms modxcms unlocked this conversation Nov 26, 2021
@mishanthrop
Copy link
Contributor

@smg6511 - First of all I'd like to express that I appreciate you taking the time to write new features..

> It would, however, be much more useful for you to review the functionality and the usability of what is being implemented
Changes to the code can affect functionality. Therefore, first of all, I pay attention to the quality (maintainability, readability, structure etc) of the code.

> in some instances, personal coding/code-style preferences
In "some", but not all. Updated my nit-comments.

> Does the PR solve the problem it's trying to solve?
I do not question the value of your PR. But I want to take part in the development of MODX and it is important for me that the code is pleasant to work with.

> I'm just saying I think your initial focus is in the wrong place.
Don't take my comments personally, this is a common practice in code reviews. Reviewers writes comments, and the author fixes the code or does not fix it. The main thing is to find a compromise.

@JoshuaLuckers
Copy link
Contributor

@smg6511 You're on fire! There's a lot to cover in this PR. Please take a look at the technical implementation of PR #14009. Some similar changes have been made relevant to some of yours.

I'll do my best and find some time in the upcoming days to provide feedback on the functionality and usability of the fixes and new features in this PR.

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 27, 2021

@JoshuaLuckers - I appreciate the encouragement! Yes, this initial PR is a bit to review, I know ;-) But, once finalized, it will provide an pretty easy framework to update the rest of the grids. I'll take a look at 14009 in just a bit...

OK, I just took a quick look through 14009 and I don't see any conflicts with what I've done. Even so, assuming that my PR in some form gets merged and I move on to other grids (in 14009's case the policy/policy template grids), I'll refer back to 14009 to ensure I don't walk back anything important.

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 27, 2021

@mishantrop - Don't worry, I'm not taking anything personally. And I, of course, agree that code quality, consistency, and conformity to a standard is also very important. (I also know from comments on other issues/PRs that you and I might not share the same viewpoint on some stylistic and implementation conventions, but that's Ok.) I was just pointing out that I don't think that's the first thing to focus on when you're reviewing a PR.

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 27, 2021

@JoshuaLuckers - Don't know if you withdrew the comments you were making re #14009. (I was about to respond and they disappeared.) Anyway, I see I'll want to carry through how the core items are defined in constants in each object's base class, as they were done in the modAccessPolicy one.

However, I do think my implementation of the final relay of those permissions via prepareRow of each object class is more straightforward and immediately readable. So when you're reading the record in js, your getting, for example:

id: 3,
permissions: {
   create: false,
   update: true,
   delete: false
}

instead of:

id: 3,
cls: 'pedit'

The way I've done it is also very easy to iterate through without any additional fuss.

Note also, I think it makes sense is to use CRUD nomenclature across the back end (in code), which is why I've changed for example edit to update and remove to delete. On the front end, it makes more sense (in my mind at least) to use natural language (e.g., Edit X instead of Update X). Could be over-thinking it though ;-)

@JoshuaLuckers
Copy link
Contributor

Yes I have deleted my comment. I have a lot to share but I still need to find time to properly work it out haha.

But your in the right direction with where I want to go, but you're right about the class names like pedit.

If your on Slack I could share my unstructured notes.

@smg6511
Copy link
Collaborator Author

smg6511 commented Nov 27, 2021

Yes, I am on Slack for another project I work on. How shall we connect?

Fix merge issue from last rebase
Conflict resolution
Change back to public class props
Various functionality-related changes to grid base js class and utilities lib
Various additions and optimizations, as well as code quality improvements
Various additions and optimizations, as well as code quality improvements
Small tweaks and carry through of methods used in other core object classes to identify protected data keys
Clean up and remove creator sorting
Clean up and remove creator sorting
Clean up and remove creator sorting
Clean up and remove creator sorting
Small code formatting update
Small css update for text links
Formatting, style, optimization updates. No substantive changes.
Formatting, code style, optimization, modernization only. No substantive changes.
Adds a few shared methods for generating common grid UI elements
WIP, mute translation render method; changing the way this works and probably will not need the method after rework...
@smg6511 smg6511 force-pushed the 3.x-grids-perms-validation-pr01 branch from 70babce to 5dbf124 Compare October 29, 2024 06:32
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. in-progress PRs that are in progress by the author or contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants