feat(core): Add maxDiscountAmount to PromotionItemAction

[Feelw00]

Description

In the case of PromotionItemAction, there is an issue where the maximum discount amount cannot be specified because the orderLine quantity is multiplied after executing the apply function of the Promotion Entity.

To address this, we have added a maxDiscountAmount method to the PromotionItemAction class, which takes the same arguments as the execute method, allowing customization of the maximum discount amount. Additionally, we have modified the PromotionItemAction apply process to compare the discount amount with the maxDiscountAmount and apply the maximum discount amount if necessary.

The maxDiscountAmount is optional and will not conflict with existing Promotion Actions.

• Issue

Checklist

📌 Always:

• I have set a clear title
• My PR is small and contains a single feature
• I have checked my own PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	Jul 24, 2024 9:17am

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[Feelw00]

I have read the CLA Document and I hereby sign the CLA

[Feelw00]

It appears that the unit test failures in this PR are due to the value of the compatibility field in the TestPlugin.

Some TestPlugins had their compatibility set to ^2.0.0 when this PR was created, and they have now all been changed to ^3.0.0.

If the tests need to be rerun, please leave a comment!

[michaelbromley]

Hi, thanks for your contribution.

I want to make sure I fully understand this change. From your explanation and from looking at the code changes, it looks like the result of the maxDiscountAmount function will be used as a lower-bound for the OrderLine-level discount.

The problem this seems to be fixing is the specific scenario where you want to have a specific fixed discount on an OrderLine which is consistent for any quantity value.

I understand the current issue whereby the return value of the execute() function can sometimes lead to an unwanted total discount due to rounding issues.

Given that, my first thought is that I'd like to explore alternative solutions to adding the maxDiscountAmount() function. My problem with the current proposal is that:

1. The role of maxDiscountAmount() is a bit hard to understand. It took me a while to figure out what it is supposed to be doing.
2. Most of the logic between execute() & maxDiscountAmount() would be duplicated in most scenarios I am guessing. That's not ideal because it means we have to double the computation when applying promotions.
3. I always like to explore ways to avoid adding to the API surface of Vendure. I feel like we can possibly address the problem without adding a new function.

Alternative proposal

Here's an example of another approach which might be able to solve the issue in a backward-compatible way:

What if we allow the execute() function to return an object containing not only the discount amount, but a flag indicating that this is the total for the whole line:

  execute(ctx, orderLine, args) {
        if (lineContainsIds(args.productVariantIds, orderLine)) {
            const unitPrice = ctx.channel.pricesIncludeTax ? orderLine.unitPriceWithTax : orderLine.unitPrice;
            const lineDiscount = args.discount * -1;
            return {
              amount: lineDiscount,
              discountMode: 'line' // 'unit' would be the other possibility
            }
        }
        return 0;
    },

Then we would know, if discountMode === 'line' we don't need to then multiply by quantity.

Just returning a number (the current API) is equivalent to setting discountMode: 'unit'.

Happy to hear any feedback you have on that idea.

[Feelw00]

@michaelbromley

Hi, thank you for the great idea proposal.

The core of this PR is to add a maximum discount amount to the PromotionItemAction, which specifies the maximum discount amount for each orderLine unit.

I wanted to solve this issue, and the maxDiscountAmount() function is one way I proposed to do so.

I considered other methods, such as adding a column to the Promotion Entity or extending the type of adjustment, but I thought the simplest way to implement this without modifying the admin UI was to add the maxDiscountAmount function with the same arguments as the execute function.

However, I think your proposal to return the response of the execute function as an object and add a discountMode to determine whether to apply the quantity multiplication to the orderLine is a good alternative.

Implementing it this way would allow not only setting a maximum discount amount but also a fixed discount amount.

Thank you for your idea proposal. I will add a commit to the PR to revise it according to your suggestions.

[michaelbromley]

Thanks for the quick follow-up. Do you think you would also be able to include relevant e2e tests for this feature? If so they would go in this file: https://github.com/vendure-ecommerce/vendure/blob/f1753432c42c8616cd51add8629f65cc107899d6/packages/core/e2e/order-promotion.e2e-spec.ts

[Feelw00]

@michaelbromley

Hi

To add an object to the return type of the execute method in PromotionAction, I created the ExecutePromotionActionResult type as follows:

type ExecutePromotionActionResult = {
    amount: number;
    discountMode: 'line' | 'unit';
};

Additionally, I added test case and included the order-line-fixed-discount-action.ts in the defaultAction for testing purposes.

However, there is one issue.
This PR works perfectly on my local environment, but when running the e2e tests, the totalWithTax is calculated correctly as 2640 when ordering 3 items of item-1000 with a fixed discount of 800 on the line. However, the total is calculated as 2199 instead of 2200.

I would like some advice on this issue.

[michaelbromley]

This PR works perfectly on my local environment, but when running the e2e tests, the totalWithTax is calculated correctly as 2640 when ordering 3 items of item-1000 with a fixed discount of 800 on the line. However, the total is calculated as 2199 instead of 2200.

I see that all of the e2e tests are passing currently. In the above comment, are you referring to a test you made locally that you did not include in the PR?

If so, you could commit the failing test and I can take a look at it.

[Feelw00]

I have added the failed test case.
Please check it.

[michaelbromley]

OK I looked into this and found the reason for the test failure: it is because of the default MoneyStrategy that Vendure uses, which applies rounding in a non-optimal way (it rounds the amount and then multiplies by quantity), for backwards-compatibility reasons.

To fix this we can create a custom MoneyStrategy for the test suite:

class TestMoneyStrategy extends DefaultMoneyStrategy {
    round(value: number, quantity = 1): number {
        return Math.round(value * quantity);
    }
}

describe('Promotions applied to Orders', () => {
    const { server, adminClient, shopClient } = createTestEnvironment(
        mergeConfig(testConfig(), {
            dbConnectionOptions: { logging: true },
            paymentOptions: {
                paymentMethodHandlers: [testSuccessfulPaymentMethod],
            },
            // add here
            entityOptions: {
                moneyStrategy: new TestMoneyStrategy(),
            },
        }),
    );

This should make the failing test pass.

Actually we should also update the default money strategy for the next minor version to use this more correct rounding logic, but that can be a separate issue.

[Feelw00]

Understood.

I believe that it would be better in the long term to not only implement a more precise rounding logic but also to separate promotionAction into promotionOrderAction, promotionItemAction, and promotionLineAction, and configure it to return a number type instead of discountMode.

However, I don't think this is an issue that should be addressed in this PR.

What do you think about this?

[michaelbromley]

I believe that it would be better in the long term to not only implement a more precise rounding logic but also to separate promotionAction into promotionOrderAction, promotionItemAction, and promotionLineAction, and configure it to return a number type instead of discountMode.

If we were to plan introducing a promotionLineAction, then I would not merge this PR otherwise we will be creating a new API which would then be made redundant.

Pros:

• Keeps the return type simple and consistent as a number for every type of promotion action

Cons:

• Potentially less flexible than this solution. E.g. with this solution you can make a single "fixed discount" promotion action and then let the user configure at runtime whether it should be applied to the unit or to the line. With a separate promotionLineAction this would not be possible.

I actually think I prefer that suggestion - the simplicity of always having a number return type is a strong argument for that approach.

[Feelw00]

I agree.
I believe we should focus on the fact that Vendure is open source.

While flexible code can be valuable in actual operating environments,
Vendure, being open source, should be accessible to people who are not familiar with Vendure.
Therefore, to ensure code consistency, I think it's better to implement a new promotionLineAction.
Of course, this means we will have to revise all the existing work. 🙁

I will submit a new PR for the promotionLineAction.
I'll leave the decision to you. @michaelbromley

[Feelw00]

Since a PR with the same purpose has been merged, I will close this PR.