Add new variation modal (#39522)

* Hook up add new variations options modal

* Fix duplicate logic and test

* Add changelog

* Match local attributes by name case incentive

* Remove console log

* Make use of some function instead of findIndex

packages/js/product-editor/changelog/add-38884_add_variation_modal

@@ -0,0 +1,4 @@
+Significance: minor
+Type: add
+
+Add new attribute modal to variations field and include tests for useProductAttributes hook.

packages/js/product-editor/src/blocks/variation-options/edit.tsx

@@ -3,8 +3,9 @@
  */
 import { __ } from '@wordpress/i18n';
 import { useBlockProps } from '@wordpress/block-editor';
-import { createElement } from '@wordpress/element';
+import { createElement, createInterpolateElement } from '@wordpress/element';
 import { ProductAttribute } from '@woocommerce/data';
+import { Link } from '@woocommerce/components';
 // eslint-disable-next-line @typescript-eslint/ban-ts-comment
 // @ts-ignore No types for this exist yet.
 // eslint-disable-next-line @woocommerce/dependency-group
@@ -43,6 +44,21 @@ export function Edit() {
 						'Add variation options',
 						'woocommerce'
 					),
+					newAttributeModalDescription: createInterpolateElement(
+						__(
+							'Select from existing <globalAttributeLink>global attributes</globalAttributeLink> or create options for buyers to choose on the product page. You can change the order later.',
+							'woocommerce'
+						),
+						{
+							globalAttributeLink: (
+								<Link
+									href="https://woocommerce.com/document/variable-product/#add-attributes-to-use-for-variations"
+									type="external"
+									target="_blank"
+								/>
+							),
+						}
+					),
 					attributeRemoveLabel: __(
 						'Remove variation option',
 						'woocommerce'

packages/js/product-editor/src/blocks/variations/edit.tsx

@@ -4,15 +4,23 @@
 import classNames from 'classnames';
 import type { BlockEditProps } from '@wordpress/blocks';
 import { Button } from '@wordpress/components';
-import { useEntityProp } from '@wordpress/core-data';
-import { Product } from '@woocommerce/data';
-import { createElement } from '@wordpress/element';
+import { Link } from '@woocommerce/components';
+import { Product, ProductAttribute } from '@woocommerce/data';
+import {
+	createElement,
+	useState,
+	createInterpolateElement,
+} from '@wordpress/element';
 import { __ } from '@wordpress/i18n';
 import {
 	useBlockProps,
 	// @ts-expect-error no exported member.
 	useInnerBlocksProps,
 } from '@wordpress/block-editor';
+// eslint-disable-next-line @typescript-eslint/ban-ts-comment
+// @ts-ignore No types for this exist yet.
+// eslint-disable-next-line @woocommerce/dependency-group
+import { useEntityProp, useEntityId } from '@wordpress/core-data';
 
 /**
  * Internal dependencies
@@ -20,6 +28,12 @@ import {
 import { sanitizeHTML } from '../../utils/sanitize-html';
 import { VariationsBlockAttributes } from './types';
 import { EmptyVariationsImage } from './empty-variations-image';
+import { NewAttributeModal } from '../../components/attribute-control/new-attribute-modal';
+import {
+	EnhancedProductAttribute,
+	useProductAttributes,
+} from '../../hooks/use-product-attributes';
+import { getAttributeId } from '../../components/attribute-control/utils';
 
 function hasAttributesUsedForVariations(
 	productAttributes: Product[ 'attributes' ]
@@ -32,10 +46,18 @@ export function Edit( {
 }: BlockEditProps< VariationsBlockAttributes > ) {
 	const { description } = attributes;
 
-	const [ productAttributes ] = useEntityProp< Product[ 'attributes' ] >(
-		'postType',
-		'product',
-		'attributes'
+	const [ isNewModalVisible, setIsNewModalVisible ] = useState( false );
+	const [ productAttributes, setProductAttributes ] = useEntityProp<
+		Product[ 'attributes' ]
+	>( 'postType', 'product', 'attributes' );
+
+	const { attributes: variationOptions, handleChange } = useProductAttributes(
+		{
+			allAttributes: productAttributes,
+			onChange: setProductAttributes,
+			isVariationAttributes: true,
+			productId: useEntityId( 'postType', 'product' ),
+		}
 	);
 
 	const hasAttributes = hasAttributesUsedForVariations( productAttributes );
@@ -54,6 +76,27 @@ export function Edit( {
 		{ templateLock: 'all' }
 	);
 
+	const openNewModal = () => {
+		setIsNewModalVisible( true );
+	};
+
+	const closeNewModal = () => {
+		setIsNewModalVisible( false );
+	};
+
+	const handleAdd = ( newOptions: EnhancedProductAttribute[] ) => {
+		handleChange( [
+			...newOptions.filter(
+				( newAttr ) =>
+					! variationOptions.find(
+						( attr: ProductAttribute ) =>
+							getAttributeId( newAttr ) === getAttributeId( attr )
+					)
+			),
+		] );
+		closeNewModal();
+	};
+
 	return (
 		<div { ...blockProps }>
 			<div className="wp-block-woocommerce-product-variations-fields__heading">
@@ -65,13 +108,41 @@ export function Edit( {
 					dangerouslySetInnerHTML={ sanitizeHTML( description ) }
 				/>
 				<div className="wp-block-woocommerce-product-variations-fields__heading-actions">
-					<Button variant="primary" aria-disabled="true">
+					<Button variant="primary" onClick={ openNewModal }>
 						{ __( 'Add variation options', 'woocommerce' ) }
 					</Button>
 				</div>
 			</div>
 
 			<div { ...innerBlockProps } />
+			{ isNewModalVisible && (
+				<NewAttributeModal
+					title={ __( 'Add variation options', 'woocommerce' ) }
+					description={ createInterpolateElement(
+						__(
+							'Select from existing <globalAttributeLink>global attributes</globalAttributeLink> or create options for buyers to choose on the product page. You can change the order later.',
+							'woocommerce'
+						),
+						{
+							globalAttributeLink: (
+								<Link
+									href="https://woocommerce.com/document/variable-product/#add-attributes-to-use-for-variations"
+									type="external"
+									target="_blank"
+								/>
+							),
+						}
+					) }
+					notice={ '' }
+					onCancel={ () => {
+						closeNewModal();
+					} }
+					onAdd={ handleAdd }
+					selectedAttributeIds={ variationOptions.map(
+						( attr ) => attr.id
+					) }
+				/>
+			) }
 		</div>
 	);
 }

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

@@ -48,6 +48,7 @@ type AttributeControlProps = {
 		emptyStateSubtitle?: string;
 		newAttributeListItemLabel?: string;
 		newAttributeModalTitle?: string;
+		newAttributeModalDescription?: string | React.ReactElement;
 		newAttributeModalNotice?: string;
 		customAttributeHelperMessage?: string;
 		attributeRemoveLabel?: string;
@@ -237,6 +238,7 @@ export const AttributeControl: React.FC< AttributeControlProps > = ( {
 			{ isNewModalVisible && (
 				<NewAttributeModal
 					title={ uiStrings.newAttributeModalTitle }
+					description={ uiStrings.newAttributeModalDescription }
 					notice={ uiStrings.newAttributeModalNotice }
 					onCancel={ () => {
 						closeNewModal();

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

@@ -31,6 +31,7 @@ import { getProductAttributeObject } from './utils';
 
 type NewAttributeModalProps = {
 	title?: string;
+	description?: string | React.ReactElement;
 	notice?: string;
 	attributeLabel?: string;
 	valueLabel?: string;
@@ -56,6 +57,7 @@ type AttributeForm = {
 
 export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 	title = __( 'Add attributes', 'woocommerce' ),
+	description = '',
 	notice = __(
 		'By default, attributes are filterable and visible on the product page. You can change these settings for each attribute separately later.',
 		'woocommerce'
@@ -235,6 +237,8 @@ export const NewAttributeModal: React.FC< NewAttributeModalProps > = ( {
 								</Notice>
 							) }
 
+							{ description && <p>{ description }</p> }
+
 							<div className="woocommerce-new-attribute-modal__body">
 								<table className="woocommerce-new-attribute-modal__table">
 									<thead>

packages/js/product-editor/src/hooks/test/use-product-attributes.test.ts

@@ -0,0 +1,387 @@
+/**
+ * External dependencies
+ */
+import { renderHook, cleanup } from '@testing-library/react-hooks';
+import { ProductAttribute, ProductAttributeTerm } from '@woocommerce/data';
+import { resolveSelect } from '@wordpress/data';
+
+/**
+ * Internal dependencies
+ */
+import { useProductAttributes } from '../use-product-attributes';
+
+const attributeTerms: Record< number, ProductAttributeTerm[] > = {
+	2: [
+		{
+			id: 64,
+			name: 'Blue',
+			slug: 'blue',
+			description: '',
+			menu_order: 0,
+			count: 2,
+		},
+		{
+			id: 76,
+			name: 'Green',
+			slug: 'green',
+			description: '',
+			menu_order: 0,
+			count: 1,
+		},
+		{
+			id: 63,
+			name: 'Red',
+			slug: 'red',
+			description: '',
+			menu_order: 0,
+			count: 2,
+		},
+		{
+			id: 65,
+			name: 'Velvet',
+			slug: 'velvet',
+			description: '',
+			menu_order: 0,
+			count: 2,
+		},
+	],
+	3: [
+		{
+			id: 64,
+			name: 'Small',
+			slug: 'small',
+			description: '',
+			menu_order: 0,
+			count: 2,
+		},
+		{
+			id: 76,
+			name: 'Medium',
+			slug: 'medium',
+			description: '',
+			menu_order: 0,
+			count: 1,
+		},
+		{
+			id: 63,
+			name: 'Large',
+			slug: 'large',
+			description: '',
+			menu_order: 0,
+			count: 2,
+		},
+	],
+};
+
+jest.useFakeTimers();
+jest.mock( '@wordpress/data', () => ( {
+	...jest.requireActual( '@wordpress/data' ),
+	resolveSelect: jest.fn().mockReturnValue( {
+		getProductAttributeTerms: jest
+			.fn()
+			.mockImplementation( ( { attribute_id } ) => {
+				return new Promise( ( resolve ) => {
+					setTimeout( () => {
+						if ( attributeTerms[ attribute_id ] ) {
+							return resolve( attributeTerms[ attribute_id ] );
+						}
+						return resolve( [] );
+					}, 100 );
+				} );
+			} ),
+	} ),
+} ) );
+
+const testAttributes: ProductAttribute[] = [
+	{
+		id: 0,
+		name: 'Local',
+		options: [ 'option 1', 'option 2' ],
+		position: 0,
+		variation: false,
+		visible: false,
+	},
+	{
+		id: 2,
+		name: 'Global: Color',
+		options: [ 'Red', 'Yellow' ],
+		position: 1,
+		variation: false,
+		visible: true,
+	},
+	{
+		id: 3,
+		name: 'Global: Size',
+		options: [ 'Small', 'Medium', 'Large' ],
+		position: 2,
+		variation: false,
+		visible: true,
+	},
+];
+
+describe( 'useProductAttributes', () => {
+	afterEach( () => {
+		cleanup();
+		( resolveSelect as jest.Mock ).mockClear();
+		jest.runOnlyPendingTimers();
+	} );
+
+	it( 'should return empty array when no attributes', async () => {
+		const { result, waitForNextUpdate } = renderHook(
+			useProductAttributes,
+			{
+				initialProps: {
+					allAttributes: [],
+					onChange: jest.fn(),
+					isVariationAttributes: false,
+					productId: 123,
+				},
+			}
+		);
+		await waitForNextUpdate();
+		expect( resolveSelect ).not.toHaveBeenCalled();
+		expect( result.current.attributes ).toEqual( [] );
+	} );
+
+	describe( 'handleChange', () => {
+		it( 'should call onChange when handleChange is called with updated attributes', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 1 ] },
+				{ ...testAttributes[ 2 ] },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: false,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			result.current.handleChange( [
+				allAttributes[ 0 ],
+				allAttributes[ 1 ],
+				{ ...testAttributes[ 0 ] },
+			] );
+			expect( onChange ).toHaveBeenCalledWith( [
+				{ ...allAttributes[ 0 ], position: 0 },
+				{ ...allAttributes[ 1 ], position: 1 },
+				{ ...testAttributes[ 0 ], variation: false, position: 2 },
+			] );
+		} );
+
+		it( 'should keep both variable and non variable as part of the onChange list, when isVariation is false', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 1 ], variation: true },
+				{ ...testAttributes[ 2 ], variation: true },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: false,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			result.current.handleChange( [ { ...testAttributes[ 0 ] } ] );
+			expect( onChange ).toHaveBeenCalledWith( [
+				{ ...testAttributes[ 0 ], variation: false, position: 0 },
+				{ ...allAttributes[ 0 ], position: 1 },
+				{ ...allAttributes[ 1 ], position: 2 },
+			] );
+		} );
+
+		it( 'should keep both variable and non variable as part of the onChange list, when isVariation is true', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 1 ] },
+				{ ...testAttributes[ 2 ] },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: true,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			result.current.handleChange( [ { ...testAttributes[ 0 ] } ] );
+			expect( onChange ).toHaveBeenCalledWith( [
+				{ ...allAttributes[ 0 ], position: 0 },
+				{ ...allAttributes[ 1 ], position: 1 },
+				{ ...testAttributes[ 0 ], variation: true, position: 2 },
+			] );
+		} );
+
+		it( 'should remove duplicate globals', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 1 ] },
+				{ ...testAttributes[ 2 ] },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: true,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			result.current.handleChange( [ { ...testAttributes[ 1 ] } ] );
+			expect( onChange ).toHaveBeenCalledWith( [
+				{ ...allAttributes[ 1 ], position: 0 },
+				{ ...allAttributes[ 0 ], position: 1, variation: true },
+			] );
+		} );
+
+		it( 'should remove duplicate locals by name', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 0 ] },
+				{ ...testAttributes[ 1 ] },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: true,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			result.current.handleChange( [ { ...testAttributes[ 0 ] } ] );
+			expect( onChange ).toHaveBeenCalledWith( [
+				{ ...allAttributes[ 1 ], position: 0 },
+				{ ...allAttributes[ 0 ], position: 1, variation: true },
+			] );
+		} );
+	} );
+
+	describe( 'is not variation', () => {
+		it( 'should filter out variation attributes', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 0 ] },
+				{ ...testAttributes[ 1 ], variation: true },
+				{ ...testAttributes[ 2 ] },
+			];
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange: jest.fn(),
+						isVariationAttributes: false,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			expect( result.current.attributes.length ).toBe( 2 );
+			// Sets global attributes first.
+			expect( result.current.attributes[ 0 ].name ).toEqual(
+				allAttributes[ 2 ].name
+			);
+			expect( result.current.attributes[ 1 ].name ).toEqual(
+				allAttributes[ 0 ].name
+			);
+		} );
+
+		it( 'should update array if allAttributes update', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 0 ] },
+				{ ...testAttributes[ 1 ], variation: true },
+				{ ...testAttributes[ 2 ] },
+			];
+			const onChange = jest.fn();
+			const { result, rerender, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: false,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			expect( result.current.attributes.length ).toBe( 2 );
+
+			const filteredAttributes = [
+				allAttributes[ 0 ],
+				allAttributes[ 1 ],
+			];
+
+			rerender( {
+				allAttributes: filteredAttributes,
+				onChange,
+				isVariationAttributes: false,
+				productId: 123,
+			} );
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			expect( result.current.attributes.length ).toBe( 1 );
+			expect( result.current.attributes[ 0 ].name ).toEqual(
+				allAttributes[ 0 ].name
+			);
+		} );
+
+		it( 'sets terms for any global attributes and options to empty array', async () => {
+			const allAttributes = [
+				{ ...testAttributes[ 0 ] },
+				{ ...testAttributes[ 1 ] },
+				{ ...testAttributes[ 2 ] },
+			];
+			const onChange = jest.fn();
+			const { result, waitForNextUpdate } = renderHook(
+				useProductAttributes,
+				{
+					initialProps: {
+						allAttributes,
+						onChange,
+						isVariationAttributes: false,
+						productId: 123,
+					},
+				}
+			);
+			jest.runOnlyPendingTimers();
+			await waitForNextUpdate();
+			expect( result.current.attributes.length ).toBe( 3 );
+			expect( result.current.attributes[ 0 ].terms ).toEqual(
+				attributeTerms[ result.current.attributes[ 0 ].id ]
+			);
+			expect( result.current.attributes[ 0 ].options ).toEqual( [] );
+			expect( result.current.attributes[ 1 ].terms ).toEqual(
+				attributeTerms[ result.current.attributes[ 1 ].id ]
+			);
+			expect( result.current.attributes[ 1 ].options ).toEqual( [] );
+		} );
+	} );
+} );

packages/js/product-editor/src/hooks/use-product-attributes.ts

@@ -26,25 +26,24 @@ export type EnhancedProductAttribute = ProductAttribute & {
 	visible?: boolean;
 };
 
+const getFilteredAttributes = (
+	attr: ProductAttribute[],
+	isVariationAttributes: boolean
+) => {
+	return isVariationAttributes
+		? attr.filter( ( attribute ) => !! attribute.variation )
+		: attr.filter( ( attribute ) => ! attribute.variation );
+};
+
 export function useProductAttributes( {
 	allAttributes = [],
 	isVariationAttributes = false,
 	onChange,
 	productId,
 }: useProductAttributesProps ) {
-	const getFilteredAttributes = () => {
-		return isVariationAttributes
-			? allAttributes.filter( ( attribute ) => !! attribute.variation )
-			: allAttributes.filter( ( attribute ) => ! attribute.variation );
-	};
-
 	const [ attributes, setAttributes ] = useState<
 		EnhancedProductAttribute[]
-	>( getFilteredAttributes() );
-	const [ localAttributes, globalAttributes ]: ProductAttribute[][] = sift(
-		attributes,
-		( attr: ProductAttribute ) => attr.id === 0
-	);
+	>( getFilteredAttributes( allAttributes, isVariationAttributes ) );
 
 	const fetchTerms = useCallback(
 		( attributeId: number ) => {
@@ -78,27 +77,72 @@ export function useProductAttributes( {
 		};
 	};
 
-	const getAugmentedAttributes = ( atts: ProductAttribute[] ) => {
+	const getAugmentedAttributes = (
+		atts: ProductAttribute[],
+		variation: boolean,
+		startPosition: number
+	) => {
 		return atts.map( ( attribute, index ) => ( {
 			...attribute,
-			variation: isVariationAttributes,
-			position: attributes.length + index,
+			variation,
+			position: startPosition + index,
 		} ) );
 	};
 
 	const handleChange = ( newAttributes: ProductAttribute[] ) => {
-		const augmentedAttributes = getAugmentedAttributes( newAttributes );
-		const otherAttributes = isVariationAttributes
+		let otherAttributes = isVariationAttributes
 			? allAttributes.filter( ( attribute ) => ! attribute.variation )
 			: allAttributes.filter( ( attribute ) => !! attribute.variation );
-		setAttributes( augmentedAttributes );
-		onChange( [ ...otherAttributes, ...augmentedAttributes ] );
+
+		// Remove duplicate global attributes.
+		otherAttributes = otherAttributes.filter( ( attr ) => {
+			if (
+				attr.id > 0 &&
+				newAttributes.some( ( a ) => a.id === attr.id )
+			) {
+				return false;
+			}
+			// Local attributes we check by name.
+			if (
+				attr.id === 0 &&
+				newAttributes.some(
+					( a ) => a.name.toLowerCase() === attr.name.toLowerCase()
+				)
+			) {
+				return false;
+			}
+			return true;
+		} );
+		const newAugmentedAttributes = getAugmentedAttributes(
+			newAttributes,
+			isVariationAttributes,
+			isVariationAttributes ? otherAttributes.length : 0
+		);
+		const otherAugmentedAttributes = getAugmentedAttributes(
+			otherAttributes,
+			! isVariationAttributes,
+			isVariationAttributes ? 0 : newAttributes.length
+		);
+
+		if ( isVariationAttributes ) {
+			onChange( [
+				...otherAugmentedAttributes,
+				...newAugmentedAttributes,
+			] );
+		} else {
+			onChange( [
+				...newAugmentedAttributes,
+				...otherAugmentedAttributes,
+			] );
+		}
 	};
 
 	useEffect( () => {
-		if ( ! getFilteredAttributes().length || attributes.length ) {
-			return;
-		}
+		const [ localAttributes, globalAttributes ]: ProductAttribute[][] =
+			sift(
+				getFilteredAttributes( allAttributes, isVariationAttributes ),
+				( attr: ProductAttribute ) => attr.id === 0
+			);
 
 		Promise.all(
 			globalAttributes.map( ( attr ) => fetchTerms( attr.id ) )
@@ -110,7 +154,7 @@ export function useProductAttributes( {
 				...localAttributes,
 			] );
 		} );
-	}, [ allAttributes, attributes, fetchTerms ] );
+	}, [ allAttributes, isVariationAttributes, fetchTerms ] );
 
 	return {
 		attributes,