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 Link block - add unit tests to cover complex onChange handler #31379

Closed
getdave opened this issue Apr 30, 2021 · 1 comment
Closed
Labels
[Block] Navigation Affects the Navigation Block

Comments

@getdave
Copy link
Contributor

getdave commented Apr 30, 2021

What problem does this address?

Currently in the core/navigation-link block we have a rather complex function which handles any change events occurring within the <LinkControl /> component.

onChange={ ( {
title: newTitle = '',
url: newURL = '',
opensInNewTab: newOpensInNewTab,
id,
kind: newKind = '',
type: newType = '',
} = {} ) =>
setAttributes( {
url: encodeURI( newURL ),
label: ( () => {
const normalizedTitle = newTitle.replace(
/http(s?):\/\//gi,
''
);
const normalizedURL = newURL.replace(
/http(s?):\/\//gi,
''
);
if (
newTitle !== '' &&
normalizedTitle !==
normalizedURL &&
label !== newTitle
) {
return escape( newTitle );
} else if ( label ) {
return label;
}
// If there's no label, add the URL.
return escape( normalizedURL );
} )(),
opensInNewTab: newOpensInNewTab,
id,
...( newKind && {
kind: newKind,
} ),
...( newType &&
newType !== 'URL' &&
newType !== 'post-format' && {
type: newType,
} ),
} )
}

This is a critical piece of code because it ultimately determines (by use of setAttributes()) the data about the block that will be saved:

  1. as serialized block HTML (post/site editor).
  2. to the database as nav_menu_item objects.

Therefore any errors or edge cases in this code will result in a broken block and/or navigation editor screen.

The code is currently pretty difficult to read and it's easy enough to introduce bugs as there is no (direct) test coverage.

Context

During dev of #31004 I discovered a bug whereby the id prop could inadvertently set to undefined if an onChange was called without the id prop being passed.

This was happening when the Opens in new tab setting control was toggled on core/navigation-link block variations that represent entities (eg: post-link variation) that need to be reference to the id of the object they represent:

Fixing this was simply a matter of only updating the id prop if the value was passed in the onChange handler and otherwise leaving it untouched. However the bug was very obscure and difficult to trace.

What is your proposed solution?

I propose we extract this onChange to a named function (perhaps mapLinkChangeToBlockAttributes?) and then export it so that it can be unit tested. Given how critical the code is it's important we don't inadvertently introduce regressions. We could cover this via e2e tests but it could be quite complex to cover all the possible scenarios.

@gwwar
Copy link
Contributor

gwwar commented May 10, 2021

This one should be closed by #31259

@gwwar gwwar closed this as completed May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Development

No branches or pull requests

2 participants