-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix: apply currency default tax rate when currency changes in manual expense create flow #94496
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
Changes from all commits
6d720bf
624bad3
f9b8a90
03379fd
58a7fad
cbb2c74
c70c504
a1b56ec
1e6c3b1
29ce74d
6bd27a3
56d8dbc
0060acc
f82d957
a01f421
37106dd
7ab258e
cd71be8
edd77b0
2aa7c6c
7117f3d
c166047
dd52b39
1ee4be6
927d463
5696f8b
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,9 +45,10 @@ import {rand64} from './NumberUtils'; | |||||||
| import {getParticipantsOption, getReportOption} from './OptionsListUtils'; | ||||||||
| import Permissions from './Permissions'; | ||||||||
| import {getLoginByAccountID} from './PersonalDetailsUtils'; | ||||||||
| import {getPolicyExpenseChat, getTransactionDetails, isMoneyRequestReport, isSelfDM, shouldEnableNegative} from './ReportUtils'; | ||||||||
| import {isTaxTrackingEnabled} from './PolicyUtils'; | ||||||||
| import {getPolicyExpenseChat, getTransactionDetails, isMoneyRequestReport, isPolicyExpenseChat, isSelfDM, shouldEnableNegative} from './ReportUtils'; | ||||||||
| import shouldUseDefaultExpensePolicy from './shouldUseDefaultExpensePolicy'; | ||||||||
| import {calculateTaxAmount, getAmount, getCurrency, getDefaultTaxCode, getIsFromGlobalCreate, getTaxValue, hasReceipt} from './TransactionUtils'; | ||||||||
| import {calculateTaxAmount, getAmount, getCurrency, getDefaultTaxCode, getIsFromGlobalCreate, getTaxValue, hasReceipt, isExpenseUnreported} from './TransactionUtils'; | ||||||||
|
|
||||||||
| type SubmitAmountArgs = { | ||||||||
| report: OnyxEntry<OnyxTypes.Report>; | ||||||||
|
|
@@ -136,6 +137,23 @@ function getIsP2PForAmount({chatReportForP2P, currentUserAccountID}: GetIsP2PFor | |||||||
| return isParticipantP2P(firstParticipant); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Determines whether a transaction's current tax code is still the auto-applied default for the given currency | ||||||||
| * (i.e. the user has not manually picked a different rate). | ||||||||
| */ | ||||||||
| function isTaxCodeAutoDefaultForCurrency( | ||||||||
| policy: OnyxEntry<OnyxTypes.Policy>, | ||||||||
| transaction: OnyxEntry<OnyxTypes.Transaction>, | ||||||||
| currency: string | undefined, | ||||||||
| taxCode: string | undefined, | ||||||||
| ): boolean { | ||||||||
| if (taxCode === '') { | ||||||||
| return false; | ||||||||
| } | ||||||||
| const defaultTaxCodeForCurrency = getDefaultTaxCode(policy, transaction, currency); | ||||||||
| return !taxCode || taxCode === defaultTaxCodeForCurrency; | ||||||||
| } | ||||||||
|
|
||||||||
| type SubmitAmountContext = { | ||||||||
| isEditing: boolean; | ||||||||
| isCreateAction: boolean; | ||||||||
|
|
@@ -491,8 +509,26 @@ function submitCreateAmount(args: SubmitAmountArgs, ctx: SubmitAmountContext): v | |||||||
|
|
||||||||
| setMoneyRequestAmount(transactionID, amountInSmallestCurrencyUnits, selectedCurrency || CONST.CURRENCY.USD, shouldKeepUserInput, hasReceipt(transaction)); | ||||||||
|
|
||||||||
| if (isMovingTransactionFromTrackExpense(action)) { | ||||||||
| const taxCode = selectedCurrency !== policy?.outputCurrency ? policy?.taxRates?.foreignTaxDefault : policy?.taxRates?.defaultExternalID; | ||||||||
| // When the currency changes, re-apply the default tax rate for the new currency so the confirmation page and the | ||||||||
| // created expense reflect the currency-appropriate default (e.g. the foreign default for a foreign currency). | ||||||||
| // Only do this when the current tax code is still the auto-applied default for the previous currency, so a tax | ||||||||
| // rate the user manually selected is preserved across the currency change. | ||||||||
| const previousCurrency = getCurrency(transaction); | ||||||||
| const isCurrentTaxAutoDefault = isTaxCodeAutoDefaultForCurrency(policy, transaction, previousCurrency, transaction?.taxCode); | ||||||||
| const isPolicyExpenseChatParticipant = transaction?.participants?.some((participant) => participant.isPolicyExpenseChat) ?? false; | ||||||||
|
|
||||||||
| // Mirror the tax contexts the confirmation list uses (see MoneyRequestConfirmationList's shouldShowTax): a Track | ||||||||
|
Contributor
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. NAB: put an empty line before the comment, can you handle that as a follow-up @Eskalifer1 please?
Suggested change
Contributor
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. Sure! @MelvinBot Let's apply this change
Contributor
Author
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. Done — pushed in |
||||||||
| // expense from a self-DM/default workspace isn't a policy expense chat, but the confirmation page still shows tax | ||||||||
| // for the track flow, so the currency recompute must run for it too. Otherwise Back → change currency → Next would | ||||||||
| // leave the stale previous-currency default tax on the draft. | ||||||||
| const isTaxEnabled = isTaxTrackingEnabled( | ||||||||
| isPolicyExpenseChat(report) || isPolicyExpenseChatParticipant || iouType === CONST.IOU.TYPE.TRACK || isExpenseUnreported(transaction), | ||||||||
| policy, | ||||||||
| false, | ||||||||
| ); | ||||||||
|
|
||||||||
| if (isMovingTransactionFromTrackExpense(action) || (isTaxEnabled && selectedCurrency !== previousCurrency && isCurrentTaxAutoDefault)) { | ||||||||
| const taxCode = getDefaultTaxCode(policy, transaction, selectedCurrency); | ||||||||
| if (taxCode) { | ||||||||
| setMoneyRequestTaxRate(transactionID, taxCode); | ||||||||
| const taxPercentage = getTaxValue(policy, transaction, taxCode) ?? ''; | ||||||||
|
|
@@ -551,10 +587,14 @@ function submitEditAmount(args: SubmitAmountArgs, ctx: SubmitAmountContext): voi | |||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| // If currency has changed, then we get the default tax rate based on currency, otherwise we use the current tax rate selected in transaction, if we have it. | ||||||||
| // When the currency changes we re-apply the new currency's default tax rate, but only when the current tax rate is | ||||||||
| // still the auto-applied default for the previous currency. A tax rate the user picked manually is preserved across | ||||||||
| // the currency change (mirrors the create-flow guard in `submitCreateAmount`). | ||||||||
| const transactionTaxCode = getTransactionDetails(currentTransaction)?.taxCode; | ||||||||
| const defaultTaxCode = getDefaultTaxCode(policy, currentTransaction, selectedCurrency) ?? ''; | ||||||||
| const taxCode = (selectedCurrency !== transactionCurrency ? defaultTaxCode : transactionTaxCode) ?? defaultTaxCode; | ||||||||
| const isCurrentTaxAutoDefault = isTaxCodeAutoDefaultForCurrency(policy, currentTransaction, transactionCurrency, transactionTaxCode); | ||||||||
| const isTransactionTaxCodeValid = transactionTaxCode === '' || getTaxValue(policy, currentTransaction, transactionTaxCode ?? '') !== undefined; | ||||||||
| const taxCode = ((selectedCurrency !== transactionCurrency && isCurrentTaxAutoDefault) || !isTransactionTaxCodeValid ? defaultTaxCode : transactionTaxCode) ?? defaultTaxCode; | ||||||||
|
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.
When an edited expense has an intentionally blank tax code, e.g. after the tax was cleared from the tax-rate editor, Useful? React with 👍 / 👎.
Contributor
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. @MelvinBot What do you think about this one? Don't make any changes yet
Contributor
Author
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 with the bot — this is a valid P2 regression, not a false positive. No changes made, per your note. Why it's real: an intentionally-cleared tax (
At Suggested direction (for when you do change it): treat empty string as a legitimate selection and only "heal" a non-empty code that no longer resolves: const isTransactionTaxCodeValid = transactionTaxCode === '' || getTaxValue(policy, currentTransaction, transactionTaxCode ?? '') !== undefined;That keeps the deleted/moved-workspace heal you added (non-empty codes that don't resolve still fall back) while preserving an explicit "no tax". Worth a unit test for the cleared-tax + amount-edit case alongside it. One thing to decide: whether a cleared tax should also survive a currency change. With the guard above it does (
Contributor
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. @MelvinBot let's implement this
Contributor
Author
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. Implemented in 37106dd. Applied the suggested guard so an intentionally-cleared tax ( const isTransactionTaxCodeValid = transactionTaxCode === '' || getTaxValue(policy, currentTransaction, transactionTaxCode ?? '') !== undefined;This keeps the deleted/moved-workspace heal for non-empty codes that no longer resolve, while preserving an explicit "no tax". As you noted, a cleared tax also correctly survives a currency change ( Added a regression unit test for the cleared-tax + amount-edit case at |
||||||||
| const taxPercentage = getTaxValue(policy, currentTransaction, taxCode) ?? ''; | ||||||||
| const taxAmount = convertToBackendAmount(calculateTaxAmount(taxPercentage, newAmount, decimals)); | ||||||||
|
|
||||||||
|
|
||||||||
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.
Can we preserve an intentionally cleared tax here?
taxCode === ''is how the create flow stores tax cleared, and the edit path below already treats''as a legitimate cleared tax.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.
@MelvinBot Let's implement this one
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.
Done — pushed in
7ab258e.isTaxCodeAutoDefaultForCurrencynow short-circuits on an empty tax code:An empty string is an intentional "no tax" selection, so treating it as not auto-default means a currency change no longer heals it back to the new currency's default — it's preserved. Without the guard the trailing
!taxCodecheck counted''as auto-default, which is exactly the wipe you flagged. This matches how the edit path already treats''as a legitimate cleared tax. Both call sites (create atAmountSubmission.ts:298and edit atAmountSubmission.ts:567) go through this helper, so the fix applies to both flows.Heads up: I couldn't run lint/typecheck/jest in this environment (the App ESLint config OOMs here and the test runner is sandbox-gated), so please run
npm run typecheck+npx jest tests/unit/AmountSubmissionTest.tslocally before marking ready. The change itself is a 3-line guard on a pure helper with no new imports.