Fix reordering items logic in attributes/options lists (#36296)

* Fix reordering list items bug

* Add tests

* Remove orphan comment

* Add changelog

* Rename const

* Update plugins/woocommerce-admin/client/products/fields/attribute-field/utils.ts

Co-authored-by: Joshua T Flowers <joshuatf@gmail.com>

* Rename `objectKey` to `attributeKey`

Co-authored-by: Fernando Marichal <contacto@fernandomarichal.com>
Co-authored-by: Joshua T Flowers <joshuatf@gmail.com>

plugins/woocommerce-admin/client/products/fields/attribute-field/attribute-field.tsx

@@ -245,7 +245,7 @@ export const AttributeField: React.FC< AttributeFieldProps > = ( {
 	const sortedAttributes = filteredAttributes.sort(
 		( a, b ) => a.position - b.position
 	);
-	const attributeKeyValues = filteredAttributes.reduce(
+	const attributeKeyValues = value.reduce(
 		(
 			keyValue: Record< number | string, ProductAttribute >,
 			attribute: ProductAttribute
@@ -253,7 +253,7 @@ export const AttributeField: React.FC< AttributeFieldProps > = ( {
 			keyValue[ getAttributeKey( attribute ) ] = attribute;
 			return keyValue;
 		},
-		{} as Record< number, ProductAttribute >
+		{} as Record< number | string, ProductAttribute >
 	);
 
 	const attribute = hydratedAttributes.find(
@@ -274,9 +274,17 @@ export const AttributeField: React.FC< AttributeFieldProps > = ( {
 		<div className="woocommerce-attribute-field">
 			<Sortable
 				onOrderChange={ ( items ) => {
+					const itemPositions = items.reduce(
+						( positions, { props }, index ) => {
+							positions[ getAttributeKey( props.attribute ) ] =
+								index;
+							return positions;
+						},
+						{} as Record< number | string, number >
+					);
 					onChange(
 						reorderSortableProductAttributePositions(
-							items,
+							itemPositions,
 							attributeKeyValues
 						)
 					);

plugins/woocommerce-admin/client/products/fields/attribute-field/test/utils.spec.ts

@@ -11,7 +11,7 @@ import {
 	reorderSortableProductAttributePositions,
 } from '../utils';
 
-const attributeList: Record< number, ProductAttribute > = {
+const attributeList: Record< number | string, ProductAttribute > = {
 	15: {
 		id: 15,
 		name: 'Automotive',
@@ -28,10 +28,18 @@ const attributeList: Record< number, ProductAttribute > = {
 		variation: true,
 		options: [ 'Beige', 'black', 'Blue' ],
 	},
+	Quality: {
+		id: 0,
+		name: 'Quality',
+		position: 2,
+		visible: true,
+		variation: false,
+		options: [ 'low', 'high' ],
+	},
 	3: {
 		id: 3,
 		name: 'Random',
-		position: 2,
+		position: 3,
 		visible: true,
 		variation: true,
 		options: [ 'Beige', 'black', 'Blue' ],
@@ -40,35 +48,25 @@ const attributeList: Record< number, ProductAttribute > = {
 
 describe( 'reorderSortableProductAttributePositions', () => {
 	it( 'should update product attribute positions depending on JSX.Element order', () => {
-		const elements = [
-			{ props: { attribute: attributeList[ '3' ] } },
-			{ props: { attribute: attributeList[ '15' ] } },
-			{ props: { attribute: attributeList[ '1' ] } },
-		] as JSX.Element[];
+		const elements = { 1: 0, 15: 1, 3: 2, Quality: 3 };
 		const newList = reorderSortableProductAttributePositions(
 			elements,
 			attributeList
 		);
 		expect( newList[ 0 ].position ).toEqual( 0 );
-		expect( newList[ 0 ].id ).toEqual( 3 );
-		expect( newList[ 1 ].position ).toEqual( 1 );
-		expect( newList[ 1 ].id ).toEqual( 15 );
-		expect( newList[ 2 ].position ).toEqual( 2 );
-		expect( newList[ 2 ].id ).toEqual( 1 );
+		expect( newList[ 0 ].id ).toEqual( 1 );
+		expect( newList[ 1 ].position ).toEqual( 2 );
+		expect( newList[ 1 ].id ).toEqual( 3 );
+		expect( newList[ 2 ].position ).toEqual( 1 );
+		expect( newList[ 2 ].id ).toEqual( 15 );
+		expect( newList[ 3 ].position ).toEqual( 3 );
+		expect( newList[ 3 ].id ).toEqual( 0 );
 	} );
 } );
 
 describe( 'getAttributeKey', () => {
-	attributeList[ '20' ] = {
-		id: 0,
-		name: 'Quality',
-		position: 3,
-		visible: true,
-		variation: true,
-		options: [ 'low', 'high' ],
-	};
 	it( 'should return the attribute key', () => {
 		expect( getAttributeKey( attributeList[ '15' ] ) ).toEqual( 15 );
-		expect( getAttributeKey( attributeList[ '20' ] ) ).toEqual( 'Quality' );
+		expect( getAttributeKey( attributeList.Quality ) ).toEqual( 'Quality' );
 	} );
 } );

plugins/woocommerce-admin/client/products/fields/attribute-field/utils.ts

@@ -16,27 +16,28 @@ export function getAttributeKey(
 }
 
 /**
- * Updates the position of a product attribute from the new items JSX.Element list.
+ * Updates the position of a product attribute from the new items list.
  *
- * @param { JSX.Element[] } items              list of JSX elements coming back from sortable container.
+ * @param { Object } items              key value pair of list items positions.
  * @param { Object } attributeKeyValues key value pair of product attributes.
  */
 export function reorderSortableProductAttributePositions(
-	items: JSX.Element[],
+	items: Record< number | string, number >,
 	attributeKeyValues: Record< number | string, ProductAttribute >
 ): ProductAttribute[] {
-	return items
-		.map( ( { props }, index ): ProductAttribute | undefined => {
-			const key = getAttributeKey( props?.attribute );
-			if ( attributeKeyValues[ key ] ) {
+	return Object.keys( attributeKeyValues ).map(
+		( attributeKey: number | string ): ProductAttribute => {
+			if ( ! isNaN( items[ attributeKey ] ) ) {
 				return {
-					...attributeKeyValues[ key ],
-					position: index,
+					...attributeKeyValues[ attributeKey ],
+					position: items[ attributeKey ],
 				};
 			}
-			return undefined;
-		} )
-		.filter( ( attr ): attr is ProductAttribute => attr !== undefined );
+			return {
+				...attributeKeyValues[ attributeKey ],
+			};
+		}
+	);
 }
 
 /**

plugins/woocommerce-admin/client/products/hooks/use-variations-order.ts

@@ -4,10 +4,6 @@
 import { useFormContext } from '@woocommerce/components';
 import type { ProductVariation } from '@woocommerce/data';
 
-/**
- * Internal dependencies
- */
-
 const KEY_SEPARATOR = ':';
 
 function getVariationKey( variation: ProductVariation ) {

plugins/woocommerce/changelog/fix-36255_reordering_list_items

@@ -0,0 +1,4 @@
+Significance: minor
+Type: fix
+
+Fix reordering list items error