revolution icon indicating copy to clipboard operation
revolution copied to clipboard

[3.x] Grid filtering via URL parameters -- Usergroup ACL

Open smg6511 opened this issue 3 years ago • 1 comments

What does it do?

This is a global application of the "remembering filter properties" work that @Ruslan-Aleev has been working on. It offers two new methods in the base grid class as well as two new utility methods to handle the the synchronization of grid filtering properties between the grid store and the URL; the feature additionally tracks the main and nested tabs via a listener on the base tabs class.

A couple other small changes to note:

  1. A fix was applied to the GetAreas class so that it outputs a unique, alpha ordered (case insensitive) list with the correct settings counts. The "None" label now shows up as well (previously the entry for an unspecified area was checked for via null instead of empty, which is why only the count currently shows).
  2. The settings grid is now not locked into 'core' being preselected. Now the initial view shows every setting for which the user has permission to see. Because of this, a user can also simply select an area (without regard for the namespace) an get all settings within that area. (The back end query is untouched, it's just a front end tweak that allows this.)

Why is it needed?

@Ruslan-Aleev requested my help a while back to make his feature more globally-applicable and to also bring in the ability to remember the correct tab. Utilizing class inheritance, this approach also will cut down on a lot of code for the filterable grids overall as, aside from a couple special cases where one filter depends on the value of another (such as grids with namespace and area filters), all filter application and clearing methods within individual grids can be removed.

How to test

The current set of changes covers the ACL Usergroup editing area (which is the most complex, having the nested vertical tabs in the Access Permissions tab).

  1. Play with filtering the grids in each area (there are a total of 7 in this section).
  2. Also, copy and paste a few of the generated URLs into a new window to verify that you arrive at the correct grid with filtering applied.
  3. Note that the Settings grid now functions properly when typing into the dropdown filters and tabbing through rather than explicitly making selections via the trigger.
  4. To avoid collisions and incompatible filtering parameters, each time a tab is changed the grid filtering from the previous tab is cleared. Verify that this works as expected.

Related issue(s)/PR(s)

Relates to all of the "remembering filter properties" PRs.

smg6511 avatar Mar 11 '22 21:03 smg6511

Sorry guys; one thing I noticed after opening this was that commit from @Ruslan-Aleev is in here (re template TVs). I'd initially pulled down #16063 as a starting point. I didn't mean for that to be a part of what I was submitting, so just cherry pick the commits I made (4 at this point).

@theboxer @Jako @Ibochkarev - Since you guys have been commenting on Ruslan's work on this feature, I added you all to the review list to get this on your radar.

smg6511 avatar Mar 11 '22 21:03 smg6511

Thanks for reviewing this @Ruslan-Aleev! I actually have almost all of the grids done and should be able to post a follow-up PR not too long after this one gets implemented.

smg6511 avatar Oct 17 '22 14:10 smg6511

@JoshuaLuckers @theboxer @Jako @Ibochkarev - Anyone willing to give this a look? Need one more reviewer to move this along ;-)

smg6511 avatar Nov 03 '22 16:11 smg6511

Codecov Report

Base: 16.91% // Head: 17.84% // Increases project coverage by +0.92% :tada:

Coverage data is based on head (f3751fc) compared to base (680df53). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16089      +/-   ##
============================================
+ Coverage     16.91%   17.84%   +0.92%     
+ Complexity    10447    10440       -7     
============================================
  Files           561      561              
  Lines         38028    39049    +1021     
============================================
+ Hits           6433     6967     +534     
- Misses        31595    32082     +487     
Impacted Files Coverage Δ
...rocessors/Element/Template/TemplateVar/GetList.php 0.00% <0.00%> (ø)
...Revolution/Processors/System/Settings/GetAreas.php 0.00% <0.00%> (ø)
core/src/Revolution/modDocument.php 50.00% <0.00%> (-12.50%) :arrow_down:
core/src/Revolution/Processors/Element/Get.php 50.00% <0.00%> (-5.00%) :arrow_down:
core/src/Revolution/Processors/Context/GetList.php 75.00% <0.00%> (-5.00%) :arrow_down:
core/src/Revolution/Processors/Element/GetList.php 78.57% <0.00%> (-4.77%) :arrow_down:
core/src/Revolution/Processors/Element/Update.php 63.04% <0.00%> (-4.40%) :arrow_down:
...olution/Processors/Element/PropertySet/GetList.php 48.78% <0.00%> (-4.00%) :arrow_down:
...ssors/Security/User/GetRecentlyEditedResources.php 27.14% <0.00%> (-3.51%) :arrow_down:
...e/src/Revolution/Processors/System/ConfigCheck.php 76.22% <0.00%> (-2.67%) :arrow_down:
... and 109 more

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

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

codecov-commenter avatar Dec 24 '22 05:12 codecov-commenter

Seems to work like expected, but I would love to see it remember the selected "vtab" as well. Now it only works if you select a filter

JoshuaLuckers avatar Dec 26 '22 12:12 JoshuaLuckers

Seems to work like expected, but I would love to see it remember the selected "vtab" as well. Now it only works if you select a filter

That could be done, but I should probably circle back around to that after getting this iteration of the change merged. Aside from the Resource TVs panel, I believe there are only two places where vertical tabs are used (ACLs and during package installation), and the ACLs area is the only place where "bookmarking" the vtab could be useful.

smg6511 avatar Dec 26 '22 16:12 smg6511

Wait, was this an alternative to all of the PRs Ruslan has scheduled for 3.1.0? @smg6511

opengeek avatar Jan 23 '23 16:01 opengeek

@opengeek - Yes, that's correct. As mentioned in the description above, Ruslan asked if I could create a more global application of this feature. This PR doesn't cover all of the grids, but I have follow up work covering most of the grids (which I'll submit in small batches [or single ones if you like]) that's been waiting for this initial PR's merging.

smg6511 avatar Jan 23 '23 16:01 smg6511

This should have been on the 3.1.0 milestone. Ugh.

opengeek avatar Jan 23 '23 16:01 opengeek