Skip to content

Commit

Permalink
fix(headless): consider only selected value ancestry for analytics, h…
Browse files Browse the repository at this point in the history
…istory and breadcrumbs (#3137)

* expunge parents

* clean+ut

* refactor catfacetval interfaces to reflect common props

https://coveord.atlassian.net/browse/KIT-2696

* clean up getActiveValueFromValueTree

* Apply suggestions from code review

Co-authored-by: dmbrooke <38883189+dmbrooke@users.noreply.github.com>

---------

Co-authored-by: dmbrooke <38883189+dmbrooke@users.noreply.github.com>
  • Loading branch information
louis-bompart and dmbrooke authored Aug 28, 2023
1 parent fec8cbf commit fdc644b
Show file tree
Hide file tree
Showing 17 changed files with 182 additions and 111 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
import {categoryFacetSetReducer as categoryFacetSet} from '../../../../features/facets/category-facet-set/category-facet-set-slice';
import {defaultCategoryFacetOptions} from '../../../../features/facets/category-facet-set/category-facet-set-slice';
import {
getActiveValueFromValueTree,
findActiveValueAncestry,
partitionIntoParentsAndValues,
} from '../../../../features/facets/category-facet-set/category-facet-utils';
import {
Expand Down Expand Up @@ -44,7 +44,7 @@ jest.mock(
);

const {
getActiveValueFromValueTree: actualGetActiveValueFromValueTree,
findActiveValueAncestry: actualFindActiveValueAncestry,
partitionIntoParentsAndValues: actualPartitionIntoParentsAndValues,
} = jest.requireActual(
'../../../../features/facets/category-facet-set/category-facet-utils'
Expand All @@ -56,9 +56,7 @@ describe('category facet', () => {
let state: SearchAppState;
let engine: MockSearchEngine;
let categoryFacet: CoreCategoryFacet;
const getActiveValueFromValueTreeMock = jest.mocked(
getActiveValueFromValueTree
);
const findActiveValueAncestryMock = jest.mocked(findActiveValueAncestry);
const partitionIntoParentsAndValuesMock = jest.mocked(
partitionIntoParentsAndValues
);
Expand All @@ -79,8 +77,8 @@ describe('category facet', () => {
facetId,
field: 'geography',
};
getActiveValueFromValueTreeMock.mockImplementation(
actualGetActiveValueFromValueTree
findActiveValueAncestryMock.mockImplementation(
actualFindActiveValueAncestry
);
partitionIntoParentsAndValuesMock.mockImplementation(
actualPartitionIntoParentsAndValues
Expand Down Expand Up @@ -135,13 +133,13 @@ describe('category facet', () => {
expect(categoryFacet.subscribe).toBeDefined();
});

it('#state.activeValue is the return value of #getActiveValueFromValueTree', () => {
it('#state.activeValue is the return value of #findActiveValueAncestry', () => {
const referencedReturnValue = buildMockCategoryFacetValue();
getActiveValueFromValueTreeMock.mockReturnValueOnce(referencedReturnValue);
findActiveValueAncestryMock.mockReturnValueOnce([referencedReturnValue]);

initCategoryFacet();

expect(getActiveValueFromValueTree).toBeCalledTimes(1);
expect(findActiveValueAncestry).toBeCalledTimes(1);
expect(categoryFacet.state.activeValue).toBe(referencedReturnValue);
});

Expand Down Expand Up @@ -281,6 +279,12 @@ describe('category facet', () => {
expect(categoryFacet.state.parents).toEqual([selectedValue]);
});

it('#state.selectedValueAncestry contains the selected leaf value', () => {
expect(categoryFacet.state.selectedValueAncestry).toEqual([
selectedValue,
]);
});

it('#state.values is an empty array', () => {
expect(categoryFacet.state.values).toEqual([]);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ import {categoryFacetRequestSelector} from '../../../../features/facets/category
import {defaultCategoryFacetOptions} from '../../../../features/facets/category-facet-set/category-facet-set-slice';
import {categoryFacetSetReducer as categoryFacetSet} from '../../../../features/facets/category-facet-set/category-facet-set-slice';
import {
getActiveValueFromValueTree,
findActiveValueAncestry,
partitionIntoParentsAndValues,
} from '../../../../features/facets/category-facet-set/category-facet-utils';
import {CategoryFacetSortCriterion} from '../../../../features/facets/category-facet-set/interfaces/request';
import {CategoryFacetValue} from '../../../../features/facets/category-facet-set/interfaces/response';
import {
CategoryFacetValue,
CategoryFacetValueCommon,
} from '../../../../features/facets/category-facet-set/interfaces/response';
import {categoryFacetSearchSetReducer as categoryFacetSearchSet} from '../../../../features/facets/facet-search-set/category/category-facet-search-set-slice';
import {defaultFacetSearchOptions} from '../../../../features/facets/facet-search-set/facet-search-reducer-helpers';
import {isFacetLoadingResponseSelector} from '../../../../features/facets/facet-set/facet-set-selectors';
Expand All @@ -50,6 +53,7 @@ import {
} from './headless-core-category-facet-options';

export type {
CategoryFacetValueCommon,
CategoryFacetValue,
CategoryFacetOptions,
CategoryFacetSearchOptions,
Expand Down Expand Up @@ -170,10 +174,17 @@ export interface CoreCategoryFacetState {

/**
* The facet's values.
* @deprecated use `valuesAsTrees` instead.
* @deprecated use `selectedValueAncestry` instead.
*/
values: CategoryFacetValue[];

/**
* The selected facet values ancestry.
* The first element is the "root" of the selected value ancestry tree.
* The last element is the selected value itself.
*/
selectedValueAncestry: CategoryFacetValue[];

/** The facet's active `sortCriterion`. */
sortCriteria: CategoryFacetSortCriterion;

Expand Down Expand Up @@ -270,7 +281,7 @@ export interface CategoryFacetSearchResult {
count: number;

/**
* The hierarchical path to the facet value.
* The hierarchical path to the selected facet value.
*/
path: string[];
}
Expand Down Expand Up @@ -385,7 +396,10 @@ export function buildCoreCategoryFacet(
const isHierarchical =
valuesAsTrees.some((value) => value.children.length > 0) ?? false;
const {parents, values} = partitionIntoParentsAndValues(response?.values);
const activeValue = getActiveValueFromValueTree(valuesAsTrees);
const selectedValueAncestry = findActiveValueAncestry(valuesAsTrees);
const activeValue = selectedValueAncestry.length
? selectedValueAncestry[selectedValueAncestry.length - 1]
: undefined;
const hasActiveValues = !!activeValue;
const canShowMoreValues =
activeValue?.moreValuesAvailable ??
Expand All @@ -398,6 +412,7 @@ export function buildCoreCategoryFacet(
return {
facetId,
parents,
selectedValueAncestry,
values,
isHierarchical,
valuesAsTrees,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {RecordValue, Schema} from '@coveo/bueno';
import {CoreEngine} from '../../../app/engine';
import {partitionIntoParentsAndValues} from '../../../features/facets/category-facet-set/category-facet-utils';
import {findActiveValueAncestry} from '../../../features/facets/category-facet-set/category-facet-utils';
import {FacetValueRequest} from '../../../features/facets/facet-set/interfaces/request';
import {RangeValueRequest} from '../../../features/facets/range-facets/generic/interfaces/range-facet';
import {getQueryInitialState} from '../../../features/query/query-state';
Expand Down Expand Up @@ -222,9 +222,7 @@ function getCategoryFacets(state: Partial<SearchParametersState>) {
const cf = Object.entries(state.categoryFacetSet)
.filter(([facetId]) => state.facetOptions?.facets[facetId]?.enabled ?? true)
.map(([facetId, slice]) => {
const {parents} = partitionIntoParentsAndValues(
slice.request.currentValues
);
const parents = findActiveValueAncestry(slice.request.currentValues);
const selectedValues = parents.map((p) => p.value);

return selectedValues.length ? {[facetId]: selectedValues} : {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ import {
CategoryFacetSearchState,
CategoryFacetState,
CategoryFacetValue,
CategoryFacetValueCommon,
CoreCategoryFacet,
CoreCategoryFacetState,
} from '../../../core/facets/category-facet/headless-core-category-facet';

export type {
CategoryFacetValueCommon,
CategoryFacetValue,
CategoryFacetOptions,
CategoryFacetSearchOptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import {configuration} from '../../../app/common-reducers';
import {ProductListingEngine} from '../../../app/product-listing-engine/product-listing-engine';
import {categoryFacetSetReducer as categoryFacetSet} from '../../../features/facets/category-facet-set/category-facet-set-slice';
import {CategoryFacetSortCriterion} from '../../../features/facets/category-facet-set/interfaces/request';
import {CategoryFacetValue} from '../../../features/facets/category-facet-set/interfaces/response';
import {
CategoryFacetValue,
CategoryFacetValueCommon,
} from '../../../features/facets/category-facet-set/interfaces/response';
import {categoryFacetSearchSetReducer as categoryFacetSearchSet} from '../../../features/facets/facet-search-set/category/category-facet-search-set-slice';
import {
logFacetClearAll,
Expand Down Expand Up @@ -40,6 +43,7 @@ import {buildCategoryFacetSearch} from './headless-product-listing-category-face

export type {
CategoryFacetValue,
CategoryFacetValueCommon,
CategoryFacetOptions,
CategoryFacetSearchOptions,
CategoryFacetProps,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
FacetResponseSection,
} from '../facet-set/facet-set-selectors';
import {AnyFacetResponse} from '../generic/interfaces/generic-facet-response';
import {partitionIntoParentsAndValues} from './category-facet-utils';
import {findActiveValueAncestry} from './category-facet-utils';
import {CategoryFacetResponse} from './interfaces/response';

function isCategoryFacetResponse(
Expand Down Expand Up @@ -38,17 +38,13 @@ export const categoryFacetResponseSelectedValuesSelector = (
facetId: string
) => {
const facetResponse = categoryFacetResponseSelector(state, facetId);
const parentsAndValues = partitionIntoParentsAndValues(facetResponse?.values);
return parentsAndValues.parents;
return findActiveValueAncestry(facetResponse?.values ?? []);
};

export const categoryFacetRequestSelectedValuesSelector = (
state: CategoryFacetSection,
facetId: string
) => {
const facetRequest = categoryFacetRequestSelector(state, facetId);
const parentsAndValues = partitionIntoParentsAndValues(
facetRequest?.currentValues
);
return parentsAndValues.parents;
return findActiveValueAncestry(facetRequest?.currentValues ?? []);
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
CategoryFacetSetState,
getCategoryFacetSetInitialState,
} from './category-facet-set-state';
import {partitionIntoParentsAndValues} from './category-facet-utils';
import {findActiveValueAncestry} from './category-facet-utils';
import {CategoryFacetOptionalParameters} from './interfaces/options';
import {
CategoryFacetRequest,
Expand Down Expand Up @@ -285,11 +285,7 @@ function isRequestInvalid(
request: CategoryFacetRequest,
response: CategoryFacetResponse
) {
const requestParents = partitionIntoParentsAndValues(
request.currentValues
).parents;
const responseParents = partitionIntoParentsAndValues(
response.values
).parents;
const requestParents = findActiveValueAncestry(request.currentValues);
const responseParents = findActiveValueAncestry(response.values);
return requestParents.length !== responseParents.length;
}
Original file line number Diff line number Diff line change
@@ -1,57 +1,58 @@
import {buildMockCategoryFacetValue} from '../../../test/mock-category-facet-value';
import {getActiveValueFromValueTree} from './category-facet-utils';
import {findActiveValueAncestry} from './category-facet-utils';
import {CategoryFacetValue} from './interfaces/response';

describe('#getActiveValueFromValueTree', () => {
it("should return undefined if there's no selected values", () => {
expect(getActiveValueFromValueTree([])).toBeUndefined();
describe('#findActiveValueAncestry', () => {
it("should return an empty array if there's no selected values", () => {
expect(findActiveValueAncestry([])).toEqual([]);
});

it.each([
{
caseName: 'doubleRootValues',
getValues: (expectedValue: CategoryFacetValue) => [
expectedValue,
buildMockCategoryFacetValue({state: 'selected'}),
],
},
{
caseName: 'rootAndNestedValues',
getValues: (expectedValue: CategoryFacetValue) => [
buildMockCategoryFacetValue({children: [expectedValue]}),
buildMockCategoryFacetValue({state: 'selected'}),
],
},
{
caseName: 'doubleNestedValues',
getValues: (expectedValue: CategoryFacetValue) => [
buildMockCategoryFacetValue({children: [expectedValue]}),
buildMockCategoryFacetValue({
children: [buildMockCategoryFacetValue({state: 'selected'})],
}),
],
},
{
caseName: 'singleNestedValue',
getValues: (expectedValue: CategoryFacetValue) => [
buildMockCategoryFacetValue({children: [expectedValue]}),
describe('when there is a value selected', () => {
const selectedValue = buildMockCategoryFacetValue({
state: 'selected',
value: 'A',
});
const notAncestorValue = buildMockCategoryFacetValue();

const buildAncestor = (children: CategoryFacetValue[]) => {
return buildMockCategoryFacetValue({children});
};

const hierarchicalTestCases = [
() => [notAncestorValue, buildAncestor([selectedValue])],
() => [buildAncestor([notAncestorValue, selectedValue])],
() => [
buildAncestor([
notAncestorValue,
buildAncestor([selectedValue, notAncestorValue]),
]),
],
},
{
caseName: 'singleRootValue',
getValues: (expectedValue: CategoryFacetValue) => [expectedValue],
},
])(
'should return the first selected values found while doing a depth first search - $caseName',
({getValues}) => {
const expectedValues = buildMockCategoryFacetValue({
state: 'selected',
value: 'A',
});

const values: CategoryFacetValue[] = getValues(expectedValues);

expect(getActiveValueFromValueTree(values)).toBe(expectedValues);
}
);
];

it('should return an array containing only the selected Value if the selected value is at the root', () => {
expect(findActiveValueAncestry([selectedValue])).toEqual([selectedValue]);
});

it.each(hierarchicalTestCases)(
'should return an array containing the selected value whole ancestry',
(generateValues) => {
expect(findActiveValueAncestry(generateValues())).not.toContain(
notAncestorValue
);
}
);

it.each(hierarchicalTestCases)(
'should return an array containing the ancestors in order, from the root to the selected value',
(generateValue) => {
const ancestry = findActiveValueAncestry(generateValue());

expect(ancestry.pop()).toBe(selectedValue);
expect(ancestry[ancestry.length - 1].children).toContain(selectedValue);
for (let i = 0; i < ancestry.length - 2; i++) {
expect(ancestry[i].children).toContain(ancestry[i + 1]);
}
}
);
});
});
Loading

0 comments on commit fdc644b

Please sign in to comment.