Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State: Set template validity on block reset #9916

Merged
merged 1 commit into from Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/reference/deprecated.md
@@ -1,5 +1,9 @@
Gutenberg's deprecation policy is intended to support backwards-compatibility for two minor releases, when possible. The current deprecations are listed below and are grouped by _the version at which they will be removed completely_. If your plugin depends on these behaviors, you must update to the recommended alternative before the noted version.

## 4.1.0

- `wp.data.dispatch( 'core/editor' ).checkTemplateValidity` has been removed. Validity is verified automatically upon block reset.

## 4.0.0

- `wp.components.RichTextProvider` has been removed. Please use `wp.data.select( 'core/editor' )` methods instead.
Expand Down
4 changes: 4 additions & 0 deletions packages/editor/CHANGELOG.md
Expand Up @@ -8,6 +8,10 @@
- `RichText` `getSettings` prop has been removed. The `unstableGetSettings` prop is available if continued use is required. Unstable APIs are strongly discouraged to be used, and are subject to removal without notice, even as part of a minor release.
- `RichText` `onSetup` prop has been removed. The `unstableOnSetup` prop is available if continued use is required. Unstable APIs are strongly discouraged to be used, and are subject to removal without notice, even as part of a minor release.

### Deprecations

- The `checkTemplateValidity` action has been deprecated. Validity is verified automatically upon block reset.

## 3.0.0 (2018-09-05)

### New Features
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/post-text-editor/index.js
Expand Up @@ -97,14 +97,13 @@ export default compose( [
};
} ),
withDispatch( ( dispatch ) => {
const { editPost, resetBlocks, checkTemplateValidity } = dispatch( 'core/editor' );
const { editPost, resetBlocks } = dispatch( 'core/editor' );
return {
onChange( content ) {
editPost( { content } );
},
onPersist( content ) {
resetBlocks( parse( content ) );
checkTemplateValidity();
},
};
} ),
Expand Down
9 changes: 9 additions & 0 deletions packages/editor/src/store/actions.js
Expand Up @@ -11,6 +11,7 @@ import {
getDefaultBlockName,
createBlock,
} from '@wordpress/blocks';
import deprecated from '@wordpress/deprecated';

