Fix: apply currency default tax rate when currency changes in manual expense create flow#94496
Fix: apply currency default tax rate when currency changes in manual expense create flow#94496MelvinBot wants to merge 26 commits into
Conversation
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
🤖 I looked into the failing checks. No code fix is needed — the only failing check is It fails because the PR body still has the placeholder Every code-related check is green or passing:
Next step: complete the Tests/QA sections and the PR Author Checklist, then the |
|
@Eskalifer1 can you push this one forward? |
|
Hi @lakchote, I don't know if I'll be able to finish this task today, but I think it'll be ready by Monday! While I was working on the testing, I had a question: 🔍 Behavior Inconsistency: Edit flow always resets manually chosen tax rate on currency change📋 ContextThis PR fixes a bug where the tax rate on the confirmation page was not updated after changing currency in the create flow. As part of the fix, a guard was introduced to preserve the user's manually chosen tax rate: const isCurrentTaxAutoDefault =
!transaction?.taxCode || transaction?.taxCode === previousDefaultTaxCode;
if (selectedCurrency !== previousCurrency && isCurrentTaxAutoDefault) {
// update to new currency's default
}🐛 The ProblemThe edit flow ( // Before
const taxCode =
(selectedCurrency !== transactionCurrency
? defaultTaxCode
: transactionTaxCode) ?? defaultTaxCode;Reproduction steps
Expected: ✅ The FixApplied the same const previousDefaultTaxCode = getDefaultTaxCode(
policy,
currentTransaction,
transactionCurrency,
);
const isCurrentTaxAutoDefault =
!transactionTaxCode || transactionTaxCode === previousDefaultTaxCode;
const taxCode =
(selectedCurrency !== transactionCurrency && isCurrentTaxAutoDefault
? defaultTaxCode
: transactionTaxCode) ?? defaultTaxCode;This makes create and edit flows consistent: auto-default tax rates follow the new currency, manually chosen rates are preserved. 🎯 ❓ Should we fix this or we should make create flow a bit simpler and always reset tax no matter if user changed tax manually? |
|
Video of bug above! edit-flow-bug.mov |
|
@MelvinBot I have found a bug: 🔍 RCA: Tax rate not updated when changing currency from FAB flow📋 SummaryWhen a user starts creating an expense from the FAB (Floating Action Button), changes the currency on the amount step, and navigates forward, the tax rate on the confirmation page is not updated to match the new currency — it always shows the workspace default tax rate. 🔍 Root CauseThe bug is caused by
const policyID = isTrackExpense ? policyForMovingExpensesID : report?.policyID;
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`);When starting from the workspace chat, report?.policyID is defined — policy resolves correctly. When starting from the FAB, there is no report context. report is undefined or an empty object, so policyID = undefined and therefore policy = undefined for the entire lifetime of the amount step screen. 🔗 How policy = undefined causes the bug const previousDefaultTaxCode = getDefaultTaxCode(policy, transaction, previousCurrency);
const isCurrentTaxAutoDefault = !transaction?.taxCode || transaction?.taxCode === previousDefaultTaxCode;
if (isMovingTransactionFromTrackExpense(action) || (selectedCurrency !== previousCurrency && isCurrentTaxAutoDefault)) {
const taxCode = getDefaultTaxCode(policy, transaction, selectedCurrency);
if (taxCode) {
setMoneyRequestTaxRate(transactionID, taxCode);
...
}
}Since policy = undefined, getDefaultTaxCode(undefined, ...) always returns undefined. The if (taxCode) guard is never entered, so setMoneyRequestTaxRate is never called. The transaction draft is persisted to Onyx with taxCode still undefined. 🤔 Why does the initial tax display correctly? This is why the initial value appears correct — it is computed on the confirmation page at render time, not persisted in the draft. 🚨 Why does the currency change not propagate? The confirmation page remounts fresh. useTaxAmount uses usePrevious to detect currency changes: const previousDefaultTaxCode = getDefaultTaxCode(policy, transaction, previousTransactionCurrency);
const shouldKeepCurrentTaxSelection = hasTaxRateWithMatchingValue(policy, transaction) && transaction?.taxCode !== previousDefaultTaxCode;Because the confirmation page is a fresh mount, previousTransactionCurrency initializes from the current (already new) currency. This means previousDefaultTaxCode equals the foreign currency default. Meanwhile transaction.taxCode is undefined, which does not equal the foreign default — so shouldKeepCurrentTaxSelection = true, and TaxController early-returns without applying the new currency's tax rate. The result: the workspace default tax rate remains on the confirmation page regardless of the selected currency. How would you fix this? |
|
@MelvinBot take a look at this comment: #94496 (comment) |
|
Hi @lakchote, please take a look at this comment. And i have one more problem. For some reason Melvin does not post any response here, He adds 👀 emoji, but never post a response |
I think the expected result is correct, I've verified on Classic:
Do you confirm @trjExpensify? |
cc @Julesssss @justinpersaud maybe Melvin should post a message in case of failure? |
|
@MelvinBot there's a bug identified in #94496 (comment), how would you fix this? |
If that's what we do on Classic, makes sense to do the same here. 👍 |
Thanks for reporting, created an internal issue here to fix that asap. In the meantime, it might help if you are a bit more direct about Melvin responding to you:
|
|
@MelvinBot take a look at this comment |
|
Your RCA is right about the core gap: from the FAB, One correction to the mechanism (doesn't change your conclusion): on the second confirmation visit
→ How I'd fix it: teach In const previousDefaultTaxCode = getDefaultTaxCode(policy, transaction, previousTransactionCurrency);
// A tax code the user never manually chose: it is one of the policy's auto-defaults,
// but not the default for the *current* currency — i.e. left over from before a currency
// change (e.g. the FAB flow, where the amount step has no policy to recompute it).
const isStaleAutoDefault =
(transaction?.taxCode === policy?.taxRates?.defaultExternalID || transaction?.taxCode === policy?.taxRates?.foreignTaxDefault) &&
transaction?.taxCode !== defaultTaxCode;
const shouldKeepCurrentTaxSelection = hasTaxRateWithMatchingValue(policy, transaction) && transaction?.taxCode !== previousDefaultTaxCode && !isStaleAutoDefault;Why this holds up:
Same design keeps the amount-step gate for the workspace-chat flow (it corrects the draft before confirmation when the policy is known) and adds the confirmation-page safety net for the FAB flow where the policy isn't. It shares the one unavoidable limitation the amount-step gate already has: manually re-selecting a code that happens to equal a currency default is indistinguishable from the auto-applied state and will be treated as auto. Want me to implement this on the branch and add a |
|
@MelvinBot Find every possible regression or risk with solution you have mentioned here |
|
Good call to pressure-test it — the heuristic has a real, new regression, plus a few smaller risks. Here's the full list. 🚑 Headline regression: user can no longer manually pick a rate that equals the other currency's defaultBecause Repro: foreign-currency expense on the confirmation page → open the tax picker → select the workspace This is worse than the amount-step gate, which only fires on a currency change. Today ( Other risks
Recommended mitigationThe root problem behind almost all of these is the missing auto-vs-manual signal. The clean fix is to persist a "tax manually set" flag on the draft: set it in the tax-rate picker action ( If a flag is too invasive for this PR, the cheaper containment is to only run the stale-default correction when a currency change is actually detected (e.g. compare against a persisted "currency when tax was last applied" marker) rather than on every render — that removes the live-revert-without-currency-change case, though it leaves the deliberate-default ambiguity. Want me to implement the manual-set-flag approach? |
|
@MelvinBot Why can't we use something like that? --- a/src/pages/iou/request/step/IOURequestStepAmount.tsx
+++ b/src/pages/iou/request/step/IOURequestStepAmount.tsx
@@ -68,7 +68,7 @@ function IOURequestStepAmount({
const selfDMReport = useSelfDMReport();
const isReportArchived = useReportIsArchived(report?.reportID);
- const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`);
+ const [reportPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`);
const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`);
const iouOrExpenseReport = useReportOrReportDraft(report?.chatReportID);
const actualChatReportID = iouOrExpenseReport && isMoneyRequestReport(iouOrExpenseReport) ? iouOrExpenseReport.chatReportID : undefined;
@@ -79,6 +79,14 @@ function IOURequestStepAmount({
const [skipConfirmation] = useOnyx(`${ONYXKEYS.COLLECTION.SKIP_CONFIRMATION}${transactionID}`);
const defaultExpensePolicy = useDefaultExpensePolicy();
const personalPolicy = usePersonalPolicy();
+
+ // In the FAB flow, report?.policyID is undefined so reportPolicy is undefined.
+ // Fall back to the policy derived from already-chosen participants (workspace expense chat)
+ // so tax-rate logic in submitAmount has a policy when the user returns from the confirmation page
+ // after having selected a workspace participant.
+ const participantPolicyID = transaction?.participants?.find((p) => p.isPolicyExpenseChat)?.policyID;
+ const [participantPolicy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${participantPolicyID}`);
+ const policy = reportPolicy ?? participantPolicy;
const [userBillingGracePeriodEnds] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_USER_BILLING_GRACE_PERIOD_END);
const [transactionDrafts] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_DRAFT, {selector: validTransactionDraftsSelector});
const [transactionViolations] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS);What possible regressions have this solution? |
|
Yes, that works — and it's arguably a cleaner fix than my confirmation-page suggestion, because it keeps the correction in one place and lets the FAB flow reuse the amount-step gate you already built. The The main thing I'd change, plus the regressions to watch: 1. const participantPolicyID = transaction?.participants?.find((participant) => participant.isPolicyExpenseChat)?.policyID;
const policyID = (isTrackExpense ? policyForMovingExpensesID : report?.policyID) ?? participantPolicyID;
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`);
const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`);2. It broadens two other amount-step behaviors in the FAB return trip. Both currently get
3. Split / multiple participants. 4. Coverage. This only corrects currency changes that route back through this amount step — which is the reported repro (Back → change currency → Next always does). If there's any path that changes currency without re-entering Net: I'd go with your approach using the single- |
|
@MelvinBot What is about second point? How can i test it, maybe we have now some bug related to this on main branch? Can you descripe a bit more, so i could check it and maybe our changes will fix it? |
|
Good questions — and yes, the "second point" is a real bug you can reproduce on What the second point actually saysBy "second point" I mean my correction: on the second trip to the confirmation page, the draft's So when you come back with the new currency, the draft still carries the old currency's default. That leftover value is what makes
→ Is there a bug on
|
|
@MelvinBot Yep, that's actually the point we want to fix. So i proposed to fix it via workspace. I was asking about this second point:
Is there any bug related to this one or not? It goes without saying that we're fixing the bug with taxes. I was asking if there might be any other issues that this could affect that aren't related to taxes? |
|
Short answer: no — neither introduces a non-tax bug. Both are gated behind The key reason both are safe: the 1. 2. So the only observable effect of the fallback in the FAB return trip is the tax recompute you intend. The one thing worth a sanity check isn't these two lines but point #1 from before — keying Want me to apply the single- |
|
@MelvinBot Yes, let's implement this change, also include every possible improvement |
|
@MelvinBot Take a look at all the other suggested changes that we haven't implemented yet, and implement them. They all relate to comments. |
Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
Done — pushed the remaining comment-only suggestions in 7117f3d. Removed (all were
Already applied earlier, so nothing to do:
No logic changed — these are comment-only deletions, so no typecheck/test impact. |
|
@MelvinBot Let's merge main |
|
@MelvinBot Merge main |
|
@MelvinBot Merge main into this branch |
|
@MelvinBot merge main and resolve conflicts |
…ppliedOnCurrencyChange Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com> # Conflicts: # src/libs/IOUAmountSubmission.ts
|
Merged One conflict, in the file this PR edits — which Resolution: kept
Both are used by the currency-change tax gate at Validation:
|
|
@lakchote Take a look at this again pls! |
@Eskalifer1 I'm looking to fix it in the BE so the FE and BE are on par, then I'll review it again |
|
BE PR is still in review. |
|
BE PR has been merged (waiting for deploy now). @Eskalifer1 can you fix conflicts please? |
|
@MelvinBot Let's merge main and resolve conflicts |
…ppliedOnCurrencyChange Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com> # Conflicts: # src/libs/IOUAmountSubmission.ts
|
Merged This wasn't a trivial conflict — What happened
How I resolved itRather than hand-merge the clashing hunks, I took main's fully-refactored version as the base and re-applied this PR's three documented changes on top, so main's new architecture is preserved verbatim:
I also added the two imports main didn't have ( One intentional detail: main's create path had inlined the tax-code lookup as The full resolution is contained in the merge commit
|
|
@lakchote Let me know when BE will be ready, so i can test all again. Thank you! |
BE is now ready @Eskalifer1, feel free to test again, thanks! |
|
@MelvinBot Let's merge main and resolve conflicts |
…ppliedOnCurrencyChange Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com> # Conflicts: # src/libs/IOUAmountSubmission.ts
|
🤖 Merged There was a single conflict in
I kept both sets of imports since all three symbols are used in the merged file, preserving alphabetical import order. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ee4be6dcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const previousCurrency = getCurrency(transaction); | ||
| const isCurrentTaxAutoDefault = isTaxCodeAutoDefaultForCurrency(policy, transaction, previousCurrency, transaction?.taxCode); | ||
| const isPolicyExpenseChatParticipant = transaction?.participants?.some((participant) => participant.isPolicyExpenseChat) ?? false; | ||
| const isTaxEnabled = isTaxTrackingEnabled(isPolicyExpenseChat(report) || isPolicyExpenseChatParticipant, policy, false); |
There was a problem hiding this comment.
In the Track expense amount step from a self-DM/default workspace, report is not a policy expense chat and the transaction does not have a policy-expense-chat participant, even though the screen has resolved a tax-enabled policy via usePolicyForMovingExpenses and the confirmation list still shows tax for isTrackExpense. With this guard, Back → change currency → Next leaves the old default tax code in the draft for that Track flow, so the stale workspace-currency tax is preserved instead of switching to the foreign default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think Codex is right — this is a real gap (medium-high confidence). I haven't changed anything.
The mismatch: the guard at IOUAmountSubmission.ts:519 derives isTaxEnabled from isPolicyExpenseChat(report) || isPolicyExpenseChatParticipant only. For a Track expense from a self-DM, report isn't a policy expense chat and there's no policy-expense-chat participant, so isTaxEnabled is false — even though policy is a real, tax-enabled policy resolved via usePolicyForMovingExpenses (IOURequestStepAmount.tsx:82-89).
Meanwhile the confirmation list decides whether to show tax with isTaxTrackingEnabled(isPolicyExpenseChat || isTrackExpense, …) at MoneyRequestConfirmationList.tsx:298 — it does include isTrackExpense. So tax is shown for the track flow, but the amount-step currency recompute is skipped → Back → change currency → Next keeps the stale (previous-currency) default. The first disjunct isMovingTransactionFromTrackExpense(action) doesn't save it either — that only fires when moving an existing tracked expense, not creating one.
The parameter is literally named isPolicyExpenseChatOrUnreportedExpense (PolicyUtils.ts:1412), and every other caller that gates track-expense tax includes a track/unreported term (MoneyRequestConfirmationList.tsx:298, IOURequestStepDistanceRate.tsx:114, useExpenseSubmission.ts:309) — this new guard is the only one that omits it.
Suggested fix — mirror what the confirmation list uses so the guard matches when tax is actually displayed. iouType is already in args:
const isTaxEnabled = isTaxTrackingEnabled(
isPolicyExpenseChat(report) || isPolicyExpenseChatParticipant || iouType === CONST.IOU.TYPE.TRACK || isExpenseUnreported(transaction),
policy,
false,
);(isExpenseUnreported would need importing from TransactionUtils.)
Worth a manual check to confirm: self-DM Track expense on a tax-enabled workspace → verify tax shows on confirmation, then Back → change currency → Next and verify the foreign default is applied (and that a manually-picked rate is still preserved).
Want me to implement this?
There was a problem hiding this comment.
🤖 Implemented in 927d463.
I extended the isTaxEnabled guard in submitCreateAmount to include the track-expense tax contexts, mirroring what MoneyRequestConfirmationList's shouldShowTax uses — so the currency recompute now runs for a Track expense from a self-DM/default workspace, not just for policy-expense-chat contexts:
const isTaxEnabled = isTaxTrackingEnabled(
isPolicyExpenseChat(report) || isPolicyExpenseChatParticipant || iouType === CONST.IOU.TYPE.TRACK || isExpenseUnreported(transaction),
policy,
false,
);isExpenseUnreported is now imported from ./TransactionUtils. typecheck-tsgo and lint both pass.
Please still do the manual check I flagged: self-DM Track expense on a tax-enabled workspace → confirm tax shows on the confirmation page, then Back → change currency → Next and verify the foreign default is applied (and a manually-picked rate is still preserved).
…te guard
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Seems like Be changes working well! I think we can merge it now cc: @lakchote |
| 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 |
There was a problem hiding this comment.
NAB: put an empty line before the comment, can you handle that as a follow-up @Eskalifer1 please?
| // Mirror the tax contexts the confirmation list uses (see MoneyRequestConfirmationList's shouldShowTax): a Track | |
| // Mirror the tax contexts the confirmation list uses (see MoneyRequestConfirmationList's shouldShowTax): a Track |
There was a problem hiding this comment.
Done — pushed in 5696f8b. Added the empty line before the // Mirror the tax contexts… comment as suggested.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
5 participants
Explanation of Change
When creating a manual expense on a tax-enabled workspace, changing the currency before reaching the confirmation page did not re-apply the new currency's default tax rate — the previous (workspace-currency) default stayed on the confirmation page and on the created expense instead of switching to the foreign-currency default.
The create flow's
navigateToNextPageonly recomputed the tax code when moving a transaction from track-expense, so a normal currency change left the staletaxCode/taxAmounton the draft. The confirmation page'sTaxControllerwas supposed to correct this, but itsshouldKeepCurrentTaxSelectionheuristic relies onusePrevious(transaction.currency), which seeds with the current value on a fresh mount (the confirmation step is a separate route that remounts on Back→Next) — so it mistook the auto-applied default for a manual selection and suppressed the correction.This fixes it at the source in
AmountSubmission.ts: the track-expense-only tax recompute is generalized to also run whenever the currency actually changed, so the persisted draft is always correct regardless of the confirmation page's render timing. To avoid the regression of overwriting a tax rate the user picked manually, the recompute is gated onisCurrentTaxAutoDefault— it only re-applies the currency default when the current tax code is still the auto-applied default for the previous currency (using the samegetDefaultTaxCodeheuristic the confirmation page already uses). A manually-selected non-default rate is preserved across the currency change.This implements the approach the issue requester specified in #93772 (comment).
Fixed Issues
$ #93772
PROPOSAL: #93772 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review".
// Please describe what tests you performed that validate your change worked.
Suggested manual steps:
Default(e.g. 5%) and the foreign-currency default toForeign(e.g. 10%).Defaultrate is applied.Foreignrate and recalculated tax amount.Defaultrate is restored.Offline tests
Same as Tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas they need to test for regressions.
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videosundefined