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

Fix: Make it impossible to use the UI to add a reusable block inside the same reusable block #11316

Merged
merged 1 commit into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Oct 31, 2018

Fixes client-side problems in #11206.

It was possible to use the UI to add a reusable block inside itself. This PR addresses this problem.

In order to address this problem, a new selector isAncestorOf is created.

How has this been tested?

Add a columns block write some paragraphs inside it.
Make the columns block reusable block and save it.
Edit the reusable block add a new paragraph use the slash inserter "/" on the new paragraph and verify it is not possible to insert the same reusable block. (On master it was and the editor crashed).

* @return {boolean} True if possibleAncestorId is an ancestor
* of possibleDescendentId, and false otherwise.
*/
export const isAncestorOf = createSelector(

This comment has been minimized.

@noisysocks

noisysocks Nov 12, 2018

Member

Do we need to export this? It looks like it's only used by getInserterItems.

/**
* Checks if possibleAncestorId is an ancestor of possibleDescendentId.
*
* @param {Object} state Editor state.

This comment has been minimized.

@noisysocks

noisysocks Nov 12, 2018

Member

nit: These @param comments don't align.

@@ -1708,6 +1733,10 @@ export const getInserterItems = createSelector(
return false;
}

if ( isAncestorOf( state, reusableBlock.clientId, rootClientId ) ) {
return false;

This comment has been minimized.

@noisysocks

noisysocks Nov 12, 2018

Member

From memory, getInserterItems has some fairly thorough unit tests. Let's keep that up and add a test case for this new edge case.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 12, 2018

Fixes client-side problems in #11206.

Do you think it's worthwhile having a server-side fix? How would one work?

@noisysocks noisysocks added this to the 4.4 milestone Nov 12, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/it-is-possible-to-add-a-resuable-block-inside-the-same-reusable-block branch 2 times, most recently from 0059362 to 76c5f1f Nov 14, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 14, 2018

Thank you for your review @noisysocks 👍 All the feedback was addressed.

Do you think it's worthwhile having a server-side fix? How would one work?

For now, I addressed the client-side problem as it is possible to use the UI to trigger it. So after this PR, the bug will only happen using code.

I think the server side problem is hard to trigger/and In my perspective seems something a normal user would not do (unless trying to trigger the bug). One possible solution for the problem would be having a hook on the server the executes when saving posts and if it is saving a reusable block, it would parse the blocks inside iterate on the tree and check if there is not a block referencing its own id.

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/it-is-possible-to-add-a-resuable-block-inside-the-same-reusable-block branch from 76c5f1f to cc64ce4 Nov 16, 2018

@jorgefilipecosta jorgefilipecosta force-pushed the fix/it-is-possible-to-add-a-resuable-block-inside-the-same-reusable-block branch from cc64ce4 to c999bb2 Nov 16, 2018

@jorgefilipecosta jorgefilipecosta merged commit c60894e into master Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/it-is-possible-to-add-a-resuable-block-inside-the-same-reusable-block branch Nov 16, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix: Make it impossible to use the UI to add a reusable block inside …
…the same reusable block. (WordPress#11316)

It was possible to use the UI to add a reusable block inside itself. This commit addresses this problem.

In order to address this problem, a new selector isAncestorOf is created.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment