From cbcc61e26bd03a3f473dceb90c885d03ecd2fc55 Mon Sep 17 00:00:00 2001 From: Kelly Dwan Date: Thu, 2 Jul 2020 19:00:20 -0400 Subject: [PATCH] Block Directory: Remove custom permission check in favor of `canUser` (#23624) * Block Directory: Switch to `canUser` helper * Remove custom permissions checks * Remove mocked OPTIONS response, only mock the GET requests This lets the permissions request fall through to the site's API, which should give a correct reply (the previous issue was that the empty result was overriding all methods, making it seem like the permission check failed). * Show the permission error only when hasPermission is known When the request is pending, `hasPermission` is undefined. --- .../downloadable-blocks-panel/index.js | 10 +++-- packages/block-directory/src/store/actions.js | 12 ----- packages/block-directory/src/store/reducer.js | 17 ------- .../block-directory/src/store/resolvers.js | 39 ++-------------- .../block-directory/src/store/selectors.js | 11 ----- .../block-directory/src/store/test/reducer.js | 18 +------- .../plugins/block-directory-add.test.js | 44 +++---------------- 7 files changed, 16 insertions(+), 135 deletions(-) diff --git a/packages/block-directory/src/components/downloadable-blocks-panel/index.js b/packages/block-directory/src/components/downloadable-blocks-panel/index.js index df2060eb265816..e78077c99e47f5 100644 --- a/packages/block-directory/src/components/downloadable-blocks-panel/index.js +++ b/packages/block-directory/src/components/downloadable-blocks-panel/index.js @@ -21,7 +21,7 @@ function DownloadableBlocksPanel( { isWaiting, debouncedSpeak, } ) { - if ( ! hasPermission ) { + if ( false === hasPermission ) { debouncedSpeak( __( 'No blocks found in your library. Please contact your site administrator to install new blocks.' @@ -38,7 +38,7 @@ function DownloadableBlocksPanel( { ); } - if ( isLoading || isWaiting ) { + if ( typeof hasPermission === 'undefined' || isLoading || isWaiting ) { return (

@@ -86,11 +86,13 @@ export default compose( [ withSelect( ( select, { filterValue } ) => { const { getDownloadableBlocks, - hasInstallBlocksPermission, isRequestingDownloadableBlocks, } = select( 'core/block-directory' ); - const hasPermission = hasInstallBlocksPermission(); + const hasPermission = select( 'core' ).canUser( + 'read', + 'block-directory/search' + ); const downloadableItems = hasPermission ? getDownloadableBlocks( filterValue ) : []; diff --git a/packages/block-directory/src/store/actions.js b/packages/block-directory/src/store/actions.js index 7dc899ebf7e1da..bb98dc483d3098 100644 --- a/packages/block-directory/src/store/actions.js +++ b/packages/block-directory/src/store/actions.js @@ -38,18 +38,6 @@ export function receiveDownloadableBlocks( downloadableBlocks, filterValue ) { }; } -/** - * Returns an action object used in signalling that the user does not have - * permission to install blocks. - * - * @param {boolean} hasPermission User has permission to install blocks. - * - * @return {Object} Action object. - */ -export function setInstallBlocksPermission( hasPermission ) { - return { type: 'SET_INSTALL_BLOCKS_PERMISSION', hasPermission }; -} - /** * Action triggered to install a block plugin. * diff --git a/packages/block-directory/src/store/reducer.js b/packages/block-directory/src/store/reducer.js index fabc863e72a482..37b75b59d1e965 100644 --- a/packages/block-directory/src/store/reducer.js +++ b/packages/block-directory/src/store/reducer.js @@ -75,22 +75,6 @@ export const blockManagement = ( return state; }; -/** - * Reducer returning an array of downloadable blocks. - * - * @param {Object} state Current state. - * @param {Object} action Dispatched action. - * - * @return {Object} Updated state. - */ -export function hasPermission( state = true, action ) { - if ( action.type === 'SET_INSTALL_BLOCKS_PERMISSION' ) { - return action.hasPermission; - } - - return state; -} - /** * Reducer returning an object of error notices. * @@ -113,6 +97,5 @@ export const errorNotices = ( state = {}, action ) => { export default combineReducers( { downloadableBlocks, blockManagement, - hasPermission, errorNotices, } ); diff --git a/packages/block-directory/src/store/resolvers.js b/packages/block-directory/src/store/resolvers.js index e5724e87ff76c9..98c7ae64bb7766 100644 --- a/packages/block-directory/src/store/resolvers.js +++ b/packages/block-directory/src/store/resolvers.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { camelCase, get, hasIn, includes, mapKeys } from 'lodash'; +import { camelCase, mapKeys } from 'lodash'; /** * WordPress dependencies @@ -11,11 +11,7 @@ import { apiFetch } from '@wordpress/data-controls'; /** * Internal dependencies */ -import { - fetchDownloadableBlocks, - receiveDownloadableBlocks, - setInstallBlocksPermission, -} from './actions'; +import { fetchDownloadableBlocks, receiveDownloadableBlocks } from './actions'; export default { *getDownloadableBlocks( filterValue ) { @@ -35,35 +31,6 @@ export default { ); yield receiveDownloadableBlocks( blocks, filterValue ); - } catch ( error ) { - if ( error.code === 'rest_block_directory_cannot_view' ) { - yield setInstallBlocksPermission( false ); - } - } - }, - *hasInstallBlocksPermission() { - try { - const response = yield apiFetch( { - method: 'OPTIONS', - path: `wp/v2/block-directory/search`, - parse: false, - } ); - - let allowHeader; - if ( hasIn( response, [ 'headers', 'get' ] ) ) { - // If the request is fetched using the fetch api, the header can be - // retrieved using the 'get' method. - allowHeader = response.headers.get( 'allow' ); - } else { - // If the request was preloaded server-side and is returned by the - // preloading middleware, the header will be a simple property. - allowHeader = get( response, [ 'headers', 'Allow' ], '' ); - } - - const isAllowed = includes( allowHeader, 'GET' ); - yield setInstallBlocksPermission( isAllowed ); - } catch ( error ) { - yield setInstallBlocksPermission( false ); - } + } catch ( error ) {} }, }; diff --git a/packages/block-directory/src/store/selectors.js b/packages/block-directory/src/store/selectors.js index 1e9e93b31bbb37..02dac1432d09c2 100644 --- a/packages/block-directory/src/store/selectors.js +++ b/packages/block-directory/src/store/selectors.js @@ -45,17 +45,6 @@ export function getDownloadableBlocks( state, filterValue ) { return state.downloadableBlocks[ filterValue ].results; } -/** - * Returns true if user has permission to install blocks. - * - * @param {Object} state Global application state. - * - * @return {boolean} User has permission to install blocks. - */ -export function hasInstallBlocksPermission( state ) { - return state.hasPermission; -} - /** * Returns the block types that have been installed on the server. * diff --git a/packages/block-directory/src/store/test/reducer.js b/packages/block-directory/src/store/test/reducer.js index 14ccd5e2eb66c0..f18335916b2335 100644 --- a/packages/block-directory/src/store/test/reducer.js +++ b/packages/block-directory/src/store/test/reducer.js @@ -6,12 +6,7 @@ import deepFreeze from 'deep-freeze'; /** * Internal dependencies */ -import { - blockManagement, - downloadableBlocks, - errorNotices, - hasPermission, -} from '../reducer'; +import { blockManagement, downloadableBlocks, errorNotices } from '../reducer'; import { blockTypeInstalled, downloadableBlock } from './fixtures'; describe( 'state', () => { @@ -89,17 +84,6 @@ describe( 'state', () => { } ); } ); - describe( 'hasPermission()', () => { - it( 'should update permissions appropriately', () => { - const state = hasPermission( true, { - type: 'SET_INSTALL_BLOCKS_PERMISSION', - hasPermission: false, - } ); - - expect( state ).toEqual( false ); - } ); - } ); - describe( 'errorNotices()', () => { it( 'should set an error notice', () => { const initialState = deepFreeze( {} ); diff --git a/packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js b/packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js index 697d51fb2863b5..249e5bba64598b 100644 --- a/packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js +++ b/packages/e2e-tests/specs/editor/plugins/block-directory-add.test.js @@ -83,53 +83,21 @@ const block = `( function() { } ); } )();`; -const MOCK_OPTIONS = { - namespace: 'wp/v2', - methods: [ 'GET' ], - endpoints: [ - { - methods: [ 'GET' ], - args: {}, - }, - ], - schema: { - $schema: 'http://json-schema.org/draft-04/schema#', - title: 'block-directory-item', - type: 'object', - properties: {}, - }, -}; - -const MOCK_OPTIONS_RESPONSE = { - match: ( request ) => - matchUrl( request.url(), SEARCH_URLS ) && - request.method() === 'OPTIONS', - onRequestMatch: async ( request ) => { - const response = { - content: 'application/json', - body: JSON.stringify( MOCK_OPTIONS ), - headers: { - Allow: 'GET', - }, - }; - - return request.respond( response ); - }, -}; - const MOCK_EMPTY_RESPONSES = [ - MOCK_OPTIONS_RESPONSE, { - match: ( request ) => matchUrl( request.url(), SEARCH_URLS ), + match: ( request ) => + matchUrl( request.url(), SEARCH_URLS ) && + request.method() === 'GET', onRequestMatch: createJSONResponse( [] ), }, ]; const MOCK_BLOCKS_RESPONSES = [ - MOCK_OPTIONS_RESPONSE, { // Mock response for search with the block - match: ( request ) => matchUrl( request.url(), SEARCH_URLS ), + match: ( request ) => + matchUrl( request.url(), SEARCH_URLS ) && + request.method() === 'GET', onRequestMatch: createJSONResponse( [ MOCK_BLOCK1, MOCK_BLOCK2 ] ), }, {