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 ACL Permissions Grids and Filtering #16355

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Jan 18, 2023

After difficulties rebasing and resolving conflicts of the original PR (#16251) in preparation for final merge, elected to create this clean re-submission that resolves all issues.

What does it do?

Made a number of changes and additions in the processors and grid JS files to correct issues with the grids’ grouping functionality and more accurately build the grid filters depending on the data shown. More specifically:

  1. Replaced the column model renderers (used to render links on column data) with a templatecolumn specification (generated by a new method in the grids base class), as using renderers on grouping grids interferes with the grouping functionality (because a renderer is an interceptor method in Ext).
  2. Moved menu specifications (grid context menus) out of processors and into the grid js for all sections (context was already this way). The primary reason being that some events (such as afterRemoveRow) were not firing when set up this way; monitoring these events is necessary for maintaining accurate filter options when making changes to a grid's data. Also, there's no reason to specify js in php unless the item in question will change on the fly and needs setup data from the processor.
  3. Grid view, including the bottom toolbar, now correctly updates when a row is deleted.
  4. Created new methods to update dependent filters (when two or more filters are present) based upon the selected value of any given filter.
  5. Fixed some filters that were missing options due to an incorrect specification of their baseParams.group property.

Why is it needed?

Grouping in these grids was not working correctly. Also, providing filter options where no relevant data is available in a given grid is a UX problem that needs to be fixed not only here, but across all grids with filters.

How to test

  1. Ensure modx and browser caches are fully cleared.
  2. Create a number of policies in each area (if you don't already have some set up) and play with the grouping and filtering of items in each grid. (These are the five Access Permissions grids — Context, Resource Groups, Element Categories, Media Sources, and Namespaces.) Verify that filters only show relevant options depending on the grid data and other filters you've made selections on.
  3. Verify that other operations (editing and deleting) affect filter options as expected.

Related issue(s)/PR(s)

Resolves #16239.

Special Note

Because these updates involve a lot of changes, I didn't want to execute them for all relevant grids at once. This PR completes a logical section and, depending on any requested changes, other PRs will follow what I've done here for other grids.

Before and After Examples

Currently, in MODx 3.0.3-dev, note each of the following problems (in order of appearance in the video):

  1. Each ACL area grid does not group properly; where an area's rows should be grouped under a common heading, they are not. To see this clearly, pause each time I click on each area and note where grouping should be occurring (for example, in Contexts, the three assigned the Editors - 1000 Role should be grouped under one row heading for that Role).
  2. [0:19] Next, notice how the grid filters show irrelevant options where they should only show those present in the grid data. The Filter by Context combo shows all Contexts instead of those present (dupctx1, dupctx2, dev2, and www2). The same goes for the Filter by Policy combo. [0:42] Additionally, the Filter by Policy combo is mis-configured in places; here the problem in the Contexts area is illustrated. When adding a new Context, you are given the full Policies list in the Create window, but the filter combo shows an incomplete subset. [1:02] Lastly, when attempting to create additional policies for the same Context, an error occurs due to an incorrect validation for a unique context key.
  3. [1:32] When deleting a row, see how the count is not updated in the lower right grid status area.
  4. [1:50] Next, when filtering, a choice in one combo should update the others to show only entries relevant to the current state of the query. They do not currently react to changes as one would expect.
grid-filt-grouping-current.mov

Now, note how each problem is fixed (action taken in same order as above):

grid-filt-grouping-fixed.mov

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Jan 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2023

Codecov Report

Base: 17.95% // Head: 17.89% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (034a793) compared to base (32aba73).
Patch coverage: 4.62% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16355      +/-   ##
============================================
- Coverage     17.95%   17.89%   -0.06%     
- Complexity    10442    10475      +33     
============================================
  Files           561      561              
  Lines         39051    39203     +152     
============================================
+ Hits           7010     7014       +4     
- Misses        32041    32189     +148     
Impacted Files Coverage Δ
...tion/Processors/Security/Access/Policy/GetList.php 0.00% <0.00%> (ø)
...urity/Access/UserGroup/AccessNamespace/GetList.php 0.00% <0.00%> (ø)
...ors/Security/Access/UserGroup/Category/GetList.php 0.00% <ø> (ø)
...sors/Security/Access/UserGroup/Context/GetList.php 0.00% <0.00%> (ø)
...ecurity/Access/UserGroup/ResourceGroup/GetList.php 0.00% <0.00%> (ø)
...ssors/Security/Access/UserGroup/Source/GetList.php 0.00% <0.00%> (ø)
...tion/Processors/Security/ResourceGroup/GetList.php 0.00% <0.00%> (ø)
core/src/Revolution/Processors/Source/GetList.php 0.00% <0.00%> (ø)
.../Processors/Workspace/PackageNamespace/GetList.php 0.00% <0.00%> (ø)
core/src/Revolution/Processors/Context/GetList.php 57.14% <12.50%> (-17.86%) ⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@smg6511 smg6511 added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Jan 18, 2023
@smg6511 smg6511 modified the milestones: v3.1.0, v3.0.4 Jan 18, 2023
@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 20, 2023

@Mark-H @JoshuaLuckers - I see you're active on GH right now ... any chance you guys can give this a quick peek and (re)give it your blessing?

@Mark-H
Copy link
Collaborator

Mark-H commented Jan 20, 2023

Is there an urgency I'm missing that warrant 2 pings for a review on a PR created 2 days ago? ;)

It's a big one so it'll take up a bit more time to check it again compared to giving a quick thumbs up on something small. Just trying to keep up with notifications in between actual work. I haven't done a full review round since before the 3.0.3 release so I'm not ignoring you, simply just haven't sat down for something of this magnitude.

@Mark-H Mark-H added pr/review-needed Pull request requires review and testing. and removed pr/ready-for-merging Pull request reviewed and tested and ready for merging. labels Jan 20, 2023
@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 20, 2023

I'm only hounding you guys (sorry) because this work has already been reviewed and approved — it's just been repackaged because of the rebasing snags I hit in the original. So my thought is that it should be quick and easy.

@smg6511 smg6511 modified the milestones: v3.0.4, v3.1.0 Jan 23, 2023
@Mark-H
Copy link
Collaborator

Mark-H commented Jan 24, 2023

I'm only hounding you guys (sorry) because this work has already been reviewed and approved — it's just been repackaged because of the rebasing snags I hit in the original. So my thought is that it should be quick and easy.

That'd be my excuse if I want to sneak in something nasty in a PR with 2k changed lines, too! :D

(Just kidding, but I'm still gonna go through it before giving you my thumbs.)

@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 24, 2023

@Mark-H - Yes, and I'd assume that — gotta make sure I didn't drop something along the way. Just saying it should be easier to look at your second time around this block ;-)

@Mark-H Mark-H added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Jan 24, 2023
@Mark-H Mark-H changed the title [Replacement] Fix and Enhance Usergroup ACL Permissions Grids and Filtering [Requires build] Fix and Enhance Usergroup ACL Permissions Grids and Filtering Jan 24, 2023
@opengeek opengeek changed the title [Requires build] Fix and Enhance Usergroup ACL Permissions Grids and Filtering Fix and Enhance Usergroup ACL Permissions Grids and Filtering Jan 26, 2023
@opengeek opengeek added the requires build Grunt build is required for integration label Jan 26, 2023
After difficulties rebasing and resolving conflicts of the original PR in preparation for final merge, elected to create this clean re-submission that resolves all issues.
@opengeek opengeek force-pushed the 3.x-improve-usergroup-permissions-grids-filters-resubmit branch from 034a793 to f3860a2 Compare January 26, 2023 15:55
@opengeek opengeek merged commit 2247fe1 into modxcms:3.x Jan 26, 2023
@opengeek
Copy link
Member

@smg6511 — just to confirm, this doesn't need to be ported back into 3.0.x, correct?

opengeek added a commit that referenced this pull request Jan 26, 2023
somehow missed pushing this before merging
@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 26, 2023

@smg6511 — just to confirm, this doesn't need to be ported back into 3.0.x, correct?

Correct, this can wait for the feature release.

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. pr/ready-for-merging Pull request reviewed and tested and ready for merging. requires build Grunt build is required for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access Policy Context not selectable
5 participants