Skip to content

Commit

Permalink
refactor: disconnect variations from product view model (#1644)
Browse files Browse the repository at this point in the history
BREAKING CHANGE: The variations property on the product view interface was removed. Variations can now be retrieved via the product context facade or the shopping facade.
  • Loading branch information
dhhyi authored and shauke committed May 22, 2024
1 parent ffc542d commit 5ffa2cc
Show file tree
Hide file tree
Showing 12 changed files with 104 additions and 62 deletions.
6 changes: 6 additions & 0 deletions docs/guides/migrations.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ The dead code detection script was improved and extended and is now checking Ang
This resulted in more variables and methods being declared as `private` and some unused code being removed.
This should not affect PWA based projects as long as such internal declarations were not used, else compiling will fail and the code would need to be adapted/reverted accordingly.

Product variations were eagerly loaded via effects.
In projects with a lot of Variations, this can lead to performance issues, especially if the variations data is not needed for the current views.
For that reason product variations are now loaded lazily through the following changes that might need adaptions with project customizations.

- The variations property on the product view interface was removed. Variations can now be retrieved via the product context facade or the shopping facade.

## From 5.0 to 5.1

The OrderListComponent is strictly presentational, components using it have to supply the data.
Expand Down
22 changes: 20 additions & 2 deletions src/app/core/facades/product-context.facade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ import { Pricing } from 'ish-core/models/price/price.model';
import { ProductLinksDictionary } from 'ish-core/models/product-links/product-links.model';
import { ProductVariationHelper } from 'ish-core/models/product-variation/product-variation.helper';
import { ProductView } from 'ish-core/models/product-view/product-view.model';
import { ProductCompletenessLevel, ProductHelper, SkuQuantityType } from 'ish-core/models/product/product.model';
import {
ProductCompletenessLevel,
ProductHelper,
SkuQuantityType,
VariationProduct,
} from 'ish-core/models/product/product.model';
import { Promotion } from 'ish-core/models/promotion/promotion.model';
import { generateProductUrl } from 'ish-core/routing/product/product.route';
import { mapToProperty, whenTruthy } from 'ish-core/utils/operators';
Expand Down Expand Up @@ -102,6 +107,7 @@ export interface ProductContext {
links: ProductLinksDictionary;
promotions: Promotion[];
parts: SkuQuantityType[];
variations: VariationProduct[];

// variation handling
variationCount: number;
Expand Down Expand Up @@ -138,6 +144,13 @@ export class ProductContextFacade extends RxState<ProductContext> implements OnD
mapToProperty('sku')
);

private masterProductSKU$ = this.select('product').pipe(
switchMap(product =>
ProductHelper.isMasterProduct(product) ? this.validProductSKU$ : this.select('product', 'productMasterSKU')
),
whenTruthy()
);

constructor(
private shoppingFacade: ShoppingFacade,
private appFacade: AppFacade,
Expand Down Expand Up @@ -441,6 +454,9 @@ export class ProductContextFacade extends RxState<ProductContext> implements OnD
};

switch (k1) {
case 'variations':
wrap('variations', this.shoppingFacade.productVariations$(this.masterProductSKU$));
break;
case 'links':
wrap('links', this.shoppingFacade.productLinks$(this.validProductSKU$));
break;
Expand Down Expand Up @@ -499,7 +515,9 @@ export class ProductContextFacade extends RxState<ProductContext> implements OnD
}

changeVariationOption(name: string, value: string) {
this.set('sku', () => ProductVariationHelper.findPossibleVariation(name, value, this.get('product')));
this.set('sku', () =>
ProductVariationHelper.findPossibleVariation(name, value, this.get('product'), this.get('variations'))
);
}

addToBasket() {
Expand Down
8 changes: 8 additions & 0 deletions src/app/core/facades/shopping.facade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
getProductLinks,
getProductParts,
getProductVariationCount,
getProductVariations,
loadProduct,
loadProductIfNotLoaded,
loadProductLinks,
Expand Down Expand Up @@ -152,6 +153,13 @@ export class ShoppingFacade {
);
}

productVariations$(sku: string | Observable<string>) {
return toObservable(sku).pipe(
whenTruthy(),
switchMap(plainSKU => this.store.pipe(select(getProductVariations(plainSKU))))
);
}

productVariationCount$(sku: string) {
return toObservable(sku).pipe(switchMap(plainSKU => this.store.pipe(select(getProductVariationCount(plainSKU)))));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,21 +67,15 @@ const productMaster = {
],
} as VariationProductMaster;

const variationProductView = {
const variationProduct = {
...productVariations[0],
productMaster,
variations: productVariations,
} as ProductView;

const masterProductView = {
...productMaster,
variations: productVariations,
} as ProductView;

describe('Product Variation Helper', () => {
describe('buildVariationOptionGroups', () => {
it('should build variation option groups for variation product', () => {
const result = ProductVariationHelper.buildVariationOptionGroups(variationProductView);
const result = ProductVariationHelper.buildVariationOptionGroups(variationProduct, productVariations);
expect(result).toMatchInlineSnapshot(`
[
{
Expand Down Expand Up @@ -176,19 +170,27 @@ describe('Product Variation Helper', () => {

describe('findPossibleVariation', () => {
it('should find perfect match when first attribute is changed', () => {
expect(ProductVariationHelper.findPossibleVariation('a2', 'B', variationProductView)).toEqual('333');
expect(ProductVariationHelper.findPossibleVariation('a2', 'B', variationProduct, productVariations)).toEqual(
'333'
);
});

it('should find perfect match when second attribute is changed', () => {
expect(ProductVariationHelper.findPossibleVariation('a1', 'B', variationProductView)).toEqual('444');
expect(ProductVariationHelper.findPossibleVariation('a1', 'B', variationProduct, productVariations)).toEqual(
'444'
);
});

it('should find variation match when second attribute is changed and no perfect match could be found', () => {
expect(ProductVariationHelper.findPossibleVariation('a2', 'C', variationProductView)).toEqual('666');
expect(ProductVariationHelper.findPossibleVariation('a2', 'C', variationProduct, productVariations)).toEqual(
'666'
);
});

it('should return original sku when impossible selection is selected', () => {
expect(ProductVariationHelper.findPossibleVariation('a2', 'Z', variationProductView)).toEqual('222');
expect(ProductVariationHelper.findPossibleVariation('a2', 'Z', variationProduct, productVariations)).toEqual(
'222'
);
});
});

Expand All @@ -198,7 +200,7 @@ describe('Product Variation Helper', () => {
});

it('should use variation length when no filters are given', () => {
expect(ProductVariationHelper.productVariationCount(masterProductView, undefined)).toEqual(5);
expect(ProductVariationHelper.productVariationCount(productVariations, undefined)).toEqual(5);
});

it('should ignore irrelevant selections when counting', () => {
Expand All @@ -211,7 +213,7 @@ describe('Product Variation Helper', () => {
],
} as FilterNavigation;

expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(5);
expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(5);
});

it('should filter for products matching single selected attributes', () => {
Expand All @@ -231,7 +233,7 @@ describe('Product Variation Helper', () => {
],
} as FilterNavigation;

expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(2);
expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(2);
});

it('should filter for products matching complex value attributes', () => {
Expand All @@ -244,7 +246,7 @@ describe('Product Variation Helper', () => {
],
} as FilterNavigation;

expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(3);
expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(3);
});

it('should filter for products matching multiple selected attributes', () => {
Expand All @@ -260,7 +262,7 @@ describe('Product Variation Helper', () => {
],
} as FilterNavigation;

expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(3);
expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(3);
});

it('should filter for products matching multiple selected attributes over multiple facets', () => {
Expand All @@ -277,7 +279,7 @@ describe('Product Variation Helper', () => {
],
} as FilterNavigation;

expect(ProductVariationHelper.productVariationCount(masterProductView, filters)).toEqual(1);
expect(ProductVariationHelper.productVariationCount(productVariations, filters)).toEqual(1);
});
});
});
32 changes: 21 additions & 11 deletions src/app/core/models/product-variation/product-variation.helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { groupBy } from 'lodash-es';

import { FilterNavigation } from 'ish-core/models/filter-navigation/filter-navigation.model';
import { ProductView } from 'ish-core/models/product-view/product-view.model';
import { VariationProduct } from 'ish-core/models/product/product.model';
import { omit } from 'ish-core/utils/functions';

import { VariationAttribute } from './variation-attribute.model';
Expand All @@ -12,7 +13,7 @@ export class ProductVariationHelper {
/**
* Build select value structure
*/
static buildVariationOptionGroups(product: ProductView): VariationOptionGroup[] {
static buildVariationOptionGroups(product: ProductView, variations: VariationProduct[]): VariationOptionGroup[] {
if (!product?.variableVariationAttributes?.length) {
return [];
}
Expand All @@ -38,7 +39,7 @@ export class ProductVariationHelper {
}))
.map(option => ({
...option,
alternativeCombination: ProductVariationHelper.alternativeCombinationCheck(option, product),
alternativeCombination: ProductVariationHelper.alternativeCombinationCheck(option, product, variations),
}));

// group options list by attributeId
Expand All @@ -57,13 +58,18 @@ export class ProductVariationHelper {
});
}

static findPossibleVariation(name: string, value: string, product: ProductView): string {
static findPossibleVariation(
name: string,
value: string,
product: ProductView,
variations: VariationProduct[]
): string {
const target = omit(
ProductVariationHelper.simplifyVariableVariationAttributes(product.variableVariationAttributes),
name
);

const candidates = product.variations
const candidates = variations
.filter(variation =>
variation.variableVariationAttributes.some(
attr => attr.variationAttributeId === name && ProductVariationHelper.isEqual(attr.value, value)
Expand Down Expand Up @@ -92,11 +98,11 @@ export class ProductVariationHelper {
return product && !!product.defaultVariationSKU;
}

static productVariationCount(product: ProductView, filters: FilterNavigation): number {
if (!product) {
static productVariationCount(variations: VariationProduct[], filters: FilterNavigation): number {
if (!variations?.length) {
return 0;
} else if (!filters?.filter) {
return product.variations?.length;
return variations?.length;
}

const selectedFacets = filters.filter
Expand All @@ -105,10 +111,10 @@ export class ProductVariationHelper {
.map(selected => selected.split('='));

if (!selectedFacets.length) {
return product.variations?.length;
return variations?.length;
}

return product.variations
return variations
.map(p => p.variableVariationAttributes)
.filter(attrs =>
attrs.every(
Expand All @@ -130,7 +136,11 @@ export class ProductVariationHelper {
* @param product The given product containing the related product variations
* @returns Indicates if no perfect match is found.
*/
private static alternativeCombinationCheck(option: VariationSelectOption, product: ProductView): boolean {
private static alternativeCombinationCheck(
option: VariationSelectOption,
product: ProductView,
variations: VariationProduct[]
): boolean {
if (!product.variableVariationAttributes?.length) {
return;
}
Expand All @@ -148,7 +158,7 @@ export class ProductVariationHelper {
}

// check if one of the variation products has the same attributes as the attributes to be compared
return !product.variations.some(variation =>
return !variations.some(variation =>
ProductVariationHelper.variationAttributeArrayEquals(variation.variableVariationAttributes, comparisonAttributes)
);
}
Expand Down
6 changes: 0 additions & 6 deletions src/app/core/models/product-view/product-view.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ interface SimpleProductView extends Product {

interface VariationProductView extends VariationProduct, SimpleProductView {
type: VariationProduct['type'];
variations: VariationProduct[];
productMaster: VariationProductMaster;
}

interface VariationProductMasterView extends VariationProductMaster, SimpleProductView {
type: VariationProductMaster['type'];
variations: VariationProduct[];
defaultVariationSKU: string;
}

Expand All @@ -33,30 +31,26 @@ export function createProductView(product: Product, defaultCategory?: CategoryVi
export function createVariationProductMasterView(
product: VariationProductMaster,
defaultVariationSKU: string,
variations: VariationProduct[],
defaultCategory?: CategoryView
): VariationProductMasterView {
return (
product && {
...createProductView(product, defaultCategory),
type: 'VariationProductMaster',
defaultVariationSKU,
variations,
}
);
}

export function createVariationProductView(
product: VariationProduct,
variations: VariationProduct[],
productMaster: VariationProductMaster,
defaultCategory?: CategoryView
): VariationProductView {
return (
product && {
...createProductView(product, defaultCategory),
type: 'VariationProduct',
variations,
productMaster,
}
);
Expand Down
15 changes: 9 additions & 6 deletions src/app/core/store/shopping/products/products.selectors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
getProductEntities,
getProductLinks,
getProductParts,
getProductVariations,
getSelectedProduct,
} from './products.selectors';

Expand Down Expand Up @@ -330,14 +331,16 @@ describe('Products Selectors', () => {
"defaultVariationSKU": "VAR",
"sku": "SKU",
"type": "VariationProductMaster",
"variations": [
{
"sku": "VAR",
"type": "VariationProduct",
},
],
}
`);
expect(getProductVariations('SKU')(store$.state)).toMatchInlineSnapshot(`
[
{
"sku": "VAR",
"type": "VariationProduct",
},
]
`);
});
});

Expand Down
Loading

0 comments on commit 5ffa2cc

Please sign in to comment.