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

Jump to bottom

Core Data: Support entities in the 'canUser' selector #63322

Merged
merged 10 commits into from
Jul 11, 2024
Merged

Core Data: Support entities in the 'canUser' selector #63322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4ea1c4f
Core Data: Support entities in the 'canUser' selector
Mamaduka Jul 9, 2024
2cf879f
Cleanup 'canUser' unit tests
Mamaduka Jul 10, 2024
2230256
Add unit tests
Mamaduka Jul 10, 2024
b99da12
Replace 'canUserEditEntityRecord' selector usages
Mamaduka Jul 10, 2024
b6aeb12
Make 'canUserEditEntityRecord' forwarded selector/resolver
Mamaduka Jul 10, 2024
4e185e1
Add checks when resource is an entity
Mamaduka Jul 10, 2024
92b2859
Return false when entity arg is malformed
Mamaduka Jul 10, 2024
c637582
Update types and JSDoc
Mamaduka Jul 10, 2024
5ff9407
Throw an error when an entity resource object is malformed
Mamaduka Jul 10, 2024
df1dbd2
Deprecate canUserEditEntityRecord
Mamaduka Jul 10, 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
2 changes: 1 addition & 1 deletion docs/reference-guides/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ _Parameters_

- _state_ `State`: Data state.
- _action_ `string`: Action to check. One of: 'create', 'read', 'update', 'delete'.
- _resource_ `string`: REST resource to check, e.g. 'media' or 'posts'.
- _resource_ `string | EntityResource`: Entity resource to check. Accepts entity object `{ kind: 'root', name: 'media', id: 1 }` or REST base as a string - `media`.
- _id_ `EntityRecordKey`: Optional ID of the rest resource to check.

