TT | 3471 | "Introduce allow-list and disallow-list for blocks."

[ttahmouch]

Description

This is meant to be treated as a draft.

Idea

The general idea behind this work is that there is a desire to allow the Gutenberg Block Editor to restrict the block types it offers in various contexts it is presented in to the customer. It came from this feature request.

The changes found here should, in theory, only impact the blocks presented in the block picker, and not affect the blocks displayed within the editor if someone were to manually modify the markup.

The "requirements" that I thought seemed reasonable to me would be:

0. If you pass in neither an allow list nor a disallow list, then proceed by including all registered blocks, i.e., core/, jetpack/, or otherwise.
1. If you pass in an allow list, and not a disallow list, then proceed by only including what is in the allow list from the entire set of registered blocks, i.e., core/, jetpack/, or otherwise.
2. If you pass in a disallow list, and not an allow list, then proceed by including the entire set of registered blocks, i.e., core/, jetpack/, or otherwise, sans the blocks in the disallow list.
3. If you pass in both an allow and disallow list, then proceed by ignoring the disallow list and and only including the blocks that result from the allow list detailed in 1 (as it is the more restrictive case).
   • However, this use case isn't realistic in my opinion. The consumer should only provide an allow or disallow list whichever is more convenient for the context.
   • An allow list is preferred when the amount of blocks we'd like to present to the customer is small.
   • A disallow list is preferred when the amount of blocks we'd like to present is large.

Notes

Previous Attempt

I made a previous attempt to implement the concept of allowing and disallowing block types. I tried ensuring that registration of the disallowed blocks never occured using registerBlockType if the block name wasn't included the provided allow list, or if the block name was provided in the disallow list.

This attempt had some unintended side-effects. These are detailed in this P2 post if you would like more context.

Remaining Work

• [ ] Document the interfaces with JSDoc and include useful contextual knowledge.
• [x] Automate tests to reflect the requirements defined here (since it seems that this approach is okay to move forward with. I may still need some help understanding the best possible testing strategy from a functional or integration test perspective as opposed to basic unit testing.)
• [x] Consider adding a comment in the modified test explaining the reason for using the inline require (as noted by @fluiddot).
• [ ] Refine the PR description for the current approach and update the testing instructions.
• [ ] Update PR to "Ready for Review" when the PR description and testing is "ready."
• [x] Change implementation to ensure hideBlocks gets ignored if showBlocks and hideBlocks are provided together (if that makes sense).

Demo

Providing empty sets of showBlocks and hideBlocks.	Providing only the jetpack/contact-info block type in the showBlocks set while capable.

Providing only the jetpack/contact-info block type in the hideBlocks set while capable.	Providing only the jetpack/contact-info block type in the showBlocks set while uncapable.

To test: TBD

PR submission checklist: TBD

• [x] I have considered adding unit tests where possible.
• [ ] I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile[bot]]

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

[ttahmouch]

I made these changes as noted in my commit log, @fluiddot . I would appreciate your feedback. I tried to explain my rationale behind the changes in our other thread, but I can bring that to the PR if it's easier to track. I'm also not opposed to trying a different approach to the implementation. This approach just appears to work well.

• src/allowed-blocks-setup.js
  • Added jetpack/layout-grid as an "Available Jetpack Block." However, it's availability is irrelevant since it is not registered with registerJetpackBlock but rather registerBlockType.
  • Added a centralized entry point for registration and hiding of block types called setupBlocks that should be executed in the gutenberg-mobile native.render action as early as possible in the editor initialization.
  • Removed the redundant dispatching of the SHOW_BLOCK_TYPES action for the inverse of the block types that are hidden with the dispatching of the HIDE_BLOCK_TYPES action to @fluiddot 's point.
  • Ensured that all jetpack/ block types export their own registerBlock entry point function to make registration conditional for non-core/ block types.
  • Added registerJetpackBlocksIfCapable to ensure that jetpack/contact-info, jetpack/layout-grid, and jetpack/story are only registered if their respective capabilities passed through the editor intial props indicate they should be enabled for the customer.
  • Simplified the registration of all jetpack/ block types previously done in setupJetpackEditor and setJetpackData in the new setupJetpackBlocks function. The setTimeout to ensure jetpack/ blocks are conditionally hidden on next tick is no longer needed if not attempting to select the capabilities from the core/block-editor store, but rather just leverage the props passed directly into the initialization of the editor. The inline require is no longer needed since it now exports functions used to register lazily as opposed to implied registration when the module is loaded. toggleBlock is also no longer needed as the hiding of all block types, core/, jetpack/, or future types has been unified.
  • Removed the unnecessary dependencies argument from the setupAllowedBlocks function signature. We can just leverage Jest module mocking for dependency injection when necessary.
  • Removed the unnecessary setTimeout from the setupAllowedBlocks function as it no longer has to wait for jetpack/ block types to be hidden first since that flow can now be done on the current tick of the event loop instead of a future tick waiting for the redux store state to have changed.
