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.

Sign up for GitHub

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

Fix: the trash post action doesn't take into account user capabilities. #62589

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Fix: the trash post action doesn't take into account user capabilities. #62589

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d3b8e0
Fix: User capacities are not taken into account in the trash post act…
jorgefilipecosta Jun 14, 2024
93a35ee
Minimize diff
ellatrix Jun 20, 2024
df4207a
Add inline comment.
jorgefilipecosta Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions packages/editor/src/components/post-actions/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import { external, trash, backup } from '@wordpress/icons';
import { addQueryArgs } from '@wordpress/url';
import { useDispatch, useSelect } from '@wordpress/data';
import { useDispatch, useSelect, useRegistry } from '@wordpress/data';
import { decodeEntities } from '@wordpress/html-entities';
import { store as coreStore } from '@wordpress/core-data';
import { __, _n, sprintf, _x } from '@wordpress/i18n';
Expand Down Expand Up @@ -307,6 +307,37 @@ const trashPostAction = {
},
};

function useTrashPostAction( postType ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like where this is going. I understand why this was done but I like to think actions should be global objects that don't require a hook to define them. This is going in the opposite direction of all my extensibility PRs.

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @youknowriad, this seems important to address on WP 6.6, we will be showing the trash button on the post editor on posts a user has no permission to trash, which is a considerable regression. As of right now there is no reliable way to fix the issue without using a hook, as soon as our APIs allow it and we can fix the issue without using a hook I'm happy to iterate and remove the hook here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed 6.6 is close, so I'm totally fine with this fix for 6.6 but we should definitely look at better APIs for actions after that.

const registry = useRegistry();
const { resource, cachedCanUserResolvers } = useSelect(
( select ) => {
const { getPostType, getCachedResolvers } = select( coreStore );
return {
resource: getPostType( postType )?.rest_base || '',
cachedCanUserResolvers: getCachedResolvers().canUser,
};
},
[ postType ]
);
return useMemo(
() => ( {
...trashPostAction,
isEligible( item ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that the isEligible call here won't be re-called when the value of canUser( 'delete', resource, item.id ) changes no?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe we need something like useEligible somewhere. Or maybe this object should define which capabilities to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related discussion #62505 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

It will because we make the memo depend on canUserResolvers (details in #62589 (comment)) so when the resolver finishes a rerender happens and isEligible will be recalled.

Copy link
Member

Choose a reason for hiding this comment

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

🤨 seems fine for now but not ideal

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very hard to follow. I'm not sure I understand what canUserResolvers is?

Copy link
Member Author

Choose a reason for hiding this comment

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

canUserResolvers is the result getCachedResolvers().canUser. So basically when there is a change in the caching of canUser resolvers we will rerender making isEligible recalled. I will add some inline comments, explaining what we discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also why keeping the action as object just to be used inside the hook action? Anyway since it's for 6.6, but it becomes clear that an action needs a way to have access to the store in more places than the callback and we should work towards that direction.

Copy link
Member

Choose a reason for hiding this comment

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

@ntsekouras I did that to minimise the diff, it was huge and unclear what changed + less likely to have cherry pick conflicts, and if it does, easier to resolve manually (I don't know what has changed in these files recently)

Copy link
Member

Choose a reason for hiding this comment

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

And it's useful for reusability #62754 (comment)

return (
trashPostAction.isEligible( item ) &&
registry
.select( coreStore )
.canUser( 'delete', resource, item.id )
);
},
} ),
// We are making this use memo depend on cachedCanUserResolvers as a way to make the component using this hook re-render
// when user capabilities are resolved. This makes sure the isEligible function is re-evaluated.
// eslint-disable-next-line react-hooks/exhaustive-deps
[ registry, resource, cachedCanUserResolvers ]
);
}

const permanentlyDeletePostAction = {
id: 'permanently-delete',
label: __( 'Permanently delete' ),
Expand Down Expand Up @@ -1020,6 +1051,7 @@ export function usePostActions( { postType, onActionPerformed, context } ) {
);

const duplicatePostAction = useDuplicatePostAction( postType );
const trashPostActionForPostType = useTrashPostAction( postType );
const isTemplateOrTemplatePart = [
TEMPLATE_POST_TYPE,
TEMPLATE_PART_POST_TYPE,
Expand Down Expand Up @@ -1048,7 +1080,7 @@ export function usePostActions( { postType, onActionPerformed, context } ) {
isTemplateOrTemplatePart ? resetTemplateAction : restorePostAction,
isTemplateOrTemplatePart || isPattern
? deletePostAction
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need the same for deletePostAction?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need the same for some other actions, my plan was to define the fix here and after we agree on a fix I will replicate it to the other cases where it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Is permanently delete a separate capability? I'm still seeing this:

image

Copy link
Member

Choose a reason for hiding this comment

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

I tested with the testing instructions, and it seems to be the same capability: I can also not delete it permanently in trunk.

: trashPostAction,
: trashPostActionForPostType,
! isTemplateOrTemplatePart && permanentlyDeletePostAction,
...defaultActions,
].filter( Boolean );
Expand Down Expand Up @@ -1113,6 +1145,7 @@ export function usePostActions( { postType, onActionPerformed, context } ) {
isPattern,
postTypeObject?.viewable,
duplicatePostAction,
trashPostActionForPostType,
onActionPerformed,
isLoaded,
supportsRevisions,
Expand Down
Loading