_Returns_
Expand Down
10 changes: 5 additions & 5 deletions packages/block-library/src/post-title/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ export default function PostTitleEdit( {
if ( isDescendentOfQueryLoop ) {
return false;
}
return select( coreStore ).canUserEditEntityRecord(
'postType',
postType,
postId
);
return select( coreStore ).canUser( 'update', {
kind: 'postType',
name: postType,
id: postId,
} );
},
[ isDescendentOfQueryLoop, postType, postId ]
);
Expand Down
6 changes: 5 additions & 1 deletion packages/block-library/src/utils/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import { useViewportMatch } from '@wordpress/compose';
export function useCanEditEntity( kind, name, recordId ) {
return useSelect(
( select ) =>
select( coreStore ).canUserEditEntityRecord( kind, name, recordId ),
select( coreStore ).canUser( 'update', {
kind,
name,
id: recordId,
} ),
[ kind, name, recordId ]
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ _Parameters_

- _state_ `State`: Data state.
- _action_ `string`: Action to check. One of: 'create', 'read', 'update', 'delete'.
- _resource_ `string`: REST resource to check, e.g. 'media' or 'posts'.
- _resource_ `string | EntityResource`: Entity resource to check. Accepts entity object `{ kind: 'root', name: 'media', id: 1 }` or REST base as a string - `media`.
- _id_ `EntityRecordKey`: Optional ID of the rest resource to check.

_Returns_
Expand Down
65 changes: 43 additions & 22 deletions packages/core-data/src/resolvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,23 +352,48 @@ export const getEmbedPreview =
* Checks whether the current user can perform the given action on the given
* REST resource.
*
* @param {string} requestedAction Action to check. One of: 'create', 'read', 'update',
* 'delete'.
* @param {string} resource REST resource to check, e.g. 'media' or 'posts'.
* @param {?string} id ID of the rest resource to check.
* @param {string} requestedAction Action to check. One of: 'create', 'read', 'update',
* 'delete'.
* @param {string|Object} resource Entity resource to check. Accepts entity object `{ kind: 'root', name: 'media', id: 1 }`
* or REST base as a string - `media`.
* @param {?string} id ID of the rest resource to check.
*/
export const canUser =
( requestedAction, resource, id ) =>
async ( { dispatch, registry } ) => {
const { hasStartedResolution } = registry.select( STORE_NAME );

const resourcePath = id ? `${ resource }/${ id }` : resource;
const retrievedActions = [ 'create', 'read', 'update', 'delete' ];

if ( ! retrievedActions.includes( requestedAction ) ) {
throw new Error( `'${ requestedAction }' is not a valid action.` );
}

let resourcePath = null;
if ( typeof resource === 'object' ) {
if ( ! resource.kind || ! resource.name ) {
throw new Error( 'The entity resource object is not valid.' );
}

const configs = await dispatch(
getOrLoadEntitiesConfig( resource.kind, resource.name )
);
const entityConfig = configs.find(
( config ) =>
config.name === resource.name &&
config.kind === resource.kind
);
if ( ! entityConfig ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we log an error in case kind or name don't correspond to an entity type?

Copy link
Member Author

Choose a reason for hiding this comment

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

No other entity resolver does this; maybe we should re-evaluate missing config cases together?

return;
}

resourcePath =
entityConfig.baseURL + ( resource.id ? '/' + resource.id : '' );
} else {
// @todo: Maybe warn when detecting a legacy usage.
resourcePath = `/wp/v2/${ resource }` + ( id ? '/' + id : '' );
}

const { hasStartedResolution } = registry.select( STORE_NAME );

// Prevent resolving the same resource twice.
for ( const relatedAction of retrievedActions ) {
if ( relatedAction === requestedAction ) {
Expand All @@ -387,7 +412,7 @@ export const canUser =
let response;
try {
response = await apiFetch( {
path: `/wp/v2/${ resourcePath }`,
path: resourcePath,
method: 'OPTIONS',
parse: false,
} );
Expand Down Expand Up @@ -416,10 +441,15 @@ export const canUser =

registry.batch( () => {
for ( const action of retrievedActions ) {
dispatch.receiveUserPermission(
`${ action }/${ resourcePath }`,
permissions[ action ]
);
const key = (
typeof resource === 'object'
? [ action, resource.kind, resource.name, resource.id ]
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved
: [ action, resource, id ]
)
.filter( Boolean )
.join( '/' );

dispatch.receiveUserPermission( key, permissions[ action ] );
}
} );
};
Expand All @@ -435,16 +465,7 @@ export const canUser =
export const canUserEditEntityRecord =
( kind, name, recordId ) =>
async ( { dispatch } ) => {
const configs = await dispatch( getOrLoadEntitiesConfig( kind, name ) );
const entityConfig = configs.find(
( config ) => config.name === name && config.kind === kind
);
if ( ! entityConfig ) {
return;
}

const resource = entityConfig.__unstable_rest_base;
await dispatch( canUser( 'update', resource, recordId ) );
await dispatch( canUser( 'update', { kind, name, id: recordId } ) );
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved
};

/**
Expand Down
32 changes: 23 additions & 9 deletions packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ type EntityRecordArgs =
| [ string, string, EntityRecordKey ]
| [ string, string, EntityRecordKey, GetRecordsHttpQuery ];

type EntityResource = { kind: string; name: string; id?: EntityRecordKey };

/**
* Shared reference to an empty object for cases where it is important to avoid
* returning a new object reference on every invocation, as in a connected or
Expand Down Expand Up @@ -1136,7 +1138,8 @@ export function isPreviewEmbedFallback( state: State, url: string ): boolean {
*
* @param state Data state.
* @param action Action to check. One of: 'create', 'read', 'update', 'delete'.
* @param resource REST resource to check, e.g. 'media' or 'posts'.
* @param resource Entity resource to check. Accepts entity object `{ kind: 'root', name: 'media', id: 1 }`
* or REST base as a string - `media`.
* @param id Optional ID of the rest resource to check.
*
* @return Whether or not the user can perform the action,
Expand All @@ -1145,10 +1148,22 @@ export function isPreviewEmbedFallback( state: State, url: string ): boolean {
export function canUser(
state: State,
action: string,
resource: string,
resource: string | EntityResource,
id?: EntityRecordKey
): boolean | undefined {
const key = [ action, resource, id ].filter( Boolean ).join( '/' );
const isEntity = typeof resource === 'object';
if ( isEntity && ( ! resource.kind || ! resource.name ) ) {
return false;
}

const key = (
isEntity
? [ action, resource.kind, resource.name, resource.id ]
: [ action, resource, id ]
Copy link
Member

Choose a reason for hiding this comment

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

Let's do some consistency checks on the resource object, and also prevent conflicting keys. The following calls (some of them buggy) lead to the same key:

canUser( 'update', 'posts' );
canUser( 'update', { id: 'posts' } );
canUser( 'update', { name: 'posts' } );

The key should have some prefix (ent:postType/post/1, res:posts/1) and also we should trigger failure if there is resource.id and no resource.name etc.

Copy link
Member

Choose a reason for hiding this comment

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

Where is there a benefit for strings? It doesn't look like it's needed for memoization?

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about the key that is used for lookup in Redux state: state.userPermissions[ key ]. That is a string. The resource parameter for canUser is another beast.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can return null from the selector when the resource is an object, but the kind and name properties are missing. These two are required to make any entity queries. Resolve can bail early and not make a request.

I can add a warning or error, but these are rare in WP stores.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I meant by "failure", simply returning null 🙂 All we need is to avoid storing or returning the wrong data.

Copy link
Member

Choose a reason for hiding this comment

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

It could also be useful to log an error so the developer knows about unexpected usage.

@Mamaduka I see you mentioned those are rare in stores, but at the same time, when debugging store errors one often has to go deep, exactly because errors will often be silent or swallowed.

Copy link
Member Author

@Mamaduka Mamaduka Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could throw an error when the resource object is malformed, and the error will be accessible via getResolutionError. We're already doing something similar when action isn't supported.

I don't know the historical reason why resolvers/selectors don't log errors; my best guess is that people often report them as bugs. So basically, support overhead.

cc @youknowriad

Copy link
Member

@jsnajdr jsnajdr Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not very happy about throwing an error from the selector. It's not forward compatible. It's likely that we will add a new resource type in the future (e.g., for checking capabilities on the site) and then the new code will be failing on an older WordPress. It's better to return false, that means you cannot do the thing you ask for on an older WP version, and your new feature is effectively disabled. Let's just log a warning to the console, so that the error is not completely swallowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error is thrown from resolver b1e5dfc.

I think returning false is the correct approach. Core does the same with current_user_can. Calling current_user_can( 'fly' ) will return false. It's up to the devs to pass the correct data. Otherwise, we'll have to pollute the codebase with warnings.

Fun fact: Calling current_user_can( 'exist' ) will always return true.

Copy link
Member

Choose a reason for hiding this comment

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

The error is thrown from resolver

Oh, I missed that, in resolver it's fine.

Fun fact: Calling current_user_can( 'exist' ) will always return true.

It was added in 2010 here: https://core.trac.wordpress.org/ticket/14696#comment:5

Add an 'exist' capability that all users have, even if they don't have a role on a blog. current_user_can('exist'), yeah, cute. Suggestions welcome.

)
.filter( Boolean )
.join( '/' );

return state.userPermissions[ key ];
}

Expand All @@ -1173,13 +1188,12 @@ export function canUserEditEntityRecord(
name: string,
recordId: EntityRecordKey
): boolean | undefined {
const entityConfig = getEntityConfig( state, kind, name );
if ( ! entityConfig ) {
return false;
}
const resource = entityConfig.__unstable_rest_base;
deprecated( `wp.data.select( 'core' ).canUserEditEntityRecord()`, {
since: '6.7',
alternative: `wp.data.select( 'core' ).canUser( 'update', { kind, name, id } )`,
} );

return canUser( state, 'update', resource, recordId );
return canUser( state, 'update', { kind, name, id: recordId } );
}

/**
Expand Down
Loading
Loading