fix(core): Fix OrderMergeStrategy implementation

Relates to #669. The old implementation was not able to correctly handle OrderLine removal nor correctly account for custom fields. BREAKING CHANGE: The signature of the `OrderMergeStrategy.merge()` method has changed. If you have implemented a custom OrderMergeStrategy, you'll need to update it to return the expected type.
vendure-ecommerce · Feb 2, 2021 · 3193080 · 3193080
1 parent 57197ce
commit 3193080
Show file tree

Hide file tree

Showing 13 changed files with 347 additions and 189 deletions.
diff --git a/packages/core/src/config/order/merge-orders-strategy.spec.ts b/packages/core/src/config/order/merge-orders-strategy.spec.ts
@@ -1,6 +1,6 @@
 import { RequestContext } from '../../api/common/request-context';
 import { Order } from '../../entity/order/order.entity';
-import { createOrderFromLines, parseLines } from '../../testing/order-test-utils';
+import { createOrderFromLines } from '../../testing/order-test-utils';
 
 import { MergeOrdersStrategy } from './merge-orders-strategy';
 
@@ -24,7 +24,7 @@ describe('MergeOrdersStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(guestLines);
+        expect(result).toEqual([{ orderLineId: 1, quantity: 2 }]);
     });
 
     it('guestOrder empty', () => {
@@ -34,7 +34,7 @@ describe('MergeOrdersStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
+        expect(result).toEqual([{ orderLineId: 1, quantity: 2 }]);
     });
 
     it('both orders have non-conflicting lines', () => {
@@ -52,16 +52,16 @@ describe('MergeOrdersStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual([
-            { lineId: 21, quantity: 2, productVariantId: 201 },
-            { lineId: 22, quantity: 1, productVariantId: 202 },
-            { lineId: 1, quantity: 1, productVariantId: 101 },
-            { lineId: 2, quantity: 1, productVariantId: 102 },
-            { lineId: 3, quantity: 1, productVariantId: 103 },
+        expect(result).toEqual([
+            { orderLineId: 21, quantity: 2 },
+            { orderLineId: 22, quantity: 1 },
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 1 },
+            { orderLineId: 3, quantity: 1 },
         ]);
     });
 
-    it('both orders have conflicting lines, some of which conflict', () => {
+    it('both orders have lines, some of which conflict', () => {
         const guestLines = [
             { lineId: 21, quantity: 2, productVariantId: 102 },
             { lineId: 22, quantity: 1, productVariantId: 202 },
@@ -76,11 +76,40 @@ describe('MergeOrdersStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual([
-            { lineId: 22, quantity: 1, productVariantId: 202 },
-            { lineId: 1, quantity: 1, productVariantId: 101 },
-            { lineId: 2, quantity: 1, productVariantId: 102 },
-            { lineId: 3, quantity: 1, productVariantId: 103 },
+        expect(result).toEqual([
+            { orderLineId: 22, quantity: 1 },
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 2 },
+            { orderLineId: 3, quantity: 1 },
+        ]);
+    });
+
+    it('equivalent customFields are merged', () => {
+        const guestLines = [{ lineId: 21, quantity: 2, productVariantId: 102, customFields: { foo: 'bar' } }];
+        const existingLines = [
+            { lineId: 2, quantity: 1, productVariantId: 102, customFields: { foo: 'bar' } },
+        ];
+        const guestOrder = createOrderFromLines(guestLines);
+        const existingOrder = createOrderFromLines(existingLines);
+
+        const result = strategy.merge(ctx, guestOrder, existingOrder);
+
+        expect(result).toEqual([{ orderLineId: 2, quantity: 2, customFields: { foo: 'bar' } }]);
+    });
+
+    it('differing customFields are not merged', () => {
+        const guestLines = [{ lineId: 21, quantity: 2, productVariantId: 102, customFields: { foo: 'bar' } }];
+        const existingLines = [
+            { lineId: 2, quantity: 1, productVariantId: 102, customFields: { foo: 'quux' } },
+        ];
+        const guestOrder = createOrderFromLines(guestLines);
+        const existingOrder = createOrderFromLines(existingLines);
+
+        const result = strategy.merge(ctx, guestOrder, existingOrder);
+
+        expect(result).toEqual([
+            { orderLineId: 21, quantity: 2, customFields: { foo: 'bar' } },
+            { orderLineId: 2, quantity: 1, customFields: { foo: 'quux' } },
         ]);
     });
 

diff --git a/packages/core/src/config/order/merge-orders-strategy.ts b/packages/core/src/config/order/merge-orders-strategy.ts
@@ -2,7 +2,7 @@ import { RequestContext } from '../../api/common/request-context';
 import { OrderLine } from '../../entity/order-line/order-line.entity';
 import { Order } from '../../entity/order/order.entity';
 
-import { OrderMergeStrategy } from './order-merge-strategy';
+import { MergedOrderLine, OrderMergeStrategy, toMergedOrderLine } from './order-merge-strategy';
 
 /**
  * @description
@@ -13,19 +13,28 @@ import { OrderMergeStrategy } from './order-merge-strategy';
  * @docsPage Merge Strategies
  */
 export class MergeOrdersStrategy implements OrderMergeStrategy {
-    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): OrderLine[] {
-        const mergedLines = existingOrder.lines.slice();
+    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): MergedOrderLine[] {
+        const mergedLines: MergedOrderLine[] = existingOrder.lines.map(toMergedOrderLine);
         const guestLines = guestOrder.lines.slice();
         for (const guestLine of guestLines.reverse()) {
             const existingLine = this.findCorrespondingLine(existingOrder, guestLine);
             if (!existingLine) {
-                mergedLines.unshift(guestLine);
+                mergedLines.unshift(toMergedOrderLine(guestLine));
+            } else {
+                const matchingMergedLine = mergedLines.find(l => l.orderLineId === existingLine.id);
+                if (matchingMergedLine) {
+                    matchingMergedLine.quantity = guestLine.quantity;
+                }
             }
         }
         return mergedLines;
     }
 
     private findCorrespondingLine(existingOrder: Order, guestLine: OrderLine): OrderLine | undefined {
-        return existingOrder.lines.find(line => line.productVariant.id === guestLine.productVariant.id);
+        return existingOrder.lines.find(
+            line =>
+                line.productVariant.id === guestLine.productVariant.id &&
+                JSON.stringify(line.customFields) === JSON.stringify(guestLine.customFields),
+        );
     }
 }
diff --git a/packages/core/src/config/order/order-merge-strategy.ts b/packages/core/src/config/order/order-merge-strategy.ts
@@ -1,8 +1,31 @@
+import { ID } from '@vendure/common/lib/shared-types';
+
 import { RequestContext } from '../../api/common/request-context';
 import { InjectableStrategy } from '../../common/types/injectable-strategy';
 import { OrderLine } from '../../entity/order-line/order-line.entity';
 import { Order } from '../../entity/order/order.entity';
 
+/**
+ * @description
+ * The result of the {@link OrderMergeStrategy} `merge` method.
+ *
+ * @docsCategory orders
+ * @docsPage OrderMergeStrategy
+ */
+export interface MergedOrderLine {
+    orderLineId: ID;
+    quantity: number;
+    customFields?: any;
+}
+
+export function toMergedOrderLine(line: OrderLine): MergedOrderLine {
+    return {
+        orderLineId: line.id,
+        quantity: line.quantity,
+        customFields: line.customFields,
+    };
+}
+
 /**
  * @description
  * An OrderMergeStrategy defines what happens when a Customer with an existing Order
@@ -12,12 +35,14 @@ import { Order } from '../../entity/order/order.entity';
  * of OrderLines. The OrderMergeStrategy defines the rules governing this reconciliation.
  *
  * @docsCategory orders
+ * @docsPage OrderMergeStrategy
+ * @docsWeight 0
  */
 export interface OrderMergeStrategy extends InjectableStrategy {
     /**
      * @description
      * Merges the lines of the guest Order with those of the existing Order which is associated
      * with the active customer.
      */
-    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): OrderLine[];
+    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): MergedOrderLine[];
 }
diff --git a/packages/core/src/config/order/use-existing-strategy.spec.ts b/packages/core/src/config/order/use-existing-strategy.spec.ts
@@ -1,6 +1,6 @@
 import { RequestContext } from '../../api/common/request-context';
 import { Order } from '../../entity/order/order.entity';
-import { createOrderFromLines, parseLines } from '../../testing/order-test-utils';
+import { createOrderFromLines } from '../../testing/order-test-utils';
 
 import { UseExistingStrategy } from './use-existing-strategy';
 
@@ -24,7 +24,7 @@ describe('UseExistingStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual([]);
+        expect(result).toEqual([]);
     });
 
     it('guestOrder empty', () => {
@@ -34,7 +34,7 @@ describe('UseExistingStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
+        expect(result).toEqual([{ orderLineId: 1, quantity: 2 }]);
     });
 
     it('both orders have non-conflicting lines', () => {
@@ -52,7 +52,11 @@ describe('UseExistingStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
+        expect(result).toEqual([
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 1 },
+            { orderLineId: 3, quantity: 1 },
+        ]);
     });
 
     it('both orders have conflicting lines, some of which conflict', () => {
@@ -70,18 +74,10 @@ describe('UseExistingStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
-    });
-
-    it('returns a new array', () => {
-        const guestLines = [{ lineId: 21, quantity: 2, productVariantId: 102 }];
-        const existingLines = [{ lineId: 1, quantity: 1, productVariantId: 101 }];
-        const guestOrder = createOrderFromLines(guestLines);
-        const existingOrder = createOrderFromLines(existingLines);
-
-        const result = strategy.merge(ctx, guestOrder, existingOrder);
-
-        expect(result).not.toBe(guestOrder.lines);
-        expect(result).not.toBe(existingOrder.lines);
+        expect(result).toEqual([
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 1 },
+            { orderLineId: 3, quantity: 1 },
+        ]);
     });
 });
