-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Reusable blocks: Improve UX for non-privileged users #12378
Changes from all commits
74e9344
267b9e4
2a8658a
89471ec
710b4b4
9364bee
40938e6
781de9f
1890cfc
23c65ac
d85724a
5546c58
692e12f
8797da3
7b8d040
d6a6b1c
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 |
---|---|---|
|
@@ -84,6 +84,7 @@ | |
'lodash', | ||
'wp-api-fetch', | ||
'wp-data', | ||
'wp-deprecated', | ||
'wp-url', | ||
), | ||
'wp-data' => array( | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,12 +1,13 @@ | ||||||
/** | ||||||
* External dependencies | ||||||
*/ | ||||||
import { find, includes, get, hasIn } from 'lodash'; | ||||||
import { find, includes, get, hasIn, compact } from 'lodash'; | ||||||
|
||||||
/** | ||||||
* WordPress dependencies | ||||||
*/ | ||||||
import { addQueryArgs } from '@wordpress/url'; | ||||||
import deprecated from '@wordpress/deprecated'; | ||||||
|
||||||
/** | ||||||
* Internal dependencies | ||||||
|
@@ -16,7 +17,7 @@ import { | |||||
receiveEntityRecords, | ||||||
receiveThemeSupports, | ||||||
receiveEmbedPreview, | ||||||
receiveUploadPermissions, | ||||||
receiveUserPermission, | ||||||
} from './actions'; | ||||||
import { getKindEntities } from './entities'; | ||||||
import { apiFetch } from './controls'; | ||||||
|
@@ -101,9 +102,57 @@ export function* getEmbedPreview( url ) { | |||||
|
||||||
/** | ||||||
* Requests Upload Permissions from the REST API. | ||||||
* | ||||||
* @deprecated since 5.0. Callers should use the more generic `canUser()` selector instead of | ||||||
* `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. | ||||||
*/ | ||||||
export function* hasUploadPermissions() { | ||||||
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. Should I mark this as deprecated? I'm unsure on what the policy is post 5.0 🙂 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 don't know either. Might be worth pinging someone on slack. Will hold off approving until the answer is known. 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. cc. @youknowriad 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. It's probably fine leaving this as is where we support both 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. We might want to use a soft deprecation by using @deprecated in JSDoc and maybe filter out all actions and selectors from the generated docs. 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. Yes, for now, we should leave those, we can add a deprecation message if needed but without specifying a version. This is something we should discuss more in the next #core-js chats. 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 noted this in the agenda for the next meeting. 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. @noisysocks I noticed you 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. That's a good point. I think selectors are the public interface here but it wouldn't hurt to have 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. Done in 4ea46a5! |
||||||
const response = yield apiFetch( { path: '/wp/v2/media', method: 'OPTIONS', parse: false } ); | ||||||
deprecated( "select( 'core' ).hasUploadPermissions()", { | ||||||
alternative: "select( 'core' ).canUser( 'create', 'media' )", | ||||||
} ); | ||||||
yield* canUser( 'create', 'media' ); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Checks whether the current user can perform the given action on the given | ||||||
* REST resource. | ||||||
* | ||||||
* @param {string} action 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. | ||||||
*/ | ||||||
export function* canUser( action, resource, id ) { | ||||||
const methods = { | ||||||
create: 'POST', | ||||||
read: 'GET', | ||||||
update: 'PUT', | ||||||
delete: 'DELETE', | ||||||
}; | ||||||
|
||||||
const method = methods[ action ]; | ||||||
if ( ! method ) { | ||||||
throw new Error( `'${ action }' is not a valid action.` ); | ||||||
} | ||||||
|
||||||
const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`; | ||||||
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.
Suggested change
would be shorter, not sure what reads better though :) 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 like that with the more verbose version that you can glance at the code and see the "shape" of the URL. |
||||||
|
||||||
let response; | ||||||
try { | ||||||
response = yield apiFetch( { | ||||||
path, | ||||||
// Ideally this would always be an OPTIONS request, but unfortunately there's | ||||||
// a bug in the REST API which causes the Allow header to not be sent on | ||||||
// OPTIONS requests to /posts/:id routes. | ||||||
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. Is that just for /post/:id of for the all resources? I mentioned that because the logic below happens for all resources (so doesn't match the comment). Is there a ticket/issue covering the bug? 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've only checked posts but I suspect the bug affects all REST endpoints. Will do some investigating and open up a Trac ticket 👍 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. If this is the case, it should be fixed /cc @danielbachhuber @kadamwhite 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. If you just want 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. Looking into this further:
It seems like Can you provide steps to reproduce? 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 briefly debugged this when I noticed the issue and from memory it was something to do with
Above steps also work with posts: just change 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 created a ticket here: https://core.trac.wordpress.org/ticket/45753 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. Added a link to the Trac ticket in cf2d701. |
||||||
// https://core.trac.wordpress.org/ticket/45753 | ||||||
method: id ? 'GET' : 'OPTIONS', | ||||||
parse: false, | ||||||
} ); | ||||||
} catch ( error ) { | ||||||
// Do nothing if our OPTIONS request comes back with an API error (4xx or | ||||||
// 5xx). The previously determined isAllowed value will remain in the store. | ||||||
return; | ||||||
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. 😟 This seems like it should throw. Why are we catching an ignoring the error without doing anything with it? 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.
|
||||||
} | ||||||
|
||||||
let allowHeader; | ||||||
if ( hasIn( response, [ 'headers', 'get' ] ) ) { | ||||||
|
@@ -116,5 +165,7 @@ export function* hasUploadPermissions() { | |||||
allowHeader = get( response, [ 'headers', 'Allow' ], '' ); | ||||||
} | ||||||
|
||||||
yield receiveUploadPermissions( includes( allowHeader, 'POST' ) ); | ||||||
const key = compact( [ action, resource, id ] ).join( '/' ); | ||||||
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 trick :) 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'd need template strings for the first part:
Suggested change
I think it's a bit easier to sort out though, yeah. 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. Was this suggestion meant to go in this thread? #12378 (comment) |
||||||
const isAllowed = includes( allowHeader, method ); | ||||||
yield receiveUserPermission( key, isAllowed ); | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,13 @@ | |
* External dependencies | ||
*/ | ||
import createSelector from 'rememo'; | ||
import { map, find, get, filter } from 'lodash'; | ||
import { map, find, get, filter, compact, defaultTo } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { select } from '@wordpress/data'; | ||
import deprecated from '@wordpress/deprecated'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -171,12 +172,46 @@ export function isPreviewEmbedFallback( state, url ) { | |
} | ||
|
||
/** | ||
* Return Upload Permissions. | ||
* Returns whether the current user can upload media. | ||
* | ||
* @param {Object} state State tree. | ||
* Calling this may trigger an OPTIONS request to the REST API via the | ||
* `canUser()` resolver. | ||
* | ||
* @return {boolean} Upload Permissions. | ||
* https://developer.wordpress.org/rest-api/reference/ | ||
* | ||
* @deprecated since 5.0. Callers should use the more generic `canUser()` selector instead of | ||
* `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. | ||
* | ||
* @param {Object} state Data state. | ||
* | ||
* @return {boolean} Whether or not the user can upload media. Defaults to `true` if the OPTIONS | ||
* request is being made. | ||
*/ | ||
export function hasUploadPermissions( state ) { | ||
return state.hasUploadPermissions; | ||
deprecated( "select( 'core' ).hasUploadPermissions()", { | ||
alternative: "select( 'core' ).canUser( 'create', 'media' )", | ||
} ); | ||
return defaultTo( canUser( state, 'create', 'media' ), true ); | ||
} | ||
|
||
/** | ||
* Returns whether the current user can perform the given action on the given | ||
talldan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* REST resource. | ||
* | ||
* Calling this may trigger an OPTIONS request to the REST API via the | ||
* `canUser()` resolver. | ||
* | ||
* https://developer.wordpress.org/rest-api/reference/ | ||
* | ||
* @param {Object} state Data state. | ||
* @param {string} action Action to check. One of: 'create', 'read', 'update', 'delete'. | ||
* @param {string} resource REST resource to check, e.g. 'media' or 'posts'. | ||
* @param {string=} id Optional ID of the rest resource to check. | ||
* | ||
* @return {boolean|undefined} Whether or not the user can perform the action, | ||
* or `undefined` if the OPTIONS request is still being made. | ||
*/ | ||
export function canUser( state, action, resource, id ) { | ||
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 like the expressiveness of the canUser selector 😄 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. Props to @gziolo for suggesting the API 🙂 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 glad you like it :) |
||
const key = compact( [ action, resource, id ] ).join( '/' ); | ||
return get( state, [ 'userPermissions', key ] ); | ||
} |
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.
There should have been a corresponding change to the root
package-lock.json
. I'm not sure how this passed the build task which checks for local uncommitted changes, but it shouldn't have.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.
Huh! Looks like
check-local-changes
doesn't pick this up becauseprecheck-local-changes
doesn't runnpm install
. CI doesn't pick it up because it usesnpm ci
.Will fix this up in a seperate PR.
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.
#13462