From 74e93447f6ffe06f81671b9c644ec7e008755039 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 5 Dec 2018 19:00:56 +1100 Subject: [PATCH 01/16] Improve reusable block UX for non-privileged users Improves the UX of creating, editing, and deleting a reusable block when logged in as an author or contributor by disabling the _Add to Reusable Blocks_, _Edit_, and _Remove from Reusable Blocks_ buttons when necessary. This is accomplished under the hood by introducing the `canUser()` selector to `core-data` which allows callers to query whether the REST API supports performing a given action on a given resource, e.g. one can query whether the logged in user can create posts by running `wp.data.select( 'core' ).canUser( 'create', 'posts' )`. The existing `hasUploadPermissions()` selector is changed to use `canUser( 'create', 'media' )` under the hood. --- .../developers/data/data-core.md | 29 +++++- .../src/block/edit-panel/index.js | 3 +- packages/block-library/src/block/edit.js | 6 +- packages/core-data/src/actions.js | 22 ++++- packages/core-data/src/reducer.js | 18 ++-- packages/core-data/src/resolvers.js | 49 +++++++++- packages/core-data/src/selectors.js | 21 +++- packages/core-data/src/test/actions.js | 12 ++- packages/core-data/src/test/reducer.js | 28 +++--- packages/core-data/src/test/resolvers.js | 95 ++++++++++++++++++- packages/core-data/src/test/selectors.js | 28 ++++++ .../reusable-block-convert-button.js | 4 + .../reusable-block-delete-button.js | 17 +++- .../reusable-block-delete-button.js.snap | 1 + .../test/reusable-block-convert-button.js | 2 + .../test/reusable-block-delete-button.js | 6 +- 16 files changed, 301 insertions(+), 40 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index 2a98c3bb5d3974..879fd7fbd5ae31 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -148,6 +148,23 @@ Return Upload Permissions. Upload Permissions. +### canUser + +Returns whether the current user can perform the given action on the given +REST resource. + +*Parameters* + + * state: Data state. + * action: Action to check. One of: 'create', 'read', 'update', + 'delete'. + * resource: REST resource to check, e.g. 'media' or 'posts'. + * id: ID of the rest resource to check. + +*Returns* + +Whether or not the user can perform the action. + ## Actions ### receiveUserQuery @@ -213,4 +230,14 @@ Returns an action object used in signalling that Upload permissions have been re *Parameters* - * hasUploadPermissions: Does the user have permission to upload files? \ No newline at end of file + * hasUploadPermissions: Does the user have permission to upload files? + +### receiveUserPermissions + +Returns an action object used in signalling that the current user has +permission to perform an action on a REST resource. + +*Parameters* + + * key: A key that represents the action and REST resource. + * isAllowed: Whether or not the user can perform the action. \ No newline at end of file diff --git a/packages/block-library/src/block/edit-panel/index.js b/packages/block-library/src/block/edit-panel/index.js index e91c76cc6bf52e..eb620a877b63a1 100644 --- a/packages/block-library/src/block/edit-panel/index.js +++ b/packages/block-library/src/block/edit-panel/index.js @@ -53,7 +53,7 @@ class ReusableBlockEditPanel extends Component { } render() { - const { isEditing, title, isSaving, onEdit, instanceId } = this.props; + const { isEditing, title, isSaving, isEditDisabled, onEdit, instanceId } = this.props; return ( @@ -66,6 +66,7 @@ class ReusableBlockEditPanel extends Component { ref={ this.editButton } isLarge className="reusable-block-edit-panel__button" + disabled={ isEditDisabled } onClick={ onEdit } > { __( 'Edit' ) } diff --git a/packages/block-library/src/block/edit.js b/packages/block-library/src/block/edit.js index d95b07aedb9150..80f1c16be71699 100644 --- a/packages/block-library/src/block/edit.js +++ b/packages/block-library/src/block/edit.js @@ -97,7 +97,7 @@ class ReusableBlockEdit extends Component { } render() { - const { isSelected, reusableBlock, block, isFetching, isSaving } = this.props; + const { isSelected, reusableBlock, block, isFetching, isSaving, canUpdateBlock } = this.props; const { isEditing, title, changedAttributes } = this.state; if ( ! reusableBlock && isFetching ) { @@ -130,6 +130,7 @@ class ReusableBlockEdit extends Component { isEditing={ isEditing } title={ title !== null ? title : reusableBlock.title } isSaving={ isSaving && ! reusableBlock.isTemporary } + isEditDisabled={ ! canUpdateBlock } onEdit={ this.startEditing } onChangeTitle={ this.setTitle } onSave={ this.save } @@ -151,6 +152,8 @@ export default compose( [ __experimentalIsSavingReusableBlock: isSavingReusableBlock, getBlock, } = select( 'core/editor' ); + const { canUser } = select( 'core' ); + const { ref } = ownProps.attributes; const reusableBlock = getReusableBlock( ref ); @@ -159,6 +162,7 @@ export default compose( [ isFetching: isFetchingReusableBlock( ref ), isSaving: isSavingReusableBlock( ref ), block: reusableBlock ? getBlock( reusableBlock.clientId ) : null, + canUpdateBlock: !! reusableBlock && ! reusableBlock.isTemporary && canUser( 'update', 'blocks', ref ), }; } ), withDispatch( ( dispatch, ownProps ) => { diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index ebbcfb7e761500..bc6deae1c53eba 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -137,7 +137,25 @@ export function* saveEntityRecord( kind, name, record ) { */ export function receiveUploadPermissions( hasUploadPermissions ) { return { - type: 'RECEIVE_UPLOAD_PERMISSIONS', - hasUploadPermissions, + type: 'RECEIVE_USER_PERMISSIONS', + key: 'create/media', + isAllowed: hasUploadPermissions, + }; +} + +/** + * Returns an action object used in signalling that the current user has + * permission to perform an action on a REST resource. + * + * @param {string} key A key that represents the action and REST resource. + * @param {boolean} isAllowed Whether or not the user can perform the action. + * + * @return {Object} Action object. + */ +export function receiveUserPermissions( key, isAllowed ) { + return { + type: 'RECEIVE_USER_PERMISSIONS', + key, + isAllowed, }; } diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index bb14a0283b7ca5..676edba2198587 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -218,17 +218,21 @@ export function embedPreviews( state = {}, action ) { } /** - * Reducer managing Upload permissions. + * State which tracks whether the user can perform an action on a REST + * resource. * - * @param {Object} state Current state. - * @param {Object} action Dispatched action. + * @param {Object} state Current state. + * @param {Object} action Dispatched action. * * @return {Object} Updated state. */ -export function hasUploadPermissions( state = true, action ) { +export function userPermissions( state = {}, action ) { switch ( action.type ) { - case 'RECEIVE_UPLOAD_PERMISSIONS': - return action.hasUploadPermissions; + case 'RECEIVE_USER_PERMISSIONS': + return { + ...state, + [ action.key ]: action.isAllowed, + }; } return state; @@ -241,5 +245,5 @@ export default combineReducers( { themeSupports, entities, embedPreviews, - hasUploadPermissions, + userPermissions, } ); diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index b8bd4ed2382854..f988e2f14f13fc 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { find, includes, get, hasIn } from 'lodash'; +import { find, includes, get, hasIn, compact } from 'lodash'; /** * WordPress dependencies @@ -16,7 +16,7 @@ import { receiveEntityRecords, receiveThemeSupports, receiveEmbedPreview, - receiveUploadPermissions, + receiveUserPermissions, } from './actions'; import { getKindEntities } from './entities'; import { apiFetch } from './controls'; @@ -103,7 +103,46 @@ export function* getEmbedPreview( url ) { * Requests Upload Permissions from the REST API. */ export function* hasUploadPermissions() { - const response = yield apiFetch( { path: '/wp/v2/media', method: 'OPTIONS', parse: false } ); + 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 }`; + + 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. + method: id ? 'GET' : 'OPTIONS', + parse: false, + } ); + } catch ( error ) { + return; + } let allowHeader; if ( hasIn( response, [ 'headers', 'get' ] ) ) { @@ -116,5 +155,7 @@ export function* hasUploadPermissions() { allowHeader = get( response, [ 'headers', 'Allow' ], '' ); } - yield receiveUploadPermissions( includes( allowHeader, 'POST' ) ); + const key = compact( [ action, resource, id ] ).join( '/' ); + const isAllowed = includes( allowHeader, method ); + yield receiveUserPermissions( key, isAllowed ); } diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 95e9f867aa3c02..e5ba53ab5d385f 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -2,7 +2,7 @@ * External dependencies */ import createSelector from 'rememo'; -import { map, find, get, filter } from 'lodash'; +import { map, find, get, filter, compact } from 'lodash'; /** * WordPress dependencies @@ -178,5 +178,22 @@ export function isPreviewEmbedFallback( state, url ) { * @return {boolean} Upload Permissions. */ export function hasUploadPermissions( state ) { - return state.hasUploadPermissions; + return canUser( state, 'create', 'media' ); +} + +/** + * Returns whether the current user can perform the given action on the given + * REST resource. + * + * @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 ID of the rest resource to check. + * + * @return {boolean} Whether or not the user can perform the action. + */ +export function canUser( state, action, resource, id ) { + const key = compact( [ action, resource, id ] ).join( '/' ); + return get( state, [ 'userPermissions', key ], true ); } diff --git a/packages/core-data/src/test/actions.js b/packages/core-data/src/test/actions.js index 86e7f50ed53c1e..75cff77676a96f 100644 --- a/packages/core-data/src/test/actions.js +++ b/packages/core-data/src/test/actions.js @@ -1,7 +1,7 @@ /** * Internal dependencies */ -import { saveEntityRecord, receiveEntityRecords } from '../actions'; +import { saveEntityRecord, receiveEntityRecords, receiveUserPermissions } from '../actions'; describe( 'saveEntityRecord', () => { it( 'triggers a POST request for a new record', async () => { @@ -58,3 +58,13 @@ describe( 'saveEntityRecord', () => { expect( received ).toEqual( receiveEntityRecords( 'root', 'postType', postType, undefined, true ) ); } ); } ); + +describe( 'receiveUserPermissions', () => { + it( 'builds an action object', () => { + expect( receiveUserPermissions( 'create/media', true ) ).toEqual( { + type: 'RECEIVE_USER_PERMISSIONS', + key: 'create/media', + isAllowed: true, + } ); + } ); +} ); diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index f6647becf07433..eca3b629581796 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -7,7 +7,7 @@ import { filter } from 'lodash'; /** * Internal dependencies */ -import { terms, entities, embedPreviews, hasUploadPermissions } from '../reducer'; +import { terms, entities, embedPreviews, userPermissions } from '../reducer'; describe( 'terms()', () => { it( 'returns an empty object by default', () => { @@ -118,21 +118,25 @@ describe( 'embedPreviews()', () => { } ); } ); -describe( 'hasUploadPermissions()', () => { - it( 'returns true by default', () => { - const state = hasUploadPermissions( undefined, {} ); - - expect( state ).toEqual( true ); +describe( 'userPermissions()', () => { + it( 'defaults to an empty object', () => { + const state = userPermissions( undefined, {} ); + expect( state ).toEqual( {} ); } ); - it( 'returns with updated upload permissions value', () => { - const originalState = true; + it( 'updates state with whether an action is allowed', () => { + const original = deepFreeze( { + 'create/media': false, + } ); - const state = hasUploadPermissions( originalState, { - type: 'RECEIVE_UPLOAD_PERMISSIONS', - hasUploadPermissions: false, + const state = userPermissions( original, { + type: 'RECEIVE_USER_PERMISSIONS', + key: 'create/media', + isAllowed: true, } ); - expect( state ).toEqual( false ); + expect( state ).toEqual( { + 'create/media': true, + } ); } ); } ); diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 8b008bc1cad685..05acabcf32d3a1 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -1,8 +1,9 @@ /** * Internal dependencies */ -import { getEntityRecord, getEntityRecords, getEmbedPreview } from '../resolvers'; -import { receiveEntityRecords, receiveEmbedPreview } from '../actions'; +import { getEntityRecord, getEntityRecords, getEmbedPreview, canUser } from '../resolvers'; +import { receiveEntityRecords, receiveEmbedPreview, receiveUserPermissions } from '../actions'; +import { apiFetch } from '../controls'; describe( 'getEntityRecord', () => { const POST_TYPE = { slug: 'post' }; @@ -68,3 +69,93 @@ describe( 'getEmbedPreview', () => { expect( received ).toEqual( receiveEmbedPreview( UNEMBEDDABLE_URL, UNEMBEDDABLE_RESPONSE ) ); } ); } ); + +describe( 'canUser', () => { + it( 'does nothing when there is an API error', () => { + const generator = canUser( 'create', 'media' ); + + let received = generator.next(); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( apiFetch( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ) ); + + received = generator.throw( { status: 404 } ); + expect( received.done ).toBe( true ); + expect( received.value ).toBeUndefined(); + } ); + + it( 'receives false when the user is not allowed to perform an action', () => { + const generator = canUser( 'create', 'media' ); + + let received = generator.next(); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( apiFetch( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ) ); + + received = generator.next( { + headers: { + Allow: 'GET', + }, + } ); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( receiveUserPermissions( 'create/media', false ) ); + + received = generator.next(); + expect( received.done ).toBe( true ); + expect( received.value ).toBeUndefined(); + } ); + + it( 'receives true when the user is allowed to perform an action', () => { + const generator = canUser( 'create', 'media' ); + + let received = generator.next(); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( apiFetch( { + path: '/wp/v2/media', + method: 'OPTIONS', + parse: false, + } ) ); + + received = generator.next( { + headers: { + Allow: 'POST, GET, PUT, DELETE', + }, + } ); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( receiveUserPermissions( 'create/media', true ) ); + + received = generator.next(); + expect( received.done ).toBe( true ); + expect( received.value ).toBeUndefined(); + } ); + + it( 'receives true when the user is allowed to perform an action on a specific resource', () => { + const generator = canUser( 'update', 'blocks', 123 ); + + let received = generator.next(); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( apiFetch( { + path: '/wp/v2/blocks/123', + method: 'GET', + parse: false, + } ) ); + + received = generator.next( { + headers: { + Allow: 'POST, GET, PUT, DELETE', + }, + } ); + expect( received.done ).toBe( false ); + expect( received.value ).toEqual( receiveUserPermissions( 'update/blocks/123', true ) ); + + received = generator.next(); + expect( received.done ).toBe( true ); + expect( received.value ).toBeUndefined(); + } ); +} ); diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index b982ada4f3a9d2..bce4ae67b693e8 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -11,6 +11,7 @@ import { getEntityRecords, getEmbedPreview, isPreviewEmbedFallback, + canUser, } from '../selectors'; describe( 'getEntityRecord', () => { @@ -117,3 +118,30 @@ describe( 'isPreviewEmbedFallback()', () => { expect( isPreviewEmbedFallback( state, 'http://example.com/' ) ).toEqual( true ); } ); } ); + +describe( 'canUser', () => { + it( 'returns true by default', () => { + const state = deepFreeze( { + userPermissions: {}, + } ); + expect( canUser( state, 'create', 'media' ) ).toBe( true ); + } ); + + it( 'returns whether an action can be performed', () => { + const state = deepFreeze( { + userPermissions: { + 'create/media': false, + }, + } ); + expect( canUser( state, 'create', 'media' ) ).toBe( false ); + } ); + + it( 'returns whether an action can be performed for a given resource', () => { + const state = deepFreeze( { + userPermissions: { + 'create/media/123': false, + }, + } ); + expect( canUser( state, 'create', 'media', 123 ) ).toBe( false ); + } ); +} ); diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js index 6d39538aa5e5ab..75b675e6c404de 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js @@ -16,6 +16,7 @@ import { compose } from '@wordpress/compose'; export function ReusableBlockConvertButton( { isVisible, isStaticBlock, + canCreateBlocks, onConvertToStatic, onConvertToReusable, } ) { @@ -29,6 +30,7 @@ export function ReusableBlockConvertButton( { { __( 'Add to Reusable Blocks' ) } @@ -54,6 +56,7 @@ export default compose( [ canInsertBlockType, __experimentalGetReusableBlock: getReusableBlock, } = select( 'core/editor' ); + const { canUser } = select( 'core' ); const blocks = getBlocksByClientId( clientIds ); @@ -80,6 +83,7 @@ export default compose( [ ! isReusableBlock( blocks[ 0 ] ) || ! getReusableBlock( blocks[ 0 ].attributes.ref ) ), + canCreateBlocks: canUser( 'create', 'blocks' ), }; } ), withDispatch( ( dispatch, { clientIds, onToggle = noop } ) => { diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js index 52f2ad08108f93..53d0737abc452e 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js @@ -12,8 +12,8 @@ import { __ } from '@wordpress/i18n'; import { isReusableBlock } from '@wordpress/blocks'; import { withSelect, withDispatch } from '@wordpress/data'; -export function ReusableBlockDeleteButton( { reusableBlock, onDelete } ) { - if ( ! reusableBlock ) { +export function ReusableBlockDeleteButton( { id, isDisabled, onDelete } ) { + if ( ! id ) { return null; } @@ -21,8 +21,8 @@ export function ReusableBlockDeleteButton( { reusableBlock, onDelete } ) { onDelete( reusableBlock.id ) } + disabled={ isDisabled } + onClick={ () => onDelete( id ) } > { __( 'Remove from Reusable Blocks' ) } @@ -35,9 +35,16 @@ export default compose( [ getBlock, __experimentalGetReusableBlock: getReusableBlock, } = select( 'core/editor' ); + const { canUser } = select( 'core' ); + const block = getBlock( clientId ); + + const id = block && isReusableBlock( block ) ? block.attributes.ref : null; return { - reusableBlock: block && isReusableBlock( block ) ? getReusableBlock( block.attributes.ref ) : null, + id, + isDisabled: !! id && ( + getReusableBlock( id ).isTemporary || ! canUser( 'delete', 'blocks', id ) + ), }; } ), withDispatch( ( dispatch, { onToggle = noop } ) => { diff --git a/packages/editor/src/components/block-settings-menu/test/__snapshots__/reusable-block-delete-button.js.snap b/packages/editor/src/components/block-settings-menu/test/__snapshots__/reusable-block-delete-button.js.snap index c31a0f8e9f9b46..23e876d36a9c6d 100644 --- a/packages/editor/src/components/block-settings-menu/test/__snapshots__/reusable-block-delete-button.js.snap +++ b/packages/editor/src/components/block-settings-menu/test/__snapshots__/reusable-block-delete-button.js.snap @@ -3,6 +3,7 @@ exports[`ReusableBlockDeleteButton matches the snapshot 1`] = ` diff --git a/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js index 1aabafc55ef925..13a000306d930a 100644 --- a/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js @@ -28,12 +28,14 @@ describe( 'ReusableBlockConvertButton', () => { ); expect( wrapper.props.children[ 1 ] ).toBeFalsy(); const button = wrapper.props.children[ 0 ]; expect( button.props.children ).toBe( 'Add to Reusable Blocks' ); + expect( button.props.disabled ).toBe( false ); button.props.onClick(); expect( onConvert ).toHaveBeenCalled(); } ); diff --git a/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js b/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js index 9da36d7f24e601..ec1e18959d4e88 100644 --- a/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js +++ b/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js @@ -20,7 +20,8 @@ describe( 'ReusableBlockDeleteButton', () => { const wrapper = getShallowRenderOutput( ); @@ -32,7 +33,8 @@ describe( 'ReusableBlockDeleteButton', () => { const onDelete = jest.fn(); const wrapper = getShallowRenderOutput( ); From 267b9e45a4273bed82ba89f6160e3415e0e39445 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 7 Dec 2018 12:48:19 +1100 Subject: [PATCH 02/16] Expand canUser() selector documentation --- docs/designers-developers/developers/data/data-core.md | 5 +++++ packages/core-data/src/selectors.js | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index 879fd7fbd5ae31..cc2bdd3682711f 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -153,6 +153,11 @@ Upload Permissions. Returns whether the current user can perform the given action on the given REST resource. +Calling this may trigger an OPTIONS request to the REST API via the +`canUser()` resolver. + +https://developer.wordpress.org/rest-api/reference/ + *Parameters* * state: Data state. diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index e5ba53ab5d385f..384e2e602da071 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -185,6 +185,11 @@ export function hasUploadPermissions( state ) { * Returns whether the current user can perform the given action on the given * 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'. From 2a8658affcc67581a164d04df89ea63f7e94ceb1 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 7 Dec 2018 12:52:21 +1100 Subject: [PATCH 03/16] Preload OPTIONS request that determins if user can create a block --- lib/client-assets.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/client-assets.php b/lib/client-assets.php index 42624b3e1c2943..630a50434c72fa 100644 --- a/lib/client-assets.php +++ b/lib/client-assets.php @@ -1086,6 +1086,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) { sprintf( '/wp/v2/types/%s?context=edit', $post_type ), sprintf( '/wp/v2/users/me?post_type=%s&context=edit', $post_type ), array( '/wp/v2/media', 'OPTIONS' ), + array( '/wp/v2/blocks', 'OPTIONS' ), ); /** From 89471ec0672457ad4e8c10c81ab28a52a1506d12 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Fri, 7 Dec 2018 13:48:25 +1100 Subject: [PATCH 04/16] Hide 'Add to Reusable Blocks' and 'Remove from Reusable Block' buttons Hide the 'Add to Reusable Blocks' and 'Remove from Reusable Blocks' buttons instead of disabling them when the user does not have the necessary permissions. --- .../reusable-block-convert-button.js | 53 ++++++++++--------- .../reusable-block-delete-button.js | 25 +++++---- .../test/reusable-block-convert-button.js | 6 +-- .../test/reusable-block-delete-button.js | 14 +++-- 4 files changed, 55 insertions(+), 43 deletions(-) diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js index 75b675e6c404de..3932c7723a6a29 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js @@ -15,8 +15,7 @@ import { compose } from '@wordpress/compose'; export function ReusableBlockConvertButton( { isVisible, - isStaticBlock, - canCreateBlocks, + isReusable, onConvertToStatic, onConvertToReusable, } ) { @@ -26,17 +25,16 @@ export function ReusableBlockConvertButton( { return ( - { isStaticBlock && ( + { ! isReusable && ( { __( 'Add to Reusable Blocks' ) } ) } - { ! isStaticBlock && ( + { isReusable && ( ( - // Guard against the case where a regular block has *just* been converted to a - // reusable block and doesn't yet exist in the editor store. - !! block && - // Only show the option to covert to reusable blocks on valid blocks. - block.isValid && - // Make sure the block supports being converted into a reusable block (by default that is the case). - hasBlockSupport( block.name, 'reusable', true ) - ) ) + const isReusable = ( + blocks.length === 1 && + blocks[ 0 ] && + isReusableBlock( blocks[ 0 ] ) && + !! getReusableBlock( blocks[ 0 ].attributes.ref ) ); return { - isVisible, - isStaticBlock: isVisible && ( - blocks.length !== 1 || - ! isReusableBlock( blocks[ 0 ] ) || - ! getReusableBlock( blocks[ 0 ].attributes.ref ) + // Show 'Convert to Regular Block' when selected block is a reusable block + isVisible: isReusable || ( + // Hide 'Add to Reusable Blocks' when reusable blocks are disabled + canInsertBlockType( 'core/block' ) && + + every( blocks, ( block ) => ( + // Guard against the case where a regular block has *just* been converted + !! block && + + // Hide 'Add to Reusable Blocks' on invalid blocks + block.isValid && + + // Hide 'Add to Reusable Blocks' when block doesn't support being made reusable + hasBlockSupport( block.name, 'reusable', true ) + ) ) && + + // Hide 'Add to Reusable Blocks' when current doesn't have permission to do that + canUser( 'create', 'blocks' ) ), - canCreateBlocks: canUser( 'create', 'blocks' ), + + isReusable, }; } ), withDispatch( ( dispatch, { clientIds, onToggle = noop } ) => { diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js index 53d0737abc452e..0bd65bbaa2b1db 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js @@ -12,8 +12,8 @@ import { __ } from '@wordpress/i18n'; import { isReusableBlock } from '@wordpress/blocks'; import { withSelect, withDispatch } from '@wordpress/data'; -export function ReusableBlockDeleteButton( { id, isDisabled, onDelete } ) { - if ( ! id ) { +export function ReusableBlockDeleteButton( { isVisible, isDisabled, onDelete } ) { + if ( ! isVisible ) { return null; } @@ -22,7 +22,7 @@ export function ReusableBlockDeleteButton( { id, isDisabled, onDelete } ) { className="editor-block-settings-menu__control" icon="no" disabled={ isDisabled } - onClick={ () => onDelete( id ) } + onClick={ () => onDelete() } > { __( 'Remove from Reusable Blocks' ) } @@ -39,21 +39,23 @@ export default compose( [ const block = getBlock( clientId ); - const id = block && isReusableBlock( block ) ? block.attributes.ref : null; + const reusableBlock = block && isReusableBlock( block ) ? + getReusableBlock( block.attributes.ref ) : + null; + return { - id, - isDisabled: !! id && ( - getReusableBlock( id ).isTemporary || ! canUser( 'delete', 'blocks', id ) - ), + isVisible: !! reusableBlock && canUser( 'delete', 'blocks', reusableBlock.id ), + isDisabled: reusableBlock && reusableBlock.isTemporary, }; } ), - withDispatch( ( dispatch, { onToggle = noop } ) => { + withDispatch( ( dispatch, { clientId, onToggle = noop }, { select } ) => { const { __experimentalDeleteReusableBlock: deleteReusableBlock, } = dispatch( 'core/editor' ); + const { getBlock } = select( 'core/editor' ); return { - onDelete( id ) { + onDelete() { // TODO: Make this a component or similar // eslint-disable-next-line no-alert const hasConfirmed = window.confirm( __( @@ -62,7 +64,8 @@ export default compose( [ ) ); if ( hasConfirmed ) { - deleteReusableBlock( id ); + const block = getBlock( clientId ); + deleteReusableBlock( block.attributes.ref ); onToggle(); } }, diff --git a/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js index 13a000306d930a..c6fba313e31b34 100644 --- a/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/test/reusable-block-convert-button.js @@ -27,15 +27,13 @@ describe( 'ReusableBlockConvertButton', () => { const wrapper = getShallowRenderOutput( ); expect( wrapper.props.children[ 1 ] ).toBeFalsy(); const button = wrapper.props.children[ 0 ]; expect( button.props.children ).toBe( 'Add to Reusable Blocks' ); - expect( button.props.disabled ).toBe( false ); button.props.onClick(); expect( onConvert ).toHaveBeenCalled(); } ); @@ -45,7 +43,7 @@ describe( 'ReusableBlockConvertButton', () => { const wrapper = getShallowRenderOutput( ); diff --git a/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js b/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js index ec1e18959d4e88..39299becf29c24 100644 --- a/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js +++ b/packages/editor/src/components/block-settings-menu/test/reusable-block-delete-button.js @@ -16,11 +16,19 @@ describe( 'ReusableBlockDeleteButton', () => { return renderer.getRenderOutput(); } + it( 'should not render when isVisible is false', () => { + const wrapper = getShallowRenderOutput( + + ); + + expect( wrapper ).toBe( null ); + } ); + it( 'matches the snapshot', () => { const wrapper = getShallowRenderOutput( @@ -33,13 +41,13 @@ describe( 'ReusableBlockDeleteButton', () => { const onDelete = jest.fn(); const wrapper = getShallowRenderOutput( ); wrapper.props.onClick(); - expect( onDelete ).toHaveBeenCalledWith( 123 ); + expect( onDelete ).toHaveBeenCalled(); } ); } ); From 710b4b4b11419c6f15b05213cbdaf210df8426b0 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Mon, 24 Dec 2018 15:59:04 +1100 Subject: [PATCH 05/16] Link to the Trac ticket for the REST API bug that we're working around --- packages/core-data/src/resolvers.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index f988e2f14f13fc..849b6c4db634b8 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -137,6 +137,7 @@ export function* canUser( action, resource, id ) { // 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. + // https://core.trac.wordpress.org/ticket/45753 method: id ? 'GET' : 'OPTIONS', parse: false, } ); From 9364beebbce3773c9e293df88e4213240e4cde76 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 10:36:15 +1100 Subject: [PATCH 06/16] End error messages with a period --- packages/core-data/src/resolvers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 849b6c4db634b8..70268c49ab6b2c 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -125,7 +125,7 @@ export function* canUser( action, resource, id ) { const method = methods[ action ]; if ( ! method ) { - throw new Error( `'${ action }' is not a valid action` ); + throw new Error( `'${ action }' is not a valid action.` ); } const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`; From 40938e62b7326b738adf052370ea6d2902a221df Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 10:43:05 +1100 Subject: [PATCH 07/16] s/receiveUserPermissions/receiveUserPermission --- docs/designers-developers/developers/data/data-core.md | 4 ++-- packages/core-data/src/actions.js | 6 +++--- packages/core-data/src/reducer.js | 2 +- packages/core-data/src/resolvers.js | 4 ++-- packages/core-data/src/test/actions.js | 8 ++++---- packages/core-data/src/test/reducer.js | 2 +- packages/core-data/src/test/resolvers.js | 8 ++++---- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index cc2bdd3682711f..eb259f00b01a68 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -237,7 +237,7 @@ Returns an action object used in signalling that Upload permissions have been re * hasUploadPermissions: Does the user have permission to upload files? -### receiveUserPermissions +### receiveUserPermission Returns an action object used in signalling that the current user has permission to perform an action on a REST resource. @@ -245,4 +245,4 @@ permission to perform an action on a REST resource. *Parameters* * key: A key that represents the action and REST resource. - * isAllowed: Whether or not the user can perform the action. \ No newline at end of file + * isAllowed: Whether or not the user can perform the action. diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js index bc6deae1c53eba..c15bd647b9fcd6 100644 --- a/packages/core-data/src/actions.js +++ b/packages/core-data/src/actions.js @@ -137,7 +137,7 @@ export function* saveEntityRecord( kind, name, record ) { */ export function receiveUploadPermissions( hasUploadPermissions ) { return { - type: 'RECEIVE_USER_PERMISSIONS', + type: 'RECEIVE_USER_PERMISSION', key: 'create/media', isAllowed: hasUploadPermissions, }; @@ -152,9 +152,9 @@ export function receiveUploadPermissions( hasUploadPermissions ) { * * @return {Object} Action object. */ -export function receiveUserPermissions( key, isAllowed ) { +export function receiveUserPermission( key, isAllowed ) { return { - type: 'RECEIVE_USER_PERMISSIONS', + type: 'RECEIVE_USER_PERMISSION', key, isAllowed, }; diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js index 676edba2198587..7785667822d68c 100644 --- a/packages/core-data/src/reducer.js +++ b/packages/core-data/src/reducer.js @@ -228,7 +228,7 @@ export function embedPreviews( state = {}, action ) { */ export function userPermissions( state = {}, action ) { switch ( action.type ) { - case 'RECEIVE_USER_PERMISSIONS': + case 'RECEIVE_USER_PERMISSION': return { ...state, [ action.key ]: action.isAllowed, diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 70268c49ab6b2c..0f85c4997b99dd 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -16,7 +16,7 @@ import { receiveEntityRecords, receiveThemeSupports, receiveEmbedPreview, - receiveUserPermissions, + receiveUserPermission, } from './actions'; import { getKindEntities } from './entities'; import { apiFetch } from './controls'; @@ -158,5 +158,5 @@ export function* canUser( action, resource, id ) { const key = compact( [ action, resource, id ] ).join( '/' ); const isAllowed = includes( allowHeader, method ); - yield receiveUserPermissions( key, isAllowed ); + yield receiveUserPermission( key, isAllowed ); } diff --git a/packages/core-data/src/test/actions.js b/packages/core-data/src/test/actions.js index 75cff77676a96f..85c94eeaa52246 100644 --- a/packages/core-data/src/test/actions.js +++ b/packages/core-data/src/test/actions.js @@ -1,7 +1,7 @@ /** * Internal dependencies */ -import { saveEntityRecord, receiveEntityRecords, receiveUserPermissions } from '../actions'; +import { saveEntityRecord, receiveEntityRecords, receiveUserPermission } from '../actions'; describe( 'saveEntityRecord', () => { it( 'triggers a POST request for a new record', async () => { @@ -59,10 +59,10 @@ describe( 'saveEntityRecord', () => { } ); } ); -describe( 'receiveUserPermissions', () => { +describe( 'receiveUserPermission', () => { it( 'builds an action object', () => { - expect( receiveUserPermissions( 'create/media', true ) ).toEqual( { - type: 'RECEIVE_USER_PERMISSIONS', + expect( receiveUserPermission( 'create/media', true ) ).toEqual( { + type: 'RECEIVE_USER_PERMISSION', key: 'create/media', isAllowed: true, } ); diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js index eca3b629581796..51bc4611ad7d98 100644 --- a/packages/core-data/src/test/reducer.js +++ b/packages/core-data/src/test/reducer.js @@ -130,7 +130,7 @@ describe( 'userPermissions()', () => { } ); const state = userPermissions( original, { - type: 'RECEIVE_USER_PERMISSIONS', + type: 'RECEIVE_USER_PERMISSION', key: 'create/media', isAllowed: true, } ); diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 05acabcf32d3a1..325e4ce9c322a3 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -2,7 +2,7 @@ * Internal dependencies */ import { getEntityRecord, getEntityRecords, getEmbedPreview, canUser } from '../resolvers'; -import { receiveEntityRecords, receiveEmbedPreview, receiveUserPermissions } from '../actions'; +import { receiveEntityRecords, receiveEmbedPreview, receiveUserPermission } from '../actions'; import { apiFetch } from '../controls'; describe( 'getEntityRecord', () => { @@ -104,7 +104,7 @@ describe( 'canUser', () => { }, } ); expect( received.done ).toBe( false ); - expect( received.value ).toEqual( receiveUserPermissions( 'create/media', false ) ); + expect( received.value ).toEqual( receiveUserPermission( 'create/media', false ) ); received = generator.next(); expect( received.done ).toBe( true ); @@ -128,7 +128,7 @@ describe( 'canUser', () => { }, } ); expect( received.done ).toBe( false ); - expect( received.value ).toEqual( receiveUserPermissions( 'create/media', true ) ); + expect( received.value ).toEqual( receiveUserPermission( 'create/media', true ) ); received = generator.next(); expect( received.done ).toBe( true ); @@ -152,7 +152,7 @@ describe( 'canUser', () => { }, } ); expect( received.done ).toBe( false ); - expect( received.value ).toEqual( receiveUserPermissions( 'update/blocks/123', true ) ); + expect( received.value ).toEqual( receiveUserPermission( 'update/blocks/123', true ) ); received = generator.next(); expect( received.done ).toBe( true ); From 781de9f73e6fac50d5a36e94a90f26443db8f48b Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 11:49:03 +1100 Subject: [PATCH 08/16] Have canUser() return false by default --- .../developers/data/data-core.md | 25 ++++++++----- packages/core-data/src/selectors.js | 36 ++++++++++++------- packages/core-data/src/test/selectors.js | 11 ++++-- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index eb259f00b01a68..959c8147411756 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -138,15 +138,21 @@ Is the preview for the URL an oEmbed link fallback. ### hasUploadPermissions -Return Upload Permissions. +Returns whether the current user can upload media. + +Calling this may trigger an OPTIONS request ot the REST API via the +`canUser()` resolver. + +https://developer.wordpress.org/rest-api/reference/ *Parameters* - * state: State tree. + * state: Data state. *Returns* -Upload Permissions. +Whether or not the user can upload media. Defaults to `true` if the OPTIONS + request is being made. ### canUser @@ -161,14 +167,17 @@ https://developer.wordpress.org/rest-api/reference/ *Parameters* * state: Data state. - * action: Action to check. One of: 'create', 'read', 'update', - 'delete'. + * action: Action to check. One of: 'create', 'read', 'update', 'delete'. * resource: REST resource to check, e.g. 'media' or 'posts'. - * id: ID of the rest resource to check. + * id: Optional ID of the rest resource to check. + * defaultIsAllowed: What to return when we don't know if the current user can + perform the given action on the given resource. Defaults to + `false`. *Returns* -Whether or not the user can perform the action. +Whether or not the user can perform the action, or `defaultIsAllowed` if the + OPTIONS request is still being made. ## Actions @@ -245,4 +254,4 @@ permission to perform an action on a REST resource. *Parameters* * key: A key that represents the action and REST resource. - * isAllowed: Whether or not the user can perform the action. + * isAllowed: Whether or not the user can perform the action. \ No newline at end of file diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 384e2e602da071..e1a1bf70f9e60c 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -171,14 +171,23 @@ 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 ot the REST API via the + * `canUser()` resolver. + * + * https://developer.wordpress.org/rest-api/reference/ + * + * @deprecated since 4.8. Callers should use the more generic `canUser()` selector instead of + * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. + * + * @param {Object} state Data state. * - * @return {boolean} Upload Permissions. + * @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 canUser( state, 'create', 'media' ); + return canUser( state, 'create', 'media', undefined, true ); } /** @@ -190,15 +199,18 @@ export function hasUploadPermissions( state ) { * * 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 ID of the rest resource to check. + * @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. + * @param {boolean=} defaultIsAllowed What to return when we don't know if the current user can + * perform the given action on the given resource. Defaults to + * `false`. * - * @return {boolean} Whether or not the user can perform the action. + * @return {boolean} Whether or not the user can perform the action, or `defaultIsAllowed` if the + * OPTIONS request is still being made. */ -export function canUser( state, action, resource, id ) { +export function canUser( state, action, resource, id = undefined, defaultIsAllowed = false ) { const key = compact( [ action, resource, id ] ).join( '/' ); - return get( state, [ 'userPermissions', key ], true ); + return get( state, [ 'userPermissions', key ], defaultIsAllowed ); } diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index bce4ae67b693e8..7e96dfcb3e7d6e 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -120,11 +120,18 @@ describe( 'isPreviewEmbedFallback()', () => { } ); describe( 'canUser', () => { - it( 'returns true by default', () => { + it( 'returns false by default', () => { const state = deepFreeze( { userPermissions: {}, } ); - expect( canUser( state, 'create', 'media' ) ).toBe( true ); + expect( canUser( state, 'create', 'media' ) ).toBe( false ); + } ); + + it( 'returns true by default if defaultIsAllowed is specified', () => { + const state = deepFreeze( { + userPermissions: {}, + } ); + expect( canUser( state, 'create', 'media', undefined, true ) ).toBe( true ); } ); it( 'returns whether an action can be performed', () => { From 1890cfcdb4e390560ded0080126207ddd18aa187 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 12:03:04 +1100 Subject: [PATCH 09/16] Move isVisible logic into its own variable --- .../reusable-block-convert-button.js | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js index 3932c7723a6a29..11bfdc752533e1 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js @@ -65,28 +65,29 @@ export default compose( [ !! getReusableBlock( blocks[ 0 ].attributes.ref ) ); - return { - // Show 'Convert to Regular Block' when selected block is a reusable block - isVisible: isReusable || ( - // Hide 'Add to Reusable Blocks' when reusable blocks are disabled - canInsertBlockType( 'core/block' ) && + // Show 'Convert to Regular Block' when selected block is a reusable block + const isVisible = isReusable || ( + // Hide 'Add to Reusable Blocks' when reusable blocks are disabled + canInsertBlockType( 'core/block' ) && - every( blocks, ( block ) => ( - // Guard against the case where a regular block has *just* been converted - !! block && + every( blocks, ( block ) => ( + // Guard against the case where a regular block has *just* been converted + !! block && - // Hide 'Add to Reusable Blocks' on invalid blocks - block.isValid && + // Hide 'Add to Reusable Blocks' on invalid blocks + block.isValid && - // Hide 'Add to Reusable Blocks' when block doesn't support being made reusable - hasBlockSupport( block.name, 'reusable', true ) - ) ) && + // Hide 'Add to Reusable Blocks' when block doesn't support being made reusable + hasBlockSupport( block.name, 'reusable', true ) + ) ) && - // Hide 'Add to Reusable Blocks' when current doesn't have permission to do that - canUser( 'create', 'blocks' ) - ), + // Hide 'Add to Reusable Blocks' when current doesn't have permission to do that + canUser( 'create', 'blocks' ) + ); + return { isReusable, + isVisible, }; } ), withDispatch( ( dispatch, { clientIds, onToggle = noop } ) => { From 23c65aca21c5f6fcf3b1b1ac895ce58632a894f2 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 12:17:31 +1100 Subject: [PATCH 10/16] Add comment explaining why we ignore API errors --- packages/core-data/src/resolvers.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 0f85c4997b99dd..6e6f7aa2cd6f7f 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -142,6 +142,8 @@ export function* canUser( action, resource, id ) { 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; } From d85724ab8c72c0ccefb27bf29f04cc28530d0475 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 3 Jan 2019 12:41:21 +1100 Subject: [PATCH 11/16] Typo: s/4.8/4.9/ --- packages/core-data/src/selectors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index e1a1bf70f9e60c..7e37da20ecd874 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -178,7 +178,7 @@ export function isPreviewEmbedFallback( state, url ) { * * https://developer.wordpress.org/rest-api/reference/ * - * @deprecated since 4.8. Callers should use the more generic `canUser()` selector instead of + * @deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. * * @param {Object} state Data state. From 5546c58ac060c10cd7cf38874a02b8f76fdc8c9b Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Tue, 8 Jan 2019 09:50:24 +1100 Subject: [PATCH 12/16] Add @deprecated comment to hasUploadPermissions() resolver --- packages/core-data/src/resolvers.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 6e6f7aa2cd6f7f..4c4b46c322024a 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -101,6 +101,9 @@ export function* getEmbedPreview( url ) { /** * Requests Upload Permissions from the REST API. + * + * @deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of + * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. */ export function* hasUploadPermissions() { yield* canUser( 'create', 'media' ); From 692e12f17b16722e437c9275a6138fe0199d558f Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 16 Jan 2019 15:24:50 +1100 Subject: [PATCH 13/16] Use deprecated() to mark hasUploadPermissions() as deprecated --- lib/packages-dependencies.php | 1 + packages/core-data/package.json | 1 + packages/core-data/src/resolvers.js | 6 +++++- packages/core-data/src/selectors.js | 4 ++++ packages/editor/src/components/media-placeholder/index.js | 4 ++-- packages/editor/src/components/media-upload/check.js | 4 ++-- 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/packages-dependencies.php b/lib/packages-dependencies.php index f1b785f15b011d..ef1b2bc22cdb90 100644 --- a/lib/packages-dependencies.php +++ b/lib/packages-dependencies.php @@ -84,6 +84,7 @@ 'lodash', 'wp-api-fetch', 'wp-data', + 'wp-deprecated', 'wp-url', ), 'wp-data' => array( diff --git a/packages/core-data/package.json b/packages/core-data/package.json index b28d5c2eb0c24f..388ab7b45e8f0a 100644 --- a/packages/core-data/package.json +++ b/packages/core-data/package.json @@ -24,6 +24,7 @@ "@babel/runtime": "^7.0.0", "@wordpress/api-fetch": "file:../api-fetch", "@wordpress/data": "file:../data", + "@wordpress/deprecated": "file:../deprecated", "@wordpress/url": "file:../url", "equivalent-key-map": "^0.2.2", "lodash": "^4.17.10", diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 4c4b46c322024a..0c2bef3655c8d8 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -7,6 +7,7 @@ import { find, includes, get, hasIn, compact } from 'lodash'; * WordPress dependencies */ import { addQueryArgs } from '@wordpress/url'; +import deprecated from '@wordpress/deprecated'; /** * Internal dependencies @@ -103,9 +104,12 @@ export function* getEmbedPreview( url ) { * Requests Upload Permissions from the REST API. * * @deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of - * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. + * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. */ export function* hasUploadPermissions() { + deprecated( "select( 'core' ).hasUploadPermissions()", { + alternative: "select( 'core' ).canUser( 'create', 'media' )", + } ); yield* canUser( 'create', 'media' ); } diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 7e37da20ecd874..21bf90f908c720 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -8,6 +8,7 @@ import { map, find, get, filter, compact } from 'lodash'; * WordPress dependencies */ import { select } from '@wordpress/data'; +import deprecated from '@wordpress/deprecated'; /** * Internal dependencies @@ -187,6 +188,9 @@ export function isPreviewEmbedFallback( state, url ) { * request is being made. */ export function hasUploadPermissions( state ) { + deprecated( "select( 'core' ).hasUploadPermissions()", { + alternative: "select( 'core' ).canUser( 'create', 'media' )", + } ); return canUser( state, 'create', 'media', undefined, true ); } diff --git a/packages/editor/src/components/media-placeholder/index.js b/packages/editor/src/components/media-placeholder/index.js index c1b64eb0d9a64a..e85d4e9d9d8223 100644 --- a/packages/editor/src/components/media-placeholder/index.js +++ b/packages/editor/src/components/media-placeholder/index.js @@ -258,10 +258,10 @@ export class MediaPlaceholder extends Component { } const applyWithSelect = withSelect( ( select ) => { - const { hasUploadPermissions } = select( 'core' ); + const { canUser } = select( 'core' ); return { - hasUploadPermissions: hasUploadPermissions(), + hasUploadPermissions: canUser( 'create', 'media', undefined, true ), }; } ); diff --git a/packages/editor/src/components/media-upload/check.js b/packages/editor/src/components/media-upload/check.js index d72b48497e0afb..a5832a6368f43b 100644 --- a/packages/editor/src/components/media-upload/check.js +++ b/packages/editor/src/components/media-upload/check.js @@ -8,9 +8,9 @@ export function MediaUploadCheck( { hasUploadPermissions, fallback = null, child } export default withSelect( ( select ) => { - const { hasUploadPermissions } = select( 'core' ); + const { canUser } = select( 'core' ); return { - hasUploadPermissions: hasUploadPermissions(), + hasUploadPermissions: canUser( 'create', 'media', undefined, true ), }; } )( MediaUploadCheck ); From 8797da342e6fc56e7079f7c0cf7ac95e70958624 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 23 Jan 2019 14:36:30 +1100 Subject: [PATCH 14/16] Typo: s/ot/to/ --- docs/designers-developers/developers/data/data-core.md | 2 +- packages/core-data/src/selectors.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index 959c8147411756..1d96f05468230d 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -140,7 +140,7 @@ Is the preview for the URL an oEmbed link fallback. Returns whether the current user can upload media. -Calling this may trigger an OPTIONS request ot the REST API via the +Calling this may trigger an OPTIONS request to the REST API via the `canUser()` resolver. https://developer.wordpress.org/rest-api/reference/ diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 21bf90f908c720..4d614de3433454 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -174,7 +174,7 @@ export function isPreviewEmbedFallback( state, url ) { /** * Returns whether the current user can upload media. * - * Calling this may trigger an OPTIONS request ot the REST API via the + * Calling this may trigger an OPTIONS request to the REST API via the * `canUser()` resolver. * * https://developer.wordpress.org/rest-api/reference/ From 7b8d040a9c96e92717b9ee163ae2569f5a3a4a7f Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 23 Jan 2019 15:28:21 +1100 Subject: [PATCH 15/16] Have `canUser()` return `undefined` by default --- .../developers/data/data-core.md | 14 ++++++++------ packages/block-library/src/block/edit.js | 2 +- packages/core-data/src/selectors.js | 15 ++++++--------- packages/core-data/src/test/selectors.js | 11 ++--------- .../reusable-block-convert-button.js | 2 +- .../reusable-block-delete-button.js | 2 +- .../src/components/media-placeholder/index.js | 4 ++-- .../editor/src/components/media-upload/check.js | 7 ++++++- 8 files changed, 27 insertions(+), 30 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index 1d96f05468230d..b556767e30c40d 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -136,7 +136,7 @@ get back from the oEmbed preview API. Is the preview for the URL an oEmbed link fallback. -### hasUploadPermissions +### hasUploadPermissions (deprecated) Returns whether the current user can upload media. @@ -145,6 +145,11 @@ Calling this may trigger an OPTIONS request to the REST API via the https://developer.wordpress.org/rest-api/reference/ +*Deprecated* + +Deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of + `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. + *Parameters* * state: Data state. @@ -170,14 +175,11 @@ https://developer.wordpress.org/rest-api/reference/ * action: Action to check. One of: 'create', 'read', 'update', 'delete'. * resource: REST resource to check, e.g. 'media' or 'posts'. * id: Optional ID of the rest resource to check. - * defaultIsAllowed: What to return when we don't know if the current user can - perform the given action on the given resource. Defaults to - `false`. *Returns* -Whether or not the user can perform the action, or `defaultIsAllowed` if the - OPTIONS request is still being made. +Whether or not the user can perform the action, + or `undefined` if the OPTIONS request is still being made. ## Actions diff --git a/packages/block-library/src/block/edit.js b/packages/block-library/src/block/edit.js index 80f1c16be71699..62e4acf0640d15 100644 --- a/packages/block-library/src/block/edit.js +++ b/packages/block-library/src/block/edit.js @@ -162,7 +162,7 @@ export default compose( [ isFetching: isFetchingReusableBlock( ref ), isSaving: isSavingReusableBlock( ref ), block: reusableBlock ? getBlock( reusableBlock.clientId ) : null, - canUpdateBlock: !! reusableBlock && ! reusableBlock.isTemporary && canUser( 'update', 'blocks', ref ), + canUpdateBlock: !! reusableBlock && ! reusableBlock.isTemporary && !! canUser( 'update', 'blocks', ref ), }; } ), withDispatch( ( dispatch, ownProps ) => { diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 4d614de3433454..6b84abe5dd216f 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -2,7 +2,7 @@ * External dependencies */ import createSelector from 'rememo'; -import { map, find, get, filter, compact } from 'lodash'; +import { map, find, get, filter, compact, defaultTo } from 'lodash'; /** * WordPress dependencies @@ -191,7 +191,7 @@ export function hasUploadPermissions( state ) { deprecated( "select( 'core' ).hasUploadPermissions()", { alternative: "select( 'core' ).canUser( 'create', 'media' )", } ); - return canUser( state, 'create', 'media', undefined, true ); + return defaultTo( canUser( state, 'create', 'media' ), true ); } /** @@ -207,14 +207,11 @@ export function hasUploadPermissions( 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. - * @param {boolean=} defaultIsAllowed What to return when we don't know if the current user can - * perform the given action on the given resource. Defaults to - * `false`. * - * @return {boolean} Whether or not the user can perform the action, or `defaultIsAllowed` if the - * OPTIONS request is still being made. + * @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 = undefined, defaultIsAllowed = false ) { +export function canUser( state, action, resource, id ) { const key = compact( [ action, resource, id ] ).join( '/' ); - return get( state, [ 'userPermissions', key ], defaultIsAllowed ); + return get( state, [ 'userPermissions', key ] ); } diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 7e96dfcb3e7d6e..f2a2885e776628 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -120,18 +120,11 @@ describe( 'isPreviewEmbedFallback()', () => { } ); describe( 'canUser', () => { - it( 'returns false by default', () => { + it( 'returns undefined by default', () => { const state = deepFreeze( { userPermissions: {}, } ); - expect( canUser( state, 'create', 'media' ) ).toBe( false ); - } ); - - it( 'returns true by default if defaultIsAllowed is specified', () => { - const state = deepFreeze( { - userPermissions: {}, - } ); - expect( canUser( state, 'create', 'media', undefined, true ) ).toBe( true ); + expect( canUser( state, 'create', 'media' ) ).toBe( undefined ); } ); it( 'returns whether an action can be performed', () => { diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js index 11bfdc752533e1..2e528a01e7d911 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-convert-button.js @@ -82,7 +82,7 @@ export default compose( [ ) ) && // Hide 'Add to Reusable Blocks' when current doesn't have permission to do that - canUser( 'create', 'blocks' ) + !! canUser( 'create', 'blocks' ) ); return { diff --git a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js index 0bd65bbaa2b1db..5a73912b36ee7a 100644 --- a/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js +++ b/packages/editor/src/components/block-settings-menu/reusable-block-delete-button.js @@ -44,7 +44,7 @@ export default compose( [ null; return { - isVisible: !! reusableBlock && canUser( 'delete', 'blocks', reusableBlock.id ), + isVisible: !! reusableBlock && !! canUser( 'delete', 'blocks', reusableBlock.id ), isDisabled: reusableBlock && reusableBlock.isTemporary, }; } ), diff --git a/packages/editor/src/components/media-placeholder/index.js b/packages/editor/src/components/media-placeholder/index.js index e85d4e9d9d8223..11b80bcc58b0f6 100644 --- a/packages/editor/src/components/media-placeholder/index.js +++ b/packages/editor/src/components/media-placeholder/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { every, get, noop, startsWith } from 'lodash'; +import { every, get, noop, startsWith, defaultTo } from 'lodash'; import classnames from 'classnames'; /** @@ -261,7 +261,7 @@ const applyWithSelect = withSelect( ( select ) => { const { canUser } = select( 'core' ); return { - hasUploadPermissions: canUser( 'create', 'media', undefined, true ), + hasUploadPermissions: defaultTo( canUser( 'create', 'media' ), true ), }; } ); diff --git a/packages/editor/src/components/media-upload/check.js b/packages/editor/src/components/media-upload/check.js index a5832a6368f43b..5dde4c69fa827c 100644 --- a/packages/editor/src/components/media-upload/check.js +++ b/packages/editor/src/components/media-upload/check.js @@ -1,3 +1,8 @@ +/** + * External dependencies + */ +import { defaultTo } from 'lodash'; + /** * WordPress dependencies */ @@ -11,6 +16,6 @@ export default withSelect( ( select ) => { const { canUser } = select( 'core' ); return { - hasUploadPermissions: canUser( 'create', 'media', undefined, true ), + hasUploadPermissions: defaultTo( canUser( 'create', 'media' ), true ), }; } )( MediaUploadCheck ); From d6a6b1ce31174478f19780c6bcb9fdaf28a591a5 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 23 Jan 2019 16:29:00 +1100 Subject: [PATCH 16/16] Update version: s/4.9/5.0/ --- docs/designers-developers/developers/data/data-core.md | 2 +- packages/core-data/src/resolvers.js | 2 +- packages/core-data/src/selectors.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core.md b/docs/designers-developers/developers/data/data-core.md index b556767e30c40d..9b01fe325c295b 100644 --- a/docs/designers-developers/developers/data/data-core.md +++ b/docs/designers-developers/developers/data/data-core.md @@ -147,7 +147,7 @@ https://developer.wordpress.org/rest-api/reference/ *Deprecated* -Deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of +Deprecated since 5.0. Callers should use the more generic `canUser()` selector instead of `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. *Parameters* diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 0c2bef3655c8d8..009fe1b7d5b42d 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -103,7 +103,7 @@ export function* getEmbedPreview( url ) { /** * Requests Upload Permissions from the REST API. * - * @deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of + * @deprecated since 5.0. Callers should use the more generic `canUser()` selector instead of * `hasUploadPermissions()`, e.g. `canUser( 'create', 'media' )`. */ export function* hasUploadPermissions() { diff --git a/packages/core-data/src/selectors.js b/packages/core-data/src/selectors.js index 6b84abe5dd216f..b9ed8b575e0863 100644 --- a/packages/core-data/src/selectors.js +++ b/packages/core-data/src/selectors.js @@ -179,7 +179,7 @@ export function isPreviewEmbedFallback( state, url ) { * * https://developer.wordpress.org/rest-api/reference/ * - * @deprecated since 4.9. Callers should use the more generic `canUser()` selector instead of + * @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.