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

Grid filtering refactor for improved accuracy and persistence via URL parameters #16369

Merged

Conversation

smg6511
Copy link
Collaborator

@smg6511 smg6511 commented Jan 30, 2023

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

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Jan 30, 2023
@smg6511 smg6511 added pr/review-needed Pull request requires review and testing. requires build Grunt build is required for integration labels Jan 30, 2023
@smg6511 smg6511 added this to the v3.1.0 milestone Jan 30, 2023
@Mark-H
Copy link
Collaborator

Mark-H commented Jan 30, 2023

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
Copy link
Collaborator Author

smg6511 commented Jan 30, 2023

@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
Copy link
Member

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
Copy link
Collaborator

Mark-H commented Jan 30, 2023

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
Copy link
Collaborator Author

smg6511 commented Jan 30, 2023

@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
Copy link
Collaborator

Mark-H commented Jan 30, 2023

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
Copy link

codecov-commenter commented Jan 30, 2023

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.

@smg6511 smg6511 changed the title Grid filtering via URL parameters -- Policies, Policy Templates Grid filtering via URL parameters -- Batch 1 Jan 30, 2023
@JoshuaLuckers
Copy link
Contributor

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

@Mark-H
Copy link
Collaborator

Mark-H commented Jan 31, 2023

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
Copy link
Collaborator Author

smg6511 commented Jan 31, 2023

@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
Copy link
Member

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?

@opengeek opengeek added the in-progress PRs that are in progress by the author or contributors label Jan 31, 2023
@smg6511
Copy link
Collaborator Author

smg6511 commented Jan 31, 2023

@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
Copy link
Collaborator

Mark-H commented Jan 31, 2023

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
Copy link
Collaborator Author

smg6511 commented Jan 31, 2023

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 smg6511 force-pushed the 3.x-global-filter-persistence--acl-usergroup branch from 3578dfc to dad52e3 Compare February 1, 2023 02:48
@modxcms modxcms deleted a comment from mishanthrop Feb 7, 2023
Fixes and tweaks to address PR review issues
Fixes to address PR review issue
Fixes to address PR review issues
Fixes to address PR review issues
@smg6511
Copy link
Collaborator Author

smg6511 commented Sep 13, 2023

@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 ;-)

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator Author

smg6511 commented Oct 17, 2023

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

Adds new component id for Package grid that matches the pattern of all others ('modx-grid-[x]')
Minor changes made to the category filter combo to make it correctly display category names when persisted via state/URL
Additional logic to fix issues raised in PR review
@smg6511
Copy link
Collaborator Author

smg6511 commented Oct 30, 2023

@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
Copy link
Member

theboxer commented Nov 6, 2023

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

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good now, thanks!

@modxcms modxcms deleted a comment from cla-bot bot Feb 10, 2024
@modxcms modxcms deleted a comment from cla-bot bot Feb 10, 2024
@modxcms modxcms deleted a comment from cla-bot bot Feb 10, 2024
@modxcms modxcms deleted a comment from cla-bot bot Feb 10, 2024
@modxcms modxcms deleted a comment from cla-bot bot Feb 10, 2024
Mark-H added a commit that referenced this pull request Feb 10, 2024
…the manager (#16369)

Merge remote-tracking branch 'upstream/pr/16369' into 3.x

* upstream/pr/16369: (33 commits)
  Remove Ext Statefulness from TVs Panel
  Update UserGroup Namespaces Access Grid
  Filter Persistence: Lexicons Grid
  Filter Persistence: System Events Grid
  Filter Persistence: Settings Grids (4)
  Filter Persistence: User Messages Grid
  Filter Persistence: TV Templates Grid
  Filter Persistence: Template TVs Grid
  Filter Persistence: Groups Panel and Users Grid
  Filter Persistence: Users Grid
  Update related to commit c7fd92b
  Update related to commit 0e0778d
  Filter Persistence: Media Sources Grid
  Filter Persistence: Packages Grid
  Filter Persistence: Dashboard Widgets Grid
  Filter Persistence: Dashboards Grid
  Filter Persistence: Form Customization Sets Grid
  Filter Persistence: Form Customization Profiles Grid
  Filter Persistence: Contexts Grid
  Filter Persistence: Namespaces Grid
  ...
@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

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
Copy link
Collaborator

Mark-H commented Feb 10, 2024

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

@Mark-H Mark-H closed this Feb 10, 2024
@Mark-H Mark-H reopened this Feb 10, 2024
@Mark-H Mark-H closed this Feb 10, 2024
@Mark-H Mark-H reopened this Feb 10, 2024
Mark-H added a commit to Mark-H/revolution that referenced this pull request Feb 10, 2024
…rids in the manager (modxcms#16369)"

This reverts commit 73ae298, reversing
changes made to 7a54b6e.
@Mark-H Mark-H merged commit 5162df4 into modxcms:3.x Feb 10, 2024
10 of 11 checks passed
@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

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

@smg6511
Copy link
Collaborator Author

smg6511 commented Feb 10, 2024

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) ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/review-needed Pull request requires review and testing. requires build Grunt build is required for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants