Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Create PromotionLineAction #2971

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

Feelw00
Copy link
Contributor

@Feelw00 Feelw00 commented Jul 25, 2024

Description

To address the issue mentioned in the previous PR, I modified the return type of the execute method in PromotionItemAction to return either a Number or ExecutePromotionActionResult, allowing the inclusion of discountMode.

However, to maintain code consistency, I have added PromotionLineAction and adjusted all PromotionActions to return a Number type.

Other tasks follow the contents of the attached previous PR.

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

Copy link

vercel bot commented Jul 25, 2024

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

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jul 25, 2024 10:10am

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - just 1 comment on the rounding. If there is a good reason to do it this was please let me know, else pass the quantity in to the roundMoney() function.

@@ -149,6 +158,13 @@ export class Promotion
const promotionAction = this.allActions[action.code];
if (promotionAction instanceof PromotionItemAction) {
if (this.isOrderItemArg(args)) {
const { orderLine } = args;
amount +=
roundMoney(await promotionAction.execute(ctx, orderLine, action.args, state, this)) *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the roundMoney() function accepts a 2nd argument for quantity, so that the configured MoneyStrategy can control the multiplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review.
It seems the issue arose because the previous quantity calculation logic was simply transferred.
I have added the calculation logic as an argument to roundMoney.

@michaelbromley michaelbromley merged commit 0ff8288 into vendure-ecommerce:minor Jul 25, 2024
12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
@michaelbromley
Copy link
Member

Thank you very much for your contribution and continued input in developing this feature!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants