Fix: solve a empty order line with discount modification NaN error

[jyling]

Description

Please include a summary of the changes and the related issue.
This PR solves #3008, where there's an order level discount and we are updating the order to remove an order line by setting the quantity to 0. Currently, it throws a NaN error.

The solution is just to have a fallback of 0, when the distribution[i] returns undefined.

Breaking changes

Does this PR include any breaking changes we should be aware of?
should not break existing functionality.

Screenshots

Note: I'm not sure why this test failed on my local machine, my changes shouldn't affect this test

Checklist

📌 Always:

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

👍 Most of the time:

• I have added or updated test cases
• I have updated the README if needed

[vercel]

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

Name	Status	Preview	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	Aug 21, 2024 3:00am

[github-actions]

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

[jyling]

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

[michaelbromley]

Thanks again - I reviewed this part of the code more closely and while you have indeed prevented the error from being thrown, I discovered another subtle bug that also needs to be addressed (my bug, not yours :D)

In the e2e test, if we add the following lines at the end, it will fail:

            // ensure correct adjustments applied
            // The first line should have a linePrice of 0 because it has zero quantity
            expect(modifyOrder.lines[0].linePriceWithTax).toBe(0);
            expect(modifyOrder.lines[0].proratedLinePriceWithTax).toBe(0);
            // The second line should have the proratedLinePriceWithTax discounted per the promotion
            expect(modifyOrder.lines[1].proratedLinePriceWithTax).toBe(
                modifyOrder.lines[1].discountedLinePriceWithTax / 2,
            );

The reason for this is subtle: on line 252 of order-calculator.ts we are filtering out zero-quantity order lines. This then means that the weights array (and the derived distribution array) will have fewer elements than the order.lines array.

So when on line 255 we loop over all the order lines, this is where we run into the error of trying to get an out-of-range index from the distribution array. Defaulting to 0 as in your fix will prevent the NaN error, but is still not correct, which is why the added e2e test assertions will fail.

The solution is to remove the .filter() call on line 252, to ensure that we do not end up with a smaller array and that distribution.length === order.lines.length at all times.

In summary, I recommend the following changes to make this fix correct:

1. Add the e2e assertions as above to your existing test
2. Remove the filter call on line 252
3. Change the map call to .map(l => (l.quantity !== 0 ? l.proratedLinePriceWithTax : 0));
4. At this point you can also remove the ?? 0 default since the distribution array will always have the correct length.

[jyling]

In summary, I recommend the following changes to make this fix correct:

Thank you for the detailed explanation of the code that caused this bug, I will update the code and rerun the test.

[jyling]

I implemented the proposed fixes, which indeed solved the NaN issue, however i encountered another issue where the discounts is missing

I will look into this later, but just to update on this PR

[michaelbromley]

very strange - all e2e tests are passing in CI. Looks like there is something in your environment perhaps which is causing this?

edit: make sure you re-build the core package after changes. This might be the issue.

[michaelbromley]

Tested locally and it looks good to me 👍