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

Navigation Block: Add tests for Nav block uncontrolled blocks dirty state checking #46329

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions packages/block-library/src/navigation/edit/are-blocks-dirty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
export function areBlocksDirty( originalBlocks, blocks ) {
return ! isDeepEqual( originalBlocks, blocks, ( prop, x ) => {
// Skip inner blocks of page list during comparison as they
// are **always** controlled and may be updated async due to
// syncing with entity records. Left unchecked this would
// inadvertently trigger the dirty state.
if ( x?.name === 'core/page-list' && prop === 'innerBlocks' ) {
return true;
}
} );
}

/**
* Conditionally compares two candidates for deep equality.
* Provides an option to skip a given property of an object during comparison.
*
* @param {*} x 1st candidate for comparison
* @param {*} y 2nd candidate for comparison
* @param {Function|undefined} shouldSkip a function which can be used to skip a given property of an object.
* @return {boolean} whether the two candidates are deeply equal.
*/
const isDeepEqual = ( x, y, shouldSkip ) => {
if ( x === y ) {
return true;
} else if (
typeof x === 'object' &&
x !== null &&
x !== undefined &&
typeof y === 'object' &&
y !== null &&
y !== undefined
) {
if ( Object.keys( x ).length !== Object.keys( y ).length ) return false;

for ( const prop in x ) {
if ( y.hasOwnProperty( prop ) ) {
// Afford skipping a given property of an object.
if ( shouldSkip && shouldSkip( prop, x ) ) {
return true;
}

if ( ! isDeepEqual( x[ prop ], y[ prop ], shouldSkip ) )
return false;
} else return false;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is the only line that's not tested here. Do we want to cover the case where if ( y.hasOwnProperty( prop ) ) isn't true?

I don't think we need 100% coverage though! These tests look great to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the recent commit should now address that?

Copy link
Member

Choose a reason for hiding this comment

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

I'm still seeing the same coverage with the latest commit, although if I move dropCap to the first array, and add a different property to the second array, like this:

		expect(
			areBlocksDirty(
				[ { name: 'core/paragraph' }, { dropCap: false } ],
				[ { name: 'core/paragraph' }, { content: 'Some content' } ]
			)
		).toBe( true );

This is reported as 100% coverage. I think it's because we're looping through the properties on the first array to compare if they're in the second, so there needs to be at least one different property on the first array in order for it to jump into line 44.

The line is also covered if we just compare name vs something else like content on its own, but I guess name will most likely always exist, so there's not much point in testing if that is different on its own, e.g.:

		expect(
			areBlocksDirty(
				[ { name: 'core/paragraph' } ],
				[ { content: 'core/paragraph' } ]
			)
		).toBe( true );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikachan Are you using a tool to check the coverage? Can you explain how I can debug this myself 🙏

Looks like this auto merged but you would of course be super welcome to raise a PR if you felt able.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sure. I was running npm run test:unit are-blocks-dirty.js -- --coverage.

I don't think getting to 100% is a big deal, but there was only this one line that wasn't covered here, so it's so close I thought we may as well cover it. I've created a quick PR: #46355

}

return true;
}

return false;
};
118 changes: 118 additions & 0 deletions packages/block-library/src/navigation/edit/test/are-blocks-dirty.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/**
* Internal dependencies
*/
import { areBlocksDirty } from '../are-blocks-dirty';

