From 3e363530fba643772ec7ccdf4b49ecd5d567a759 Mon Sep 17 00:00:00 2001 From: Lucio Giannotta Date: Mon, 13 Mar 2023 17:15:38 +0100 Subject: [PATCH 1/2] Fix Attribute terms sometimes being assigned children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attribute terms (e.g. “Blue”, “Yellow”) have their own `id` property which sometimes can overlap with their Taxonomy `id` (e.g. in the test case, “Blue” had the same `id` as the “Size” taxonomy), and the function building the tree representation for the `SearchListControl` would erroneously assign children to it. To fix this, we do an ugly patch here: we rename the `id` key of `AttributeTerm` when changed to a `SearchListItem` to `termId` instead, so we can differentiate. We do this because other items passed to `SearchListControl` might possibly have several layers of children, but we are sure that in the case of terms, we only have on level deep. This may need a better refactoring at some point. --- .../js/editor-components/search-list-control/types.ts | 8 ++++++-- .../js/editor-components/search-list-control/utils.tsx | 8 ++++++-- assets/js/utils/attributes.ts | 10 ++++++++-- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/assets/js/editor-components/search-list-control/types.ts b/assets/js/editor-components/search-list-control/types.ts index b7852a251ac..01ce66a56bb 100644 --- a/assets/js/editor-components/search-list-control/types.ts +++ b/assets/js/editor-components/search-list-control/types.ts @@ -38,11 +38,15 @@ export type SearchListItem = { breadcrumbs: string[]; children?: SearchListItem[]; count: number; - id: string | number; + id?: string | number; name: string; parent: number; + termId?: string | number; value: string; -}; +} & ( + | { id: string | number; termId?: never } + | { id?: never; termId: string | number } + ); export interface SearchListItemsContainerProps extends SearchListControlProps, diff --git a/assets/js/editor-components/search-list-control/utils.tsx b/assets/js/editor-components/search-list-control/utils.tsx index 886752ceba8..b2bb9d91e99 100644 --- a/assets/js/editor-components/search-list-control/utils.tsx +++ b/assets/js/editor-components/search-list-control/utils.tsx @@ -57,8 +57,12 @@ export const buildTermsTree = ( const fillWithChildren = ( terms: SearchListItem[] ): SearchListItem[] => { return terms.map( ( term ) => { - const children = termsByParent[ term.id ]; - builtParents.push( '' + term.id ); + const id = ( term.id ?? term.termId ) as string | number; + // If the object has `termId` property, it is an `AttributeTerm`. + // Those can't have children, but sometimes they have the same `id` + // as an `AttributeObject`, causing possible overlaps. + const children = term.termId ? null : termsByParent[ id ]; + builtParents.push( '' + id ); return { ...term, breadcrumbs: getParentsName( listById[ term.parent ] ), diff --git a/assets/js/utils/attributes.ts b/assets/js/utils/attributes.ts index 9a52106f26f..5091a746a48 100644 --- a/assets/js/utils/attributes.ts +++ b/assets/js/utils/attributes.ts @@ -55,14 +55,20 @@ export const convertAttributeObjectToSearchItem = ( ): SearchListItem => { const { count, id, name, parent } = attribute; + const base = isAttributeTerm( attribute ) + ? { + termId: id, + value: attribute.attr_slug, + } + : { id, value: '' }; + return { + ...base, count, - id, name, parent, breadcrumbs: [], children: [], - value: isAttributeTerm( attribute ) ? attribute.attr_slug : '', }; }; From 05b9b404015cad4c67981600c2cce8a133099ef9 Mon Sep 17 00:00:00 2001 From: Lucio Giannotta Date: Mon, 13 Mar 2023 17:20:20 +0100 Subject: [PATCH 2/2] Add comments with links to the PR --- assets/js/editor-components/search-list-control/types.ts | 3 +++ assets/js/editor-components/search-list-control/utils.tsx | 1 + 2 files changed, 4 insertions(+) diff --git a/assets/js/editor-components/search-list-control/types.ts b/assets/js/editor-components/search-list-control/types.ts index 01ce66a56bb..55baf500d42 100644 --- a/assets/js/editor-components/search-list-control/types.ts +++ b/assets/js/editor-components/search-list-control/types.ts @@ -43,6 +43,9 @@ export type SearchListItem = { parent: number; termId?: string | number; value: string; + // This is to avoid a problem with overlapping + // ids with terms and their parents. + // For more context: https://github.com/woocommerce/woocommerce-blocks/pull/8720 } & ( | { id: string | number; termId?: never } | { id?: never; termId: string | number } diff --git a/assets/js/editor-components/search-list-control/utils.tsx b/assets/js/editor-components/search-list-control/utils.tsx index b2bb9d91e99..2023b0ac1f1 100644 --- a/assets/js/editor-components/search-list-control/utils.tsx +++ b/assets/js/editor-components/search-list-control/utils.tsx @@ -61,6 +61,7 @@ export const buildTermsTree = ( // If the object has `termId` property, it is an `AttributeTerm`. // Those can't have children, but sometimes they have the same `id` // as an `AttributeObject`, causing possible overlaps. + // For more context: https://github.com/woocommerce/woocommerce-blocks/pull/8720 const children = term.termId ? null : termsByParent[ id ]; builtParents.push( '' + id ); return {