Grid filtering refactor for improved accuracy and persistence via URL parameters

[smg6511]

What does it do?

Built upon work merged in #16089 and #16355 to provide better filtering functionality and persistence for the majority of grids. Additional methods were added to the Tabs and Grids base classes to further optimize the code (e.g., centralizing the repetitive task of creating the query and clear buttons, which are for the most part exactly the same everywhere they are applied).

Why is it needed?

To apply the new global persistence feature to the referenced grids and improve their UIs.

Grids covered

1. Media Sources - Media > Media Sources (/manager/?a=source)
2. Packages - Extras > Installer (/manager/?a=workspaces)
3. Users - Manage > Users (/manager/?a=security/user)
4. User Settings - Manage > Users > [User] Settings (manager/?a=security/user/update&id=1)
5. Messages - User (user icon) > Messages (/manager/?a=security/message)
6. System Settings - Settings (gear) > System Settings (/manager/?a=system/settings)
7. System Events - Settings (gear) > System Settings, 2nd tab (/manager/?a=system/settings)
8. Form Customization Profiles - Settings (gear) > Form Customization (/manager/?a=security/forms)
9. Profile - Settings (gear) > Form Customization > [Profile] (manager/?a=security/forms/profile/update&id=1)
10. Dashboards - Settings (gear) > Dashboards (/manager/?a=system/dashboards)
11. Widgets - Settings (gear) > Dashboards, 2nd tab (/manager/?a=system/dashboards)
12. Contexts - Settings (gear) > Contexts (/manager/?a=context)
13. Context Settings - Settings (gear) > Contexts > [Context], 2nd tab (/manager/?a=context/update&key=[yourcontext])
14. User Groups & Users - Settings (gear) > Access Control Lists [click UG in left tree] (/manager/?a=security/permission&group=1)
15. Access Policies - Settings (gear) > Access Control Lists, 3rd tab (/manager/?a=security/permission)
16. Policy Templates - Settings (gear) > Access Control Lists, 4th tab (/manager/?a=security/permission)
17. Access Permissions/Contexts - Settings (gear) > Access Control Lists > [User Group], 2nd tab (manager/?a=security/usergroup/update&id=1)
18. Access Permissions/Resource Groups - (same path as # 17, 2nd vertical tab)
19. Access Permissions/Element Categories - (same path as # 17, 3rd vertical tab)
20. Access Permissions/Media Sources - (same path as # 17, 4th vertical tab)
21. Access Permissions/Namespaces - (same path as # 17, 5th vertical tab)
22. User Group/Users - Settings (gear) > Access Control Lists > [User Group], 3rd tab (manager/?a=security/usergroup/update&id=1)
23. User Group/Settings - Settings (gear) > Access Control Lists > [User Group], 4th tab (manager/?a=security/usergroup/update&id=1)
24. Lexicon Management - Settings (gear) > Lexicons (/manager/?a=workspaces/lexicon)
25. Namespaces - Settings (gear) > Namespaces (/manager/?a=workspaces/namespace)
26. Trash Bin - [Trash icon in Resources/Elements/Categories tab panel] (/manager/?a=resource/trash)
27. Template/Template Variables - Elements Tab > [Template], 2nd tab (/manager/?a=element/template/update&id=1)
28. Template Variables/Template Access - Elements Tab > [Template Variable], 4th tab (/manager/?a=element/tv/update&id=1)
29. Plugins/System Events - Elements Tab > [Plugin], 2nd tab (/manager/?a=element/plugin/update&id=1)

How to test

1. Ensure all caches a cleared.
2. Visit all grids with filtering capabilities and perform various search queries and clear them, verifying expected behavior.
3. For each grid, perform a search and copy the URL in your browser. Paste that URL in a new window or browser tab to verify navigation to the correct tab, and that the search query is intact.

Related issue(s)/PR(s)

Replaces the closed PRs: #16081, #16084, #16063, #15946, #15942, and #15935

[Mark-H]

Many other grids have these enhancements ready to submit, but rely on the base class changes committed in this PR. As soon as this is tested and merged, I'll put up additional PRs to cover other grids.

If the changes to the other grids are the same as the updated grids already here (i.e. like 20 lines of changes (ignoring whitespace) that only re-define the tbar), feel free to add those onto this PR so it can all be tested at once. I've developed quite the distaste for this concept of dependent PRs and it's not thousands of lines.

[smg6511]

@Mark-H - Let me ask you this: I have several other changed grids updated to use this feature (remember that this global handling will ultimately apply to all grids that have filtering). In all, excluding the files changed in this PR, I have ~45 changed files (involving roughly 13 grids) in my local work in progress; most of them don't involve big changes, but it's still a lot of grids to test. How many would you want submitted in this PR? I'm happy to add as much as you think can practically be tested at once.

[opengeek]

I agree with @Mark-H on including all of these changes related to the "base class changes" introduced here in one PR. Having PRs withheld waiting on a base change, unless the base change is being submitted separately and can be evaluated in isolation from changes expected to be made on top of it, creates more complication than reviewing a larger-sized PR.

[Mark-H]

There is no one-size-fits all. There is no number of lines or number of files that makes one PR harder to review.

There is a mental load to keep in mind. 13 small PRs that are all essentially the same except it affects a different file, are IMO harder to review and test than one PR that changes all the grids, not in the least because you're then in need of 26 approvals vs just 2, but also because every PR requires rebuilding the mental model of what is happening. Context switching is expensive, so as long as it's all the same context, combining makes sense.

Testing also. I'm definitely not going to spin up a test environment 13 times for the individual changes because the time to set that up will exceed the time it takes to test it. But I will spin up the test environment for a single bigger PR and take the time to test each grid.

It becomes a different story when we're starting to make a wide range of changes with different goals. Then it makes sense to break it up so one pr = one goal.

So, as long as your per-grid changes (e.g. manager/assets/modext/widgets/security/modx.grid.access.policy.js) are all very similar and do not involve individual refactors (excluding whitespace) exceeding a couple dozen lines, just dump 'm all in, we'll review all of it once, and get this whole multi-month saga over with already.

[smg6511]

@opengeek @Mark-H - Ok, I'm totally down with that. I think I still question how to go about this because of mixed signals earlier on (not necessarily from you guys) with a couple of my larger PRs. I'll spin up the rest of what I've got ready and add it in here. Is it at all useful to you for me to make multiple commits organized by grid (having 2 or 3 files at the most as tweaks to each grid's processor were made)?

[Mark-H]

Yes, atomic commits (where one commit = one finished change for one specific item) are always a good idea. In the end it'll all get squashed per PR but if there is additional context to be had, great.

[codecov-commenter]

Codecov Report

Attention: 328 lines in your changes are missing coverage. Please review.

Comparison is base (8feb62f) 21.73% compared to head (334d553) 21.58%.
Report is 57 commits behind head on 3.x.

Files	Patch %	Lines
core/src/Revolution/modLexicon.php	25.00%	78 Missing ⚠️
.../Processors/Workspace/PackageNamespace/GetList.php	0.00%	68 Missing ⚠️
...Revolution/Processors/System/Settings/GetAreas.php	0.00%	50 Missing ⚠️
...rocessors/Element/Template/TemplateVar/GetList.php	0.00%	38 Missing ⚠️
core/src/Revolution/Processors/Context/GetList.php	9.37%	29 Missing ⚠️
...evolution/Processors/Workspace/Lexicon/GetList.php	0.00%	12 Missing ⚠️
...rocessors/Element/TemplateVar/Template/GetList.php	0.00%	9 Missing ⚠️
...volution/Processors/Security/Forms/Set/GetList.php	0.00%	6 Missing ⚠️
...tion/Processors/Security/Forms/Profile/GetList.php	0.00%	5 Missing ⚠️
...olution/Processors/Security/Group/User/GetList.php	0.00%	5 Missing ⚠️
... and 9 more

Additional details and impacted files

@@             Coverage Diff              @@
##                3.x   #16369      +/-   ##
============================================
- Coverage     21.73%   21.58%   -0.16%     
- Complexity    10483    10564      +81     
============================================
  Files           561      561              
  Lines         31620    31926     +306     
============================================
+ Hits           6873     6890      +17     
- Misses        24747    25036     +289     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JoshuaLuckers]

@opengeek @Mark-H I think the release with these changes should be released as a beta first.

[Mark-H]

Yeah, always in favor of at least one release candidate for minor releases anyway. :)

I've been keeping track of the commits added so far and am not seeing any issues yet.

[smg6511]

@Mark-H - Couple questions:

1. Roughly how long will it be before we get to a beta/rc for 3.1? I ask because I want to be sure to get in the last two major grids beforehand (lexicons and system settings); those are the most complicated of all of them and I have a little more tweaking to do there before committing them.
2. If I perform a rebase now will that cause any issues for you? I ask because there are two grids with new filtering capability that just recently got merged (after I began putting this PR together) that I'd like to get in as well. See Add filtering for ACL policy and policy template grids #16219.

[opengeek]

We don't do betas for minor releases, and generally don't do RC's for them, either. But if we want to do an RC cycle for minors, that can certainly be accommodated. There are no specific timelines set for this, but I would hope we could get this complete feature set worked out before we release.

You do not need to rebase anything unless there are conflicts or there are JS changes that would affect this functionality. What grids with filtering are you referring to?

[smg6511]

@opengeek - I'm referring to the ones merged in #16219. I suppose I could just copy those two classes as they stand and make changes to them in this branch, avoiding the need for a rebase.

[Mark-H]

Rebasing is a way to deal with conflicts, as-is this branch isn't conflicting yet. However if you start editing the files modified in #16219 again, either making the same or different changes that affect the same lines, then that will cause conflicts and a rebase will become necessary. Does that help decide on how to proceed at this point?

[smg6511]

So it seems to me if I can rebase, I should if I want to get those two files in line with this PR (as one way or another it would become necessary). Does that sound reasonable? (BTW: I'm finding that there is something there to be able to rebase.)

[smg6511]

@JoshuaLuckers - I finally was able to focus on the remaining issue you'd pointed out in your review and all should be fixed and ready to go. Please take a second look when you get a chance.
@Ruslan-Aleev - Since this PR was motivated by the work you had begun and your request for a global solution, you may want to review this as well if you've got the time ;-)

[theboxer]

@smg6511 Impressive work on this one, here are my notes from testing:

?a=workspaces

• switching to Providers tab doesn't clear the query params

?a=security/message

• clear filter causes console error & doesn't clear the search field

?a=system/settings

• search field is shared between system settings & system events

?a=context/update&key=mgr

• After adding/editing/deleting new context setting, an error shows up in console:
  • The filter component with itemId 'filter-namespace' could not be retrieved

?a=security/usergroup/update&id=1

• Access Permissions
  • Contexts
    • Double reload clears the URL params
  • Element Categories
    • Double reload clears the URL params
    • Filter by category doesn't persist correctly (after reload the field is empty)
  • Media Sources
    • Double reload clears the URL params
  • Namespaces
    • Double reload clears the URL params
• Users
  • Double reload clears search field
• Settings
  • After adding/editing/deleting new context setting, an error shows up in console:
    • The filter component with itemId 'filter-namespace' could not be retrieved

[smg6511]

@theboxer - Thanks for looking this over closely. I'll sift through the issues you found and provide fixes later this week...

[smg6511]

@theboxer - Ok, I believe I've resolved all the issues you raised. Please take a look when you get a chance. Also, if you can find the time, it'd be great if you could look at #16469 as well ;-)

[theboxer]

@smg6511 I hope I'll get to this this week, sorry for the delay

[theboxer]

Seems good now, thanks!

[Mark-H]

Thank you for your hard work on this @smg6511 - took a while to get it done, but we got there, eventually. Appreciate your persistence!

[Mark-H]

GitHub isn't auto closing because of the way I did the merge (resolving conflicts), but this has been done in 73ae298

[Mark-H]

I goofed up merging this so had to revert and apply it again - sorry for the confusion.

[smg6511]

Thank you for your hard work on this @smg6511 - took a while to get it done, but we got there, eventually. Appreciate your persistence!

Thanks for getting this in at last! There's more I'd been working on waiting in the wings in terms of grids improvements (but not nearly of this magnitude) ;-)