fix(core): Fix order incorrect refund amount when modifying Order

Fixes #1865

packages/core/e2e/graphql/shared-definitions.ts

@@ -972,3 +972,26 @@ export const TRANSITION_PAYMENT_TO_STATE = gql`
     }
     ${PAYMENT_FRAGMENT}
 `;
+
+export const GET_PRODUCT_VARIANT_LIST = gql`
+    query GetProductVariantLIST($options: ProductVariantListOptions, $productId: ID) {
+        productVariants(options: $options, productId: $productId) {
+            items {
+                id
+                name
+                sku
+                price
+                priceWithTax
+            }
+            totalItems
+        }
+    }
+`;
+
+export const DELETE_PROMOTION = gql`
+    mutation DeletePromotion($id: ID!) {
+        deletePromotion(id: $id) {
+            result
+        }
+    }
+`;

packages/core/e2e/order-modification.e2e-spec.ts

@@ -8,6 +8,7 @@ import {
     freeShipping,
     manualFulfillmentHandler,
     mergeConfig,
+    minimumOrderAmount,
     orderFixedDiscount,
     orderPercentageDiscount,
     productsPercentageDiscount,
@@ -18,7 +19,7 @@ import gql from 'graphql-tag';
 import path from 'path';
 
 import { initialData } from '../../../e2e-common/e2e-initial-data';
-import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';
+import { TEST_SETUP_TIMEOUT_MS, testConfig } from '../../../e2e-common/test-config';
 
 import {
     failsToSettlePaymentMethod,
@@ -33,12 +34,18 @@ import {
     CreatePromotionMutation,
     CreatePromotionMutationVariables,
     CreateShippingMethod,
+    DeletePromotionMutation,
+    DeletePromotionMutationVariables,
     ErrorCode,
     GetOrder,
     GetOrderHistory,
+    GetOrderQuery,
+    GetOrderQueryVariables,
     GetOrderWithModifications,
     GetOrderWithModificationsQuery,
     GetOrderWithModificationsQueryVariables,
+    GetProductVariantListQuery,
+    GetProductVariantListQueryVariables,
     GetStockMovement,
     GlobalFlag,
     HistoryEntryType,
@@ -66,8 +73,10 @@ import {
     CREATE_FULFILLMENT,
     CREATE_PROMOTION,
     CREATE_SHIPPING_METHOD,
+    DELETE_PROMOTION,
     GET_ORDER,
     GET_ORDER_HISTORY,
+    GET_PRODUCT_VARIANT_LIST,
     GET_STOCK_MOVEMENT,
     UPDATE_CHANNEL,
     UPDATE_PRODUCT_VARIANTS,
@@ -153,7 +162,7 @@ describe('Order modification', () => {
                 ],
             },
             productsCsvPath: path.join(__dirname, 'fixtures/e2e-products-full.csv'),
-            customerCount: 2,
+            customerCount: 3,
         });
         await adminClient.asSuperAdmin();
 
@@ -1405,8 +1414,8 @@ describe('Order modification', () => {
         });
     });
 
