-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Use entity details when calling 'canUser' selectors #63415
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,11 +127,14 @@ export default function TemplatePartInnerBlocks( { | |
const { canViewTemplatePart, canEditTemplatePart } = useSelect( | ||
( select ) => { | ||
return { | ||
canViewTemplatePart: | ||
select( coreStore ).canUser( 'read', 'templates' ) ?? false, | ||
canEditTemplatePart: | ||
select( coreStore ).canUser( 'create', 'templates' ) ?? | ||
false, | ||
canViewTemplatePart: !! select( coreStore ).canUser( 'read', { | ||
kind: 'postType', | ||
name: 'wp_template', | ||
} ), | ||
canEditTemplatePart: !! select( coreStore ).canUser( 'create', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not from this PR, but it felt odd that we call this "can edit" but check for whether they can create ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
kind: 'postType', | ||
name: 'wp_template', | ||
} ), | ||
}; | ||
}, | ||
[] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,7 +93,11 @@ function InstalledFonts() { | |
const { canUser } = select( coreStore ); | ||
return ( | ||
customFontFamilyId && | ||
canUser( 'delete', 'font-families', customFontFamilyId ) | ||
canUser( 'delete', { | ||
kind: 'postType', | ||
name: 'wp_font_family', | ||
id: customFontFamilyId, | ||
Comment on lines
+97
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the explicit syntax, it makes it clearer as to what we're doing. Previously, one had to search what |
||
} ) | ||
); | ||
}, | ||
[ customFontFamilyId ] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -372,15 +372,14 @@ export default function PostList( { postType } ) { | |
const { getEntityRecord, getPostType, canUser } = | ||
select( coreStore ); | ||
const siteSettings = getEntityRecord( 'root', 'site' ); | ||
const postTypeObject = getPostType( postType ); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice simplification 👍 I wonder why the pre-existing indirection and the default `'posts`` was necessary 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing to avoid passing I fixed a similar issue in #63423 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True. But then it can be overridden 🤷♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can do almost everything in WP 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the default was to avoid the Aside this, I'm catching up with @Mamaduka's work on these parts and is great! Thank you! |
||
), | ||
canCreateRecord: canUser( 'create', { | ||
kind: 'postType', | ||
name: postType, | ||
} ), | ||
}; | ||
}, | ||
[ postType ] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not a blocker for the PR 👍