describe( 'areBlocksDirty', () => {
it( 'should be clean if the blocks are the same', () => {
expect(
areBlocksDirty(
[ { name: 'core/paragraph', content: 'I am not dirty.' } ],
[ { name: 'core/paragraph', content: 'I am not dirty.' } ]
)
).toBe( false );
} );

it( `should be dirty if the blocks' attributes are different`, () => {
expect(
areBlocksDirty(
[ { name: 'core/paragraph', content: 'I am not dirty.' } ],
[ { name: 'core/paragraph', content: 'I am actually dirty.' } ]
)
).toBe( true );
} );

it( `should be dirty if the blocks' attributes don't match`, () => {
expect(
areBlocksDirty(
[ { name: 'core/paragraph' } ],
[ { name: 'core/paragraph', dropCap: false } ]
)
).toBe( true );
} );

it( `should be dirty if the blocks' inner blocks are dirty`, () => {
expect(
areBlocksDirty(
[
{
name: 'core/social-links',
innerBlocks: [
{
name: 'core/social-link',
url: 'www.wordpress.org',
},
],
},
],
[
{
name: 'core/social-links',
innerBlocks: [
{
name: 'core/social-link',
service: 'wordpress',
url: 'www.wordpress.org',
},
{
name: 'core/social-link',
service: 'wordpress',
url: 'make.wordpress.org',
},
],
},
]
)
).toBe( true );
} );

describe( 'Controlled Page List block specific exceptions', () => {
it( 'should be clean if only page list inner blocks have changed', () => {
expect(
areBlocksDirty(
[
{ name: 'core/paragraph' },
{
name: 'core/page-list',
innerBlocks: [],
},
],
[
{ name: 'core/paragraph' },
{
name: 'core/page-list',
innerBlocks: [ { name: 'core/page-list-item' } ],
},
]
)
).toBe( false );
} );

it( 'should be dirty if other blocks have changed alongside page list inner blocks', () => {
expect(
areBlocksDirty(
[
{
name: 'core/paragraph',
content: 'This is some text',
},
{
name: 'core/page-list',
innerBlocks: [],
},
],
[
{
name: 'core/paragraph',
content: 'This is some text that has changed',
},
{
name: 'core/page-list',
innerBlocks: [ { name: 'core/page-list-item' } ],
},
]
)
).toBe( true );
} );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { useContext, useEffect, useRef, useMemo } from '@wordpress/element';
* Internal dependencies
*/
import useNavigationMenu from '../use-navigation-menu';
import { areBlocksDirty } from './are-blocks-dirty';

const EMPTY_OBJECT = {};
const DRAFT_MENU_PARAMS = [
Expand All @@ -35,46 +36,6 @@ const ALLOWED_BLOCKS = [
'core/navigation-submenu',
];

/**
* Conditionally compares two candidates for deep equality.
* Provides an option to skip a given property of an object during comparison.
*
* @param {*} x 1st candidate for comparison
* @param {*} y 2nd candidate for comparison
* @param {Function|undefined} shouldSkip a function which can be used to skip a given property of an object.
* @return {boolean} whether the two candidates are deeply equal.
*/
const isDeepEqual = ( x, y, shouldSkip ) => {
if ( x === y ) {
return true;
} else if (
typeof x === 'object' &&
x !== null &&
x !== undefined &&
typeof y === 'object' &&
y !== null &&
y !== undefined
) {
if ( Object.keys( x ).length !== Object.keys( y ).length ) return false;

for ( const prop in x ) {
if ( y.hasOwnProperty( prop ) ) {
// Afford skipping a given property of an object.
if ( shouldSkip && shouldSkip( prop, x ) ) {
return true;
}

if ( ! isDeepEqual( x[ prop ], y[ prop ], shouldSkip ) )
return false;
} else return false;
}

return true;
}

return false;
};

export default function UnsavedInnerBlocks( {
blocks,
createNavigationMenu,
Expand All @@ -98,18 +59,9 @@ export default function UnsavedInnerBlocks( {
// of the page list are controlled and may be updated async due to syncing with
// entity records. As a result we need to perform a deep equality check skipping
// the page list's inner blocks.
const innerBlocksAreDirty = ! isDeepEqual(
originalBlocks.current,
blocks,
( prop, x ) => {
// Skip inner blocks of page list during comparison as they
// are **always** controlled and may be updated async due to
// syncing with enitiy records. Left unchecked this would
// inadvertently trigger the dirty state.
if ( x?.name === 'core/page-list' && prop === 'innerBlocks' ) {
return true;
}
}
const innerBlocksAreDirty = areBlocksDirty(
originalBlocks?.current,
blocks
);

const shouldDirectInsert = useMemo(
Expand Down