• src/block-experiments-setup.js
  • Removed the module to consolidate block type registration and hiding into src/allowed-blocks-setup.js.
• src/block-support/supported-blocks.json
  • Added jetpack/layout-grid and jetpack/story to the list of supported blocks. This appears to only be used in the iOS Swift React Native Bridge.
• src/index.js
  • Consolidated the entry points used for registering and hiding jetpack/contact-info, jetpack/layout-grid, and jetpack/story to setupBlocks from setupJetpackEditor and setupBlockExperiments.
• src/jetpack-editor-setup.js
  • Removed the module to consolidate block type registration and hiding into src/allowed-blocks-setup.js.
• src/test/index.js
  • Updated the staging of the existing test that ensures jetpack/ block types register successfully during the editor initialization.

[ttahmouch]

Thanks @ttahmouch for making the changes on the PR 🙇.

You're welcome, @fluiddot . Thank you for being so patient through this process. 🙏

I've added some comments in the code but I think that the PR is ready to be reviewed so please when you're available, let me know if you could change it to "Ready for review". It would be also nice if you could also wrap up the PR by explaining the current approach and provide some testing instructions for reviewers to use as a guide.

Thanks for adding comments throughout the code. I appreciate it. I didn't feel comfortable changing its status from draft until I knew that this approach was going to be the one we intend to move forward with. I didn't want to add unnecessary automated tests for an approach that may not realistically be used.

I will try to make the PR description clearer with respect to the current approach and update the testing instructions. Apologies if it was confusing to anyone.

From my side, I went ahead and tested the PR and except for a couple of issues everything is working fine 🎊 , here are the insights:

• I tested disabling some of the capabilities used in the Jetpack blocks and including the disabled blocks in the allow-list and I confirmed that they weren't displayed ✅ .
• I tested the requirements described in the PR's description and they were fulfilled except one, here are the details:

If you pass in neither an allow list nor a disallow list, then proceed by using all core blocks.

This works as expected ✅ .

If you pass in an allow list, and not a disallow list, then proceed by only including what is in the allow list from the entire set of core blocks.

This works as expected ✅ .

If you pass in a disallow list, and not an allow list, then proceed by including the entire set of core blocks sans the blocks in the disallow list.

This works as expected ✅ .

Thank you for testing and providing feedback regarding your insights. 🙏 🙇

I'm glad these cases worked as intended.

If you pass in both an allow and disallow list, then proceed by removing blocks named in the disallow list first, then keeping the blocks named in the allow list second.

For this case, I included the same block in both allow/disallow lists and I noticed that is not displayed so looks like that this requirement is not being fulfilled ❌.

I used the following values:

showBlocks: [ 'core/paragraph' ],
hideBlocks: [ 'core/paragraph' ],

The reason this is no longer working as intended I believe is simply because I am no longer dispatching the SHOW action as the PR once was. So it never removes the core/paragraph block type from the hide list in the store state in your example.

That being said this has always been the use case that has never made particularly intuitive sense to me. If the consumer says "I want you to take the set of all registered block types, reduce it by hiding a few of them, and then reduce it further by only showing a few of them" it would likely make practical sense for either:

0. the hideBlocks to just go ignored altogether and only acknowledge the showBlocks since it is more restrictive.
1. taking a stricter stance by simply not allowing both hideBlocks and showBlocks to be provided in unison.

I'd lean in favor of the former over the latter. I do think that ignoring the hideBlocks and letting showBlocks supersede actually makes sense. WDYT?

It really only matters in your edge case that they're providing the same block type in both sets.

Practical Example?

// Registered Blocks [ 'core/audio', 'core/paragraph', 'core/video' ]
// First, reduce set to [ 'core/audio', 'core/video' ].
// Then, reduce remaining set to [ 'core/audio' ].
hideBlocks: [ 'core/paragraph' ],
showBlocks: [ 'core/audio' ],

Impractical Example?

// Registered Blocks [ 'core/audio', 'core/paragraph', 'core/video' ]
// First, reduce set to [ 'core/audio', 'core/video' ].
// Then, reduce remaining set to [ 'core/paragraph' ]?
hideBlocks: [ 'core/paragraph' ],
showBlocks: [ 'core/paragraph' ],

• [ ] I will try to make the PR description clearer with respect to the current approach and update the testing instructions.
• [ ] I will try to update the automated tests since it seems that this approach is okay to move forward with. (I may still need some help understanding the best possible testing strategy from a functional or integration test perspective as opposed to basic unit testing.)
• [ ] I will change it to "Ready for Review" when I add automated testing and update the description.
• [ ] I will make the change to ensure hideBlocks gets ignored if showBlocks and hideBlocks are provided together (if that makes sense).

