Product Editor: disable Add button when no terms or options when creating variations (#48928)

* fix doc. improve doc

* disabe `Add` button when not terms nor options

* variant="primary" > isPrimary

* move helpers to utils file. improve jsdoc

* add tests for hasTermsOrOptions helper

* add isAttributeFilledOut unit tests

* changelog

* allow any type in unit tests

* show a toolip with not possible to add attributes

* fix wrong inline code comment

* expect the Add button is initially disabled

* check the `Add attributes` button is enabled

* changelog

* remove obvios jsdoc

* check all terms are accepted

* minor changes

* update tests

* remove console.log to dev purpose

* improve E2E testing remviing attr row

packages/js/product-editor/changelog/update-product-editor-disable-add-button-when-no-attributes

@@ -0,0 +1,4 @@
+Significance: patch
+Type: update
+
+Product Editor: disable `Add` button when no terms or options when creating variations

packages/js/product-editor/src/components/attribute-control/new-attribute-modal.tsx

@@ -15,7 +15,7 @@ import {
 	type ProductAttributeTerm,
 	type ProductAttribute,
 } from '@woocommerce/data';
-import { Button, Modal, Notice } from '@wordpress/components';
+import { Button, Modal, Notice, Tooltip } from '@wordpress/components';
 import { recordEvent } from '@woocommerce/tracks';
 
 /**
@@ -25,6 +25,7 @@ import { TRACKS_SOURCE } from '../../constants';
 import { AttributeTableRow } from './attribute-table-row';
 import type { EnhancedProductAttribute } from '../../hooks/use-product-attributes';
 import type { AttributesComboboxControlItem } from '../attribute-combobox-field/types';
+import { isAttributeFilledOut } from './utils';
 
 type NewAttributeModalProps = {
 	title?: string;
@@ -111,14 +112,9 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 		onAddAnother();
 	};
 
-	const hasTermsOrOptions = ( attribute: EnhancedProductAttribute ) => {
+	const isGlobalAttribute = (
-		return (
+		attribute: EnhancedProductAttribute
-			( attribute.terms && attribute.terms.length > 0 ) ||
+	): boolean => {
-			( attribute.options && attribute.options.length > 0 )
-		);
-	};
-
-	const isGlobalAttribute = ( attribute: EnhancedProductAttribute ) => {
 		return attribute.id !== 0;
 	};
 
@@ -136,16 +132,6 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 			: attribute.options;
 	};
 
-	const isAttributeFilledOut = (
-		attribute: EnhancedProductAttribute | null
-	): attribute is EnhancedProductAttribute => {
-		return (
-			attribute !== null &&
-			attribute.name.length > 0 &&
-			hasTermsOrOptions( attribute )
-		);
-	};
-
 	const getVisibleOrTrue = ( attribute: EnhancedProductAttribute ) =>
 		attribute.visible !== undefined ? attribute.visible : defaultVisibility;
 
@@ -238,6 +224,10 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 					// eslint-disable-next-line @typescript-eslint/no-explicit-any
 					setValue: ( name: string, value: any ) => void;
 				} ) => {
+					const isAddButtonDisabled = ! values.attributes.every(
+						( attr ) => isAttributeFilledOut( attr )
+					);
+
 					/**
 					 * Select the attribute in the form field.
 					 * If the attribute does not exist, create it.
@@ -327,14 +317,10 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 						index: number,
 						attribute?: EnhancedProductAttribute
 					) {
-						/*
-						 * By convention, it's a global attribute if the attribute ID is 0.
-						 * For global attributes, the field name suffix
-						 * to set the attribute terms is 'options',
-						 * for local attributes, the field name suffix is 'terms'.
-						 */
 						const attributeTermPropName =
-							attribute?.id === 0 ? 'options' : 'terms';
+							attribute && isGlobalAttribute( attribute )
+								? 'terms'
+								: 'options';
 
 						const fieldName = `attributes[${ index }].${ attributeTermPropName }`;
 
@@ -472,21 +458,34 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 								>
 									{ cancelLabel }
 								</Button>
-								<Button
+								<Tooltip
-									isPrimary
+									text={
-									label={ addAccessibleLabel }
+										isAddButtonDisabled
-									disabled={
+											? __(
-										values.attributes.length === 1 &&
+													'Add at least one attribute and one value. Press Enter to select.',
-										( values.attributes[ 0 ] === null ||
+													'woocommerce'
-											values.attributes[ 0 ] ===
+											  )
-												undefined )
+											: ''
-									}
-									onClick={ () =>
-										onAddingAttributes( values )
 									}
 								>
-									{ addLabel }
+									{ /*
-								</Button>
+									 * we need to wrap the button in a div to make the tooltip work,
+									 * since when the button is disabled, the tooltip is not shown.
+									 */ }
+									<div>
+										<Button
+											variant="primary"
+											label={ addAccessibleLabel }
+											showTooltip={ true }
+											disabled={ isAddButtonDisabled }
+											onClick={ () =>
+												onAddingAttributes( values )
+											}
+										>
+											{ addLabel }
+										</Button>
+									</div>
+								</Tooltip>
 							</div>
 						</Modal>
 					);

packages/js/product-editor/src/components/attribute-control/test/utils.spec.ts

@@ -1,15 +1,21 @@
 /**
  * External dependencies
  */
-import { ProductProductAttribute } from '@woocommerce/data';
+import {
+	ProductAttributeTerm,
+	ProductProductAttribute,
+} from '@woocommerce/data';
 
 /**
  * Internal dependencies
  */
 import {
 	getAttributeKey,
+	hasTermsOrOptions,
+	isAttributeFilledOut,
 	reorderSortableProductAttributePositions,
 } from '../utils';
+import { EnhancedProductAttribute } from '../../../hooks/use-product-attributes';
 
 const attributeList: Record< number | string, ProductProductAttribute > = {
 	15: {
@@ -74,3 +80,170 @@ describe( 'getAttributeKey', () => {
 		expect( getAttributeKey( attributeList.Quality ) ).toEqual( 'Quality' );
 	} );
 } );
+
+describe( 'hasTermsOrOptions', () => {
+	it( 'should return true if the attribute has local terms (options)', () => {
+		const attribute: EnhancedProductAttribute = {
+			name: 'Color',
+			id: 0,
+			slug: 'color',
+			position: 0,
+			visible: true,
+			variation: true,
+			options: [ 'Beige', 'black', 'Blue' ],
+		};
+
+		expect( hasTermsOrOptions( attribute ) ).toBe( true );
+	} );
+
+	it( 'should return true if the attribute has global terms', () => {
+		const terms: ProductAttributeTerm[] = [
+			{
+				id: 1,
+				name: 'red',
+				slug: 'red',
+				description: 'red color',
+				count: 1,
+				menu_order: 0,
+			},
+			{
+				id: 2,
+				name: 'blue',
+				slug: 'blue',
+				description: 'blue color',
+				count: 1,
+				menu_order: 1,
+			},
+			{
+				id: 3,
+				name: 'green',
+				slug: 'green',
+				description: 'green color',
+				count: 1,
+				menu_order: 2,
+			},
+		];
+
+		const attribute: EnhancedProductAttribute = {
+			name: 'Color',
+			id: 123,
+			slug: 'color',
+			position: 0,
+			visible: true,
+			variation: true,
+			terms,
+			options: [],
+		};
+
+		expect( hasTermsOrOptions( attribute ) ).toBe( true );
+	} );
+
+	it( 'should return false if the attribute has neither terms nor options', () => {
+		const attribute: EnhancedProductAttribute = {
+			name: 'Empty',
+			id: 999,
+			slug: 'empty',
+			position: 0,
+			visible: true,
+			variation: true,
+			options: [],
+		};
+		expect( hasTermsOrOptions( attribute ) ).toBe( false );
+	} );
+
+	it( 'should return false if the attribute is null', () => {
+		const attribute = null;
+		expect( hasTermsOrOptions( attribute ) ).toBe( false );
+	} );
+} );
+
+describe( 'isAttributeFilledOut', () => {
+	it( 'should return true if the attribute has a name and local terms (options)', () => {
+		const attribute: EnhancedProductAttribute = {
+			name: 'Color',
+			id: 0,
+			slug: 'color',
+			position: 0,
+			visible: true,
+			variation: true,
+			options: [ 'Beige', 'black', 'Blue' ],
+		};
+
+		expect( isAttributeFilledOut( attribute ) ).toBe( true );
+	} );
+
+	it( 'should return true if the attribute has a name and global terms', () => {
+		const terms: ProductAttributeTerm[] = [
+			{
+				id: 1,
+				name: 'red',
+				slug: 'red',
+				description: 'red color',
+				count: 1,
+				menu_order: 0,
+			},
+			{
+				id: 2,
+				name: 'blue',
+				slug: 'blue',
+				description: 'blue color',
+				count: 1,
+				menu_order: 1,
+			},
+			{
+				id: 3,
+				name: 'green',
+				slug: 'green',
+				description: 'green color',
+				count: 1,
+				menu_order: 2,
+			},
+		];
+
+		const attribute: EnhancedProductAttribute = {
+			name: 'Color',
+			id: 123,
+			slug: 'color',
+			position: 0,
+			visible: true,
+			variation: true,
+			terms,
+			options: [],
+		};
+
+		expect( isAttributeFilledOut( attribute ) ).toBe( true );
+	} );
+
+	it( 'should return false if the attribute has a name but no terms or options', () => {
+		const attribute: EnhancedProductAttribute = {
+			name: 'Empty',
+			id: 999,
+			slug: 'empty',
+			position: 0,
+			visible: true,
+			variation: true,
+			options: [],
+		};
+
+		expect( isAttributeFilledOut( attribute ) ).toBe( false );
+	} );
+
+	it( 'should return false if the attribute is null', () => {
+		const attribute = null;
+		expect( isAttributeFilledOut( attribute ) ).toBe( false );
+	} );
+
+	it( 'should return false if the attribute has no name', () => {
+		const attribute: EnhancedProductAttribute = {
+			name: '',
+			id: 0,
+			slug: 'color',
+			position: 0,
+			visible: true,
+			variation: true,
+			options: [ 'Beige', 'black', 'Blue' ],
+		};
+
+		expect( isAttributeFilledOut( attribute ) ).toBe( false );
+	} );
+} );

packages/js/product-editor/src/components/attribute-control/utils.ts

@@ -3,6 +3,11 @@
  */
 import type { ProductProductAttribute } from '@woocommerce/data';
 
+/**
+ * Internal dependencies
+ */
+import type { EnhancedProductAttribute } from '../../hooks/use-product-attributes';
+
 /**
  * Returns the attribute key. The key will be the `id` or the `name` when the id is 0.
  *
@@ -48,3 +53,26 @@ export function reorderSortableProductAttributePositions(
 		}
 	);
 }
+
+/**
+ * Checks if the given attribute has
+ * either terms (global attributes) or options (local attributes).
+ *
+ * @param {EnhancedProductAttribute} attribute - The attribute to check.
+ * @return {boolean} True if the attribute has terms or options, false otherwise.
+ */
+export const hasTermsOrOptions = (
+	attribute: EnhancedProductAttribute | null
+): boolean => !! ( attribute?.terms?.length || attribute?.options?.length );
+
+/**
+ * Checks if the given attribute is filled out,
+ * meaning it has a name and either terms or options.
+ *
+ * @param {EnhancedProductAttribute | null} attribute - The attribute to check.
+ * @return {attribute is EnhancedProductAttribute} - True if the attribute is filled out, otherwise false.
+ */
+export const isAttributeFilledOut = (
+	attribute: EnhancedProductAttribute | null
+): attribute is EnhancedProductAttribute =>
+	!! attribute?.name.length && hasTermsOrOptions( attribute );

plugins/woocommerce/changelog/update-e2e-check-add-button-when-creating-new-global-attribute-terms

@@ -0,0 +1,4 @@
+Significance: patch
+Type: update
+
+E2E: check the `Add` button when creating product variations in the new Product Editor

plugins/woocommerce/tests/e2e-pw/tests/merchant/products/block-editor/product-attributes-block-editor.spec.js

@@ -120,6 +120,11 @@ test(
 				.isVisible();
 
 			await page.waitForLoadState( 'domcontentloaded' );
+
+			// Confirm the Add button is disabled
+			await expect(
+				page.getByRole( 'button', { name: 'Add attributes' } )
+			).toBeDisabled();
 		} );
 
 		await test.step( 'create local attributes with terms', async () => {
@@ -131,10 +136,7 @@ test(
 				'.woocommerce-new-attribute-modal__table-row'
 			);
 
-			/*
+			// First, check the app loads the attributes,
-			 * First, check the app loads the attributes,
-			 * based on the Spinner visibility.
-			 */
 			await waitForGlobalAttributesLoaded( page );
 
 			for ( const attribute of attributesData ) {
@@ -168,15 +170,33 @@ test(
 					await FormTokenFieldInputLocator.press( 'Enter' );
 				}
 
-				await page.getByLabel( 'Add another attribute' ).click();
+				// Terms accepted, so the Add button should be enabled.
-			}
+				await expect(
+					page.getByRole( 'button', { name: 'Add attributes' } )
+				).toBeEnabled();
 
-			// Add the product attributes
+				await page.getByLabel( 'Add another attribute' ).click();
-			await page
+
-				.getByRole( 'button', { name: 'Add attributes' } )
+				// Attribute no defined, so the Add button should be disabled.
-				.click();
+				await expect(
+					page.getByRole( 'button', { name: 'Add attributes' } )
+				).toBeDisabled();
+			}
 		} );
 
+		// Remove the last row, as it was added by the last click on "Add another attribute".
+		await page
+			.getByRole( 'button', { name: 'Remove attribute' } )
+			.last()
+			.click();
+
+		await expect(
+			page.getByRole( 'button', { name: 'Add attributes' } )
+		).toBeEnabled();
+
+		// Add the product attributes
+		await page.getByRole( 'button', { name: 'Add attributes' } ).click();
+
 		await test.step( 'verify attributes in product editor', async () => {
 			// Locate the main attributes list element
 			const attributesListLocator = page.locator(