Grid filtering refactor for improved accuracy and persistence via URL parameters #16369
Conversation
smg6511
added
pr/review-needed
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. |
|
@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. |
|
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. |
|
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. |
|
@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)? |
|
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. |
smg6511
changed the title
|
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. |
|
@Mark-H - Couple questions:
|
|
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
added
the
in-progress
|
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? |
|
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
force-pushed
the
3.x-global-filter-persistence--acl-usergroup
branch
from
3578dfc to
dad52e3
Compare
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
|
@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. |
There was a problem hiding this comment.
@smg6511 Impressive work on this one, here are my notes from testing:
?a=workspaces
- switching to
Providerstab 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
- Contexts
- 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
- After adding/editing/deleting new context setting, an error shows up in console:
|
@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 I hope I'll get to this this week, sorry for the delay |
…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 ...
|
Thank you for your hard work on this @smg6511 - took a while to get it done, but we got there, eventually. Appreciate your persistence! |
|
GitHub isn't auto closing because of the way I did the merge (resolving conflicts), but this has been done in 73ae298 |
…rids in the manager (modxcms#16369)" This reverts commit 73ae298, reversing changes made to 7a54b6e.
|
I goofed up merging this so had to revert and apply it again - sorry for the confusion. |
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) ;-) |
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
How to test
Related issue(s)/PR(s)
Replaces the closed PRs: #16081, #16084, #16063, #15946, #15942, and #15935