Use entity details when calling 'canUser' selectors #63415
Conversation
|
Size Change: +456 B (+0.03%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6c48757. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9903440164
|
Mamaduka
force-pushed
the
update/can-user-selector-usage
branch
from
fed3d8f
to
3598447
Compare
| } ), | ||
| canCreateTemplatePart: canUser( 'create', { | ||
| kind: 'postType', | ||
| name: 'wp_template_part', |
There was a problem hiding this comment.
Could we use TEMPLATE_PART_POST_TYPE here, and in similar in other places?
There was a problem hiding this comment.
Update to use constants where it made sense - where the constant was available.
| !! canUser( 'create', { | ||
| kind: 'postType', | ||
| name: 'wp_block', | ||
| } ); |
There was a problem hiding this comment.
Tbh, I wish this API looks looked more similar to getEntityRecord, or the other way around. Then you could maybe also reuse the same args for fetching and checking permissions.
There was a problem hiding this comment.
I think under the hood on the REST API level it's exactly what happens as it sometimes has to call OPTIONS request so sharing the same list of params in that case was warranted when using canUserEditEntityRecord.
There was a problem hiding this comment.
Yeah, could we still change it, since canUser has only just been modified?
There was a problem hiding this comment.
Or should getEntityRecord(s) allow (clearer) named parameters? I guess it would be good to have more opinions here. Cc @jsnajdr @youknowriad
There was a problem hiding this comment.
If we use similar arguments, it would be hard to distinguish whether the user wants an entity-related or REST API resource query.
How should we handle cases like this without collision? While IDs are usually an int, this isn't a requirement.
canUser( 'create', 'media' );
canUser( 'create', 'root', 'media' );
There was a problem hiding this comment.
Yeah, I think using "resource" in the first place was a wrong idea but it seems the ship has sailed here.
There was a problem hiding this comment.
Backward compatibility driven development is a factor here 😄
There was a problem hiding this comment.
Btw, the block-directory package has a valid use case for "resource".
Mamaduka
force-pushed
the
update/can-user-selector-usage
branch
from
3598447
to
6c48757
Compare
Mamaduka
changed the title
Mamaduka
requested review from
draganescu,
adamziel,
kevin940726 and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This is ready for the review. I'll handle the cc @WordPress/gutenberg-core |
| false, | ||
| canViewTemplatePart: !! select( coreStore ).canUser( 'read', { | ||
| kind: 'postType', | ||
| name: 'wp_template', |
There was a problem hiding this comment.
Similar to what @ellatrix suggested about the template part post type, we could use TEMPLATE_POST_TYPE for these.
There was a problem hiding this comment.
I've updated the part when these constants were available. They're package-specific. Most entity-related blocks hardcode their entity names.
Are we worried that these names might change?
Also, I don't mind defining a couple of constants at the file level, but for the initial run, I tried to change only the selector syntax and leave the rest of the code alone.
There was a problem hiding this comment.
Definitely not a blocker for the PR 👍
| kind: 'root', | ||
| name: 'media', |
There was a problem hiding this comment.
I've always found it odd that the media entity is a separate root entity, and we don't use the attachment post type. Somehow, this special case is more visible now, which I find potentially confusing. Not much to do obviously, since this is about inherited complexity.
There was a problem hiding this comment.
100%, but WP always treated Media as a separate entity, even though it's just a post type.
P.S. Jorge discovered another odd case recently: #63438.
| kind: 'postType', | ||
| name: 'wp_font_family', | ||
| id: customFontFamilyId, |
There was a problem hiding this comment.
I appreciate the explicit syntax, it makes it clearer as to what we're doing. Previously, one had to search what font-families is - is it a taxonomy, is it a post type?
Mamaduka
force-pushed
the
update/can-user-selector-usage
branch
from
6c48757
to
755d66b
Compare
|
Is this ready to be merged, and can I get approval? 🙇 |
| kind: 'postType', | ||
| name: 'wp_template', | ||
| } ), | ||
| canEditTemplatePart: !! select( coreStore ).canUser( 'create', { |
There was a problem hiding this comment.
Not from this PR, but it felt odd that we call this "can edit" but check for whether they can create (POST) and not update (PUT). Haven't checked if that makes more sense in the context it's used.
There was a problem hiding this comment.
I think introducing the pattern might have been my fault. The WP maps all template CRUD permissions to the same capability, so I reused already existing call for edit capability check to avoid making extra OPTIONS request to the server.
The duplicate requests issue was fixed (#43480). I forgot to update my old code, and the pattern spread into new places.
I will follow up on this in a separate PR.
| return { | ||
| frontPageId: siteSettings?.page_on_front, | ||
| postsPageId: siteSettings?.page_for_posts, | ||
| labels: getPostType( postType )?.labels, | ||
| canCreateRecord: canUser( | ||
| 'create', | ||
| postTypeObject?.rest_base || 'posts' |
There was a problem hiding this comment.
Nice simplification 👍
I wonder why the pre-existing indirection and the default `'posts`` was necessary 🤔
There was a problem hiding this comment.
I'm guessing to avoid passing undefined when postTypeObject hasn't been resolved yet, which would result in sending a request to the general namespace. However, I'm unsure why the posts fallback was selected.
I fixed a similar issue in #63423 (comment).
There was a problem hiding this comment.
Back in the day, when we were writing up custom admin menus, for example, and we were displaying labels for custom post types, we were falling back to the default labels for post types - because when registering a custom post type, not all labels are guaranteed to be declared. Maybe this assumed that the rest_base could not be declared, too.
There was a problem hiding this comment.
The rest_base should default to the post type slug.
There was a problem hiding this comment.
True. But then it can be overridden 🤷♂️
There was a problem hiding this comment.
You can do almost everything in WP 😅
There was a problem hiding this comment.
Yup. I'm just trying to understand why this was here. Maybe it was a temporary thing as we were working to make the posts list more flexible in the future? It's a recent addition (#62705), perhaps @ntsekouras could remember.
There was a problem hiding this comment.
Yeah, the default was to avoid the undefined value. Regarding why we need the resource, it's because we are using the same component for posts and pages and will probably be the base component of other post types too. Finally regarding the default:posts, didn't think it mattered much :).
Aside this, I'm catching up with @Mamaduka's work on these parts and is great! Thank you!
| @@ -40,7 +40,11 @@ export default function EditTemplateBlocksNotification( { contentRef } ) { | |||
|
|
|||
| const canEditTemplate = useSelect( | |||
| ( select ) => | |||
| select( coreStore ).canUser( 'create', 'templates' ) ?? false | |||
| !! select( coreStore ).canUser( 'create', { | |||
There was a problem hiding this comment.
Explicit conversion to boolean works well as a substitute to ?? false.
* Use entity details when calling 'canUser' selectors * Feedback Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
priethor
added
[Type] Code Quality
What?
This is a follow-up to #63322.
PR updates the codebase to use new entity-are syntax for the
canUserselector.How?
I just switched the selector calls to the new syntax and tried to make as few changes as possible to the logic.
Testing Instructions
I hope e2e tests have our back for these changes, but smoke testing the editor functionality will help.
Testing Instructions for Keyboard
Same.