diff --git a/packages/core/src/config/order/use-existing-strategy.ts b/packages/core/src/config/order/use-existing-strategy.ts
@@ -1,8 +1,7 @@
 import { RequestContext } from '../../api/common/request-context';
-import { OrderLine } from '../../entity/order-line/order-line.entity';
 import { Order } from '../../entity/order/order.entity';
 
-import { OrderMergeStrategy } from './order-merge-strategy';
+import { MergedOrderLine, OrderMergeStrategy, toMergedOrderLine } from './order-merge-strategy';
 
 /**
  * @description
@@ -12,7 +11,7 @@ import { OrderMergeStrategy } from './order-merge-strategy';
  * @docsPage Merge Strategies
  */
 export class UseExistingStrategy implements OrderMergeStrategy {
-    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): OrderLine[] {
-        return existingOrder.lines.slice();
+    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): MergedOrderLine[] {
+        return existingOrder.lines.map(toMergedOrderLine);
     }
 }
diff --git a/packages/core/src/config/order/use-guest-if-existing-empty-strategy.spec.ts b/packages/core/src/config/order/use-guest-if-existing-empty-strategy.spec.ts
@@ -1,6 +1,6 @@
 import { RequestContext } from '../../api/common/request-context';
 import { Order } from '../../entity/order/order.entity';
-import { createOrderFromLines, parseLines } from '../../testing/order-test-utils';
+import { createOrderFromLines } from '../../testing/order-test-utils';
 
 import { UseGuestIfExistingEmptyStrategy } from './use-guest-if-existing-empty-strategy';
 
@@ -24,7 +24,7 @@ describe('UseGuestIfExistingEmptyStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(guestLines);
+        expect(result).toEqual([{ orderLineId: 1, quantity: 2 }]);
     });
 
     it('guestOrder empty', () => {
@@ -34,7 +34,7 @@ describe('UseGuestIfExistingEmptyStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
+        expect(result).toEqual([{ orderLineId: 1, quantity: 2 }]);
     });
 
     it('both orders have non-conflicting lines', () => {
@@ -52,10 +52,14 @@ describe('UseGuestIfExistingEmptyStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
+        expect(result).toEqual([
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 1 },
+            { orderLineId: 3, quantity: 1 },
+        ]);
     });
 
-    it('both orders have conflicting lines, some of which conflict', () => {
+    it('both orders have lines, some of which conflict', () => {
         const guestLines = [
             { lineId: 21, quantity: 2, productVariantId: 102 },
             { lineId: 22, quantity: 1, productVariantId: 202 },
@@ -70,18 +74,10 @@ describe('UseGuestIfExistingEmptyStrategy', () => {
 
         const result = strategy.merge(ctx, guestOrder, existingOrder);
 
-        expect(parseLines(result)).toEqual(existingLines);
-    });
-
-    it('returns a new array', () => {
-        const guestLines = [{ lineId: 21, quantity: 2, productVariantId: 102 }];
-        const existingLines = [{ lineId: 1, quantity: 1, productVariantId: 101 }];
-        const guestOrder = createOrderFromLines(guestLines);
-        const existingOrder = createOrderFromLines(existingLines);
-
-        const result = strategy.merge(ctx, guestOrder, existingOrder);
-
-        expect(result).not.toBe(guestOrder.lines);
-        expect(result).not.toBe(existingOrder.lines);
+        expect(result).toEqual([
+            { orderLineId: 1, quantity: 1 },
+            { orderLineId: 2, quantity: 1 },
+            { orderLineId: 3, quantity: 1 },
+        ]);
     });
 });
diff --git a/packages/core/src/config/order/use-guest-if-existing-empty-strategy.ts b/packages/core/src/config/order/use-guest-if-existing-empty-strategy.ts
@@ -1,8 +1,7 @@
 import { RequestContext } from '../../api/common/request-context';
-import { OrderLine } from '../../entity/order-line/order-line.entity';
 import { Order } from '../../entity/order/order.entity';
 
-import { OrderMergeStrategy } from './order-merge-strategy';
+import { MergedOrderLine, OrderMergeStrategy, toMergedOrderLine } from './order-merge-strategy';
 
 /**
  * @description
@@ -12,7 +11,9 @@ import { OrderMergeStrategy } from './order-merge-strategy';
  * @docsPage Merge Strategies
  */
 export class UseGuestIfExistingEmptyStrategy implements OrderMergeStrategy {
-    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): OrderLine[] {
-        return existingOrder.lines.length ? existingOrder.lines.slice() : guestOrder.lines.slice();
+    merge(ctx: RequestContext, guestOrder: Order, existingOrder: Order): MergedOrderLine[] {
+        return existingOrder.lines.length
+            ? existingOrder.lines.map(toMergedOrderLine)
+            : guestOrder.lines.map(toMergedOrderLine);
     }
 }