Skip to content

Commit

Permalink
fix(core): Fix removal of order item promotions
Browse files Browse the repository at this point in the history
  • Loading branch information
@michaelbromley
michaelbromley committed Jun 5, 2020
1 parent 0d536cb commit f385d69
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 25 deletions.
18 changes: 18 additions & 0 deletions packages/core/e2e/graphql/generated-e2e-shop-types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2277,6 +2277,12 @@ export type TestOrderFragmentFragment = { __typename?: 'Order' } & Pick<
lines: Array<
{ __typename?: 'OrderLine' } & Pick<OrderLine, 'id' | 'quantity'> & {
productVariant: { __typename?: 'ProductVariant' } & Pick<ProductVariant, 'id'>;
adjustments: Array<
{ __typename?: 'Adjustment' } & Pick<
Adjustment,
'adjustmentSource' | 'amount' | 'description' | 'type'
>
>;
}
>;
shippingMethod?: Maybe<
Expand All @@ -2303,6 +2309,12 @@ export type AddItemToOrderMutation = { __typename?: 'Mutation' } & {
lines: Array<
{ __typename?: 'OrderLine' } & Pick<OrderLine, 'id' | 'quantity'> & {
productVariant: { __typename?: 'ProductVariant' } & Pick<ProductVariant, 'id'>;
adjustments: Array<
{ __typename?: 'Adjustment' } & Pick<
Adjustment,
'adjustmentSource' | 'amount' | 'description' | 'type'
>
>;
}
>;
adjustments: Array<
Expand Down Expand Up @@ -2677,6 +2689,9 @@ export namespace TestOrderFragment {
export type Adjustments = NonNullable<TestOrderFragmentFragment['adjustments'][0]>;
export type Lines = NonNullable<TestOrderFragmentFragment['lines'][0]>;
export type ProductVariant = NonNullable<TestOrderFragmentFragment['lines'][0]>['productVariant'];
export type _Adjustments = NonNullable<
NonNullable<TestOrderFragmentFragment['lines'][0]>['adjustments'][0]
>;
export type ShippingMethod = NonNullable<TestOrderFragmentFragment['shippingMethod']>;
export type Customer = NonNullable<TestOrderFragmentFragment['customer']>;
export type User = NonNullable<NonNullable<TestOrderFragmentFragment['customer']>['user']>;
Expand All @@ -2693,6 +2708,9 @@ export namespace AddItemToOrder {
NonNullable<AddItemToOrderMutation['addItemToOrder']>['lines'][0]
>['productVariant'];
export type Adjustments = NonNullable<
NonNullable<NonNullable<AddItemToOrderMutation['addItemToOrder']>['lines'][0]>['adjustments'][0]
>;
export type _Adjustments = NonNullable<
NonNullable<AddItemToOrderMutation['addItemToOrder']>['adjustments'][0]
>;
}
Expand Down
12 changes: 12 additions & 0 deletions packages/core/e2e/graphql/shop-definitions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ export const TEST_ORDER_FRAGMENT = gql`
productVariant {
id
}
adjustments {
adjustmentSource
amount
description
type
}
}
shipping
shippingMethod {
Expand Down Expand Up @@ -58,6 +64,12 @@ export const ADD_ITEM_TO_ORDER = gql`
productVariant {
id
}
adjustments {
adjustmentSource
amount
description
type
}
}
adjustments {
adjustmentSource
Expand Down
26 changes: 21 additions & 5 deletions packages/core/e2e/order-promotion.e2e-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import gql from 'graphql-tag';
import path from 'path';

import { initialData } from '../../../e2e-common/e2e-initial-data';
import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';

import { testSuccessfulPaymentMethod } from './fixtures/test-payment-methods';
import {
Expand Down Expand Up @@ -162,7 +162,7 @@ describe('Promotions applied to Orders', () => {
it('order history records application', async () => {
const { activeOrder } = await shopClient.query<GetActiveOrder.Query>(GET_ACTIVE_ORDER);

expect(activeOrder!.history.items.map((i) => omit(i, ['id']))).toEqual([
expect(activeOrder!.history.items.map(i => omit(i, ['id']))).toEqual([
{
type: HistoryEntryType.ORDER_COUPON_APPLIED,
data: {
Expand Down Expand Up @@ -199,7 +199,7 @@ describe('Promotions applied to Orders', () => {
it('order history records removal', async () => {
const { activeOrder } = await shopClient.query<GetActiveOrder.Query>(GET_ACTIVE_ORDER);

expect(activeOrder!.history.items.map((i) => omit(i, ['id']))).toEqual([
expect(activeOrder!.history.items.map(i => omit(i, ['id']))).toEqual([
{
type: HistoryEntryType.ORDER_COUPON_APPLIED,
data: {
Expand All @@ -224,7 +224,7 @@ describe('Promotions applied to Orders', () => {
couponCode: 'NOT_THERE',
});

expect(removeCouponCode!.history.items.map((i) => omit(i, ['id']))).toEqual([
expect(removeCouponCode!.history.items.map(i => omit(i, ['id']))).toEqual([
{
type: HistoryEntryType.ORDER_COUPON_APPLIED,
data: {
Expand Down Expand Up @@ -407,6 +407,7 @@ describe('Promotions applied to Orders', () => {
quantity: 2,
});
expect(addItemToOrder!.adjustments.length).toBe(0);
expect(addItemToOrder!.lines[2].adjustments.length).toBe(2); // 2x tax
expect(addItemToOrder!.total).toBe(2640);

const { applyCouponCode } = await shopClient.query<
Expand All @@ -417,6 +418,21 @@ describe('Promotions applied to Orders', () => {
});

expect(applyCouponCode!.total).toBe(1920);
expect(applyCouponCode!.lines[2].adjustments.length).toBe(4); // 2x tax, 2x promotion

const { removeCouponCode } = await shopClient.query<
RemoveCouponCode.Mutation,
RemoveCouponCode.Variables
>(REMOVE_COUPON_CODE, {
couponCode,
});

expect(removeCouponCode!.lines[2].adjustments.length).toBe(2); // 2x tax
expect(removeCouponCode!.total).toBe(2640);

const { activeOrder } = await shopClient.query<GetActiveOrder.Query>(GET_ACTIVE_ORDER);
expect(activeOrder!.lines[2].adjustments.length).toBe(2); // 2x tax
expect(activeOrder!.total).toBe(2640);

await deletePromotion(promotion.id);
});
Expand Down Expand Up @@ -640,7 +656,7 @@ describe('Promotions applied to Orders', () => {
function getVariantBySlug(
slug: 'item-1' | 'item-12' | 'item-60' | 'item-sale-1' | 'item-sale-12',
): GetPromoProducts.Variants {
return products.find((p) => p.slug === slug)!.variants[0];
return products.find(p => p.slug === slug)!.variants[0];
}

async function deletePromotion(promotionId: string) {
Expand Down
89 changes: 69 additions & 20 deletions packages/core/src/service/helpers/order-calculator/order-calculator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class OrderCalculator {
activeTaxZone,
this.createTaxRateGetter(activeTaxZone),
);
updatedOrderLine.activeItems.forEach((item) => updatedOrderItems.add(item));
updatedOrderLine.activeItems.forEach(item => updatedOrderItems.add(item));
}
order.clearAdjustments();
this.calculateOrderTotals(order);
Expand All @@ -75,7 +75,7 @@ export class OrderCalculator {
// Then test and apply promotions
const totalBeforePromotions = order.total;
const itemsModifiedByPromotions = await this.applyPromotions(order, promotions);
itemsModifiedByPromotions.forEach((item) => updatedOrderItems.add(item));
itemsModifiedByPromotions.forEach(item => updatedOrderItems.add(item));

if (order.total !== totalBeforePromotions || itemsModifiedByPromotions.length) {
// Finally, re-calculate taxes because the promotions may have
Expand Down Expand Up @@ -158,37 +158,58 @@ export class OrderCalculator {
return updatedItems;
}

/**
* Applies promotions to OrderItems. This is a quite complex function, due to the inherent complexity
* of applying the promotions, and also due to added complexity in the name of performance
* optimization. Therefore it is heavily annotated so that the purpose of each step is clear.
*/
private async applyOrderItemPromotions(order: Order, promotions: Promotion[], utils: PromotionUtils) {
// The naive implementation updates *every* OrderItem after this function is run.
// However, on a very large order with hundreds or thousands of OrderItems, this results in
// very poor performance. E.g. updating a single quantity of an OrderLine results in saving
// all 1000 (for example) OrderItems to the DB.
// The solution is to try to be smart about tracking exactly which OrderItems have changed,
// so that we only update those.
const updatedOrderItems = new Set<OrderItem>();

for (const line of order.lines) {
// Must be re-calculated for each line, since the previous lines may have triggered promotions
// which affected the order price.
const applicablePromotions = await filterAsync(promotions, (p) => p.test(order, utils));
const applicablePromotions = await filterAsync(promotions, p => p.test(order, utils));

const forceUpdateItems = this.orderLineHasInapplicablePromotions(applicablePromotions, line);
if (forceUpdateItems) {
// This OrderLine contains Promotion adjustments for Promotions that are no longer
// applicable. So we know for sure we will need to update these OrderItems in the
// DB. Therefore add them to the `updatedOrderItems` set.
line.clearAdjustments(AdjustmentType.PROMOTION);
line.items.forEach(i => updatedOrderItems.add(i));
}

for (const promotion of applicablePromotions) {
let priceAdjusted = false;
// We need to test the promotion *again*, even though we've tested them for the line.
// This is because the previous Promotions may have adjusted the Order in such a way
// as to render later promotions no longer applicable.
if (await promotion.test(order, utils)) {
for (const item of line.items) {
const itemHasPromotions =
item.pendingAdjustments &&
!!item.pendingAdjustments.find((a) => a.type === AdjustmentType.PROMOTION);
!!item.pendingAdjustments.find(a => a.type === AdjustmentType.PROMOTION);
if (itemHasPromotions) {
item.clearAdjustments(AdjustmentType.PROMOTION);
}
if (applicablePromotions) {
const adjustment = await promotion.apply({
orderItem: item,
orderLine: line,
utils,
});
if (adjustment) {
item.pendingAdjustments = item.pendingAdjustments.concat(adjustment);
priceAdjusted = true;
updatedOrderItems.add(item);
} else if (itemHasPromotions) {
updatedOrderItems.add(item);
}
const adjustment = await promotion.apply({
orderItem: item,
orderLine: line,
utils,
});
if (adjustment) {
item.pendingAdjustments = item.pendingAdjustments.concat(adjustment);
priceAdjusted = true;
updatedOrderItems.add(item);
} else if (itemHasPromotions) {
updatedOrderItems.add(item);
}
}
if (priceAdjusted) {
Expand All @@ -197,13 +218,41 @@ export class OrderCalculator {
}
}
}
if (forceUpdateItems) {
// If we are forcing an update, we need to ensure that totals get
// re-calculated *even if* there are no applicable promotions (i.e.
// the other call to `this.calculateOrderTotals()` inside the `for...of`
// loop was never invoked).
this.calculateOrderTotals(order);
}
}
return Array.from(updatedOrderItems.values());
}

/**
* An OrderLine may have promotion adjustments from Promotions which are no longer applicable.
* For example, a coupon code might have caused a discount to be applied, and now that code has
* been removed from the order. The adjustment will still be there on each OrderItem it was applied
* to, even though that Promotion is no longer found in the `applicablePromotions` array.
*
* We need to know about this because it means that all OrderItems in the OrderLine must be
* updated.
*/
private orderLineHasInapplicablePromotions(applicablePromotions: Promotion[], line: OrderLine) {
const applicablePromotionIds = applicablePromotions.map(p => p.getSourceId());

const linePromotionIds = line.adjustments
.filter(a => a.type === AdjustmentType.PROMOTION)
.map(a => a.adjustmentSource);
const hasPromotionsThatAreNoLongerApplicable = !linePromotionIds.every(id =>
applicablePromotionIds.includes(id),
);
return hasPromotionsThatAreNoLongerApplicable;
}

private async applyOrderPromotions(order: Order, promotions: Promotion[], utils: PromotionUtils) {
order.clearAdjustments(AdjustmentType.PROMOTION);
const applicableOrderPromotions = await filterAsync(promotions, (p) => p.test(order, utils));
const applicableOrderPromotions = await filterAsync(promotions, p => p.test(order, utils));
if (applicableOrderPromotions.length) {
for (const promotion of applicableOrderPromotions) {
// re-test the promotion on each iteration, since the order total
Expand All @@ -224,7 +273,7 @@ export class OrderCalculator {
const currentShippingMethod = order.shippingMethod;
if (results && results.length && currentShippingMethod) {
let selected: { method: ShippingMethod; result: ShippingCalculationResult } | undefined;
selected = results.find((r) => idsAreEqual(r.method.id, currentShippingMethod.id));
selected = results.find(r => idsAreEqual(r.method.id, currentShippingMethod.id));
if (!selected) {
selected = results[0];
}
Expand Down Expand Up @@ -269,7 +318,7 @@ export class OrderCalculator {
}
const allFacetValues = unique([...variant.facetValues, ...variant.product.facetValues], 'id');
return facetValueIds.reduce(
(result, id) => result && !!allFacetValues.find((fv) => idsAreEqual(fv.id, id)),
(result, id) => result && !!allFacetValues.find(fv => idsAreEqual(fv.id, id)),
true as boolean,
);
},
Expand Down

0 comments on commit f385d69

Please sign in to comment.