revolution icon indicating copy to clipboard operation
revolution copied to clipboard

Have the template picker respect new resource action permissions

Open muzzwood opened this issue 2 years ago • 1 comments

What does it do?

Adds an optional restrict_actions parameter to the System/Derivatives/GetList processor. Allowed values are new,edit or delete. This allows for checking newly introduced resource action permissions.

Why is it needed?

The resource type dropdown in the template picker was not respecting these new permissions.

How to test

Create a user that does not have one or more of these permissions: new_weblink, new_symlink or new_static_resource Then open the template picker and check the resource type list.

Related issue(s)/PR(s)

#15893

muzzwood avatar Feb 20 '22 10:02 muzzwood

@muzzwood Please make the suggested changes.

Ibochkarev avatar Aug 10 '22 04:08 Ibochkarev

@muzzwood — any chance you could take a stab at rebasing this on top of the latest 3.x branch? I attempted to but it was not obvious to me how to. If you have any questions or issues on how to rebase, please don't hesitate to ask.

opengeek avatar Jan 31 '23 19:01 opengeek

@opengeek Sure, I'll try and look at that tomorrow 👍🏻

muzzwood avatar Feb 02 '23 06:02 muzzwood

Codecov Report

Base: 17.90% // Head: 17.89% // Decreases project coverage by -0.02% :warning:

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

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16067      +/-   ##
============================================
- Coverage     17.90%   17.89%   -0.02%     
- Complexity    10473    10484      +11     
============================================
  Files           561      561              
  Lines         39221    39247      +26     
============================================
  Hits           7024     7024              
- Misses        32197    32223      +26     
Impacted Files Coverage Δ
...volution/Processors/System/Derivatives/GetList.php 0.00% <0.00%> (ø)
core/src/Revolution/Sources/modMediaSource.php 41.63% <0.00%> (-0.04%) :arrow_down:

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 Feb 07 '23 02:02 codecov-commenter

Rebased to current 3.x, albeit a few days later than promised. 🙈

muzzwood avatar Feb 07 '23 02:02 muzzwood

I'm beginning to take a look at this and note that, to fully address the issue, there are two other places where the derivatives' visibility needs to be controlled:

  • The combo under Resource > Settings
  • The Resource tree's context menu; I see you may already be on to this, since your proposed new constant and method take edit and delete perms into account.

smg6511 avatar Feb 25 '23 04:02 smg6511

~~There's one critical issue I just detected that may demand this be done a little differently: Only MODX-native derivatives show in the window combo. Third-party ones, such as Collections, do not show.~~ Update on the above: It turns out this bug is not caused by this PR's changes, it's another issue in the same js file (in the picker's listeners).

It also found that this change causes an error when editing a Resource (perhaps this only happens when non-native derivatives are present, I'm not sure), because the combo definition, at the least, needs to be updated in the Resource Settings (as mentioned above).

Sorry to be the bearer of bad news :-( You probably don't need it, but I can lend a hand if you want.

When tackling this again, be aware that a recently-merged change to the template picker window will require you to rebase again. When you do, you'll want to just accept the current change and re-add your baseParams config in the correct place (up near the top of the window definition).

smg6511 avatar Feb 25 '23 05:02 smg6511

@muzzwood - Hey, not meaning to step on your toes here with my alternate PR I just submitted. It's just that I found the way to do what you wanted was totally different than how you initially approached it and seemed simpler to just submit that solution rather than relay it to this PR somehow.

smg6511 avatar Feb 27 '23 04:02 smg6511

Hi @smg6511 , No problem at all. If it's part of a bigger issue, I'm happy for you to take care of it. Closing! :)

muzzwood avatar Feb 27 '23 04:02 muzzwood

Ok, great ;-) If you have a few moments in the near future, give it a review to verify all's working. Cheers!

smg6511 avatar Feb 27 '23 04:02 smg6511