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

ttahmouch · 2021-08-08T21:27:19Z

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:

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.
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.
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.
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.
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.)
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."
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

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.

This is meant to be treated as a `draft`. I haven't spent a lot of time trying to document everything yet. I did spend some time writing type declarations in JSDoc until my comments ended up becoming larger than the actual implementation itself to which I started to question the value at that point. I can include them if it makes sense to do so. I haven't added tests that reflect the requirements defined below. I did look for existing tests for this module and wasn't able to find them, but I may just not be understanding the general testing strategy. 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. 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: + If you pass in neither an allow list nor a disallow list, then proceed by using all core blocks. + 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. + 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. + 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. 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. The disallow list is preferred when the amount of blocks we'd like to present is large. 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.  0. We have many different types of default blocks that we assume to be registered with the application for various reasons, e.g., the `BlockList.Footer` component assumes that the block type `core/paragraph` always exists as it provides a button to the customer to 'Add paragraph block', and the `core/paragraph` block type is also registered as the `setDefaultBlockName` in the editor initialization. Other examples that take place in the editor initialization are things like all social block variants being registered with `registerBlockVariations`, the classic block being registered as the `setFreeformContentHandlerName`, the missing block being registered as the `setUnregisteredTypeHandlerName`, and the group block being registered as the `setGroupingBlockName`. When `core/paragraph` was not registered with the editor, the editor would crash when attempting to `createBlock('core/paragraph')` in the `BlockList.Footer`. I was working on allowing various default block names to be registered for different use cases, i.e., default, missing, social, classic, and group, to allow other block types to replace those classifications if for example the `core/paragraph` block type is provided in a disallow list so that a different block type could become the application default, or if `core/missing` is not provided in an allow list that a different block type could be used to represent unregistered block types, or even possibly forcing a `core/missing` to never be disallowed/unregistered since would be ironic. 1. Jetpack block types are registered asynchronously after the core block types are registered in the "Jetpack Editor Setup" flow. It uses `capabilities` provided via props to the editor initialization to determine if certain block types should be shown or hidden. However, it *always registers* the blocks whether or not they should be shown or hidden based on the `capabilities`. So it was difficult to dependency inject the props provided to the editor from the consuming native app to supersede the `capabilities` if the `jetpack/*` components were disallowed. It would have required possibly selecting the allow list from the store in the latest possible registration spot, i.e., the `registerBlockType` function, to ensure that no matter where a block type was being registered that the allow list didn't need to consulted through all possible flows leading to block registration. So this sparked an idea that I shouldn't actually be messing with the possibility that a block type be registered with the application. Rather, just allow all existing block types to be registered as normal, and then use the same logic that the "Jetpack Editor Setup" uses in order to determine whether or not a block type should be shown or hidden in the `BlockList`. It seemed particularly perfect because it is literally the exact same concept except that in Jetpack's case it is using `capabilities` instead of explicit block names. The only drawback that I wasn't able to figure out when spiking this concept was how to listen for a completion of all block type registrations. There doesn't appear to be a specific event defined for that. So I just ensured that I waited until after all `core/*` and `jetpack/*` block types were registered in order to determine if the previously registered block types should be hidden. This unfortunately meant that I couldn't dispatch `SHOW` or `HIDE` actions to the Redux Store from the main initialization flow, but rather I had to `setTimeout` to ensure it happened at a future tick of the event loop after all possible block type registrations occured. This appeared to be the same basic flow that was used to determine if the `jetpack/story` or `jetypack/contact-info` block types should be hidden. I'm not fond of this implementation, but I don't think there is a roobust way to handle listening for all registrations being completed at the moment. + `src/allowed-blocks-setup.js` - I added a new `allowed-blocks-setup` module to be executed after the Core and Jetpack Block Registrations took place to hide blocks not included in provided allow list or provided in a disallow list. + `src/index.js` - I execute the module flow mentioned above after all block type registrations have taken place. + `src/jetpack-editor-setup.js` - The changes in this file are just cleanup to extract out the `toggleBlock` function to the module scope, and shift the initialization of the default `jetpackState` to this module where it is relevant. These changes are technically unnecessary for the scope of this work.

peril-wordpress-mobile · 2021-08-08T21:27:24Z

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

fluiddot

