From e234570e16541bcef25ad4fef946cb5ded830b22 Mon Sep 17 00:00:00 2001 From: Jorge Costa Date: Wed, 3 Apr 2019 15:44:01 +0100 Subject: [PATCH] Fix: Restrict block insert, move, and replace attending to allowedBlocks, locking, and child blocks (#14003) Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files Uses the select control implemented in https://github.com/WordPress/gutenberg/pull/13924. This PR changes the action creators to be a generator and have all the required validations before yielding the action. Fixes: https://github.com/WordPress/gutenberg/issues/14568; Fixes: https://github.com/WordPress/gutenberg/issues/10186; Fixes: https://github.com/WordPress/gutenberg/issues/13099; Missing the update to the unit tests. ## How has this been tested? Test block: [gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js](https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js) I pasted the test block referred in the browser console. I added the Product block. I verified I cannot move using drag & drop the Buy button out of the Product block. (child block restriction. I verified I cannot move using drag & drop a paragraph created outside of the Product block inside the product block. (allowed blocks restriction). I verified that if I press enter in the buy button or image inside the product block, a paragraph is not created (allowed blocks restriction). I added two galleries outside the product block, multi-selected them, and copied them ( ctrl + c ), I pasted them inside the list block inside the Product block and I verified they were not added (allowed blocks restriction). I verified that when we have a template lock and we press enter on the title we now do not insert unallowed blocks. Template lock code: ``` add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) { if ( $post->post_type === 'post' ) { return $allowed_block_types; } return [ 'core/image', 'core/button' ]; }, 10, 2 ); ``` I verified that if we disable the paragraph, e.g., with a filter or inside the test block, pressing enter on the title or in placeholders does not add paragraph blocks. Pressing the sibling inserter also does not insert the paragraph (PR #7226). Code to disable paragraph: ``` function myplugin_register_book_lock_post_type() { $args = array( 'public' => true, 'template_lock' => 'all', 'label' => 'Books Lock', 'show_in_rest' => true, 'template' => array( array( 'core/image', array( ) ), array( 'core/heading', array( 'placeholder' => 'Add Author...', ) ), array( 'core/paragraph', array( 'placeholder' => 'Add Description...', ) ), ), ); register_post_type( 'book_lock', $args ); } add_action( 'init', 'myplugin_register_book_lock_post_type' ); ``` I added two image blocks, multi-selected them, copied them and tried to paste them on the list inside the test block. I checked the image blocks were not added inside the test block. All these actions are possible in master. Todo: If we accept this approach, end 2 end tests will be created before the merge that replicate this manual tests. --- packages/block-editor/src/store/actions.js | 158 ++++++++-- packages/block-editor/src/store/effects.js | 22 -- .../block-editor/src/store/test/actions.js | 286 ++++++++++++++++-- .../block-editor/src/store/test/effects.js | 42 ++- 4 files changed, 417 insertions(+), 91 deletions(-) diff --git a/packages/block-editor/src/store/actions.js b/packages/block-editor/src/store/actions.js index d0040b2212b76e..16910972b1f12b 100644 --- a/packages/block-editor/src/store/actions.js +++ b/packages/block-editor/src/store/actions.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { castArray } from 'lodash'; +import { castArray, first } from 'lodash'; /** * WordPress dependencies @@ -13,6 +13,25 @@ import { getDefaultBlockName, createBlock } from '@wordpress/blocks'; */ import { select } from './controls'; +/** + * Generator which will yield a default block insert action if there + * are no other blocks at the root of the editor. This generator should be used + * in actions which may result in no blocks remaining in the editor (removal, + * replacement, etc). + */ +function* ensureDefaultBlock() { + const count = yield select( + 'core/block-editor', + 'getBlockCount', + ); + + // To avoid a focus loss when removing the last block, assure there is + // always a default block if the last of the blocks have been removed. + if ( count === 0 ) { + yield insertDefaultBlock(); + } +} + /** * Returns an action object used in signalling that blocks state should be * reset to the specified array of blocks, taking precedence over any other @@ -202,15 +221,36 @@ export function toggleSelection( isSelectionEnabled = true ) { * @param {(string|string[])} clientIds Block client ID(s) to replace. * @param {(Object|Object[])} blocks Replacement block(s). * - * @return {Object} Action object. + * @yields {Object} Action object. */ -export function replaceBlocks( clientIds, blocks ) { - return { +export function* replaceBlocks( clientIds, blocks ) { + clientIds = castArray( clientIds ); + blocks = castArray( blocks ); + const rootClientId = yield select( + 'core/block-editor', + 'getBlockRootClientId', + first( clientIds ) + ); + // Replace is valid if the new blocks can be inserted in the root block. + for ( let index = 0; index < blocks.length; index++ ) { + const block = blocks[ index ]; + const canInsertBlock = yield select( + 'core/block-editor', + 'canInsertBlockType', + block.name, + rootClientId + ); + if ( ! canInsertBlock ) { + return; + } + } + yield { type: 'REPLACE_BLOCKS', - clientIds: castArray( clientIds ), - blocks: castArray( blocks ), + clientIds, + blocks, time: Date.now(), }; + yield* ensureDefaultBlock(); } /** @@ -256,16 +296,51 @@ export const moveBlocksUp = createOnMove( 'MOVE_BLOCKS_UP' ); * @param {?string} toRootClientId Root client ID destination. * @param {number} index The index to move the block into. * - * @return {Object} Action object. + * @yields {Object} Action object. */ -export function moveBlockToPosition( clientId, fromRootClientId, toRootClientId, index ) { - return { +export function* moveBlockToPosition( clientId, fromRootClientId, toRootClientId, index ) { + const templateLock = yield select( + 'core/block-editor', + 'getTemplateLock', + fromRootClientId + ); + + // If locking is equal to all on the original clientId (fromRootClientId), + // it is not possible to move the block to any other position. + if ( templateLock === 'all' ) { + return; + } + + const action = { type: 'MOVE_BLOCK_TO_POSITION', fromRootClientId, toRootClientId, clientId, index, }; + // If moving inside the same root block the move is always possible. + if ( fromRootClientId === toRootClientId ) { + yield action; + return; + } + + const blockName = yield select( + 'core/block-editor', + 'getBlockName', + clientId + ); + + const canInsertBlock = yield select( + 'core/block-editor', + 'canInsertBlockType', + blockName, + toRootClientId + ); + + // If moving to other parent block, the move is possible if we can insert a block of the same type inside the new parent block. + if ( canInsertBlock ) { + yield action; + } } /** @@ -279,8 +354,18 @@ export function moveBlockToPosition( clientId, fromRootClientId, toRootClientId, * * @return {Object} Action object. */ -export function insertBlock( block, index, rootClientId, updateSelection = true ) { - return insertBlocks( [ block ], index, rootClientId, updateSelection ); +export function insertBlock( + block, + index, + rootClientId, + updateSelection = true, +) { + return insertBlocks( + [ block ], + index, + rootClientId, + updateSelection + ); } /** @@ -292,17 +377,37 @@ export function insertBlock( block, index, rootClientId, updateSelection = true * @param {?string} rootClientId Optional root client ID of block list on which to insert. * @param {?boolean} updateSelection If true block selection will be updated. If false, block selection will not change. Defaults to true. * - * @return {Object} Action object. - */ -export function insertBlocks( blocks, index, rootClientId, updateSelection = true ) { - return { - type: 'INSERT_BLOCKS', - blocks: castArray( blocks ), - index, - rootClientId, - time: Date.now(), - updateSelection, - }; + * @return {Object} Action object. + */ +export function* insertBlocks( + blocks, + index, + rootClientId, + updateSelection = true +) { + blocks = castArray( blocks ); + const allowedBlocks = []; + for ( const block of blocks ) { + const isValid = yield select( + 'core/block-editor', + 'canInsertBlockType', + block.name, + rootClientId + ); + if ( isValid ) { + allowedBlocks.push( block ); + } + } + if ( allowedBlocks.length ) { + return { + type: 'INSERT_BLOCKS', + blocks: allowedBlocks, + index, + rootClientId, + time: Date.now(), + updateSelection, + }; + } } /** @@ -394,16 +499,9 @@ export function* removeBlocks( clientIds, selectPrevious = true ) { clientIds, }; - const count = yield select( - 'core/block-editor', - 'getBlockCount', - ); - // To avoid a focus loss when removing the last block, assure there is // always a default block if the last of the blocks have been removed. - if ( count === 0 ) { - yield insertDefaultBlock(); - } + yield* ensureDefaultBlock(); } /** diff --git a/packages/block-editor/src/store/effects.js b/packages/block-editor/src/store/effects.js index ca46e1afb8f4a6..d61ce67fe68df8 100644 --- a/packages/block-editor/src/store/effects.js +++ b/packages/block-editor/src/store/effects.js @@ -17,14 +17,12 @@ import { replaceBlocks, selectBlock, setTemplateValidity, - insertDefaultBlock, resetBlocks, } from './actions'; import { getBlock, getBlocks, getSelectedBlockCount, - getBlockCount, getTemplateLock, getTemplate, isValidTemplate, @@ -60,23 +58,6 @@ export function validateBlocksToTemplate( action, store ) { } } -/** - * Effect handler which will return a default block insertion action if there - * are no other blocks at the root of the editor. This is expected to be used - * in actions which may result in no blocks remaining in the editor (removal, - * replacement, etc). - * - * @param {Object} action Action which had initiated the effect handler. - * @param {Object} store Store instance. - * - * @return {?Object} Default block insert action, if no other blocks exist. - */ -export function ensureDefaultBlock( action, store ) { - if ( ! getBlockCount( store.getState() ) ) { - return insertDefaultBlock(); - } -} - export default { MERGE_BLOCKS( action, store ) { const { dispatch } = store; @@ -127,9 +108,6 @@ export default { RESET_BLOCKS: [ validateBlocksToTemplate, ], - REPLACE_BLOCKS: [ - ensureDefaultBlock, - ], MULTI_SELECT: ( action, { getState } ) => { const blockCount = getSelectedBlockCount( getState() ); diff --git a/packages/block-editor/src/store/test/actions.js b/packages/block-editor/src/store/test/actions.js index 225d36152e66a8..1fb97cd38d9bde 100644 --- a/packages/block-editor/src/store/test/actions.js +++ b/packages/block-editor/src/store/test/actions.js @@ -117,65 +117,305 @@ describe( 'actions', () => { } ); describe( 'replaceBlock', () => { - it( 'should return the REPLACE_BLOCKS action', () => { + it( 'should yield the REPLACE_BLOCKS action if the new block can be inserted in the destination root block', () => { const block = { clientId: 'ribs', + name: 'core/test-block', }; - expect( replaceBlock( [ 'chicken' ], block ) ).toEqual( { + const replaceBlockGenerator = replaceBlock( 'chicken', block ); + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'chicken' ], + selectorName: 'getBlockRootClientId', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'core/test-block', undefined ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( true ).value, + ).toEqual( { type: 'REPLACE_BLOCKS', clientIds: [ 'chicken' ], blocks: [ block ], time: expect.any( Number ), } ); + + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [], + selectorName: 'getBlockCount', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( 1 ), + ).toEqual( { + value: undefined, + done: true, + } ); } ); } ); describe( 'replaceBlocks', () => { - it( 'should return the REPLACE_BLOCKS action', () => { + it( 'should not yield the REPLACE_BLOCKS action if the replacement is not possible', () => { + const blocks = [ { + clientId: 'ribs', + name: 'core/test-ribs', + }, { + clientId: 'chicken', + name: 'core/test-chicken', + } ]; + + const replaceBlockGenerator = replaceBlocks( [ 'chicken' ], blocks ); + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'chicken' ], + selectorName: 'getBlockRootClientId', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'core/test-ribs', undefined ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( true ).value, + ).toEqual( { + args: [ 'core/test-chicken', undefined ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( false ), + ).toEqual( { + value: undefined, + done: true, + } ); + } ); + + it( 'should yield the REPLACE_BLOCKS action if the all the replacement blocks can be inserted in the parent block', () => { const blocks = [ { clientId: 'ribs', + name: 'core/test-ribs', + }, { + clientId: 'chicken', + name: 'core/test-chicken', } ]; - expect( replaceBlocks( [ 'chicken' ], blocks ) ).toEqual( { + const replaceBlockGenerator = replaceBlocks( [ 'chicken' ], blocks ); + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'chicken' ], + selectorName: 'getBlockRootClientId', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [ 'core/test-ribs', undefined ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( true ).value, + ).toEqual( { + args: [ 'core/test-chicken', undefined ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( true ).value, + ).toEqual( { type: 'REPLACE_BLOCKS', clientIds: [ 'chicken' ], blocks, time: expect.any( Number ), } ); + + expect( + replaceBlockGenerator.next().value, + ).toEqual( { + args: [], + selectorName: 'getBlockCount', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + replaceBlockGenerator.next( 1 ), + ).toEqual( { + value: undefined, + done: true, + } ); } ); } ); describe( 'insertBlock', () => { - it( 'should return the INSERT_BLOCKS action', () => { + it( 'should yield the INSERT_BLOCKS action', () => { const block = { clientId: 'ribs', + name: 'core/test-block', }; const index = 5; - expect( insertBlock( block, index, 'testclientid' ) ).toEqual( { - type: 'INSERT_BLOCKS', - blocks: [ block ], - index, - rootClientId: 'testclientid', - time: expect.any( Number ), - updateSelection: true, + + const inserBlockGenerator = insertBlock( block, index, 'testclientid', true ); + expect( + inserBlockGenerator.next().value + ).toEqual( { + args: [ 'core/test-block', 'testclientid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( true ), + ).toEqual( { + done: true, + value: { + type: 'INSERT_BLOCKS', + blocks: [ block ], + index, + rootClientId: 'testclientid', + time: expect.any( Number ), + updateSelection: true, + }, } ); } ); } ); describe( 'insertBlocks', () => { - it( 'should return the INSERT_BLOCKS action', () => { - const blocks = [ { + it( 'should filter the allowed blocks in INSERT_BLOCKS action', () => { + const ribsBlock = { clientId: 'ribs', - } ]; - const index = 3; - expect( insertBlocks( blocks, index, 'testclientid' ) ).toEqual( { - type: 'INSERT_BLOCKS', - blocks, - index, - rootClientId: 'testclientid', - time: expect.any( Number ), - updateSelection: true, + name: 'core/test-ribs', + }; + const chickenBlock = { + clientId: 'chicken', + name: 'core/test-chicken', + }; + const chickenRibsBlock = { + clientId: 'chicken-ribs', + name: 'core/test-chicken-ribs', + }; + const blocks = [ + ribsBlock, + chickenBlock, + chickenRibsBlock, + ]; + + const inserBlockGenerator = insertBlocks( blocks, 5, 'testrootid', false ); + + expect( + inserBlockGenerator.next().value + ).toEqual( { + args: [ 'core/test-ribs', 'testrootid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( true ).value + ).toEqual( { + args: [ 'core/test-chicken', 'testrootid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( false ).value, + ).toEqual( { + args: [ 'core/test-chicken-ribs', 'testrootid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( true ), + ).toEqual( { + done: true, + value: { + type: 'INSERT_BLOCKS', + blocks: [ ribsBlock, chickenRibsBlock ], + index: 5, + rootClientId: 'testrootid', + time: expect.any( Number ), + updateSelection: false, + }, + } ); + } ); + + it( 'does not yield INSERT_BLOCKS action if all the blocks are impossible to insert', () => { + const ribsBlock = { + clientId: 'ribs', + name: 'core/test-ribs', + }; + const chickenBlock = { + clientId: 'chicken', + name: 'core/test-chicken', + }; + const blocks = [ + ribsBlock, + chickenBlock, + ]; + + const inserBlockGenerator = insertBlocks( blocks, 5, 'testrootid', false ); + + expect( + inserBlockGenerator.next().value + ).toEqual( { + args: [ 'core/test-ribs', 'testrootid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( false ).value, + ).toEqual( { + args: [ 'core/test-chicken', 'testrootid' ], + selectorName: 'canInsertBlockType', + storeName: 'core/block-editor', + type: 'SELECT', + } ); + + expect( + inserBlockGenerator.next( false ), + ).toEqual( { + done: true, + value: undefined, } ); } ); } ); diff --git a/packages/block-editor/src/store/test/effects.js b/packages/block-editor/src/store/test/effects.js index 34300ed6d42ff8..cdcf5460775891 100644 --- a/packages/block-editor/src/store/test/effects.js +++ b/packages/block-editor/src/store/test/effects.js @@ -97,14 +97,19 @@ describe( 'effects', () => { expect( dispatch ).toHaveBeenCalledTimes( 2 ); expect( dispatch ).toHaveBeenCalledWith( selectBlock( 'chicken', -1 ) ); - expect( dispatch ).toHaveBeenCalledWith( { - ...replaceBlocks( [ 'chicken', 'ribs' ], [ { - clientId: 'chicken', - name: 'core/test-block', - attributes: { content: 'chicken ribs' }, - } ] ), - time: expect.any( Number ), - } ); + const lastCall = dispatch.mock.calls[ 1 ]; + expect( lastCall ).toHaveLength( 1 ); + const [ lastCallArgument ] = lastCall; + const expectedGenerator = replaceBlocks( [ 'chicken', 'ribs' ], [ { + clientId: 'chicken', + name: 'core/test-block', + attributes: { content: 'chicken ribs' }, + } ] ); + expect( + Array.from( lastCallArgument ) + ).toEqual( + Array.from( expectedGenerator ) + ); } ); it( 'should not merge the blocks have different types without transformation', () => { @@ -195,14 +200,19 @@ describe( 'effects', () => { expect( dispatch ).toHaveBeenCalledTimes( 2 ); // expect( dispatch ).toHaveBeenCalledWith( focusBlock( 'chicken', { offset: -1 } ) ); - expect( dispatch ).toHaveBeenCalledWith( { - ...replaceBlocks( [ 'chicken', 'ribs' ], [ { - clientId: 'chicken', - name: 'core/test-block', - attributes: { content: 'chicken ribs' }, - } ] ), - time: expect.any( Number ), - } ); + const expectedGenerator = replaceBlocks( [ 'chicken', 'ribs' ], [ { + clientId: 'chicken', + name: 'core/test-block', + attributes: { content: 'chicken ribs' }, + } ] ); + const lastCall = dispatch.mock.calls[ 1 ]; + expect( lastCall ).toHaveLength( 1 ); + const [ lastCallArgument ] = lastCall; + expect( + Array.from( lastCallArgument ) + ).toEqual( + Array.from( expectedGenerator ) + ); } ); } );