Fix block issues introduced by the `block.json` updates

[m1r0]

trafficstars

Relates to https://github.com/Automattic/sensei/pull/5782#issuecomment-1271705659

This PR fixes multiple issues that were introduced by the updates to the block.json file and the blocks object changes. The most noticeable issue is that the Quiz Block was causing errors when you try to add it, but there are other logic related bugs as well.

The fix reverts to the previous block object structure which has the block.json metadata directly in it. It's hard to track down where the code logic uses this metadata, so I've decided to revert it to minimize risk.

Changes proposed in this Pull Request

• Revert the previous block object structure (before the updates in #5782).

Testing instructions

• Adding the Quiz Block should work.
• All other blocks should be working as before.

[codecov[bot]]

Codecov Report

Merging #5905 (778eae4) into trunk (1cf6e34) will increase coverage by 0.00%. The diff coverage is n/a.

@@            Coverage Diff            @@
##              trunk    #5905   +/-   ##
=========================================
  Coverage     44.44%   44.45%           
- Complexity     9051     9053    +2     
=========================================
  Files           441      441           
  Lines         32411    32410    -1     
  Branches        274      274           
=========================================
+ Hits          14405    14407    +2     
+ Misses        17801    17798    -3     
  Partials        205      205           

Impacted Files	Coverage Δ
includes/blocks/course-theme/class-ui.php	0.00% <0.00%> (ø)
...cludes/class-sensei-analysis-course-list-table.php	26.86% <0.00%> (+0.24%)	:arrow_up:
.../course-theme/class-sensei-course-theme-option.php	39.68% <0.00%> (+0.33%)	:arrow_up:
...r/class-sensei-reports-helper-date-range-trait.php	100.00% <0.00%> (+3.12%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update af8be49...778eae4. Read the comment docs.

[m1r0]

Hey @dadish, thanks for pointing that out! After further investigation, I think it should be fine removing that object as you suggested. Here's some more context:

The one thing that made me go in this direction was that the Loco Translate plugin not able to translate the strings from block.json. Because of that, I wasn't sure if the localization will work at all. (more info here)

To fix this I had to pass down the block.json metadata object to registerBlockType. If an object is passed to that function instead of the block name, the getBlockSettingsFromMetadata -> translateBlockSettingUsingI18nSchema is called and the Loco Translate localization works.

I just checked how things are at Sensei Pro and it looks like the plugin localization works even without passing the metadata object to registerBlockType, which is great! Unfortunately, Loco Translate is not able to modify the translated block.json strings there as well... I guess that might be a problem with Loco Translate itself.

In the end, I've decided to remove the metadata object but I'm not sure how to test the localization of the block.json strings. Do you have any idea how to test that? Thanks!

[dadish]

To fix this I had to pass down the block.json metadata object to registerBlockType. If an object is passed to that function instead of the block name, the getBlockSettingsFromMetadata -> translateBlockSettingUsingI18nSchema is called and the Loco Translate localization works.

I don't think you need to keep the block.json metadata separately in metadata property. You can simply pass the whole thing into registerBlockType function.

// block's index.js file
export default {
    ...metadata,
    edit: function () {},
    save: function () {}
}

// registering the block
blocks.forEach( ( block ) => {
  registerBlockType( block, block );
});

This should serve the same function.

In the end, I've decided to remove the metadata object but I'm not sure how to test the localization of the block.json strings. Do you have any idea how to test that? Thanks!

Unfortunately not. I did some local testing and tried different things, but could not figure out how to test language strings in block.json files.

[m1r0]

I don't think you need to keep the block.json metadata separately in metadata property. You can simply pass the whole thing into registerBlockType function.

blocks.forEach( ( block ) => {
 registerBlockType( block, block );
});

I did try this option but the localization is still not working.

I think this is because of how registerBlockType works - the first block argument (blockNameOrMetadata) gets localized, but the second block argument (settings) is not localized by translateBlockSettingUsingI18nSchema and it overwrites the first because it has the same keys... 🙂 I guess we should follow what registerBlockType is expecting to receive.

I can think of 3 options to resolve this:

1. Have a separate metadata key that only holds the block.json data and pass that as the first param to registerBlockType. This makes Loco Translate happy for some reason and the localization is testable.
2. Use the same approach as Sensei Pro which is to pass the block name as the first param to registerBlockType and the block.json data as the second param. The localization still works (not sure how) when you install the released package, but I'm not able to test locally using Loco Translate.
3. Refactor how the blocks are registered.

Is it safe to assume that if we use the same approach as Sensei Pro the localization will work? Worst case, the blocks title/description/keywords won't be localized and we will have to revisit.

We could also have the redundant metadata key for now and figure out how to refactor later. What do you think?

[dadish]

Is it safe to assume that if we use the same approach as Sensei Pro the localization will work? Worst case, the blocks title/description/keywords won't be localized and we will have to revisit.

I don't know if the translates will work the same way in Sensei as in Sensei Pro. But I agree that it's better if we have the similar ways of registering blocks and handling i18n for blocks.

We could also have the redundant metadata key for now and figure out how to refactor later. What do you think?

Agree, let's keep it. My only concern is that it's not clear from the code why the redundant metadata is needed. A person like me might think it's not needed and remove it. If we decide to keep it, could you at least add a comment explaining it a bit. You can add a comment only in assets/blocks/register-sensei-blocks.js file. That should be enough.

[m1r0]

Agree, let's keep it. My only concern is that it's not clear from the code why the redundant metadata is needed. A person like me might think it's not needed and remove it. If we decide to keep it, could you at least add a comment explaining it a bit. You can add a comment only in assets/blocks/register-sensei-blocks.js file. That should be enough.

Yep, makes total sense! I've added a few comments but also had to add a workaround for the settings object to not overwrite the localized metadata.

I think this whole approach of passing metadata and settings should be refactored, so the settings are not holding metadata as well. Unfortunately this might require a considerable amount of time. I've added a @todo for now because it's out of scope.

[m1r0]

Thanks @dadish for all the help!

[yscik]

5631824-zd-woothemes 5636709-zd-woothemes

[github-actions[bot]]

Support References

This comment is automatically generated. Please do not edit it.

• [ ] 5631824-zen
• [ ] 5636709-zen