[HOLD for payment 2024-05-22] Tax - Unable to save valid tax rate amount when changing currency for the first time

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.73-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-internal team

Action Performed:

Precondition:

• Workspace has enabled tax rate.
• Workspace currency default is tax exempt (0%).
• Foreign currency default have different tax rate (non-zero rate).

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Go to + > Submit expense.
4. Enter amount (maintain local currency) > Next.
5. On confirmation page, click Tax amount.
6. Change the currency to any foreign currency.
7. After changing to foreign currency, the tax rate will automatically adjust to the non-zero tax rate (Foreign currency default).
8. Enter a valid tax amount (now that the rate is no longer 0%).
9. Save it.

Expected Result:

User will be able to save the tax amount as long as the tax amount falls within the limit of the new rate (Foreign currency default).

Actual Result:

When saving a valid tax amount, error shows up. The maximum rate amount in the error message still follows the previous tax rate (Workspace currency default). User will need to save 0 tax amount and return to Tax amount after saving to save the new tax amount.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6479175_1715595255890.foreign_tax.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @lakchote (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open Staging deploy checklist to see the list of PRs included in this release, then work quickly on the following:

1. If you find which PR caused the issue/bug, you can reassign it to the person responsible for it.
   • If the author is OOO or won’t get online before the daily deploy is due, you are responsible for finding the best fix/path forward. Don’t hesitate to ask for help!
2. Try to reproduce the issue, if the bug is on production, remove the DeployBlocker label but stay assigned to fix it (or find out which PR broke it to get help from the author).
   • You can adjust the urgency of the issue to better represent the gravity of the bug.
   • If the issue is super low priority, feel free to un-assign yourself.
   • Be careful with PHP warnings, sometimes it is more complex than just adding a null coalescing operator as they might be uncovering some bigger bug.
   • If it was a one-off issue that requires no action (for example, Bedrock was down or it is a duplicated issue), you can close it.

Remember rule #2: Never un-assign yourself from a real DeployBlocker unless you are 100% sure someone else is assigned and will take care of it.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[izarutskaya]

@abekkala I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[izarutskaya]

We think this issue might be related to the #collect project.

[izarutskaya]

Production

Recording.2448.mp4

[ShridharGoel]

I can quickly open a PR. This was caused by #40443.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unable to save valid tax rate amount when changing currency for the first time

What is the root cause of that problem?

We are not using the selected currency while finding the tax rate in IOURequestStepTaxAmountPage.

App/src/pages/iou/request/step/IOURequestStepTaxAmountPage.tsx

Lines 37 to 50 in cf14505

	function getTaxAmount(transaction: OnyxEntry<Transaction>, policy: OnyxEntry<Policy>, isEditing: boolean): number | undefined {
	if (!transaction?.amount) {
	return;
	}
	const transactionTaxAmount = TransactionUtils.getAmount(transaction);
	const transactionTaxCode = transaction?.taxCode ?? '';
	const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction) ?? '';
	const getTaxValue = (taxCode: string) => TransactionUtils.getTaxValue(policy, transaction, taxCode);
	const defaultTaxValue = getTaxValue(defaultTaxCode);
	const moneyRequestTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : defaultTaxValue) ?? '';
	const editingTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : moneyRequestTaxPercentage) ?? '';
	const taxPercentage = isEditing ? editingTaxPercentage : moneyRequestTaxPercentage;
	return CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, transactionTaxAmount));
	}

App/src/libs/TransactionUtils.ts

Lines 661 to 665 in cf14505

	function getDefaultTaxCode(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>) {
	const defaultExternalID = policy?.taxRates?.defaultExternalID;
	const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault;
	return policy?.outputCurrency === getCurrency(transaction) ? defaultExternalID : foreignTaxDefault;
	}

We are always taking the currency via the transaction.

What changes do you think we should make in order to solve the problem?

Pass currency to getTaxAmount.

taxAmount={getTaxAmount(transaction, policy, currency, Boolean(backTo || isEditing))}

function getTaxAmount(transaction: OnyxEntry<Transaction>, policy: OnyxEntry<Policy>, currency: string | undefined, isEditing: boolean): number | undefined {
    if (!transaction?.amount) {
        return;
    }
    const transactionTaxAmount = TransactionUtils.getAmount(transaction);
    const transactionTaxCode = transaction?.taxCode ?? '';
    const defaultTaxCode = TransactionUtils.getDefaultTaxCode(policy, transaction, currency) ?? '';
    const getTaxValue = (taxCode: string) => TransactionUtils.getTaxValue(policy, transaction, taxCode);
    const defaultTaxValue = getTaxValue(defaultTaxCode);
    const moneyRequestTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : defaultTaxValue) ?? '';
    const editingTaxPercentage = (transactionTaxCode ? getTaxValue(transactionTaxCode) : moneyRequestTaxPercentage) ?? '';
    const taxPercentage = isEditing ? editingTaxPercentage : moneyRequestTaxPercentage;
    return CurrencyUtils.convertToBackendAmount(TransactionUtils.calculateTaxAmount(taxPercentage, transactionTaxAmount));
}

Pass the currency to getDefaultTaxCode:

function getDefaultTaxCode(policy: OnyxEntry<Policy>, transaction: OnyxEntry<Transaction>, currency: string | undefined) {
    const defaultExternalID = policy?.taxRates?.defaultExternalID;
    const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault;
    return policy?.outputCurrency === (currency ?? getCurrency(transaction)) ? defaultExternalID : foreignTaxDefault;
}

[allroundexperts]

@ShridharGoel's proposal works good. The RCA seems to be correct as well. We are always taking into account the transaction currency rather than the selected currency.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignees @cristipaval and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[cristipaval]

This one seems to be the offending PR. @MonilBhavsar could you please have a look at this deploy blocker?

[cristipaval]

ah, sorry @lakchote , it seems you were on it already 🙇
Unassigning myself.

[lakchote]

@ShridharGoel feel free to raise a PR whenever you can so we can test it timely and fix the blocker.

[Beamanator]

PR CP'd, @lakchote confirmed it's fixed on staging so marking NAB!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.73-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Use selected currency when utilising tax rate #42064

If no regressions arise, payment will be issued on 2024-05-22. 🎊

For reference, here are some details about the assignees on this issue:

• @allroundexperts requires payment through NewDot Manual Requests
• @ShridharGoel requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@allroundexperts] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @strepanier03 you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @allroundexperts owed $250 via NewDot
• ROLE: @ShridharGoel paid $250 via Upwork (LINK)

BugZero Checklist (@strepanier03)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[strepanier03]

I hired your in Upwork @ShridharGoel, I'll check tomorrow to see if I can pay it out.

@allroundexperts - I'll finish up the payment summary once the checklist is done, thanks!

[allroundexperts]

Checklist

1. support for offline tax tracking #40443
2. N/A since this was a regression
3. N/A
4. A regression test would be helpful. The test steps look simple enough to me!

[strepanier03]

Payment Summary

• @allroundexperts owed $250 via Manual Requests in NewDot
• @ShridharGoel paid $250 via Upwork (LINK) - Pending acceptance

@JmillsExpensify - Payment request incoming.

[strepanier03]

@ShridharGoel - Please accept the Upwork job so it can be paid?

[strepanier03]

I am out of the office from June 3-10 - If you need BZ action in that time please reach out in Slack to ask for an assist.

[JmillsExpensify]

Contributor paid out. @allroundexperts going to close this issue and see you again when I receive the payment request.

[JmillsExpensify]

$250 approved for @allroundexperts