Show UI warning to users with insufficient permissions when trying to create a new Navigation #37289
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
getdave
changed the title
|
Size Change: +177 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
This was referenced Dec 10, 2021
getdave
requested review from
spacedmonkey,
talldan,
adamziel and
draganescu
and removed request for
nerrad
|
Even if this PR doesn't go ahead, I'm interested to know whether we think the changes to |
For reference, there is a ticket at https://core.trac.wordpress.org/ticket/40197 that requests for core to actually start using this meta capability in more cases, since it's currently being under-used. Hopefully this proposal can make some progress in this area. |
38 tasks
7 tasks
talldan
reviewed
Dec 15, 2021
There was a problem hiding this comment.
This is an interesting exploration, but I don't really think we should make this switch to using 'drafts' for everything. This opens up questions:
- When do the menus get published?
- How does a contributor ever get their menu published?
There's also currently some code that publishes menus when a menu is saved via the entity panel:
https://github.com/WordPress/gutenberg/pull/36024/files#diff-c5815980e7aa19efd287aaa72512569ac7de514d74679af2f0b099f8a9369bc5R141-R150
And I expect this will error whenever a contributor tries to save their post.
I'm thinking that we should consider alternatives to this PR, like handling the 404 when a user tries to create a new menu.
packages/block-library/src/navigation/edit/use-create-navigation-menu.js
Outdated
Show resolved
Hide resolved
Comment on lines
+304
to
+310
| if ( isPublish ) { | ||
| isAllowed = !! response?.schema?.links?.find( ( link ) => | ||
| link.rel.includes( 'action-publish' ) | ||
| ); |
There was a problem hiding this comment.
Would be good to add a new unit test for this 👍
Comment on lines
174
to
180
| canUserPublishNavigation: ref | ||
| ? canUser( 'publish', 'navigation', ref ) | ||
| : undefined, | ||
| hasResolvedCanUserPublishNavigationEntity: hasFinishedResolution( | ||
| 'canUser', | ||
| [ 'publish', 'navigation', ref ] | ||
| ), |
There was a problem hiding this comment.
Like the feedback on the other PR, I think this should be folded into useNavigationMenu.
We need to avoid making the navigation block main edit file any chunkier.
| return; | ||
| } | ||
|
|
||
| noticeRef.current = uniqueId( 'navBlockNoPublishPermissions' ); |
There was a problem hiding this comment.
As per the comment on the other PR, no need to use a uniqueId here.
|
|
||
| createWarningNotice( | ||
| __( | ||
| 'You do not have permission to publish Navigations. Any edits made will not be saved.' |
There was a problem hiding this comment.
'Navigations' isn't a word 😄
It's a singular noun only.
|
This PR needs unit test and for the code to broken in to new files / functions. Once that is complete, I will re-review. |
|
This PR will need a rebase once #37286 is merged. At that point it will be easier to add the unit test and sort the files. I'd rather avoid any major refactors if we're aiming to land this in 5.9, but more than happy to pursue this post 5.9. |
I know this can't stay "as is". It's for illustration only.
This reverts commit 4fbeb72.
getdave
force-pushed
the
try/nav-block-show-ui-warning-when-trying-to-create
branch
from
b56fe5e to
1c2f26a
Compare
7 tasks
|
This isn't going to work. See alternative in #37454 |
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.
Description
Explores part of #36286 (comment) to display a warning to the user if they have insufficient permissions to publish a given Navigation block.
Publish status is not then ability to create a post. Rather it is the ability to publish it. By default only admin Role users can do this.
https://github.com/WordPress/wordpress-develop/blob/424afb85afa2e19f30d9836440f63c5237b4102e/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2115-L2116
Moreover the Publish capability is a Meta capability meaning it is on a per post basis and not a global thing (i.e. you need a Post ID to check against for "publish" capability).
As things stand
canUsercannot determine publish status so this PR updates that to enable the abiltity to do so.Moreover, when we're creating the initial Navigation post, because we're attempting to do so with the
publishstatus, the POST request fails and thus we never have a Post ID to check against for "publish" permission. This PR amends the initial creation to only create a draft Navigation Post. This allows us to get a Post ID back and use that for thecanUserpublish check. We'd have to amend the block to have a step which changes the post status topublishonce the initial creation is complete.Note this doesn't handle editing existing Navigations. That's handled in #37286.
How has this been tested?
Screenshots
Screen.Capture.on.2021-12-10.at.16-01-26.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).