fix: ensures no duplicate tax lines when completing cart #1262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
olivermrbl
previously approved these changes
Mar 29, 2022
| @@ -27,7 +27,7 @@ describe("Order Taxes", () => { | |||
| const cwd = path.resolve(path.join(__dirname, "..", "..")) | |||
| try { | |||
| dbConnection = await initDb({ cwd }) | |||
| medusaProcess = await setupServer({ cwd }) | |||
| medusaProcess = await setupServer({ cwd, verbose: true }) | |||
There was a problem hiding this comment.
nit: remove flag
Suggested change
| medusaProcess = await setupServer({ cwd, verbose: true }) | |
| medusaProcess = await setupServer({ cwd }) |
Comment on lines
+1216
to
+1221
| if (!cart.payment_session) { | ||
| throw new MedusaError( | ||
| MedusaError.Types.NOT_ALLOWED, | ||
| "You cannot complete a cart without a payment session." | ||
| ) | ||
| } |
There was a problem hiding this comment.
nit: While we are at it, should we also enforce shipping methods for cart completion to go through?
| @@ -197,6 +197,111 @@ describe("Order Taxes", () => { | |||
| expect(response.data.order.total).toEqual(2300) | |||
| }) | |||
|
|
|||
| test.only("completing cart with failure doesn't duplicate", async () => { | |||
There was a problem hiding this comment.
very, very nit:
Suggested change
| test.only("completing cart with failure doesn't duplicate", async () => { | |
| test.only("completing cart with failure doesn't duplicate tax lines", async () => { |
|
Found out that this approach of deleting everything first and then continuing is not appropriate - the issue is that if you are completing a Swap cart for example we need to move the original line items to "return lines" and these should include the original tax lines. Will try to come up with a different approach |
olivermrbl
dismissed
their stale review
Alternative approach is being investigated
olivermrbl
reviewed
Mar 31, 2022
Comment on lines
+11
to
+12
| conflict_target: ["item_id", "code"], | ||
| overwrite: ["rate", "name", "updated_at"], |
Comment on lines
159
to
187
| async upsertTaxLines_( | ||
| taxLines: (ShippingMethodTaxLine | LineItemTaxLine)[] | ||
| ): Promise<UpdateResult[]> { | ||
| const itemTaxLineRepo = this.manager_.getCustomRepository(this.taxLineRepo_) | ||
| const shippingTaxLineRepo = this.manager_.getCustomRepository( | ||
| this.smTaxLineRepo_ | ||
| ) | ||
|
|
||
| return await Promise.all( | ||
| taxLines.map(async (tl) => { | ||
| if ("item_id" in tl) { | ||
| return await itemTaxLineRepo.update( | ||
| { item_id: tl.item_id, code: tl.code }, | ||
| { | ||
| rate: tl.rate, | ||
| name: tl.name, | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| return await shippingTaxLineRepo.update( | ||
| { shipping_method_id: tl.shipping_method_id, code: tl.code }, | ||
| { | ||
| rate: tl.rate, | ||
| name: tl.name, | ||
| } | ||
| ) | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Delete since no longer used right?
There was a problem hiding this comment.
Is this used @srindom? I can't seem to find any references to it.
SGFGOV
pushed a commit
to SGFGOV/medusa
that referenced
this pull request
Apr 19, 2022
* fix: ensures that no duplicate tax lines are created when completing cart
This was referenced Nov 2, 2023
kodiakhq bot
pushed a commit
that referenced
this pull request
Nov 6, 2023
While I was developing a custom tax provider, I ran into a pretty frustrating issue related to multiple tax lines and incorrect / unexpected tax total calculations. I outlined the issue in discord, before finding the solution: https://discord.com/channels/876835651130097704/1169705457804398662 After some digging, I found this issue from a while back: #1262, where it states there is now a unique constraint on `item_id, code` and `shipping_method_id, code`. But, in the current documentation for creating a custom tax provider, it states these fields as part of the tax line items returned are optional. If these `code` values are left out, it can cause tax lines to be applied multiple times (as seen here as well: #1901) I'm not entirely sure how this should be phrased in the documentation, so I just wanted to get this up and on your radar for resolution. If it ends up being that `code` truly should not be optional, I suspect some type definitions would need to change as well?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
A small fix that ensures that when a cart is completed any existing tax lines will be deleted. Currently, if a complete cart request is performed with a failure you can risk creating an order with two tax lines.
This fix ensures that any existing tax lines on the cart line items are cleared before the new tax lines are created.Edit: New approach introduces a unique constraint on
item_id, codeandshipping_method_id, code. Two new repository methods are introduced in ShippingMethodTaxLineRepository and LineItemTaxLineRepository calledupsertLineswhich uses an insert/on conflict to ensure that duplicate lines are not created.