-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(medusa): decorate order edit line items with totals #3108
Conversation
🦋 Changeset detectedLatest commit: b87b53f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@adrien2p - can you maybe take a look at this one. Think you will have a better perspective on the totals stuff :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
discount_total: 2001, | ||
total: 1099, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: change in total amounts in this test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what change resulted in this happening now when it didn't occur previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decorate OE totals via order service now which uses the new totals service, previously we used "old" totals service directly from OE service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do something about this rounding or did you agree to something?
@adrien2p Can I get you to run another review to see if your requested changes have been addressed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor todo's LGTM 💪
integration-tests/api/__tests__/admin/order-edit/ff-tax-inclusive-pricing.js
Show resolved
Hide resolved
packages/medusa/src/api/routes/admin/order-edits/update-order-edit-line-item.ts
Outdated
Show resolved
Hide resolved
discount_total: 2001, | ||
total: 1099, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do something about this rounding or did you agree to something?
What
Testing
RESOLVES CORE-1042