[$250] Tax - Different tax rate on confirmation page & transaction thread when tax rate is disabled

lanitochka17 commented 3 months ago

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: 9.0.16-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4808605 Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Workspace has a few tax rates
Workspace currency default and Foreign currency default have different tax rate
1. Go to staging.new.expensify.com
2. Go to workspace settings > Taxes
3. Disable the tax rate for Foreign currency default
4. Go to workspace chat
5. Go to + > Split expense
6. Enter amount and select a foreign currency
7. Proceed to confirmation page
8. Note that the tax rate is Foreign currency default even though it is already disabled
9. Split the expense
10. Open the split preview and also transaction thread
11. Note that the tax rate changes to Workspace currency default

Expected Result:

In Step 8. the tax rate on confirmation page when foreign currency is used should be consistent with the tax rate on split preview and transaction thread (Step 11)

Actual Result:

In Step 8. the tax rate on confirmation page when foreign currency shows the tax rate for Foreign currency default even though it is already disabled In Step 11, after splitting the expense, the tax rate changes to Workspace currency default which is not consistent with the rate shown on the confirmation page before splitting

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/52705480-0019-498c-87da-54864ce51376

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~012b83b7675405b2d3
Upwork Job ID: 1820469144458744658
Last Price Increase: 2024-08-12

Issue Owner

Current Issue Owner: @eVoloshchak

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @isabelastisser (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.

lanitochka17 commented 3 months ago

@isabelastisser FYI 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

FitseTLT commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-02 19:45:41 UTC.

Proposal

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

Different tax rate on confirmation page & transaction thread when tax rate is disabled

What is the root cause of that problem?

We are displaying the foreign default if the policy currency and transaction currency are not the same without check if the foreign default is not disabled in the first place https://github.com/Expensify/App/blob/8abdcca86923a5a121734f34bbc53f2eeb5a45dd/src/libs/TransactionUtils.ts#L760-L768 But the BE checks that it is disabled and it will default it to the default tax

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

We need to check if the foreign tax is disabled and if it is we should set it to defaultExternalID

  const defaultExternalID = policy?.taxRates?.defaultExternalID;
    const foreignTaxDefault = policy?.taxRates?.foreignTaxDefault;
    const taxes = policy?.taxRates?.taxes;
    const isForeignDefaultDisabled = taxes?.[foreignTaxDefault ?? '']?.isDisabled;

    return policy?.outputCurrency === (currency ?? getCurrency(transaction)) || isForeignDefaultDisabled ? defaultExternalID : foreignTaxDefault;
}

Although this bug is related to getDefaultTaxCode called inside getTaxName here the function is used in a lot of places so we can make this logic of checking if the foreign default is disabled for all usages of the function or we can create a param and only use it in selected places we need them.

By the way in the case of submit expense the disabled foreign default tax is set to the expense with violation indicating the tax is no longer valid so the changes suggested above might need only be applied for split expense not submit ones. so we only apply the check if iouType is split when we get the tax name here

What alternative solutions did you explore? (Optional)

Krishna2323 commented 3 months ago

Proposal

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

Tax - Different tax rate on confirmation page & transaction thread when tax rate is disabled

What is the root cause of that problem?

We allow tax to be disabled even if it is Foreign currency default and disabled tax is returned from getDefaultTaxCode. https://github.com/Expensify/App/blob/459759d55963091c708d7271c29e0f95f3add658/src/libs/PolicyUtils.ts#L414-L419 https://github.com/Expensify/App/blob/459759d55963091c708d7271c29e0f95f3add658/src/libs/TransactionUtils/index.ts#L757-L766
What changes do you think we should make in order to solve the problem?

We should update canEditTaxRate to return false when tax is defaultExternalID or foreignTaxDefault.

function canEditTaxRate(policy: Policy, taxID: string): boolean {
    return policy.taxRates?.defaultExternalID !== taxID && policy.taxRates?.foreignTaxDefault !== taxID;
}

What alternative solutions did you explore? (Optional)

Result

https://github.com/user-attachments/assets/2a6d12a6-b63b-49c7-9432-3dd2e98ea0c5

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~012b83b7675405b2d3

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!

isabelastisser commented 3 months ago

@eVoloshchak can you please review the proposals? Thanks!

trjExpensify commented 3 months ago

CC: @luacmartins @MonilBhavsar for vis!

luacmartins commented 2 months ago

@MonilBhavsar would you be able to take a look at this since you're more familiar with the tax udf?

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@eVoloshchak, @isabelastisser 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

eVoloshchak commented 2 months ago

This was already resolved by https://github.com/Expensify/App/pull/46862, @FitseTLT, @Krishna2323, could you please double-check if this is still reproducible for you?

Krishna2323 commented 2 months ago

@eVoloshchak, not reproducible anymore. @isabelastisser we can close this.

luacmartins commented 2 months ago

Closing as per the comments above.

Expensify / App