simplified validation by passing the presence parameter

renamed validation result for consistency
improved validation for VariationPicker

packages/schema-blocks/src/core/validation/BlockValidation.ts

@@ -22,7 +22,7 @@ export enum BlockValidation {
 	/** This block (OR some of its innerblocks) have a problem. The particular problem must be specified in BlockValidationResult.issues */
 	Invalid = 300,
 	/** This block has required attributes, but these attributes are missing or empty. */
-	MissingAttribute = 301,
+	MissingRequiredAttribute = 301,
 	/** This block is defined to require a particular inner block, but that block doesn't exist. */
 	MissingRequiredBlock = 302,
 	/** There may be only one of this type of block, but we found more than one. */

packages/schema-blocks/src/core/validation/BlockValidationResult.ts

@@ -65,13 +65,13 @@ export class BlockValidationResult {
 	 *
 	 * @param blockInstance The block instance.
 	 * @param [name] Optional name.
-	 * @param blockPresence The block type.
+	 * @param [blockPresence] The block type.
 	 *
 	 * @constructor
 	 */
 	static MissingAttribute( blockInstance: BlockInstance, name?: string, blockPresence?: BlockPresence ) {
 		const blockValidation: BlockValidation = ( blockPresence === BlockPresence.Required )
-			? BlockValidation.MissingAttribute : BlockValidation.MissingRecommendedAttribute;
+			? BlockValidation.MissingRequiredAttribute : BlockValidation.MissingRecommendedAttribute;
 
 		return new BlockValidationResult(
 			blockInstance.clientId,
@@ -82,18 +82,23 @@ export class BlockValidationResult {
 	}
 
 	/**
-	 * Named constructor for a 'missing required block' validation result.
+	 * Named constructor for a 'missing recommended / required block' validation result.
 	 *
 	 * @param name The name of the missing block.
+	 * @param [blockPresence] The block presence.
 	 *
 	 * @constructor
 	 */
-	static MissingRequiredBlock( name: string ) {
+	static MissingBlock( name: string, blockPresence?: BlockPresence ) {
+		if ( blockPresence === BlockPresence.Recommended ) {
+			return BlockValidationResult.MissingRecommendedBlock( name );
+		}
+
 		return new BlockValidationResult(
 			null,
 			name,
 			BlockValidation.MissingRequiredBlock,
-			BlockPresence.Required,
+			blockPresence || BlockPresence.Unknown,
 			sprintf(
 				/* Translators: %1$s expands to the block name. */
 				__( "The '%1$s' block is required but missing.", "yoast-schema-blocks" ),
@@ -109,7 +114,7 @@ export class BlockValidationResult {
 	 *
 	 * @constructor
 	 */
-	static MissingRecommendedBlock( name: string ) {
+	private static MissingRecommendedBlock( name: string ) {
 		return new BlockValidationResult(
 			null,
 			name,

packages/schema-blocks/src/functions/presenters/SidebarWarningPresenter.ts

@@ -48,7 +48,7 @@ function getValidationResult( clientId: string ): BlockValidationResult | null {
  */
 function someRequiredBlocksNotCompleted( issues: BlockValidationResult[] ) {
 	return issues.some( issue => issue.result === BlockValidation.MissingRequiredBlock ||
-		issue.result === BlockValidation.MissingAttribute );
+		issue.result === BlockValidation.MissingRequiredAttribute );
 }
 
 /**

packages/schema-blocks/src/functions/validators/innerBlocksValid.ts

@@ -18,29 +18,21 @@ import { getHumanReadableBlockName } from "../BlockHelper";
  * Finds all blocks that should/could be in the inner blocks, but aren't.
  *
  * @param existingBlocks The actual array of all inner blocks.
- * @param allBlocks      All of the blocks that should occur (required), or could occur (recommended) in the inner blocks.
+ * @param wantedBlocks   All of the blocks that should occur (required), or could occur (recommended) in the inner blocks.
  * @param blockPresence  The block presence.
  *
  * @returns {BlockValidationResult[]} The names of blocks that should/could occur but don't, with reason 'MissingBlock'.
  */
-function findMissingBlocks( existingBlocks: BlockInstance[], allBlocks: RequiredBlock[] | RecommendedBlock[],
+function findMissingBlocks( existingBlocks: BlockInstance[], wantedBlocks: RequiredBlock[] | RecommendedBlock[],
 	blockPresence: BlockPresence ): BlockValidationResult[] {
-	const missingBlocks = allBlocks.filter( block => {
+	const missingBlocks = wantedBlocks.filter( block => {
 		// If, in the existing blocks, there are not any blocks with the name of block, that block is missing.
 		return ! existingBlocks.some( existingBlock => existingBlock.name === block.name );
 	} );
 
-	// Return a BlockValidationResult for a required block.
-	if ( blockPresence === BlockPresence.Required ) {
-		// These blocks should've existed, but they don't.
-		return missingBlocks.map( missingBlock =>
-			BlockValidationResult.MissingRequiredBlock( getHumanReadableBlockName( missingBlock.name ) ),
-		);
-	}
-
-	// Return a BlockValidationResult for a recommended block.
+	// These blocks should've existed, but they don't.
 	return missingBlocks.map( missingBlock =>
-		BlockValidationResult.MissingRecommendedBlock( getHumanReadableBlockName( missingBlock.name ) ),
+		BlockValidationResult.MissingBlock( getHumanReadableBlockName( missingBlock.name ), blockPresence ),
 	);
 }
 

packages/schema-blocks/src/instructions/blocks/Title.tsx

@@ -32,6 +32,7 @@ class Title extends VariableTagRichText {
 		multiline: boolean;
 		label: string;
 		value: string;
+		required?: boolean;
 	};
 
 	/**
@@ -44,7 +45,7 @@ class Title extends VariableTagRichText {
 	 * @returns Whether the block is completed.
 	 */
 	private isCompleted( blockInstance: BlockInstance ): boolean {
-		return attributeNotEmpty( blockInstance, this.options.name ) && attributeExists( blockInstance, this.options.name );
+		return attributeExists( blockInstance, this.options.name ) && attributeNotEmpty( blockInstance, this.options.name );
 	}
 
 	/**
@@ -61,7 +62,8 @@ class Title extends VariableTagRichText {
 		const postTitle: string = post.title;
 
 		if ( ! this.isCompleted( blockInstance ) ) {
-			return BlockValidationResult.MissingAttribute( blockInstance, this.options.name );
+			const presence = this.options.required === true ? BlockPresence.Required : BlockPresence.Recommended;
+			return BlockValidationResult.MissingAttribute( blockInstance, this.constructor.name, presence );
 		}
 
 		if ( blockTitle.toLocaleLowerCase() === postTitle.toLocaleLowerCase() ) {

packages/schema-blocks/src/instructions/blocks/VariationPicker.tsx

@@ -5,7 +5,7 @@ import BlockLeaf from "../../core/blocks/BlockLeaf";
 import { BlockInstance } from "@wordpress/blocks";
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 import { createElement } from "@wordpress/element";
-import { BlockPresence, BlockValidationResult } from "../../core/validation";
+import { BlockPresence, BlockValidation, BlockValidationResult } from "../../core/validation";
 import VariationPickerPresenter from "../../functions/presenters/VariationPickerPresenter";
 
 /**
@@ -55,13 +55,13 @@ class VariationPicker extends BlockInstruction {
 	 * @returns {BlockValidationResult} The validation result.
 	 */
 	validate( blockInstance: BlockInstance ): BlockValidationResult {
-		const required: BlockPresence = this.options.required ? BlockPresence.Required : BlockPresence.Recommended;
+		const presence: BlockPresence = this.options.required ? BlockPresence.Required : BlockPresence.Recommended;
 
 		if ( includesAVariation( blockInstance ) ) {
-			return BlockValidationResult.Valid( blockInstance, this.constructor.name, required );
+			return BlockValidationResult.Valid( blockInstance, this.constructor.name, presence );
 		}
 
-		return BlockValidationResult.MissingAttribute( blockInstance, this.constructor.name, required );
+		return BlockValidationResult.MissingBlock( this.constructor.name, presence );
 	}
 }
 

packages/schema-blocks/tests/core/blocks/BlockInstruction.test.ts

@@ -100,12 +100,12 @@ describe( "The BlockInstruction class", () => {
 
 			const result = blockInstruction.validate( blockInstance );
 			expect( result.name ).toEqual( "TestBlockInstruction" );
-			expect( result.result ).toEqual( BlockValidation.MissingAttribute );
+			expect( result.result ).toEqual( BlockValidation.MissingRequiredAttribute );
 			expect( result.issues.length ).toEqual( 1 );
 
 			const issue = result.issues[ 0 ];
 			expect( issue.name ).toEqual( "TestBlockInstruction" );
-			expect( issue.result ).toEqual( BlockValidation.MissingAttribute );
+			expect( issue.result ).toEqual( BlockValidation.MissingRequiredAttribute );
 		} );
 
 		it( "considers a required attribute to be invalid if it is empty", () => {
@@ -123,12 +123,12 @@ describe( "The BlockInstruction class", () => {
 
 			const result = blockInstruction.validate( blockInstance );
 			expect( result.name ).toEqual( "TestBlockInstruction" );
-			expect( result.result ).toEqual( BlockValidation.MissingAttribute );
+			expect( result.result ).toEqual( BlockValidation.MissingRequiredAttribute );
 			expect( result.issues.length ).toEqual( 1 );
 
 			const issue = result.issues[ 0 ];
 			expect( issue.name ).toEqual( "TestBlockInstruction" );
-			expect( issue.result ).toEqual( BlockValidation.MissingAttribute );
+			expect( issue.result ).toEqual( BlockValidation.MissingRequiredAttribute );
 		} );
 	} );
 } );

packages/schema-blocks/tests/functions/presenters/SidebarWarningPresenter.test.ts

@@ -1,6 +1,7 @@
 import { BlockValidation, BlockValidationResult } from "../../../src/core/validation";
 import getWarnings, { createAnalysisMessages } from "../../../src/functions/presenters/SidebarWarningPresenter";
 import { BlockPresence } from "../../../src/core/validation/BlockValidationResult";
+import { BlockInstance } from "@wordpress/blocks";
 
 const validations: Record<string, BlockValidationResult> = {};
 
@@ -49,7 +50,7 @@ describe( "The SidebarWarningPresenter ", () => {
 		it( "creates a footer message when some required blocks have missing attributes.", () => {
 			const testcase = new BlockValidationResult( "1", "mijnblock", BlockValidation.Invalid, BlockPresence.Unknown );
 			testcase.issues.push(
-				new BlockValidationResult( null, "missingblockattribute", BlockValidation.MissingAttribute, BlockPresence.Unknown )
+				new BlockValidationResult( null, "missingblockattribute", BlockValidation.MissingRequiredAttribute, BlockPresence.Unknown ),
 			);
 
 			const result = createAnalysisMessages( testcase );
@@ -65,7 +66,7 @@ describe( "The SidebarWarningPresenter ", () => {
 
 		it( "creates warning messages for missing required blocks, with a footer message.", () => {
 			const testcase = new BlockValidationResult( "1", "mijnblock", BlockValidation.Invalid, BlockPresence.Required );
-			testcase.issues.push( BlockValidationResult.MissingRequiredBlock( "missingblock" ) );
+			testcase.issues.push( BlockValidationResult.MissingBlock( "missingblock", BlockPresence.Required ) );
 
 			const result = createAnalysisMessages( testcase );
 
@@ -81,14 +82,15 @@ describe( "The SidebarWarningPresenter ", () => {
 				},
 			);
 		} );
-		it( "creates a warning for missing recommended blocks, but when all the required blocks are present " +
+
+		it( "creates a warning for missing recommended blocks, but when no required blocks are missing, " +
 			"the conclusion should still be green.", () => {
 			const testcase = new BlockValidationResult( "1", "mijnblock", BlockValidation.Invalid, BlockPresence.Recommended );
 			testcase.issues.push(
-				BlockValidationResult.MissingRecommendedBlock( "missing recommended block" ),
+				BlockValidationResult.MissingBlock( "missing recommended block", BlockPresence.Recommended ),
 			);
 			testcase.issues.push(
-				BlockValidationResult.MissingRecommendedBlock( "missing recommended block 2" ),
+				BlockValidationResult.MissingBlock( "missing recommended block 2", BlockPresence.Recommended ),
 			);
 
 			const result = createAnalysisMessages( testcase );
@@ -107,6 +109,29 @@ describe( "The SidebarWarningPresenter ", () => {
 				color: "green",
 			} );
 		} );
+
+		it( "creates a warning for missing recommended blocks, but when all required blocks are valid, " +
+			"the conclusion should still be green.", () => {
+			const testcase = new BlockValidationResult( "1", "mijnblock", BlockValidation.Invalid, BlockPresence.Recommended );
+			testcase.issues.push(
+				BlockValidationResult.MissingBlock( "missing recommended block", BlockPresence.Recommended ),
+			);
+			testcase.issues.push(
+				BlockValidationResult.Valid( { name: "valid required block 1", clientId: "1" } as unknown as BlockInstance, BlockPresence.Required ),
+			);
+
+			const result = createAnalysisMessages( testcase );
+
+			expect( result.length ).toEqual( 2 );
+			expect( result[ 0 ] ).toEqual( {
+				text: "The 'missing recommended block' block is recommended but missing.",
+				color: "orange",
+			} );
+			expect( result[ 1 ] ).toEqual( {
+				text: "Good job! All required blocks have been completed.",
+				color: "green",
+			} );
+		} );
 	} );
 
 	describe( "The getWarnings method ", () => {
@@ -117,7 +142,7 @@ describe( "The SidebarWarningPresenter ", () => {
 
 			expect( result ).toEqual( [ {
 				text: "Good job! All required blocks have been completed.",
-				color: "green"
+				color: "green",
 			} ] );
 		} );
 
@@ -138,7 +163,7 @@ describe( "The SidebarWarningPresenter ", () => {
 
 		it( "creates a warning for a required block with validation problems.", () => {
 			const testcase = new BlockValidationResult( "1", "myBlock", BlockValidation.Invalid, BlockPresence.Required );
-			testcase.issues.push( BlockValidationResult.MissingRequiredBlock( "innerblock1" ) );
+			testcase.issues.push( BlockValidationResult.MissingBlock( "innerblock1", BlockPresence.Required ) );
 			validations[ "1" ] = testcase;
 
 			const result = getWarnings( "1" );

packages/schema-blocks/tests/functions/validators/innerBlocksValid.test.ts

@@ -67,7 +67,7 @@ const createBlockValidationResultTestArrangement = [
 	{ name: "missingrequiredblock", reason: BlockValidation.MissingRequiredBlock },
 	{ name: "missingrecommendedblock", reason: BlockValidation.MissingRecommendedBlock },
 	{ name: "redundantblock", reason: BlockValidation.TooMany },
-	{ name: "missingattributeblock", reason: BlockValidation.MissingAttribute },
+	{ name: "missingattributeblock", reason: BlockValidation.MissingRequiredAttribute },
 	{ name: "validblock", reason: BlockValidation.Valid },
 	{ name: "invalidblock", reason: BlockValidation.Invalid },
 	{ name: "unknown", reason: BlockValidation.Unknown },
@@ -251,7 +251,7 @@ describe( "The findSelfInvalidatedBlocks function", () => {
 		} as BlockInstance;
 
 		mockDefinition( "validBlock1", "validBlock", BlockValidation.Valid );
-		mockDefinition( "missingattributeblock1", "missingattributeblock", BlockValidation.MissingAttribute );
+		mockDefinition( "missingattributeblock1", "missingattributeblock", BlockValidation.MissingRequiredAttribute );
 
 		// Act.
 		const result: BlockValidationResult[] = innerBlocksValid.validateInnerblockTree( testBlock );
@@ -266,7 +266,7 @@ describe( "The findSelfInvalidatedBlocks function", () => {
 		const missingattributeblock = result.find( x => x.clientId === "missingattributeblock1" );
 		expect( missingattributeblock.name ).toEqual( "missingattributeblock" );
 		expect( missingattributeblock.result ).toEqual( BlockValidation.Invalid );
-		expect( missingattributeblock.issues[ 0 ].result ).toEqual( BlockValidation.MissingAttribute );
+		expect( missingattributeblock.issues[ 0 ].result ).toEqual( BlockValidation.MissingRequiredAttribute );
 	} );
 } );