[fluiddot]

Thanks for adding comments throughout the code. I appreciate it. I didn't feel comfortable changing its status from draft until I knew that this approach was going to be the one we intend to move forward with. I didn't want to add unnecessary automated tests for an approach that may not realistically be used.

I will try to make the PR description clearer with respect to the current approach and update the testing instructions. Apologies if it was confusing to anyone.

No worries, ok, it makes sense, let's change it to "ready for review" once the PR is updated 👍 .

The reason this is no longer working as intended I believe is simply because I am no longer dispatching the SHOW action as the PR once was. So it never removes the core/paragraph block type from the hide list in the store state in your example.

That being said this has always been the use case that has never made particularly intuitive sense to me. If the consumer says "I want you to take the set of all registered block types, reduce it by hiding a few of them, and then reduce it further by only showing a few of them" it would likely make practical sense for either:

1. the hideBlocks to just go ignored altogether and only acknowledge the showBlocks since it is more restrictive.
2. taking a stricter stance by simply not allowing both hideBlocks and showBlocks to be provided in unison.

I'd lean in favor of the former over the latter. I do think that ignoring the hideBlocks and letting showBlocks supersede actually makes sense. WDYT?

Yeah, since providing both lists is an edge case, I think ignoring hideBlocks in case showBlocks is defined makes sense. One thing it would be great is to add a comment in the code explaining this behavior to prevent potential misleading, wdyt?

• [ ] I will try to update the automated tests since it seems that this approach is okay to move forward with. (I may still need some help understanding the best possible testing strategy from a functional or integration test perspective as opposed to basic unit testing.)

If you're referring to UI tests with "automated tests", not sure if we'll be able to cover this feature with them because, as far as I know, the initial props can't be set on this type of test.

[mchowning]

Yeah, since providing both lists is an edge case, I think ignoring hideBlocks in case showBlocks is defined makes sense. One thing it would be great is to add a comment in the code explaining this behavior to prevent potential misleading, wdyt?

Is this maybe highlighting that we could be modeling this a bit better? Did we consider not having two lists and instead just providing one with a show/hide toggle (this might have been what you meant @ttahmouch when you suggested "taking a stricter stance by simply not allowing both hideBlocks and showBlocks to be provided in unison")? I'm thinking of something like:

{
  blocks: [string],
  blockAction: "disallow" | "onlyAllow"
}

Feels like that would both make the behavior more transparent and simplify the implementation. I apologize if you already considered this, and I just missed it. I do realize this PR is in a relatively late stage of revision, and I am fine with the ignore-one-of-the-lists-if-both-are-defined since it sounds like that is what you prefer.

[ttahmouch]

If you're referring to UI tests with "automated tests", not sure if we'll be able to cover this feature with them because, as far as I know, the initial props can't be set on this type of test.

I was referring to mostly if we could test the integration of the registration flow with the hiding flow instead of just unit testing the individual functions in isolation. I usually prefer to integration test, or higher-level functional test, over isolated unit tests with everything mocked since it more accurately reflects the tested use cases from the customers' user experience. It's just a personal preference.

To be pedantic, I don't even necessarily mean functional testing as "running the application and interacting with the visual elements on the screen from a 'black box'" with something like Selenium/Appium/Espresso/XCUITest. Even something that simulates testing like that from the component layer, e.g., @testing-library/react-native, would apply.

Does this make sense, @fluiddot ?

[ttahmouch]

Yeah, since providing both lists is an edge case, I think ignoring hideBlocks in case showBlocks is defined makes sense. One thing it would be great is to add a comment in the code explaining this behavior to prevent potential misleading, wdyt?

Is this maybe highlighting that we could be modeling this a bit better? Did we consider not having two lists and instead just providing one with a show/hide toggle (this might have been what you meant @ttahmouch when you suggested "taking a stricter stance by simply not allowing both hideBlocks and showBlocks to be provided in unison")? I'm thinking of something like:

{
  blocks: [string],
  blockAction: "disallow" | "onlyAllow"
}

Feels like that would both make the behavior more transparent and simplify the implementation. I apologize if you already considered this, and I just missed it. I do realize this PR is in a relatively late stage of revision, and I am fine with the ignore-one-of-the-lists-if-both-are-defined since it sounds like that is what you prefer.

It's not my "preference." I did think that intuitively reading the interface as show or hide would be more descriptive as to the intent behind the property when initializing the editor. I do agree that having a single list would simplify the implementation details of the block hiding flow, but it also just felt like a micro-optimization for a use case that I didn't think would ever practically exist. It's just something that I thought would make a good case for explaining in a JSDoc.