-    // https://github.com/vendure-ecommerce/vendure/issues/890
     describe('refund handling when promotions are active on order', () => {
+        // https://github.com/vendure-ecommerce/vendure/issues/890
         it('refunds correct amount when order-level promotion applied', async () => {
             await adminClient.query<CreatePromotion.Mutation, CreatePromotion.Variables>(CREATE_PROMOTION, {
                 input: {
@@ -1464,6 +1473,130 @@ describe('Order modification', () => {
             expect(modifyOrder.payments![0].refunds![0].total).toBe(order.lines[0].proratedUnitPriceWithTax);
             expect(modifyOrder.totalWithTax).toBe(getOrderPaymentsTotalWithRefunds(modifyOrder));
         });
+
+        // github.com/vendure-ecommerce/vendure/issues/1865
+        describe('issue 1865', () => {
+            const promoDiscount = 5000;
+            let promoId: string;
+            let orderId2: string;
+            beforeAll(async () => {
+                const { createPromotion } = await adminClient.query<
+                    CreatePromotion.Mutation,
+                    CreatePromotion.Variables
+                >(CREATE_PROMOTION, {
+                    input: {
+                        name: '50 off orders over 100',
+                        enabled: true,
+                        conditions: [
+                            {
+                                code: minimumOrderAmount.code,
+                                arguments: [
+                                    { name: 'amount', value: '10000' },
+                                    { name: 'taxInclusive', value: 'true' },
+                                ],
+                            },
+                        ],
+                        actions: [
+                            {
+                                code: orderFixedDiscount.code,
+                                arguments: [{ name: 'discount', value: JSON.stringify(promoDiscount) }],
+                            },
+                        ],
+                    },
+                });
+                promoId = (createPromotion as any).id;
+            });
+
+            afterAll(async () => {
+                await adminClient.query<DeletePromotionMutation, DeletePromotionMutationVariables>(
+                    DELETE_PROMOTION,
+                    {
+                        id: promoId,
+                    },
+                );
+            });
+
+            it('refund handling when order-level promotion becomes invalid on modification', async () => {
+                const { productVariants } = await adminClient.query<
+                    GetProductVariantListQuery,
+                    GetProductVariantListQueryVariables
+                >(GET_PRODUCT_VARIANT_LIST, {
+                    options: {
+                        filter: {
+                            name: { contains: 'football' },
+                        },
+                    },
+                });
+                const football = productVariants.items[0];
+
+                await shopClient.query(gql(ADD_ITEM_TO_ORDER_WITH_CUSTOM_FIELDS), {
+                    productVariantId: football.id,
+                    quantity: 2,
+                } as any);
+                await proceedToArrangingPayment(shopClient);
+                const order = await addPaymentToOrder(shopClient, testSuccessfulPaymentMethod);
+                orderGuard.assertSuccess(order);
+                orderId2 = order.id;
+
+                expect(order.discounts.length).toBe(1);
+                expect(order.discounts[0].amountWithTax).toBe(-promoDiscount);
+                const shippingPrice = order.shippingWithTax;
+                const expectedTotal = football.priceWithTax * 2 + shippingPrice - promoDiscount;
+                expect(order.totalWithTax).toBe(expectedTotal);
+
+                const originalTotalWithTax = order.totalWithTax;
+
+                const transitionOrderToState = await adminTransitionOrderToState(order.id, 'Modifying');
+                orderGuard.assertSuccess(transitionOrderToState);
+
+                expect(transitionOrderToState.state).toBe('Modifying');
+
+                const { modifyOrder } = await adminClient.query<ModifyOrder.Mutation, ModifyOrder.Variables>(
+                    MODIFY_ORDER,
+                    {
+                        input: {
+                            dryRun: false,
+                            orderId: order.id,
+                            adjustOrderLines: [{ orderLineId: order.lines[0].id, quantity: 1 }],
+                            refund: {
+                                paymentId: order.payments![0].id,
+                                reason: 'requested',
+                            },
+                        },
+                    },
+                );
+                orderGuard.assertSuccess(modifyOrder);
+
+                const expectedNewTotal = order.lines[0].unitPriceWithTax + shippingPrice;
+                expect(modifyOrder.totalWithTax).toBe(expectedNewTotal);
+                expect(modifyOrder.payments![0].refunds![0].total).toBe(expectedTotal - expectedNewTotal);
+                expect(modifyOrder.totalWithTax).toBe(getOrderPaymentsTotalWithRefunds(modifyOrder));
+            });
+
+            it('transition back to original state', async () => {
+                const transitionOrderToState2 = await adminTransitionOrderToState(orderId2, 'PaymentSettled');
+                orderGuard.assertSuccess(transitionOrderToState2);
+                expect(transitionOrderToState2!.state).toBe('PaymentSettled');
+            });
+
+            it('order no longer has promotions', async () => {
+                const { order } = await adminClient.query<
+                    GetOrderWithModificationsQuery,
+                    GetOrderWithModificationsQueryVariables
+                >(GET_ORDER_WITH_MODIFICATIONS, { id: orderId2 });
+
+                expect(order?.promotions).toEqual([]);
+            });
+
+            it('order no longer has discounts', async () => {
+                const { order } = await adminClient.query<
+                    GetOrderWithModificationsQuery,
+                    GetOrderWithModificationsQueryVariables
+                >(GET_ORDER_WITH_MODIFICATIONS, { id: orderId2 });
+
+                expect(order?.discounts).toEqual([]);
+            });
+        });
     });
 
     // https://github.com/vendure-ecommerce/vendure/issues/1197
@@ -1879,7 +2012,7 @@ describe('Order modification', () => {
             expect(history.history.items.length).toBe(1);
             expect(pick(history.history.items[0]!, ['type', 'data'])).toEqual({
                 type: HistoryEntryType.ORDER_COUPON_APPLIED,
-                data: { couponCode: CODE_50PC_OFF, promotionId: 'T_4' },
+                data: { couponCode: CODE_50PC_OFF, promotionId: 'T_5' },
             });
         });
 

packages/core/e2e/product.e2e-spec.ts

@@ -1,7 +1,6 @@
 import { omit } from '@vendure/common/lib/omit';
 import { pick } from '@vendure/common/lib/pick';
 import { notNullOrUndefined } from '@vendure/common/lib/shared-utils';
-import { DefaultLogger } from '@vendure/core';
 import { createErrorResultGuard, createTestEnvironment, ErrorResultGuard } from '@vendure/testing';
 import gql from 'graphql-tag';
 import path from 'path';
@@ -14,7 +13,6 @@ import {
     AddOptionGroupToProduct,
     ChannelFragment,
     CreateProduct,
-    CreateProductOptionGroup,
     CreateProductOptionGroupMutation,
     CreateProductOptionGroupMutationVariables,
     CreateProductVariants,
@@ -54,6 +52,7 @@ import {
     GET_ASSET_LIST,
     GET_PRODUCT_LIST,
     GET_PRODUCT_SIMPLE,
+    GET_PRODUCT_VARIANT_LIST,
     GET_PRODUCT_WITH_VARIANTS,
     UPDATE_CHANNEL,
     UPDATE_GLOBAL_SETTINGS,
@@ -2146,21 +2145,6 @@ export const GET_PRODUCT_VARIANT = gql`
     }
 `;
 
-export const GET_PRODUCT_VARIANT_LIST = gql`
-    query GetProductVariantLIST($options: ProductVariantListOptions, $productId: ID) {
-        productVariants(options: $options, productId: $productId) {
-            items {
-                id
-                name
-                sku
-                price
-                priceWithTax
-            }
-            totalItems
-        }
-    }
-`;
-
 export const GET_PRODUCT_WITH_VARIANT_LIST = gql`
     query GetProductWithVariantList($id: ID, $variantListOptions: ProductVariantListOptions) {
         product(id: $id) {

packages/core/e2e/promotion.e2e-spec.ts

@@ -35,6 +35,7 @@ import {
     ASSIGN_PROMOTIONS_TO_CHANNEL,
     CREATE_CHANNEL,
     CREATE_PROMOTION,
+    DELETE_PROMOTION,
     REMOVE_PROMOTIONS_FROM_CHANNEL,
 } from './graphql/shared-definitions';
 import { assertThrowsWithMessage } from './utils/assert-throws-with-message';
@@ -393,14 +394,6 @@ function generateTestAction(code: string): PromotionAction<any> {
     });
 }
 
-const DELETE_PROMOTION = gql`
-    mutation DeletePromotion($id: ID!) {
-        deletePromotion(id: $id) {
-            result
-        }
-    }
-`;
-
 export const GET_PROMOTION_LIST = gql`
     query GetPromotionList($options: PromotionListOptions) {
         promotions(options: $options) {

packages/core/src/service/helpers/order-modifier/order-modifier.ts

@@ -24,7 +24,6 @@ import {
     NegativeQuantityError,
     OrderLimitError,
 } from '../../../common/error/generated-graphql-shop-errors';
-import { AdjustmentSource } from '../../../common/types/adjustment-source';
 import { idsAreEqual } from '../../../common/utils';
 import { ConfigService } from '../../../config/config.service';
 import { CustomFieldConfig } from '../../../config/custom-field/custom-field-types';
@@ -36,7 +35,6 @@ import { OrderModification } from '../../../entity/order-modification/order-modi
 import { Order } from '../../../entity/order/order.entity';
 import { Payment } from '../../../entity/payment/payment.entity';
 import { ProductVariant } from '../../../entity/product-variant/product-variant.entity';
-import { Promotion } from '../../../entity/promotion/promotion.entity';
 import { ShippingLine } from '../../../entity/shipping-line/shipping-line.entity';
 import { Surcharge } from '../../../entity/surcharge/surcharge.entity';
 import { EventBus } from '../../../event-bus/event-bus';
@@ -465,9 +463,15 @@ export class OrderModifier {
         const updatedOrderLines = order.lines.filter(l => updatedOrderLineIds.includes(l.id));
         const promotions = await this.promotionService.getActivePromotionsInChannel(ctx);
         const activePromotionsPre = await this.promotionService.getActivePromotionsOnOrder(ctx, order.id);
-        await this.orderCalculator.applyPriceAdjustments(ctx, order, promotions, updatedOrderLines, {
-            recalculateShipping: input.options?.recalculateShipping,
-        });
+        const updatedOrderItems = await this.orderCalculator.applyPriceAdjustments(
+            ctx,
+            order,
+            promotions,
+            updatedOrderLines,
+            {
+                recalculateShipping: input.options?.recalculateShipping,
+            },
+        );
 
         const orderCustomFields = (input as any).customFields;
         if (orderCustomFields) {
@@ -491,11 +495,7 @@ export class OrderModifier {
             if (shippingDelta < 0) {
                 refundInput.shipping = shippingDelta * -1;
             }
-            refundInput.adjustment += await this.getAdjustmentFromNewlyAppliedPromotions(
-                ctx,
-                order,
-                activePromotionsPre,
-            );
+            refundInput.adjustment += this.calculateRefundAdjustment(delta, refundInput);
             const existingPayments = await this.getOrderPayments(ctx, order.id);
             const payment = existingPayments.find(p => idsAreEqual(p.id, input.refund?.paymentId));
             if (payment) {
@@ -526,9 +526,10 @@ export class OrderModifier {
             await this.connection.getRepository(ctx, OrderItem).save(orderItems, { reload: false });
         } else {
             // Otherwise, just save those OrderItems that were specifically added/removed
+            // or updated when applying `OrderCalculator.applyPriceAdjustments()`
             await this.connection
                 .getRepository(ctx, OrderItem)
-                .save(modification.orderItems, { reload: false });
+                .save([...modification.orderItems, ...updatedOrderItems], { reload: false });
         }
         await this.connection.getRepository(ctx, ShippingLine).save(order.shippingLines, { reload: false });
         return { order, modification: createdModification };
@@ -546,35 +547,21 @@ export class OrderModifier {
         return noChanges;
     }
 
-    private async getAdjustmentFromNewlyAppliedPromotions(
-        ctx: RequestContext,
-        order: Order,
-        promotionsPre: Promotion[],
-    ) {
-        const newPromotionDiscounts = order.discounts
-            .filter(discount => {
-                const promotionId = AdjustmentSource.decodeSourceId(discount.adjustmentSource).id;
-                return !promotionsPre.find(p => idsAreEqual(p.id, promotionId));
-            })
-            .filter(discount => {
-                // Filter out any discounts that originate from ShippingLine discounts,
-                // since they are already correctly accounted for in the refund calculation.
-                for (const shippingLine of order.shippingLines) {
-                    if (
-                        shippingLine.discounts.find(
-                            shippingDiscount =>
-                                shippingDiscount.adjustmentSource === discount.adjustmentSource,
-                        )
-                    ) {
-                        return false;
-                    }
-                }
-                return true;
-            });
-        if (newPromotionDiscounts.length) {
-            return -summate(newPromotionDiscounts, 'amountWithTax');
-        }
-        return 0;
+    /**
+     * @description
+     * Because a Refund's amount is calculated based on the orderItems changed, plus shipping change,
+     * we need to make sure the amount gets adjusted to match any changes caused by other factors,
+     * i.e. promotions that were previously active but are no longer.
+     */
+    private calculateRefundAdjustment(
+        delta: number,
+        refundInput: RefundOrderInput & { orderItems: OrderItem[] },
+    ): number {
+        const existingAdjustment = refundInput.adjustment;
+        const itemAmount = summate(refundInput.orderItems, 'proratedUnitPriceWithTax');
+        const calculatedDelta = itemAmount + refundInput.shipping + existingAdjustment;
+        const absDelta = Math.abs(delta);
+        return absDelta !== calculatedDelta ? absDelta - calculatedDelta : 0;
     }
 
     private getOrderPayments(ctx: RequestContext, orderId: ID): Promise<Payment[]> {