P2 807 variation picker validation

[increddibelly]

Summary

This PR can be summarized in the following changelog entry:

• [@yoast/schema-blocks] Adds validation logic to the Variation picker instruction, to make it invalid until a variation is picked.
• [@yoast/schema-blocks] Simplified schema-blocks validation logic.

Relevant technical choices:

• added BlockPresence to named BlockValidationResult constructors, to be more consistent in use of required / recommended blocks.
• reduced complexity of the validation process by reporting the worst case of the child issues, and not reporting a nested array of valid results.
• improved validation by reporting the appropriate instruction being validated instead of the block doing the validation; this is subtle but greatly reduces the complexity of the validation store.

Test instructions

Test this PR together with this MonoRepo PR This PR can be tested by following these steps:

overview

• Add a jobs posting to a new post
• open the Redux Devtools toolbar, and browse to the yoast-seo/schema-blocks store; open the validations.
• notice the yoast/job-posting has a score of 303, indicating that a required variation has not been chosen yet.
• open the yoast/job-posting, open the issues.
• notice that Block Instruction and Classname attribute have a result of 100, indicating they are validated and correct.
• notice that the innerblocks instruction has a score of 302, indicating a required block is missing.
• notice that the score of the parent yoast/job-posting is determined by its worst child result, innerblocks.
• open the innerblocks and notice it has 10 child results, in the range of :
  • 100(valid),
  • 101(unknown),
  • 202(missing recommended block),
  • 203(missing recommended variation),
  • 301(missing required attribute),
  • 302(missing required block),
  • 303(missing required variation),

we'll fix these issues one by one and verify that the validation picks up the new changes.

detail

• under the innerblocks validation result, the first issue should concern the job-title. Score should be 301 as it is required but empty.
• type anything in the job title; the score should immediately (at most 250ms) jump to 100 (Valid)
• next is the job/salary block which scores 202, because it is a recommended block but no variation has been picked.
• pick a salary type; either one is fine, and will make the job/salary's score jump to 201. it is a recommended block, but the chosen variation has not been completed.
• complete the chosen variation (fill the fields).
• notice that the job/salary block now scores 100, because the chosen variation has been completed and is now valid.
• publish and view the post; find the yoast schema in the html. no schema for job posting should be present, because some required blocks are still invalid (score of 300+)

  repeating until valid

• repeat the process of completing job blocks for all other blocks that have a score of 200 and higher. A validation score of 200+ indicates an issue with a recommended block (but this should still result in schema output when published) and a validation score of 300+ indicates a problem with a required block, this should prevent schema output for the job posting entirely.
• emptying a valid block should invalidate it (score of 201 / 301) depending on the required/recommended level of the block.
• removing a valid block should invalidate it (score of 202 / 302) depending on the required/recommended level of the block.

  conclusion

• once all required blocks are valid, validation should show "good job". After publishing the post, schema should now be stored in the html head section.

Impact check

• This PR affects the following parts of the plugin, which may require extra testing: *

UI changes

• [ ] This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Quality assurance

• [x] I have tested this code to the best of my abilities
• [ ] I have added unittests to verify the code works as intended

Fixes #

[andizer]

It seems that the fix which is merged in #1124 is broken by this pull. The code from that pull is present, which gives me that the develop branch has been merged correctly.

When I fill in the title, description and location the job keeps saying that the required blocks aren't filled in.

When I fill in all blocks with a random value, thus also the recommended blocks I get this:

[andizer]

I also wanted to mention that the fix itself seems to work. You only have to fill in all the other fields. When leaving a block to the variation selector I will the job posting as invalid. When selecting a variation the bullet will become green when a value is present.

So I think the regression from #1124 needs to be fixed before merging this PR.

[johannadevos]

@increddibelly @hansjovis I have a couple of remarks (this is not a complete review):

• I think the salary block and the location block should have different results when no variation is picked. For the salary block, it should be 201 (MissingRecommendedBlock), and for the location block it should be 302 (MissingRequiredBlock).
• I believe the original issue is not fixed in this PR, or at least I don't get that from the way the summary and the test instructions are written. The issue was that the variations themselves (so after you have picked one in the picker) are validated as alright even when they contain no text. For example, when you choose the salary range but haven't yet filled in the numbers for the salary. Can you make sure this is addressed in this PR?

[johannadevos]

CR:

• [x] I love the new validation logging, it's so much easier to read and debug! 🎉
• [x] Overall this PR looks like a very nice simplification of the validation code.
• [x] I pushed one commit, can you verify whether you agree with that change?
• [x] Your PR summary doesn't reflect the code changes very well. Please write a summary that covers the most important changes in the code. You can use multiple bullet points if you like, they would all end up in the changelog.
• [x] The same goes for the test instructions. First, no distinction is made between the salary and the location block, while the first is recommended and the second is required. This has impact on the desired behavior. Second, I see no instructions to test for required blocks being empty (for example, choosing the office variation but not filling in any text). Third, you should mention to test this in combination with the Premium PR. Can you please refer to the Definition of Done and update the test instructions?

I also did a smoke acceptance test while reviewing, and found the following:

• [x] If no salary variation has been chosen, the analysis stays red even when title, description and location have all been completed. This shouldn't be the case, since the salary isn't required. We want to see a green bullet when all required blocks have been completed, even when no salary variation has been chosen.

[hansjovis]

@increddibelly Code looks good to me.

Did you have a chance to look into this comment by Johanna?

Your PR summary doesn't reflect the code changes very well. Please write a summary that covers the most important changes in the code. You can use multiple bullet points if you like, they would all end up in the changelog.

I see that you updated the test instructions but did not touch the changelog items.

I tend to agree with Johanna's comment. With the added refactoring the PR adds more than validation logic to the variation picker and I think the changelog items should reflect that.

[hansjovis]

Acceptance test: 🚫

• Since the VariationPickers constructor name is used to generate the missing block message in the schema analysis, we get the above result.
  • One is for the missing recommended base salary, the other is for the missing required job location.

[hansjovis]

Acceptance test: ✅

There is still an issue, but we decided to split it off to P2-855.