Repository navigation
fix: Clean-up some tax related bugs - #94007
MonilBhavsar merged 20 commits into
Conversation
… a workspace can’t be edited
…tailed view by default
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3469e8939f
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7106d2110
ℹ️ 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".
|
@MelvinBot review |
ReviewThe four fixes map cleanly to the four bugs in #94003, and I like how the My one real concern is the reliability of the
|
|
@MelvinBot Yes. The backend persists and returns |
|
Thanks for confirming — that clears my main concern. If the backend persists and returns That leaves just the smaller items, none blocking:
Otherwise the PR looks good to me — the four fixes map cleanly to the four bugs. 👍 |
|
While we're waiting for the backend fix (#94003 (comment)), I think it would be good for @truph01 to review this PR in parallel. |
Hi @dukenv0307, could you please check and confirm this backend behavior? |
@dukenv0307 The target is that, when a tax code is imported from the accounting integration, we don't want to hide it even if it has not been updated. So we need a mechanism for the FE to determine whether a tax code originated from the accounting integration. Initializing |
|
@dukenv0307 Thanks, @truph01. I have the same opinion as @truph01. Also, I'm not sure how accounting integrations sync tax codes, so I'm not sure whether always displaying the tax code from the accounting integration is the intended behavior. |
|
We are still waiting for BE change |
|
Thanks @dukenv0307. I see all taxes imported from accounting integration have the |
@dmkt9 Could you share the video? |
|
@truph01 Here's a screenshot of a policy:
|
|
@dmkt9 We have conflict |
|
Thanks. Conflicts resolved |
|
@MonilBhavsar All yours! |
|
There are conflicts again |
| // When the user explicitly selected the tax columns (customResult) and the workspace | ||
| // has taxes enabled, keep them regardless of per-transaction values — older expenses | ||
| // created before taxes were turned on still have null taxCode/taxAmount/taxValue. | ||
| const hasTaxInfo = isPolicyTaxEnabled || !!transaction.taxCode || !!transaction.taxAmount || !!transaction.taxValue; |
There was a problem hiding this comment.
NAB, I think hasAllTaxInfo is better name
|
@MonilBhavsar Thanks. Conflicts resolved |
|
🚧 MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.52-0 🚀
|
No help site changes requiredI reviewed the changes in this PR against the tax articles under Why: This PR is bug-fix cleanup that brings the UI in line with behavior the help site already documents (or doesn't cover):
Live UI verification (web)Enabled Taxes on a GBP (non-USD) workspace and opened the default rate ("Tax exempt"):
Minor unrelated note (not from this PR, so not fixing here): @dmkt9, this PR doesn't require any help site updates, so no linked docs PR was created. If you believe a specific documented workflow did change, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.52-11 🚀
Bundle Size Analysis (Sentry): |




Explanation of Change
This PR cleans up several tax display and editability issues by distinguishing between user-customized tax codes and platform/default tax identifiers.
A new
isTaxCodeCustomized()helper was added to PolicyUtils. It treats a tax code as user-customized only when its tax rate has a non-empty previousTaxCode, which lets the UI avoid exposing internal platform default tax IDs.Report table tax behavior was updated so Per Diem expenses no longer show tax name, tax code, or tax amount values. Tax code cells are now also hidden unless the tax code was actually customized by the user.
Search/report table column visibility was tightened so tax-related columns are not shown by default only because a transaction contains tax metadata. Tax rate, tax amount, and tax code columns now appear only when the user explicitly selects/customizes those columns.
Workspace tax editing was adjusted so default tax rates can still have their Name, Value, and Tax Code fields edited when the user has write access. The stricter
canEditTaxRatepermission remains used only for disabling or deleting tax rates, which preserves the existing restriction for protected/default rates while allowing field edits.The workspace tax code editor now shows an empty input for non-customized/default tax codes instead of pre-filling internal platform IDs, and saving behavior was updated so users can replace those defaults with their own tax code.
Unit coverage was added for
isTaxCodeCustomized()to verify that missing policies, missing tax codes, unknown tax rates, empty previousTaxCode, and customized tax codes are handled correctly.Fixed Issues
$ #94003
PROPOSAL:
Tests
Same as QA steps
Offline tests
Same as QA steps
QA Steps
Test 1:
Test 2:
Name,ValueandTax Codefields are editableTest 3:
Test 4:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Test.1.mp4
Test.2.mp4
Test.3.mp4
Test4.mp4