Browse Source

fix(core): Fix removal of order item promotions

Michael Bromley 5 years ago
parent
commit
f385d69936

+ 18 - 0
packages/core/e2e/graphql/generated-e2e-shop-types.ts

@@ -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<
@@ -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<
@@ -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']>;
@@ -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]
     >;
 }

+ 12 - 0
packages/core/e2e/graphql/shop-definitions.ts

@@ -20,6 +20,12 @@ export const TEST_ORDER_FRAGMENT = gql`
             productVariant {
                 id
             }
+            adjustments {
+                adjustmentSource
+                amount
+                description
+                type
+            }
         }
         shipping
         shippingMethod {
@@ -58,6 +64,12 @@ export const ADD_ITEM_TO_ORDER = gql`
                 productVariant {
                     id
                 }
+                adjustments {
+                    adjustmentSource
+                    amount
+                    description
+                    type
+                }
             }
             adjustments {
                 adjustmentSource

+ 21 - 5
packages/core/e2e/order-promotion.e2e-spec.ts

@@ -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 {
@@ -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: {
@@ -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: {
@@ -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: {
@@ -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<
@@ -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);
         });
@@ -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) {

+ 69 - 20
packages/core/src/service/helpers/order-calculator/order-calculator.ts

@@ -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);
@@ -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
@@ -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) {
@@ -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
@@ -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];
             }
@@ -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,
                 );
             },