The only drawback that I wasn't able to figure out when spiking this concept was how to listen for a completion of all block type registrations. There doesn't appear to be a specific event defined for that. So I just ensured that I waited until after all core/* and jetpack/* block types were registered in order to determine if the previously registered block types should be hidden. This unfortunately meant that I couldn't dispatch SHOW or HIDE actions to the Redux Store from the main initialization flow, but rather I had to setTimeout to ensure it happened at a future tick of the event loop after all possible block type registrations occured. This appeared to be the same basic flow that was used to determine if the jetpack/story or jetpack/contact-info block types should be hidden. I'm not fond of this implementation, but I don't think there is a robust way to handle listening for all registrations being completed at the moment.

The Jetpack blocks registration is conditioned by the capabilities, as per this comment, looks like the capabilities object obtained from the getSettings selector might not return the capabilities at the time the Jetpack setup is done and therefore a setTimeout is used. To be honest, I think the wait is not necessary and most likely there's a way to address it because the capabilities are passed through the initial props so they should be available, being this said, I wouldn't consider this flow as the best one to be based on.

Additionally and also related to Jetpack blocks, I think we're not considering the case of having a Jetpack block that was hidden by the capabilities and included in the allow list, in this case, we would be displaying a block that should be disabled, right?

src/allowed-blocks-setup.js

@fluiddot

+ `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.

fluiddot

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

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.

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 ✅ .

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' ],

src/allowed-blocks-setup.js

src/block-support/supported-blocks.json

src/test/index.js

fluiddot · 2021-08-16T14:16:22Z

src/allowed-blocks-setup.js

+	if ( mediaFilesCollectionBlock ) {
+		registerJetpackStoryBlock();
+	}
+
+	if ( contactInfoBlock ) {
+		registerJetpackContactInfoBlock();
+	}


I noticed that originally we were hiding these blocks instead of preventing registering them:

gutenberg-mobile/src/jetpack-editor-setup.js

Lines 68 to 69 in 6fea169

toggleBlock( capabilities.mediaFilesCollectionBlock, 'jetpack/story' );

toggleBlock( capabilities.contactInfoBlock, 'jetpack/contact-info' );

gutenberg-mobile/src/jetpack-editor-setup.js

Lines 47 to 53 in 6fea169

const toggleBlock = ( capability, blockName ) => {

if ( capability !== true ) {

dispatch( 'core/edit-post' ).hideBlockTypes( [ blockName ] );

} else {

dispatch( 'core/edit-post' ).showBlockTypes( [ blockName ] );

}

};

From my POV, I think it makes sense to register them conditionally to the props but I'm wondering if there was a reason to do it that way.

@illusaen do you have any insights regarding this topic?

ttahmouch · 2021-08-17T04:15:03Z

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:

the hideBlocks to just go ignored altogether and only acknowledge the showBlocks since it is more restrictive.
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 · 2021-08-17T09:49:21Z

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:

the hideBlocks to just go ignored altogether and only acknowledge the showBlocks since it is more restrictive.

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.

src/allowed-blocks-setup.js

mchowning · 2021-08-17T13:08:15Z

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.

…unction no longer does assignment, but rather simply maps one data model to another.

…H` field back to only be initialized if `isJetpackActive` is `true`. It is probably safe to be initialized regardless of if Jetpack is active because that global object has an `{jetpack: {is_active}}` field, but it's less likely to cause any side effects for the sake of this PR if it is reverted.

…ly a transitive dependency of `gutenberg-mobile` and there isn't a compelling enough reason to add it for simple `map` and `filter` array operations.

…efore `jetpack/layout-grid` and `jetpack/story` were added as supported blocks [per our conversation in GitHub](wordpress-mobile#3811 (comment)).

ttahmouch · 2021-08-18T18:36:25Z

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 · 2021-08-18T19:08:50Z

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 · 2021-08-18T20:36:55Z

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 · 2021-08-19T09:48:32Z

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 · 2021-08-19T23:49:08Z

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.

…Type initialization is comprehensively tested.

fluiddot

Thank you very much @ttahmouch for making these changes and adding such thorough integration tests for covering all the logic 🎊 .

I added a couple of comments but since this PR will probably remain paused for a while, there's no rush on reviewing them.

fluiddot · 2021-09-20T12:01:35Z

src/test/index.js

+		jest.unmock( '@wordpress/api-fetch' );
+		jest.unmock( '@wordpress/blocks' );


I'm wondering why we need to un-mock these packages, have you experienced any issues when running the tests?

fluiddot · 2021-09-20T12:07:02Z

src/test/index.js

+			// Arrange
+			const expectedCollectionTitle = 'Jetpack';
+			const props = { jetpackState: { isJetpackActive: false } };
+			require( '../index' );


I haven't tested it but would it be possible to import this only once as the rest of the import statements, located at the top of the file? I don't see anything on that file that would require to import it on every test 🤔, am I right?

ttahmouch marked this pull request as draft August 9, 2021 01:42

fluiddot reviewed Aug 9, 2021

View reviewed changes

src/allowed-blocks-setup.js Outdated Show resolved Hide resolved

src/allowed-blocks-setup.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ttahmouch mentioned this pull request Aug 13, 2021

TT | 3471 | "Introduce allow-list and disallow-list for blocks." Automattic/jetpack#20690

Closed

TT | 3471 | Updating the jetpack submodule reference.

52d5d05

fluiddot reviewed Aug 16, 2021

View reviewed changes

mchowning reviewed Aug 17, 2021

View reviewed changes

src/allowed-blocks-setup.js Outdated Show resolved Hide resolved

Tony Tahmouch added 4 commits August 17, 2021 15:23

TT | 3471 | Renaming setJetpackData to mapToJetpackData as this f…

4dd35e1

…unction no longer does assignment, but rather simply maps one data model to another.

TT | 3471 | Eliminate the dependency on lodash as it is not current…

8fdbde5

…ly a transitive dependency of `gutenberg-mobile` and there isn't a compelling enough reason to add it for simple `map` and `filter` array operations.

TT | 3471 | Reverting supported-blocks.json to its previous state b…

b0080b0

…efore `jetpack/layout-grid` and `jetpack/story` were added as supported blocks [per our conversation in GitHub](wordpress-mobile#3811 (comment)).

TT | 3471 | Ensure that the Jetpack initialization and Allowed Block …

a6fe710

…Type initialization is comprehensively tested.

fluiddot reviewed Sep 20, 2021

View reviewed changes

ttahmouch added sync-android-pr and removed sync-android-pr labels Dec 10, 2021

ttahmouch added sync:android and removed sync:android labels Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ttahmouch commented Aug 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 8, 2021 •

edited

Loading

fluiddot left a comment

This comment has been minimized.

fluiddot left a comment

fluiddot Aug 16, 2021

ttahmouch commented Aug 17, 2021

fluiddot commented Aug 17, 2021

mchowning commented Aug 17, 2021

ttahmouch commented Aug 18, 2021

ttahmouch commented Aug 18, 2021

mchowning commented Aug 18, 2021 •

edited

Loading

fluiddot commented Aug 19, 2021

ttahmouch commented Aug 19, 2021

fluiddot left a comment

fluiddot Sep 20, 2021

fluiddot Sep 20, 2021

	toggleBlock( capabilities.mediaFilesCollectionBlock, 'jetpack/story' );
	toggleBlock( capabilities.contactInfoBlock, 'jetpack/contact-info' );

	const toggleBlock = ( capability, blockName ) => {
	if ( capability !== true ) {
	dispatch( 'core/edit-post' ).hideBlockTypes( [ blockName ] );
	} else {
	dispatch( 'core/edit-post' ).showBlockTypes( [ blockName ] );
	}
	};

		jest.unmock( '@wordpress/api-fetch' );
		jest.unmock( '@wordpress/blocks' );

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

Are you sure you want to change the base?

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

Conversation

ttahmouch commented Aug 8, 2021 • edited Loading

Description

Idea

Notes

Previous Attempt

Remaining Work

Demo

peril-wordpress-mobile bot commented Aug 8, 2021 • edited Loading

fluiddot left a comment

Choose a reason for hiding this comment

This comment has been minimized.

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Aug 16, 2021

Choose a reason for hiding this comment

ttahmouch commented Aug 17, 2021

fluiddot commented Aug 17, 2021

mchowning commented Aug 17, 2021

ttahmouch commented Aug 18, 2021

ttahmouch commented Aug 18, 2021

Aside

mchowning commented Aug 18, 2021 • edited Loading

fluiddot commented Aug 19, 2021

ttahmouch commented Aug 19, 2021

Integration Tests

fluiddot left a comment

Choose a reason for hiding this comment

fluiddot Sep 20, 2021

Choose a reason for hiding this comment

fluiddot Sep 20, 2021

Choose a reason for hiding this comment

ttahmouch commented Aug 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Aug 8, 2021 •

edited

Loading

mchowning commented Aug 18, 2021 •

edited

Loading