Editor grids permissions refactor, enhancements, and fixes #16653
Conversation
|
@smg6511 Is this close to being ready? |
|
@opengeek Yes, very close - been hammering away at it. It should be ready for review by early next week. |
3045679 to
7739175
Compare
|
@smg6511 this is a big one—kudos on getting it to this stage! Since it's fresh on your mind and touches so many areas, it will take a while to fully review I suspect (was just discussing this with @jaygilmore and @opengeek). Any suggestions or checklists of all the various nooks and crannies that should be tested to get this one merged? |
Ok, here's generally how I'd suggest going through this: Initial RunthroughI overhauled the base grids classes (in Grids with New "Creator" Column and Protected RecordsThese are the grids where MODX provides built-in records to start with and/or where an Extra installs a record (which usually should only be removed by un-installing the Extra [such as in Namespaces]):
A few notable details:
All Editor Grids
General StrategyAs I was working through this PR, I kept one browser open with an Admin user with full permissions and another with an alternate user with permissions I'd vary to check my work. I basically systematically went through each nav menu item and completed updates to all relevant grids for one area at a time. I'd suggest the same for reviewing. Remember there are some grids that are a couple hops away, and can be easy to miss:
|
49b4b53 to
b3dcd12
Compare
|
@smg6511 — I am trying to get a collaborative review session together for this PR so we can get it integrated sooner than later. If possible, could you resolve the current small conflict in modx.grid.js? |
Formatting, style, optimization only
Changes to js, css, and Lexicon common to multiple areas of this PR
Apply new permissions methods
Formatting, code style changes only
Apply new permissions methods; includes adjustments to base grid class
Formatting, code style changes only
Render links and checkboxes according to user permissions
Remove legacy cls references and mark others for removal
Tweaks and optimizations; fix issue with fields and tvs grids not showing inactive rows properly
Formatting tweak
|
@opengeek - I fixed the conflict but, dagonit, unintentionally ended up merging instead of force pushing because I was trying to fix a minor format error at the same time as resolving the conflict ... grrr. Hopefully this'll be ok. |
Unfortunately, that merge commit is problematic as it now shows completely unrelated changes in the diff. |
|
How to back out of that is the question then :-/ Maybe I can revert the merge? |
|
I think you should be able to go back to 8c6a110 and then rebase the the work on the latest 3.x. I'm not sure exactly the situation, but maybe you could |
Formatting, style, optimization only
Changes to js, css, and Lexicon common to multiple areas of this PR
Formatting, code style & optimization changes only
Formatting, code style & optimization changes only
Updates display of and ability to select row actions (gear icon), as well as display of various action buttons. Also adjustments made to base grid class.
A few new fixes and additions
Tweaks, clean up, and application of new create button method to various grids
A few rule additions and updates...
This reverts commit b5a9743.
1faa3c1 to
9f90ff3
Compare
Just a few additions/updates
|
@opengeek Ok, I think I got it squared away (I hope). Don't know why the compile assets step is choking. |
Fix small type (really just to force the CI to compile assets)
|
This pull request has been mentioned on MODX Community. There might be relevant details there: https://community.modx.com/t/help-wanted-testing-a-pr-for-upcoming-revo-release/8528/1 |
| $c->select([ | ||
| 'name' => 'DISTINCT SUBSTRING_INDEX(`signature`,"-",1)' | ||
| ]); | ||
| $namespaces = $modx->getIterator(modTransportPackage::class, $c); |
There was a problem hiding this comment.
Shouldn't this be fetching from modNamespace instead of modTransportPackage? 🤔
There was a problem hiding this comment.
I don't think so, as the purpose of this method is to just get the Extras-related namespaces (and not others, like custom ones).
There was a problem hiding this comment.
I think @Mark-H is correct. MODX namespaces are namespaces, regardless if they are part of an extra or the core. There's no such thing as an Extras-related namespace based on the signature of a transport package.
There was a problem hiding this comment.
The thing is I'm guessing you're both just looking at the code and not the context of what it's being used for. No, there's not literally an Extra-specific type of namespace, but I have derived that distinction (based on installed TPs) for purposes of identifying and sorting in specific grids. For example, I often create namespaces that are a home for particular types of customization and transformations. The method we're discussing is how I'm able to identify the "Creator" as "User" instead of "Core" or "Extra" (shown in the few grids that have that column).
Am I wrong in thinking that every installed Extra does and must have its own unique namespace? I can't think of any I've installed that don't.
There was a problem hiding this comment.
You are wrong, yes. There is no requirement for an extra to have a unique namespace, nor is there any requirement preventing them from having half-a-dozen namespaces installed from a single transport package. And there is certainly no requirement that the namespace match the transport package (or extra) name.
I honestly have no idea what this Creator as User thing is about or how that is related to MODX namespaces.
There was a problem hiding this comment.
@smg6511 is this aspect of the PR necessary or do you tihnk it could be pulled for a future PR/improvement? It seems like it could be deferred. We really want to get this merged when we can, but if feels like this piece is optional and needs some review.
|
i did some testing as proposed in #15919. i tested with individual permissions the the Roles grid (ACLs/Roles ) |
Related PR
See also #15919. This is a re-packaging of that PR; this new PR seeks to cover all editor grids across the application and separates out some work (for a future PR) that had been begun around internationalizing core names and descriptions within some of the 8000 grids.
Related Issues
#14929, #16507