Skip to content

Commit

Permalink
refactor: do not mix reactive and static programming paradigmas in co…
Browse files Browse the repository at this point in the history 
…nfigurator-attribute-price-change.service.ts (#19100)
  • Loading branch information
@Uli-Tiger
Uli-Tiger authored Aug 8, 2024
1 parent df80d85 commit adda4e5
Show file tree
Hide file tree
Showing 22 changed files with 310 additions and 317 deletions.
67 changes: 16 additions & 51 deletions ...sed/components/attribute/price-change/configurator-attribute-price-change.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,13 @@ describe('ConfiguratorAttributePriceChangeService', () => {
expect(classUnderTest).toBeTruthy();
});

describe('getPriceChangedEvents', () => {
describe('getChangedPrices', () => {
it('should emit always true for the initial rendering/call', () => {
let emitCounter = 0;
classUnderTest
.getPriceChangedEvents(undefined)
.subscribe((priceChanged) => {
expect(priceChanged).toBe(true);
emitCounter++;
});
classUnderTest.getChangedPrices(undefined).subscribe((priceChanged) => {
expect(priceChanged).toEqual({});
emitCounter++;
});
mockConfig.priceSupplements = undefined;
configSubject.next(mockConfig);
expect(emitCounter).toBe(1);
Expand All @@ -86,16 +84,15 @@ describe('ConfiguratorAttributePriceChangeService', () => {
it('should detect price changes during initial rendering/call if supplements are present', () => {
let emitCounter = 0;
classUnderTest
.getPriceChangedEvents('group1@attribute_1_1')
.getChangedPrices('group1@attribute_1_1')
.subscribe((priceChanged) => {
expect(priceChanged).toBe(true);
expect(priceChanged['value_1_1']).toBeDefined();
expect(priceChanged['value_1_2']).toBeDefined();
expect(priceChanged['value_1_3']).toBeDefined();
emitCounter++;
});
configSubject.next(mockConfig);
expect(emitCounter).toBe(1);
expect(classUnderTest['valuePrices']['value_1_1']).toBeDefined();
expect(classUnderTest['valuePrices']['value_1_2']).toBeDefined();
expect(classUnderTest['valuePrices']['value_1_3']).toBeDefined();
});

describe('after initial rendering/call', () => {
Expand All @@ -108,7 +105,7 @@ describe('ConfiguratorAttributePriceChangeService', () => {

it('should not emit, if config has no price supplements', () => {
classUnderTest
.getPriceChangedEvents(undefined)
.getChangedPrices(undefined)
.pipe(skip(1))
.subscribe(() => {
fail('priceChanged observable should not emit!');
Expand All @@ -120,7 +117,7 @@ describe('ConfiguratorAttributePriceChangeService', () => {

it('should not emit, if config has no matching price supplements', () => {
classUnderTest
.getPriceChangedEvents('otherAttrKey')
.getChangedPrices('otherAttrKey')
.pipe(skip(1))
.subscribe(() => {
fail('priceChanged observable should not emit!');
Expand All @@ -132,25 +129,24 @@ describe('ConfiguratorAttributePriceChangeService', () => {
it('should emit true and store matching value prices, if config has matching price supplements', () => {
let emitCounter = 0;
classUnderTest
.getPriceChangedEvents('group1@attribute_1_1')
.getChangedPrices('group1@attribute_1_1')
.pipe(skip(1))
.subscribe((priceChanged) => {
expect(priceChanged).toBe(true);
expect(priceChanged['value_1_1']).toBeDefined();
expect(priceChanged['value_1_2']).toBeDefined();
expect(priceChanged['value_1_3']).toBeDefined();
emitCounter++;
});
simulateFirstCall();
configSubject.next(mockConfig);
expect(emitCounter).toBe(1);
expect(classUnderTest['valuePrices']['value_1_1']).toBeDefined();
expect(classUnderTest['valuePrices']['value_1_2']).toBeDefined();
expect(classUnderTest['valuePrices']['value_1_3']).toBeDefined();
});

it('should not emit again if prices are not changed', () => {
classUnderTest['lastAttributeSupplement'] =
mockConfig.priceSupplements?.[0];
classUnderTest
.getPriceChangedEvents('group1@attribute_1_1')
.getChangedPrices('group1@attribute_1_1')
.pipe(skip(1))
.subscribe(() => {
fail('priceChanged observable should not emit!');
Expand All @@ -160,35 +156,4 @@ describe('ConfiguratorAttributePriceChangeService', () => {
});
});
});

describe('mergePriceIntoValue', () => {
it('should create a new object combining value and price if onPriceChanged was called for this value before', () => {
const valuePrice = { value: 100, currencyIso: 'USD' };
classUnderTest['storeValuePrice']('valueKey', valuePrice);
expect(
classUnderTest['mergePriceIntoValue']({
valueCode: '1223',
name: 'valueKey',
})
).toEqual({
valueCode: '1223',
name: 'valueKey',
valuePrice: valuePrice,
});
});

it('should return just the value if onPriceChanged was NOT called for this value before', () => {
const valuePrice = { value: 100, currencyIso: 'USD' };
classUnderTest['storeValuePrice']('anotherValueKey', valuePrice);
expect(
classUnderTest['mergePriceIntoValue']({
valueCode: '1223',
name: 'valueKey',
})
).toEqual({
valueCode: '1223',
name: 'valueKey',
});
});
});
});
75 changes: 26 additions & 49 deletions ...ulebased/components/attribute/price-change/configurator-attribute-price-change.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ import { ConfiguratorCommonsService } from '../../../core/facade/configurator-co
import { Configurator } from '../../../core/model/configurator.model';

/**
* Stateful service to react on price changes of the configuration. Hence components using this service must provide it within their declaration:
*
* Stateful service to react on price changes of the configuration.
* Hence components using this service must provide it within their declaration, for instance:
* @Component({ providers: [ConfiguratorAttributePriceChangeService] })
*
* getPriceChangedEvents will return an observable that emits whenever the price of a monitored attribute (identified by the provided attribute key) changes,
* hence allowing the subscriber for example to trigger rerendering of the prices on the UI.
* getChangedPrices will return an observable that emits whenever the price of a monitored attribute (identified by the provided attribute key) changes.
* Hence allowing the subscriber to trigger rerendering of the prices on the UI.
*/
@Injectable()
export class ConfiguratorAttributePriceChangeService {
Expand All @@ -36,22 +36,20 @@ export class ConfiguratorAttributePriceChangeService {
protected lastAttributeSupplement:
| Configurator.AttributeSupplement
| undefined;
protected valuePrices: { [valueName: string]: Configurator.PriceDetails } =
{};

/**
* Returns an observable that shall be used by all components supporting delta rendering mode.
* It will monitor the price supplements of configuration observable and emit true if price supplements
* matching the given attribute key have changed.
* Additionally it returns always true for the first emission of the underlying configuration observable.
* This ensures that an enclosing UI component will initially render, even if the async pricing request ist still running,
* Retrieves an observable that emits whenever the price of a monitored attribute (identified by the provided attribute key) changes,
* hence allowing the subscriber to trigger rerendering of the prices on the UI.
* This ensures that an enclosing UI component will initially render, even if the async pricing request is still running,
* so that the UI is not blocked. Afterwards a rerender shall only occur if prices change.
* This all assumes that the enclosing UI component itself gets recreated or rerendered (triggered elsewhere) whenever the attribute itself changes content wise.
*
* @param attributeKey key of the attribute for which the prices should be checked for changes
* @returns observable that emits 'true' each time a price changes and hence there is the need to rerender the enclosing component
* @returns observable that emits price of monitored attribute changes and hence there is the need to rerender the enclosing component
*/
getPriceChangedEvents(attributeKey: string | undefined): Observable<boolean> {
getChangedPrices(
attributeKey: string | undefined
): Observable<Record<string, Configurator.PriceDetails>> {
let isInitialConfiguration: boolean = true;
return this.configuratorRouterExtractorService.extractRouterData().pipe(
switchMap((routerData) => {
Expand All @@ -65,13 +63,13 @@ export class ConfiguratorAttributePriceChangeService {
),
switchMap((config) => {
if (!config.priceSupplements) {
return of(true);
return of({});
}
const pricesChanged = this.checkForValuePriceChanges(
const changedPrices = this.checkForValuePriceChanges(
config,
attributeKey
);
return pricesChanged ? of(true) : EMPTY;
return changedPrices ? of(changedPrices) : EMPTY;
}),
tap(() => (isInitialConfiguration = false))
);
Expand All @@ -80,34 +78,19 @@ export class ConfiguratorAttributePriceChangeService {
);
}

/**
* Merges the stored value price data into the given value, if available.
* As the value might be read-only a new object will be returned combining price and value.
*
* @param value the value
* @returns the new value with price
*/
mergePriceIntoValue(value: Configurator.Value): Configurator.Value {
const valueName = value.name;
if (valueName && this.valuePrices[valueName]) {
value = { ...value, valuePrice: this.valuePrices[valueName] };
}
return value;
}

/**
* Extracts the relevant value prices from the price supplements
* and stores them within the component. Returns a boolean indicating
* whether there were any value price changes.
* and stores them within the component.
* Returns price of a monitored attribute changes.
*
* @param config current config
* @param config current configuration
* @param attributeKey key of the attribute for which the prices should be checked for changes
* @returns {true}, only if at least one value price changed
* @returns observable that emits price of a monitored attribute changes
*/
protected checkForValuePriceChanges(
config: Configurator.Configuration,
attributeKey: string | undefined
): boolean {
): Record<string, Configurator.PriceDetails> | undefined {
const attributeSupplement = config.priceSupplements?.find(
(supplement) => supplement.attributeUiKey === attributeKey
);
Expand All @@ -116,21 +99,15 @@ export class ConfiguratorAttributePriceChangeService {
attributeSupplement ?? {}
);
if (changed) {
const changedPrices: Record<string, Configurator.PriceDetails> = {};
this.lastAttributeSupplement = attributeSupplement;
attributeSupplement?.valueSupplements.forEach((valueSupplement) =>
this.storeValuePrice(
valueSupplement.attributeValueKey,
valueSupplement.priceValue
)
attributeSupplement?.valueSupplements.forEach(
(valueSupplement) =>
(changedPrices[valueSupplement.attributeValueKey] =
valueSupplement.priceValue)
);
return changedPrices;
}
return changed;
}

protected storeValuePrice(
valueName: string,
valuePrice: Configurator.PriceDetails
) {
this.valuePrices[valueName] = valuePrice;
return undefined;
}
}
Loading

0 comments on commit adda4e5

Please sign in to comment.