I am not opposed to changing the interface for the benefit of the consumer of the block editor. I just don't feel strongly that the approach using a single list would necessarily make the interface more intuitive since practically speaking the use case is not realistic.

If the consumer only wanted to show a few blocks, then they should prefer the showBlocks interface. If the consumer only wanted hide a few blocks, then they should prefer the hideBlocks interface. Realistically, providing both sets is effectively the same as providing the more restrictive of the two.

Does that make sense, @mchowning ?

Aside

If we did attempt it with an approach similar to {blocks: [], blockAction: ""}, then I think we'd have to also handle a different edge case or two. We'd realistically want to have the default blockAction be undefined or "default", or however we'd name it, to indicate that we'd neither like to allow nor disallow a subset of blocks, but what happens if a blocks set is still provided? Do we ignore it (treat it as empty)? Or do we default the blockAction to an action other than "default", e.g., "onlyAllow"?

It may just be shifting the concern? Does that make sense?

To be clear, I like the concept and appreciate your feedback. I'm happy to make changes if the way that I've been implementing it so far is not particularly intuitive. I just took a first pass at it as a draft. It wasn't necessarily something I was married to and intending to be my final pass.

[mchowning]

I am not opposed to changing the interface for the benefit of the consumer of the block editor. I just don't feel strongly that the approach using a single list would necessarily make the interface more intuitive since practically speaking the use case is not realistic.

What you say makes sense, and I'm totally fine sticking with the current object structure. I personally like a single list a bit better, but I think that this is something that is firmly in the realm of personal preference (and your approach is probably the more javascript-y™ one), so let's stick with it.

[fluiddot]

If you're referring to UI tests with "automated tests", not sure if we'll be able to cover this feature with them because, as far as I know, the initial props can't be set on this type of test.

I was referring to mostly if we could test the integration of the registration flow with the hiding flow instead of just unit testing the individual functions in isolation. I usually prefer to integration test, or higher-level functional test, over isolated unit tests with everything mocked since it more accurately reflects the tested use cases from the customers' user experience. It's just a personal preference.

To be pedantic, I don't even necessarily mean functional testing as "running the application and interacting with the visual elements on the screen from a 'black box'" with something like Selenium/Appium/Espresso/XCUITest. Even something that simulates testing like that from the component layer, e.g., @testing-library/react-native, would apply.

Does this make sense, @fluiddot ?

It makes totally sense, I just wanted to make sure that we were talking about integration tests and not UI tests, thanks for the clarification 👍 .

I like the idea to use @testing-library/react-native, in fact, we recently added a guideline for integration tests using this library (reference) but it's meant to be used in the tests defined in the Gutenberg repository only. However, if you would like to use it, some time ago I worked on a PR that enable them in Gutenberg Mobile, so feel free to take those changes in case they help you in this purpose.

Being this said, I foresee that the integration tests might take extra time so maybe we could add them later on and in the meantime address the rest of the subtasks:

• [ ] I will try to make the PR description clearer with respect to the current approach and update the testing instructions.
• [ ] I will make the change to ensure hideBlocks gets ignored if showBlocks and hideBlocks are provided together (if that makes sense).

What do you think?

[ttahmouch]

Being this said, I foresee that the integration tests might take extra time so maybe we could add them later on and in the meantime address the rest of the subtasks:

• [ ] I will try to make the PR description clearer with respect to the current approach and update the testing instructions.
• [ ] I will make the change to ensure hideBlocks gets ignored if showBlocks and hideBlocks are provided together (if that makes sense).

What do you think?

That's totally reasonable, @fluiddot .

I finally had a few minutes to think about what tests I intend to write, and this is a crude list of them just in case you'd like to chime in. I hope they make sense since I wrote them pretty quickly.

Integration Tests

Given native.render hook is emitted, When setupBlocks takes place, Then ...

• Ensure that the jetpack/layout-grid block is registered if capable.
• Ensure that the jetpack/layout-grid block is not registered if incapable.
• Ensure that the jetpack/story block is registered if capable.
• Ensure that the jetpack/story block is not registered if incapable.
• Ensure that the jetpack/contact-info block is registered if capable.
• Ensure that the jetpack/contact-info block is not registered if incapable.
• Ensure that Jetpack Data is set if Jetpack is Active.
• Ensure that Jetpack Data is not set if Jetpack is Inactive.
• Ensure that the HIDE action is not dispatched if neither hideBlocks nor showBlocks types are provided.
• Ensure that the HIDE action is dispatched with hide types if hideBlocks types are provided and showBlocks types are not provided.
• Ensure that the HIDE action is dispatched with inverse show types if showBlocks types are provided and hideBlocks types are not provided.
• Ensure that the HIDE action is dispatched with inverse show types if showBlocks types are provided and hideBlocks types are provided.