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

refactor: unify shipping line logic #1191

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import { AvataxCalculateTaxesPayloadService } from "./avatax-calculate-taxes-pay
import { AvataxCalculateTaxesResponseTransformer } from "./avatax-calculate-taxes-response-transformer";
import { createLogger } from "../../../logger";

export const SHIPPING_ITEM_CODE = "Shipping";

export type AvataxCalculateTaxesTarget = CreateTransactionArgs;
export type AvataxCalculateTaxesResponse = CalculateTaxesResponse;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ import { TaxBaseFragment } from "../../../../generated/graphql";
import { AvataxConfig } from "../avatax-connection-schema";
import { AvataxTaxCodeMatches } from "../tax-code/avatax-tax-code-match-repository";
import { AvataxCalculateTaxesTaxCodeMatcher } from "./avatax-calculate-taxes-tax-code-matcher";
import { SHIPPING_ITEM_CODE } from "./avatax-calculate-taxes-adapter";
import { avataxShippingLine } from "./avatax-shipping-line";

export class AvataxCalculateTaxesPayloadLinesTransformer {
transform(
taxBase: TaxBaseFragment,
config: AvataxConfig,
matches: AvataxTaxCodeMatches
matches: AvataxTaxCodeMatches,
): LineItemModel[] {
const isDiscounted = taxBase.discounts.length > 0;
const productLines: LineItemModel[] = taxBase.lines.map((line) => {
Expand All @@ -26,15 +26,11 @@ export class AvataxCalculateTaxesPayloadLinesTransformer {
});

if (taxBase.shippingPrice.amount !== 0) {
// * In AvaTax, shipping is a regular line
const shippingLine: LineItemModel = {
const shippingLine = avataxShippingLine.create({
amount: taxBase.shippingPrice.amount,
itemCode: SHIPPING_ITEM_CODE,
taxCode: config.shippingTaxCode,
quantity: 1,
taxIncluded: taxBase.pricesEnteredWithTax,
discounted: isDiscounted,
};
});

return [...productLines, shippingLine];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { numbers } from "../../taxes/numbers";
import { TaxBadPayloadError } 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 { SHIPPING_ITEM_CODE } from "./avatax-shipping-line";

export class AvataxCalculateTaxesResponseLinesTransformer {
transform(transaction: TransactionModel): CalculateTaxesResponse["lines"] {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { describe, expect, it } from "vitest";
import { avataxMockFactory } from "../avatax-mock-factory";
import { AvataxCalculateTaxesResponseShippingTransformer } from "./avatax-calculate-taxes-response-shipping-transformer";

const transformer = new AvataxCalculateTaxesResponseShippingTransformer();
import { transformAvataxTransactionModelIntoShipping } from "./avatax-calculate-taxes-response-shipping-transformer";

const TAX_EXCLUDED_NO_SHIPPING_TRANSACTION_MOCK =
avataxMockFactory.createMockTransaction("taxExcludedNoShipping");
Expand All @@ -12,9 +10,11 @@ const TAX_INCLUDED_SHIPPING_TRANSACTION_MOCK =
const TAX_EXCLUDED_SHIPPING_TRANSACTION_MOCK =
avataxMockFactory.createMockTransaction("taxExcludedShipping");

describe("AvataxCalculateTaxesResponseShippingTransformer", () => {
describe("transformAvataxTransactionModelIntoShipping", () => {
it("when shipping line is not present, returns 0s", () => {
const shippingLine = transformer.transform(TAX_EXCLUDED_NO_SHIPPING_TRANSACTION_MOCK);
const shippingLine = transformAvataxTransactionModelIntoShipping(
TAX_EXCLUDED_NO_SHIPPING_TRANSACTION_MOCK,
);

expect(shippingLine).toEqual({
shipping_price_gross_amount: 0,
Expand All @@ -23,7 +23,9 @@ describe("AvataxCalculateTaxesResponseShippingTransformer", () => {
});
});
it("when shipping line is not taxable, returns line amount", () => {
const nonTaxableShippingLine = transformer.transform(NON_TAXABLE_TRANSACTION_MOCK);
const nonTaxableShippingLine = transformAvataxTransactionModelIntoShipping(
NON_TAXABLE_TRANSACTION_MOCK,
);

expect(nonTaxableShippingLine).toEqual({
shipping_price_gross_amount: 77.51,
Expand All @@ -33,7 +35,9 @@ describe("AvataxCalculateTaxesResponseShippingTransformer", () => {
});

it("when shipping line is taxable and tax is included, returns calculated gross & net amounts", () => {
const taxableShippingLine = transformer.transform(TAX_INCLUDED_SHIPPING_TRANSACTION_MOCK);
const taxableShippingLine = transformAvataxTransactionModelIntoShipping(
TAX_INCLUDED_SHIPPING_TRANSACTION_MOCK,
);

expect(taxableShippingLine).toEqual({
shipping_price_gross_amount: 77.51,
Expand All @@ -43,7 +47,9 @@ describe("AvataxCalculateTaxesResponseShippingTransformer", () => {
});

it("when shipping line is taxable and tax is not included, returns calculated gross & net amounts", () => {
const taxableShippingLine = transformer.transform(TAX_EXCLUDED_SHIPPING_TRANSACTION_MOCK);
const taxableShippingLine = transformAvataxTransactionModelIntoShipping(
TAX_EXCLUDED_SHIPPING_TRANSACTION_MOCK,
);

expect(taxableShippingLine).toEqual({
shipping_price_gross_amount: 84.87,
Expand Down
Copy link
Member

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?

Copy link
Member Author

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.

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
@@ -1,12 +1,11 @@
import { TransactionModel } from "avatax/lib/models/TransactionModel";
import { CalculateTaxesResponse } from "../../taxes/tax-provider-webhook";
import { AvataxCalculateTaxesResponseLinesTransformer } from "./avatax-calculate-taxes-response-lines-transformer";
import { AvataxCalculateTaxesResponseShippingTransformer } from "./avatax-calculate-taxes-response-shipping-transformer";
import { transformAvataxTransactionModelIntoShipping } from "./avatax-calculate-taxes-response-shipping-transformer";

export class AvataxCalculateTaxesResponseTransformer {
transform(response: TransactionModel): CalculateTaxesResponse {
const shippingTransformer = new AvataxCalculateTaxesResponseShippingTransformer();
const shipping = shippingTransformer.transform(response);
const shipping = transformAvataxTransactionModelIntoShipping(response);

const linesTransformer = new AvataxCalculateTaxesResponseLinesTransformer();
const lines = linesTransformer.transform(response);
Expand Down
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
Expand Up @@ -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();
Expand All @@ -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,
Expand All @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

});

return [...productLines, shippingLine];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.");
Copy link
Member

Choose a reason for hiding this comment

The 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 Error

Copy link
Member Author

Choose a reason for hiding this comment

The 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: {
Expand All @@ -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
Expand Down
Loading