Repository navigation
Apply category tax rules to distance expenses without a rate tax - #101674
Krishna2323 wants to merge 17 commits into
Conversation
Signed-off-by: krishna2323 <[email protected]>
|
@MelvinBot review |
|
🤖 Reviewed — the approach is right, but there are two blockers. 1. CI is red — three existing tests fail
These encode the old "distance never picks up a category tax rule" behavior, so they need to change. But don't just flip the expectations — see below, because those three tests are set up in exactly the scenario that this PR now gets wrong. 2. Nothing guards the "Track tax is off for distance" caseThe old guard was When the workspace has Taxes on, a category tax rule exists, but Track tax is off on distance rates:
Every pre-existing Suggested fix — bail when distance tax tracking is off, not just when the rate has its own code: const distanceCustomUnit = customUnitRateID === undefined ? undefined : getDistanceRateCustomUnit(policy);
const shouldSkipDistanceTax = customUnitRateID !== undefined && (!distanceCustomUnit?.attributes?.taxEnabled || !!getDistanceRateCustomUnitRate(policy, customUnitRateID)?.attributes?.taxRateExternalID);
if (!taxRules || taxRules.length === 0 || shouldSkipDistanceTax) {
return {categoryTaxCode: undefined, categoryTaxAmount: undefined, categoryTaxValue: undefined};
}The three failing tests all use Smaller things
I didn't run the app test steps — worth doing once CI is green. Reply with |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@situchan friendly bump |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safaritest.mov |
…nses Signed-off-by: krishna2323 <[email protected]>
|
🤖 Web test steps verification results: The core fix works on web. The category rule sets TaxA on a no-tax rate, and a rate's own tax still wins. Step 1 failed only because Tax amount shows $0.00. That comes from the test setup, not the PR (see the note below the table). I found one real behavior change, a tax reset on category change, described after the table.
Regression found in code reviewChanging the category on a distance expense now overwrites the tax with the workspace default, even when the new category has no rule. Step 7 shows this live.
Manual expenses already behave this way on main, so this may be intended parity. If it is, add a test and a Lower-risk items
Ruled out: per diem, time, and manual expenses with no rule (unchanged), and render loops. CI jest, typecheck, and lint pass. The only failing check is "Check independent approval". |
|
🤖 No blockers at
One question is still open from my last review. Please confirm it is intended. A category change on a distance expense resets a hand-picked tax. Suppose you pick Nits
|
I don't think the category change reset is a regression. The repro needs "pick TaxB by hand", but Tax is read-only on distance expenses, both on confirm and on the expense view. A distance tax always comes from the rate, the category rule or the workspace default. So in step 7, moving from |
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
A few nab comments for code optimization
![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: 54d2665dc4
ℹ️ 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".
|
🤖 I found one regression and two older bugs that this PR now reaches on distance expenses. Manual expenses look clean. I traced this in code and did not run the app.
1. Regression: commuter exclusion is ignored when you pick a category
To reproduce:
Expected: the tax amount stays based on 20 miles, as on main. The category setter writes the larger amount. Nothing corrects it, because Fix: in 2. Older bugs this PR now reaches on distance expenses
Lower-confidence items and what I ruled outPlausible, but the outcome depends on the backend:
Ruled out:
Test gap: three existing "distance never gets category tax" tests pass only because |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
|
@MelvinBot review |
|
🤖 I found no blockers at
Two low-risk items are still open. Both already happen on manual expenses on main. Open items
|
|
is this ready @Krishna2323 ? |
|
@situchan friendly bump |
|
I had found bug in latest changes. Will confirm again and post |
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
1 regression and some nits
| const previousCategoryTaxCode = getCategoryTaxCode(transaction?.category ?? '', transaction, policy, previousTransactionCurrency); | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| const previousDefaultTaxCode = previousCategoryTaxCode || getDefaultTaxCode(policy, transaction, previousTransactionCurrency); | ||
| const shouldKeepCurrentTaxSelection = hasTaxRateWithMatchingValue(policy, transaction) && transaction?.taxCode !== previousDefaultTaxCode; |
There was a problem hiding this comment.
Bug: Picking the workspace default tax no longer follows a currency change when the category has a rule
- Workspace with Taxes and Rules on. Workspace currency default
Tax exempt (0%), Foreign currency defaultTaxF. Rule: CategoryCar→ TaxTaxA. - Create expense → Manual →
100in the workspace currency → pick the workspace → continue to confirm. - Set Category to
Car. Tax showsTaxA. - Tap Tax → pick
Tax exempt (0%). - Tap Amount → change to a foreign currency → save.
Expected: Tax switches to TaxF, same as main.
Actual: Tax stays Tax exempt (0%).
bug.mov
| } | ||
|
|
||
| if (isDistanceRequest(transaction)) { | ||
| const customUnitRateID = getRateID(transaction) ?? ''; |
There was a problem hiding this comment.
nit: getRateID(transaction) ?? '' never uses the '' because getRateID always returns a string.
| // Distance tax is charged on the taxable portion of the mileage rate rather than the full amount, so reuse the | ||
| // rate-aware calculation instead of taxing the expense amount. | ||
| if (isDistanceRequest(transaction)) { | ||
| const {taxCode, taxAmount, taxValue} = getDistanceRateTaxUpdates(policy, transaction, getRateID(transaction) ?? '', getCurrencyDecimals, undefined, category); |
| if (!getDistanceRateCustomUnit(policy)?.attributes?.taxEnabled || hasSelectableRateTaxCode(policy, customUnitRateID)) { | ||
| return undefined; | ||
| } | ||
| return getSelectableCategoryTaxCode(policy, taxRules, category, getDefaultTaxCode(policy, transaction, undefined, customUnitRateID) ?? ''); |
There was a problem hiding this comment.
?? '' passed into getSelectableCategoryTaxCode is the only reason useTaxAmount needs || and two eslint-disable lines.
Drop it and both can use ??.
| const defaultTaxCode = getDefaultTaxCode(policy, transaction, undefined, customUnitRateID) ?? ''; | ||
| // A rate's own tax rate wins over a category rule, because vehicle types coded to the same category can each | ||
| // reclaim at a different rate. | ||
| const taxRules = policy?.rules?.expenseRules?.filter((rule) => rule.tax) ?? []; |
There was a problem hiding this comment.
taxRules is filtered separately in both helpers.
App/src/libs/TransactionUtils/tax.ts
Line 241 in 11149f5
| const {taxCode, taxAmount, taxValue} = getDistanceRateTaxUpdates(policy, transaction, getRateID(transaction) ?? '', getCurrencyDecimals, undefined, category); | ||
| return {categoryTaxCode: taxCode, categoryTaxAmount: taxAmount, categoryTaxValue: taxValue}; | ||
| } |
There was a problem hiding this comment.
For distance, getCategoryTaxDetails resolves the code with getCategoryTaxCode, then getDistanceRateTaxUpdates resolves it again.
App/src/libs/TransactionUtils/tax.ts
Lines 264 to 276 in 11149f5
This branch has not been deployed
5 participants
Explanation of Change
Fixed Issues
$ #100544
PROPOSAL:
Tests
Prerequisites:
TaxAat 10%. Leave the workspace default asTax exempt (0%).Rate-NoTax, leaving its tax rate unset.Carand TaxTaxA. Save.Steps:
Rate-NoTax.Car.TaxA (10%)and Tax amount is 10% of the expense amount, not 0.TaxA (10%), so the confirm page matched what was created.Offline tests
QA Steps
Same as tests
[c] Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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/Videos
Android: Native
android_hybrid.mp4
Android: mWeb Chrome
android_mWeb.mp4
iOS: Native
ios_hybrid.mp4
iOS: mWeb Safari
ios_mWeb.mp4
MacOS: Chrome / Safari
web_chrome.mp4