Post Actions: Pass entity permission to the 'isEligible' method #61644
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
commented
May 14, 2024
| const { getCurrentPostType, getCurrentPost } = select( editorStore ); | ||
| const { getPostType } = select( coreStore ); | ||
| const _postType = getCurrentPostType(); | ||
| const _resource = getPostType( _postType )?.rest_base; |
There was a problem hiding this comment.
Consumers shouldn't have to do this - related #43751.
|
Size Change: +68 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
May 14, 2024
| @@ -43,8 +43,8 @@ const trashPostAction = { | |||
| label: __( 'Move to Trash' ), | |||
| isPrimary: true, | |||
| icon: trash, | |||
| isEligible( { status } ) { | |||
| return status !== 'trash'; | |||
| isEligible( item, permissions ) { | |||
There was a problem hiding this comment.
What about data views, how would data views pass the permissions object?
What alternative APIs can we offer here?
There was a problem hiding this comment.
I think we'll have to do the same for DataViews or any other place where the "Post Actions" are used. Gather permission and pass the object.
There was a problem hiding this comment.
There's another option. Support "thunks" kind of API for isEligible:
isEligible: select => item => select( something )
I'm not entirely sold on any of the two approaches for the moment.
- Your approach adds the constraint of having a new "permissions" prop that need to be passed to the DataViews component, and also suffers from the fact that permissions might not be the same for each item.
- The proposed alternative is too flexible and might leak stuff.
There was a problem hiding this comment.
suffers from the fact that permissions might not be the same for each item.
Do you mean that they should be the same for each item in the data views?
Disclaimer: I've little experience with DataView.
There was a problem hiding this comment.
No, I mean that in some cases they're obviously different, so we'll have to pass the permissions of all items to the data views component. It feels way too heavy.
There was a problem hiding this comment.
@Mamaduka If I'm understanding you're suggesting that the "permissions" object should be part of the "item" object. While for the endpoint API response, that seems logic, I'm not entirely sure about the objects in the state. I feel like these two things are separate concerns and should be in different objects. In the backend you don't have the permissions as part of the Post object for instance.
There was a problem hiding this comment.
That's not what I mean exactly 😅
The _links are already part of the response object, and the editor relies on them. Example:
gutenberg/packages/editor/src/components/post-pending-status/check.js
Lines 16 to 17 in 83faa5a
There was a problem hiding this comment.
yes, I think that's wrong :P. I think we should have removed all these metadata from the objects.
There was a problem hiding this comment.
Obviously, now we can use them if they're already there but would love to actually move away from them.
There was a problem hiding this comment.
Then, back to the idea board.
Another alternative is to introduce entity aware permission selector (#43751) and use it either by passing permission prop or making isEligible a "thunk" like callback.
We could batch resolve this selector for entity lists using the data provided by targetHints.
|
I'm going to close this. @youknowriad, you mentioned a simpler solution in (#61792 (comment)) are planning to work on it? |
youknowriad
reviewed
May 28, 2024
| }; | ||
| }, [] ); | ||
| const allActions = usePostActions( postType, onActionPerformed ); | ||
| const permissions = useResourcePermissions( resource, item.id ); |
There was a problem hiding this comment.
How important is the "item.id" here? My alternative solution would only work if we had the same permissions for all items.
There was a problem hiding this comment.
I think it's important for some roles that can delete only some items for example.
My alternative solution would only work...
Do you have a PR?
I tried today a bit what you suggested about wrapping the action with a permissions check, but couldn't figure out how to handle this in Data Views - that means without checking each item specifically outside of isEligible function.
There was a problem hiding this comment.
I don't have a PR, it would be very similar to this one, just moving this call to with usePostActions.
That said, yeah if we want per item checks, we'd have to follow something like the current PR but it would mean a lot of REST API calls unless we wait for the full core-data based solution and have the isEligible support thunks.
There was a problem hiding this comment.
Specific actions can be restricted only for specific entities, so the item.id check is important for edit/update/delete actions.
|
I'll close this PR. Planning to continue work #43751 and the |
This was referenced Jul 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
🚧 PoC for introducing entity permission to the "Post Actions"
Why?
How?
Testing Instructions
delete_postscapability -wp cap remove 'contributor' 'delete_posts'Testing Instructions for Keyboard
Same.
Screenshots or screencast