refactor: unify shipping line logic #1191
Conversation
…dPayloadTransformer
…ModelIntoShipping
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
question: I don't see a point to why did we change it into a function instead of class? Is this purely stylistic reason?
There was a problem hiding this comment.
Yes, exactly. Initializing a class with no arguments in the constructor and executing its single method takes 2 lines instead of 1 (with function). I think it makes the code less readable.
| taxCode: config.shippingTaxCode, | ||
| quantity: 1, | ||
| }; | ||
| taxIncluded: true, |
There was a problem hiding this comment.
question: Is this a good assumption in general? Sometimes you need to calculate different VAT for shipping (e.g. you're selling books with lower VAT rate and shipping is included a as a separate line with higher tax rate)
There was a problem hiding this comment.
Yes, but this is the OrderConfirmed event, when already all the taxes were calculated. We just need to pass the values to AvaTax. Otherwise, an extra tax gets charged.
| const billingAddress = order.billingAddress; | ||
|
|
||
| if (!billingAddress) { | ||
| throw new Error("Billing address not found in OrderConfirmed subscription."); |
There was a problem hiding this comment.
suggestion: I think we should use a ModernError subclass here instead of generic Error
There was a problem hiding this comment.
I agree, but it hasn't been installed in the app, and I would rather not spend time on it. Feel free to pop it in
|
this can't be merged because it needs to be ported to AvaTax app |
Context
A shipping app calculates taxes. It returns positive values. But when querying the shipping values, they are all 0s (not just the tax rate). We narrowed down the cause - it's the Taxes App.
We need to revisit the shipping calculations in the Taxes App and try to figure out why it returns 0s.
Scope of the PR
No cause was yet found, so it's WIP.
Checklist