/**
* Returns an action object used in signalling that editor has initialized with
Expand Down Expand Up @@ -373,6 +374,14 @@ export function setTemplateValidity( isValid ) {
* @return {Object} Action object.
*/
export function checkTemplateValidity() {
// TODO: Hello future deprecation remover. Please ensure also to remove all
// references to CHECK_TEMPLATE_VALIDITY, notably its effect handler.
deprecated( 'checkTemplateValidity action (`core/editor`)', {
version: '4.1',
plugin: 'Gutenberg',
hint: 'Validity is verified automatically upon block reset.',
} );

return {
type: 'CHECK_TEMPLATE_VALIDITY',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?
Imagining a plugin that adds blocks (wrongly without verifying the locking) after each block insert the plugin called checkTemplateValidity to have an explicit check and if necessary the warning would appear, now that will not work right?
I think the version should be 4.1 and not 4.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are deprecating this function should we keep the effect that allows plugins to invoke an explicit validation?

I suppose we should try to keep the behavior intact, yes. This probably explains why I hadn't removed it from #9403.

I think the version should be 4.1 and not 4.0.

Will change 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the rebased b94c076, I both restored the CHECK_TEMPLATE_VALIDITY effect handler and changed the deprecation version from 4.0 to 4.1.

};
Expand Down
78 changes: 54 additions & 24 deletions packages/editor/src/store/effects.js
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { last } from 'lodash';
import { compact, last } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -35,6 +35,7 @@ import {
getSelectedBlock,
getTemplate,
getTemplateLock,
isValidTemplate,
} from './selectors';
import {
fetchReusableBlocks,
Expand All @@ -54,6 +55,36 @@ import {
AUTOSAVE_POST_NOTICE_ID,
} from './effects/posts';

/**
* Block validity is a function of blocks state (at the point of a
* reset) and the template setting. As a compromise to its placement
* across distinct parts of state, it is implemented here as a side-
* effect of the block reset action.
*
* @param {Object} action RESET_BLOCKS action.
* @param {Object} store Store instance.
*
* @return {?Object} New validity set action if validity has changed.
*/
export function validateBlocksToTemplate( action, store ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As right now, besides the RESET_BLOCKS action this function also receives the SETUP_EDITOR_STATE, and my expectation is that this function will probably never need anything from the action besides the blocks, would it make sense for this function to receive the blocks instead of the action?

Copy link
Member Author

@aduth aduth Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So part of my purpose here in extracting this to a named function is to try to set a precedent for well-named / intentioned effect handlers. As you propose, we'd need a function which itself calls to validateBlocksToTemplate. It would likely start small, just extracting the blocks property from the action, but there'd be nothing to stop someone from adding to it, regressing to the poorer code standard.

Illustrated by code:

// Bad:

RANDOM_ACTION( action, store ) {
	// Do A

	// Do B

	// Do C
}

// Good:

RANDOM_ACTION: [
	doA( action, store ) {},
	doB( action, store ) {},
	doC( action, store ) {},
]

In general, I would have no issue with what you propose, and it'd probably make things even more clearly intentioned and easier to test. With needing the template and template lock details, it becomes a little more effort to be able to organize.

I might poke around at it to see if I can find some happy compromise, but otherwise I'm not strongly inclined to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a fair point, even a function that is just supposed to pass the blocks to another function can then be changed to a different purpose. Feel free to keep the current version as probably it will keep code more organized in the long run.

const state = store.getState();
const template = getTemplate( state );
const templateLock = getTemplateLock( state );

// Unlocked templates are considered always valid because they act
// as default values only.
const isBlocksValidToTemplate = (
! template ||
templateLock !== 'all' ||
doBlocksMatchTemplate( action.blocks, template )
);

// Update if validity has changed.
if ( isBlocksValidToTemplate !== isValidTemplate( state ) ) {
return setTemplateValidity( isBlocksValidToTemplate );
}
}

export default {
REQUEST_POST_UPDATE: ( action, store ) => {
requestPostUpdate( action, store );
Expand Down Expand Up @@ -113,26 +144,18 @@ export default {
]
) );
},
SETUP_EDITOR( action, { getState } ) {
SETUP_EDITOR( action, store ) {
const { post, autosave } = action;
const state = getState();
const template = getTemplate( state );
const templateLock = getTemplateLock( state );
const isNewPost = post.status === 'auto-draft';
const state = store.getState();

// Parse content as blocks
let blocks = parse( post.content.raw );
let isValidTemplate = true;

// Apply a template for new posts only, if exists.
const isNewPost = post.status === 'auto-draft';
const template = getTemplate( state );
if ( isNewPost && template ) {
// Apply a template for new posts only, if exists.
blocks = synchronizeBlocksWithTemplate( blocks, template );
} else {
// Unlocked templates are considered always valid because they act as default values only.
isValidTemplate = (
! template ||
templateLock !== 'all' ||
doBlocksMatchTemplate( blocks, template )
);
}

// Include auto draft title in edits while not flagging post as dirty
Expand All @@ -158,22 +181,29 @@ export default {
);
}

return [
setTemplateValidity( isValidTemplate ),
setupEditorState( post, blocks, edits ),
...( autosaveAction ? [ autosaveAction ] : [] ),
];
const setupAction = setupEditorState( post, blocks, edits );

return compact( [
setupAction,
autosaveAction,

// TODO: This is temporary, necessary only so long as editor setup
// is a separate action from block resetting.
//
// See: https://github.com/WordPress/gutenberg/pull/9403
validateBlocksToTemplate( setupAction, store ),
] );
},
RESET_BLOCKS: [
validateBlocksToTemplate,
],
SYNCHRONIZE_TEMPLATE( action, { getState } ) {
const state = getState();
const blocks = getBlocks( state );
const template = getTemplate( state );
const updatedBlockList = synchronizeBlocksWithTemplate( blocks, template );

return [
resetBlocks( updatedBlockList ),
setTemplateValidity( true ),
];
return resetBlocks( updatedBlockList );
},
CHECK_TEMPLATE_VALIDITY( action, { getState } ) {
const state = getState();
Expand Down
96 changes: 89 additions & 7 deletions packages/editor/src/store/test/effects.js
Expand Up @@ -12,22 +12,26 @@ import {
registerBlockType,
createBlock,
} from '@wordpress/blocks';
import { createRegistry } from '@wordpress/data';

/**
* Internal dependencies
*/
import {
import actions, {
updateEditorSettings,
setupEditorState,
mergeBlocks,
replaceBlocks,
resetBlocks,
selectBlock,
createErrorNotice,
setTemplateValidity,
editPost,
} from '../actions';
import effects from '../effects';
import effects, { validateBlocksToTemplate } from '../effects';
import * as selectors from '../selectors';
import reducer from '../reducer';
import applyMiddlewares from '../middlewares';

describe( 'effects', () => {
const defaultBlockSettings = { save: () => 'Saved', category: 'common', title: 'block title' };
Expand Down Expand Up @@ -441,12 +445,14 @@ describe( 'effects', () => {
template: null,
templateLock: false,
},
template: {
isValid: true,
},
} );

const result = handler( { post, settings: {} }, { getState } );

expect( result ).toEqual( [
setTemplateValidity( true ),
setupEditorState( post, [], {} ),
] );
} );
Expand All @@ -468,14 +474,16 @@ describe( 'effects', () => {
template: null,
templateLock: false,
},
template: {
isValid: true,
},
} );

const result = handler( { post }, { getState } );

expect( result[ 1 ].blocks ).toHaveLength( 1 );
expect( result[ 0 ].blocks ).toHaveLength( 1 );
expect( result ).toEqual( [
setTemplateValidity( true ),
setupEditorState( post, result[ 1 ].blocks, {} ),
setupEditorState( post, result[ 0 ].blocks, {} ),
] );
} );

Expand All @@ -495,14 +503,88 @@ describe( 'effects', () => {
template: null,
templateLock: false,
},
template: {
isValid: true,
},
} );

const result = handler( { post }, { getState } );

expect( result ).toEqual( [
setTemplateValidity( true ),
setupEditorState( post, [], { title: 'A History of Pork' } ),
] );
} );
} );

describe( 'validateBlocksToTemplate', () => {
let store;
beforeEach( () => {
store = createRegistry().registerStore( 'test', {
actions,
selectors,
reducer,
} );
applyMiddlewares( store );

registerBlockType( 'core/test-block', defaultBlockSettings );
} );

afterEach( () => {
getBlockTypes().forEach( ( block ) => {
unregisterBlockType( block.name );
} );
} );

it( 'should return undefined if no template assigned', () => {
const result = validateBlocksToTemplate( resetBlocks( [
createBlock( 'core/test-block' ),
] ), store );

expect( result ).toBe( undefined );
} );

it( 'should return undefined if invalid but unlocked', () => {
store.dispatch( updateEditorSettings( {
template: [
[ 'core/foo', {} ],
],
} ) );

const result = validateBlocksToTemplate( resetBlocks( [
createBlock( 'core/test-block' ),
] ), store );

expect( result ).toBe( undefined );
} );

it( 'should return undefined if locked and valid', () => {
store.dispatch( updateEditorSettings( {
template: [
[ 'core/test-block' ],
],
templateLock: 'all',
} ) );

const result = validateBlocksToTemplate( resetBlocks( [
createBlock( 'core/test-block' ),
] ), store );

expect( result ).toBe( undefined );
} );

it( 'should return validity set action if invalid on default state', () => {
store.dispatch( updateEditorSettings( {
template: [
[ 'core/foo' ],
],
templateLock: 'all',
} ) );

const result = validateBlocksToTemplate( resetBlocks( [
createBlock( 'core/test-block' ),
] ), store );

expect( result ).toEqual( setTemplateValidity( false ) );
} );
} );
} );