-
Notifications
You must be signed in to change notification settings - Fork 347
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
refactor: unify shipping line logic #1191
Changes from all commits
6b834ca
2ad2bc3
6d0ae14
c43c2ee
fb97d08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,61 +1,67 @@ | ||
import { TransactionModel } from "avatax/lib/models/TransactionModel"; | ||
import { numbers } from "../../taxes/numbers"; | ||
import { TaxBadProviderResponseError } from "../../taxes/tax-error"; | ||
import { taxProviderUtils } from "../../taxes/tax-provider-utils"; | ||
import { CalculateTaxesResponse } from "../../taxes/tax-provider-webhook"; | ||
import { SHIPPING_ITEM_CODE } from "./avatax-calculate-taxes-adapter"; | ||
import { TaxBadProviderResponseError } from "../../taxes/tax-error"; | ||
|
||
export class AvataxCalculateTaxesResponseShippingTransformer { | ||
transform( | ||
transaction: TransactionModel, | ||
): Pick< | ||
CalculateTaxesResponse, | ||
"shipping_price_gross_amount" | "shipping_price_net_amount" | "shipping_tax_rate" | ||
> { | ||
const shippingLine = transaction.lines?.find((line) => line.itemCode === SHIPPING_ITEM_CODE); | ||
import { avataxShippingLine } from "./avatax-shipping-line"; | ||
import { createLogger } from "@saleor/apps-logger"; | ||
|
||
if (!shippingLine) { | ||
return { | ||
shipping_price_gross_amount: 0, | ||
shipping_price_net_amount: 0, | ||
shipping_tax_rate: 0, | ||
}; | ||
} | ||
const logger = createLogger("transformAvataxTransactionModelIntoShipping"); | ||
|
||
if (!shippingLine.isItemTaxable) { | ||
return { | ||
shipping_price_gross_amount: taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.lineAmount, | ||
new TaxBadProviderResponseError("shippingLine.lineAmount is undefined"), | ||
), | ||
shipping_price_net_amount: taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.lineAmount, | ||
new TaxBadProviderResponseError("shippingLine.lineAmount is undefined"), | ||
), | ||
/* | ||
* avatax doesn't return combined tax rate | ||
* // todo: calculate percentage tax rate | ||
*/ | ||
shipping_tax_rate: 0, | ||
}; | ||
} | ||
// why is tax rate 0? | ||
export function transformAvataxTransactionModelIntoShipping( | ||
transaction: TransactionModel, | ||
): Pick< | ||
CalculateTaxesResponse, | ||
"shipping_price_gross_amount" | "shipping_price_net_amount" | "shipping_tax_rate" | ||
> { | ||
const shippingLine = avataxShippingLine.getFromTransactionModel(transaction); | ||
|
||
const shippingTaxCalculated = taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.taxCalculated, | ||
new TaxBadProviderResponseError("shippingLine.taxCalculated is undefined"), | ||
); | ||
const shippingTaxableAmount = taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.taxableAmount, | ||
new TaxBadProviderResponseError("shippingLine.taxableAmount is undefined"), | ||
); | ||
const shippingGrossAmount = numbers.roundFloatToTwoDecimals( | ||
shippingTaxableAmount + shippingTaxCalculated, | ||
if (!shippingLine) { | ||
logger.warn( | ||
"Shipping line was not found in the response from AvaTax. The app will return 0s for shipping fields.", | ||
); | ||
|
||
return { | ||
shipping_price_gross_amount: shippingGrossAmount, | ||
shipping_price_net_amount: shippingTaxableAmount, | ||
shipping_price_gross_amount: 0, | ||
shipping_price_net_amount: 0, | ||
shipping_tax_rate: 0, | ||
}; | ||
} | ||
|
||
if (!shippingLine.isItemTaxable) { | ||
return { | ||
shipping_price_gross_amount: taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.lineAmount, | ||
new TaxBadProviderResponseError("shippingLine.lineAmount is undefined"), | ||
), | ||
shipping_price_net_amount: taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.lineAmount, | ||
new TaxBadProviderResponseError("shippingLine.lineAmount is undefined"), | ||
), | ||
/* | ||
* avatax doesn't return combined tax rate | ||
* // todo: calculate percentage tax rate | ||
*/ | ||
shipping_tax_rate: 0, | ||
}; | ||
} | ||
|
||
const shippingTaxCalculated = taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.taxCalculated, | ||
new TaxBadProviderResponseError("shippingLine.taxCalculated is undefined"), | ||
); | ||
const shippingTaxableAmount = taxProviderUtils.resolveOptionalOrThrowUnexpectedError( | ||
shippingLine.taxableAmount, | ||
new TaxBadProviderResponseError("shippingLine.taxableAmount is undefined"), | ||
); | ||
const shippingGrossAmount = numbers.roundFloatToTwoDecimals( | ||
shippingTaxableAmount + shippingTaxCalculated, | ||
); | ||
|
||
return { | ||
shipping_price_gross_amount: shippingGrossAmount, | ||
shipping_price_net_amount: shippingTaxableAmount, | ||
shipping_tax_rate: 0, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import { createLogger } from "@saleor/apps-logger"; | ||
import { LineItemModel } from "avatax/lib/models/LineItemModel"; | ||
import { TransactionModel } from "avatax/lib/models/TransactionModel"; | ||
|
||
export const SHIPPING_ITEM_CODE = "Shipping"; | ||
|
||
const logger = createLogger("avataxShippingLine"); | ||
|
||
export const avataxShippingLine = { | ||
// * In AvaTax, shipping is a regular line | ||
create({ | ||
amount, | ||
taxCode, | ||
taxIncluded, | ||
}: { | ||
amount: number; | ||
taxCode: string | undefined; | ||
taxIncluded: boolean; | ||
}): LineItemModel { | ||
return { | ||
amount, | ||
taxIncluded, | ||
taxCode, | ||
itemCode: SHIPPING_ITEM_CODE, | ||
quantity: 1, | ||
}; | ||
}, | ||
getFromTransactionModel(transactionModel: TransactionModel) { | ||
const shippingLine = transactionModel.lines?.find( | ||
(line) => line.itemCode === SHIPPING_ITEM_CODE, | ||
); | ||
|
||
if (!shippingLine) { | ||
logger.warn("Shipping line not found in the transaction model"); | ||
} | ||
|
||
return shippingLine; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,15 @@ import { LineItemModel } from "avatax/lib/models/LineItemModel"; | |
import { OrderConfirmedSubscriptionFragment } from "../../../../generated/graphql"; | ||
import { numbers } from "../../taxes/numbers"; | ||
import { AvataxConfig } from "../avatax-connection-schema"; | ||
import { avataxShippingLine } from "../calculate-taxes/avatax-shipping-line"; | ||
import { AvataxTaxCodeMatches } from "../tax-code/avatax-tax-code-match-repository"; | ||
import { SHIPPING_ITEM_CODE } from "./avatax-order-confirmed-payload-transformer"; | ||
import { AvataxOrderConfirmedTaxCodeMatcher } from "./avatax-order-confirmed-tax-code-matcher"; | ||
|
||
export class AvataxOrderConfirmedPayloadLinesTransformer { | ||
transform( | ||
order: OrderConfirmedSubscriptionFragment, | ||
config: AvataxConfig, | ||
matches: AvataxTaxCodeMatches | ||
matches: AvataxTaxCodeMatches, | ||
): LineItemModel[] { | ||
const productLines: LineItemModel[] = order.lines.map((line) => { | ||
const matcher = new AvataxOrderConfirmedTaxCodeMatcher(); | ||
|
@@ -20,7 +20,7 @@ export class AvataxOrderConfirmedPayloadLinesTransformer { | |
// taxes are included because we treat what is passed in payload as the source of truth | ||
taxIncluded: true, | ||
amount: numbers.roundFloatToTwoDecimals( | ||
line.totalPrice.net.amount + line.totalPrice.tax.amount | ||
line.totalPrice.net.amount + line.totalPrice.tax.amount, | ||
), | ||
taxCode, | ||
quantity: line.quantity, | ||
|
@@ -31,18 +31,11 @@ export class AvataxOrderConfirmedPayloadLinesTransformer { | |
}); | ||
|
||
if (order.shippingPrice.net.amount !== 0) { | ||
// * In AvaTax, shipping is a regular line | ||
const shippingLine: LineItemModel = { | ||
const shippingLine = avataxShippingLine.create({ | ||
amount: order.shippingPrice.gross.amount, | ||
taxIncluded: true, | ||
itemCode: SHIPPING_ITEM_CODE, | ||
/** | ||
* * Different shipping methods can have different tax codes. | ||
* https://developer.avalara.com/ecommerce-integration-guide/sales-tax-badge/designing/non-standard-items/\ | ||
*/ | ||
taxCode: config.shippingTaxCode, | ||
quantity: 1, | ||
}; | ||
taxIncluded: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but this is the |
||
}); | ||
|
||
return [...productLines, shippingLine]; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,8 +11,6 @@ import { AvataxEntityTypeMatcher } from "../avatax-entity-type-matcher"; | |
import { AvataxTaxCodeMatches } from "../tax-code/avatax-tax-code-match-repository"; | ||
import { AvataxOrderConfirmedPayloadLinesTransformer } from "./avatax-order-confirmed-payload-lines-transformer"; | ||
|
||
export const SHIPPING_ITEM_CODE = "Shipping"; | ||
|
||
export class AvataxOrderConfirmedPayloadTransformer { | ||
private matchDocumentType(config: AvataxConfig): DocumentType { | ||
if (!config.isDocumentRecordingEnabled) { | ||
|
@@ -42,6 +40,11 @@ export class AvataxOrderConfirmedPayloadTransformer { | |
orderId: order.id, | ||
}); | ||
const customerCode = customerCodeResolver.resolveOrderCustomerCode(order); | ||
const billingAddress = order.billingAddress; | ||
|
||
if (!billingAddress) { | ||
throw new Error("Billing address not found in OrderConfirmed subscription."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I think we should use a ModernError subclass here instead of generic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
return { | ||
model: { | ||
|
@@ -55,7 +58,7 @@ export class AvataxOrderConfirmedPayloadTransformer { | |
addresses: { | ||
shipFrom: avataxAddressFactory.fromChannelAddress(avataxConfig.address), | ||
// billing or shipping address? | ||
shipTo: avataxAddressFactory.fromSaleorAddress(order.billingAddress!), | ||
shipTo: avataxAddressFactory.fromSaleorAddress(billingAddress), | ||
}, | ||
currencyCode: order.total.currency, | ||
// we can fall back to empty string because email is not a required field | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.